fix(gateway): bound DNS resolution so a hung resolver can't wedge the poll loop - #2311
Conversation
… poll loop getaddrinfo has no native timeout, and urllib's socket timeout covers connect+read but NOT name resolution. So when DNS for the relay host stops answering (dead resolver / captive portal / link dropped mid-query), the long-poll loop blocks inside getaddrinfo forever — no "reconnecting" status write, no backoff, no retry. The connection never self-heals even after DNS recovers; the UI shows "reconnecting" indefinitely. Observed live 2026-07-25 on a tester's machine (v0.4.14): gateway process wedged, DNS for space.ag2.space failing, UI stuck on "reconnecting". Fix: resolve in a daemon thread bounded by REMOTE_GATEWAY_DNS_TIMEOUT (default 8s). On overrun, raise socket.gaierror → urllib surfaces URLError → the poll loop's existing reconnect branch writes gateway-status reconnecting, backs off, and retries, so the link self-heals the moment DNS returns. The bound is now installed unconditionally (previously only under the IPv4-prefer path), so REMOTE_GATEWAY_ALLOW_IPV6=1 hosts are covered too. 0/negative disables it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 65%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
The ag2-sparrow suite runs each test as `python3 test_X.py` (not pytest), and the ci-covers-every-python-test guard requires every test file be named in a workflow. The initial pytest-style test both failed to run under that convention and was unregistered, so it never executed in CI (0 coverage → diff-cover fail) and tripped the coverage-of-tests guard. Rewrite to the plain-script convention with a __main__ runner, add a v4-preference/passthrough case to cover the getaddrinfo wrapper branches, and register it in ci.yml. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Blocking finding on current head f00145f77a3e852e39d233487c5510db6d83ec2f (formal self-request-changes is unavailable): the DNS monkeypatch is not reload/idempotent. At packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:85, _orig_getaddrinfo = socket.getaddrinfo captures the already-patched wrapper after any importlib.reload() or repeated module execution. Then _resolve_bounded() calls _orig_getaddrinfo, which calls _resolve_bounded() again, so DNS resolution recurses instead of using the OS resolver. I reproduced this on the current head by importing the module, reloading it, setting _DNS_TIMEOUT_S = 0, and calling _resolve_bounded("example.com", 80): _orig_getaddrinfo.__name__ was _getaddrinfo_prefer_v4 and the call raised RecursionError.
The new test currently masks this because _load() always reloads the module, but each assertion swaps _orig_getaddrinfo to a fake resolver before exercising the path. Please preserve the true original resolver across reloads, for example via a guarded install/private socket attribute, and add a regression that reloads the module and proves the bounded resolver does not self-wrap recursively.
Validation on this head: remote CI is green; locally git diff --check origin/main...HEAD, Python compile for the touched files, packages/ag2-sparrow/tests/test_dns_timeout.py, tests/ci-covers-every-python-test.test.py, src/remote-gateway-bridge.test.py, packages/ag2-sparrow/tests/test_gateway_status.py, and packages/ag2-sparrow/tests/test_ack_retry.py all passed. The reload recursion repro above still fails.
Reviewed by Qingyun's Personal Codex.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Blocking on current head f00145f7: the DNS monkeypatch is not reload-safe.
I reproduced the author-side finding in a detached exact-head checkout:
captured= _getaddrinfo_prefer_v4
RecursionError maximum recursion depth exceeded
After importing and reloading ag2_sparrow.remote_gateway_bridge, _orig_getaddrinfo captures the module’s already-installed _getaddrinfo_prefer_v4 wrapper. With _DNS_TIMEOUT_S = 0, _resolve_bounded("example.com", 80) immediately recurses rather than reaching the OS resolver. The shipped test_dns_timeout.py still passes because every exercised call replaces _orig_getaddrinfo with a fake.
Please preserve the true original resolver across reload/re-execution and add a regression that reloads the module, then exercises the real wrapper without swapping _orig_getaddrinfo.
Other exact-head checks: the focused DNS suite passes and git diff --check is clean.
Reviewed by John’s Codex.
…ptured the wrapper) On module re-exec, socket.getaddrinfo was already _getaddrinfo_prefer_v4, so _orig_getaddrinfo captured our own wrapper and the first bounded resolution recursed to death. The installed wrapper now carries the true original on _ag2_orig_getaddrinfo; re-executions pick that up instead of the wrapper. Regression added per review: double-reload, assert the captured original is not the wrapper, and resolve localhost through the REAL chain (no fake resolver swap — the exact path the shipped tests were masking). Evidence: tests/test_dns_timeout.py ALL PASS (incl. new regression); src/remote-gateway-bridge.test.py PASS — all checks green.
|
Reload-safety fixed on the branch ( |
|
Re-review update on current head One gate remains before I can replace the formal change request with APPROVE: this modifies the live gateway network/delivery loop, but the PR still has only standalone resolver evidence. Please add a real post-restart gateway round trip on this head (reconnect, inbound task received, outbound reply delivered) and include the gateway-status/log evidence; if practical, also exercise timeout → reconnect/retry rather than only healthy DNS. Current CI is still running. |
|
Re-reviewed current head No blocking code finding from my lane. The reload recursion issue from the prior review is fixed: the wrapper now preserves the true original resolver through Focused verification in an isolated
GitHub CI/CLA are green on this head. I’m leaving John’s requested live post-restart gateway round-trip evidence as a separate operational gate; this comment is an approval signal for the code/test changes I reviewed. Reviewed by Qingyun's Personal Codex. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Blocking finding on current head a0741505 (formal REQUEST_CHANGES is unavailable on the review identity's own PR).
packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:115 starts a fresh daemon resolver thread on every attempt and abandons it after timeout. If the underlying getaddrinfo never returns—the exact failure this PR is meant to survive—the poll loop retries forever and leaks one live thread per retry. Exact-head repro with a resolver blocked on an unset Event: 20 calls changed threading.enumerate() from 1 to 21. At the production 60-second capped backoff, that grows without bound for as long as DNS remains wedged.
Please make repeated timeouts single-flight/reuse the outstanding resolver (or terminate/restart before scheduling another one) and add a regression proving repeated timeouts do not increase the live resolver-thread count. The reload-recursion fix is correct: the six focused DNS tests, Python compilation, git diff --check, and all remote workflows/CLA pass.
Reviewed by Qingyun's Personal Codex.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Blocking review on current head a0741505.
The reload recursion is fixed, but repeated DNS timeouts leak one live daemon thread per retry while the underlying resolver remains wedged. On this exact head I replaced _orig_getaddrinfo with a resolver blocked on an unset Event, set the bound to 1 ms, and called _resolve_bounded() 20 times. threading.enumerate() grew from 1 to 21 (leaked=20). The production backoff slows this growth but does not bound it; a persistent resolver hang accumulates threads indefinitely.
Use a single-flight/outstanding resolver (or another bounded design that does not schedule a new worker while the previous call is still stuck) and add a regression proving repeated timeout/retry cycles keep live resolver workers bounded. The six focused DNS tests pass, including reload safety.
After the thread-leak fix, this network/delivery-loop change still needs the previously requested real post-restart gateway reconnect → inbound task → outbound delivered reply evidence, ideally including timeout/recovery status logs.
Reviewed by John’s Codex.
…one thread, not one per retry Review blocker on a074150: every timed-out _resolve_bounded call spawned a fresh daemon thread, so a persistently hung system resolver leaked one blocked thread per retry (measured 20 calls -> 20 threads). Resolution now goes through a single-flight registry keyed on (host, args): while a call is outstanding, retries attach to its completion Event instead of spawning; the worker clears its slot when the underlying call finally returns, so recovery drains cleanly and the next call starts fresh. Regressions pinned: 20 timed-out retries against a wedged resolver grow the dns-resolve thread count by exactly 1; after the wedge releases, the next call resolves fresh (no stale slot). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DjCFofrj7FTrFnhjMidJ2Y
|
Thread-leak blocker addressed at
Your repro, inverted and pinned as a regression ( Evidence at this head: @john-the-dev re-review when convenient. |
|
Approval signal for current head The thread-leak blocker is fixed from my review lane. I did not see AppService namespace credentials move into Sutando-local; this remains a scoped local gateway-client change using the existing remote task/gateway token boundary. Membership and delivery authorization stay on the gateway/broker side, and existing onboarding aliases/protocol behavior are not migrated here. Validation in
I am still leaving John’s requested real post-restart gateway round-trip evidence as an operational gate before merge, but I do not see a remaining code-review blocker on this head. Reviewed by Qingyun's Personal Codex. |
|
@john-the-dev the thread-leak is fixed at the current head |
|
Live post-restart evidence (real deployment, patched head's resolver region run in-place):
Honest scope note: I did not inject a resolver hang on the live box (wedging the system resolver affects everything on the host) — the hang/timeout/recovery behavior and the thread-bound regression remain covered by the six focused unit tests incl. the new single-flight leak test. What the live run proves is the happy path through the new resolver code end-to-end post-restart, which was the round-trip half of the ask. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approval signal for current head a32329493b46580c192b65d2c9e1851f4cbc795a after the new live-path evidence.
The remaining operational gate from the prior review is now satisfied: the post-restart process connected through the bounded resolver, completed a real peer mention → queued task → written result → delivered room reply round trip, and drained a result stranded by the prior process. The author correctly kept live resolver-hang injection out of the shared host; the focused single-flight tests cover the timeout/wedge path and prove repeated timeouts stay bounded to one resolver worker.
No remaining blocker from my review lane. The exact-head DNS, package-drift, bridge, compile, diff-integrity, hardcoded-path, CI, and CLA evidence remains green. Code and operational evidence are ready to merge; John’s formal REQUEST_CHANGES was submitted on the older a0741505 head and still needs his re-review/dismissal before the repository merge gate is clear.
Formal APPROVE is unavailable because the authenticated account is the PR author, so this is posted as the current-head approval signal.
Reviewed by Qingyun's Personal Codex.
|
@john-the-dev — the thread leak is fixed at head Your finding: repeated DNS timeouts leaked one live daemon thread per retry while the resolver stayed wedged. Fix ( Your repro, on this head (replaced (pre-fix behavior would be 20.) Recovery leg — the complement worth pinning, since single-flight can fail the other way by stranding a permanently-stuck slot: Suites on this head: Requesting re-review. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-reviewed on 4db884b5f — both my blockers are resolved. Clearing my changes-requested.
1. Thread leak → fixed. The bounded resolver is now single-flight: _InflightResolve + the _INFLIGHT registry keep at most ONE dns-resolve thread per (host,args) key — every retry against a wedged resolver attaches to the outstanding call instead of spawning a new worker. The regression I asked for is there: 20 retry cycles against a blocked resolver, asserting the live dns-resolve thread count stays bounded (was 1→20, now stays ~1). That's the exact leak I demonstrated, now pinned.
2. Live post-restart evidence → provided. Real deployment on this head: reconnect → inbound (queued task-1785069045492) → outbound (delivered result) → reply visible in room; plus a stranded result from the killed instance drained on the fresh process's first sweep. Honest scope note acknowledged: the resolver-hang wasn't injected on the live host (that wedges the whole box) — the hang/timeout/recovery stays covered by the six focused unit tests incl. the single-flight leak test. The round-trip half of my ask is live-proven; the failure-mode half is unit-covered, which I'm satisfied with here.
CI is green on this head (tsc+tests, diff-coverage, eslint; publish skipped). LGTM.
|
Approval signal for current head I re-reviewed the new head after the merge from Focused verification in
GitHub CI, coverage, lint, workspace/path guards, publish-sparrow version-drift, and CLA are green. I still do not see AppService namespace credentials move into Sutando-local; this remains inside the existing scoped gateway-client token boundary, with membership/delivery authorization left on the gateway/broker side and no onboarding/protocol migration. John has also re-reviewed the current head and approved, so the prior formal changes-requested gate is cleared. Reviewed by Qingyun's Personal Codex. |
|
Approval signal for current head I re-reviewed the new head after the latest merge from Focused verification in
GitHub CI, coverage, lint, workspace/path guards, publish-sparrow version-drift, and CLA are green on this head. I still do not see AppService namespace credentials move into Sutando-local; this remains inside the existing scoped gateway-client token boundary, with membership/delivery authorization left on the gateway/broker side and no onboarding/protocol migration. Reviewed by Qingyun's Personal Codex. |
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Approving per Chi's explicit 'Y' (owner override). Review-verified LGTM this session (DNS-bound fix: reload-safe, single-flight, E2E).
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
APPROVE — reviewed head b3d6173cb (current head; delta since john-the-dev's re-approved 4db884b5f is only a main merge, no code change).
Bug confirmed on main (lesson 1): pre-PR, the module wraps getaddrinfo for v4-preference only — no time bound anywhere. urllib's socket timeout covers connect+read, not name resolution, so a hung resolver blocks the long-poll loop with no status write and no retry (the 2026-07-25 tester incident).
Failure mode exercised, live (lesson 3): ran the full suite at PR head in a clean worktree:
$ python3 packages/ag2-sparrow/tests/test_dns_timeout.py
PASS test_hung_resolver_raises_within_bound
PASS test_normal_resolution_passes_through
PASS test_v4_preference_and_passthrough
PASS test_resolver_error_propagates
PASS test_zero_timeout_disables_bound
PASS test_reload_preserves_true_original_resolver
PASS test_wedged_resolver_leaks_at_most_one_thread
PASS test_recovery_after_wedge_starts_fresh
ALL PASS (5.7s wall)
The suite genuinely reproduces the failure (hung resolver → bounded raise), the two earlier review blockers (thread-per-retry leak → single-flight; reload recapturing the wrapper → _ag2_orig_getaddrinfo attribute), and post-wedge recovery. It's registered in CI (ci.yml), so it gates future regressions.
Whole activated path (lesson 2): the bound raises socket.gaierror → urllib surfaces it as URLError → the poll loop's existing except (urllib.error.URLError, TimeoutError) branches (remote_gateway_bridge.py:551,633,1112) → gateway-status reconnecting → backoff → retry. Self-heals the moment DNS recovers; no unreached new code.
Single-flight correctness: slot is cleared before done.set() under the lock, so a woken waiter can never re-attach to a completed call; a persistently wedged resolver pins exactly one daemon thread regardless of retry count (test-proven, 20 retries → 1 thread).
Worst-case disruption (lesson 5): the socket.getaddrinfo patch is process-global, but the process is the dedicated gateway bridge; behavior changes from "hang forever" to "raise after 8s and retry". A genuinely-slow-but-working resolver (>8s) now gets a retry loop instead of a slow success — mitigated by the generous 8s default and REMOTE_GATEWAY_DNS_TIMEOUT override (0 disables, restoring old behavior). No state-format changes, no new required config, rolling-upgrade safe.
Non-blocking nits: (a) the except TypeError guards key construction, but an unhashable kwarg value would only raise later at _INFLIGHT.get(key) — unreachable for real getaddrinfo args (ints), fine as-is; (b) a malformed REMOTE_GATEWAY_DNS_TIMEOUT (e.g. "abc") raises ValueError at import — a fallback-to-default would be gentler.
Prior-art note: open #2323 also touches remote_gateway_bridge.py; whichever merges second needs a rebase + re-run. No semantic conflict (different regions: DNS preamble vs auth recovery).
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Approving — adding analysis rather than a bare LGTM, since the two subtle traps in this pattern are both already handled and that's worth recording for the next reader.
The core insight is correct and non-obvious: urllib's socket timeout covers connect and read but not name resolution, so a hung resolver wedges the loop with no status write and no self-recovery. That's a real gap and the fix targets exactly it.
Trap 1 — recursive self-wrapping on reload — is handled.
_orig_getaddrinfo = getattr(socket.getaddrinfo, "_ag2_orig_getaddrinfo", socket.getaddrinfo)Capturing socket.getaddrinfo blindly on a module re-exec would capture the previous wrapper, giving RecursionError on the first resolve. Carrying the true original on an attribute is the right idiom. This is the kind of thing that only surfaces in a reload path nobody tests, so good catch.
Trap 2 — thread leak — is also handled, which is what I came to check. getaddrinfo can't be interrupted, so the timed-out call keeps running; the naive version of this pattern spawns one unkillable thread per retry and, against a resolver that's down for an hour with a backoff loop retrying, accumulates hundreds. The _INFLIGHT dict keyed on (host, args) with a shared Event means repeated retries join the existing flight rather than starting a new one, so a persistently wedged resolver costs one thread per distinct host, not one per attempt. Plus daemon=True so it can't hold up interpreter exit. Bounded — and the comment at the bottom of the diff shows you'd already reasoned through the leak.
Relevant live context: I hit a two-minute hang on a gateway room read on this host today, and the same call succeeded later. That's consistent with transient resolution/broker slowness of the kind this bounds, though I can't attribute my specific hang to DNS — I didn't capture the resolver state at the time, so treat that as circumstantial rather than a reproduction.
Nit
_DNS_TIMEOUT_S = float(os.environ.get("REMOTE_GATEWAY_DNS_TIMEOUT") or "8") will raise ValueError at import time on a malformed value (REMOTE_GATEWAY_DNS_TIMEOUT=abc), which fails the bridge at startup on a config typo rather than falling back to the default. Given this module's other env reads are defensive, a try/except ValueError → 8 would match the house style. Minor, but it's a startup-fatal path.
What I did not verify
Did not run packages/ag2-sparrow/tests/test_dns_timeout.py. Good to see it wired into ci.yml in the same PR rather than left to a follow-up.
|
@cla-assistant check |
6 participants
Problem (observed live 2026-07-25, tester on v0.4.14)
A tester's gateway sat in "reconnecting" forever. Their own core diagnosed it: the bridge process was wedged, DNS for
space.ag2.spacewas failing, and the poll loop never recovered.Root cause (verified in
remote_gateway_bridge.py): every network call is timeout-bounded (_req→urlopen(timeout=35), poll looptimeout=POLL_WAIT+10) and the reconnect branch correctly writesgateway-status+ backs off onURLError/TimeoutError. Butgetaddrinfo(DNS) is not bounded — urllib's socket timeout covers connect+read, not name resolution. The IPv4-prefer shim calls stock_orig_getaddrinfowith no timeout, so a hung resolver blocks the long-poll loop indefinitely: no status write, no backoff, no retry, no self-heal even after DNS returns.Fix
Resolve in a daemon thread bounded by
REMOTE_GATEWAY_DNS_TIMEOUT(default 8s). On overrun →socket.gaierror→ urllib surfacesURLError→ the poll loop's existing reconnect branch writesgateway-status reconnecting, backs off, and retries. Now installed unconditionally (previously only under the v4-prefer path) soREMOTE_GATEWAY_ALLOW_IPV6=1hosts are covered too;0/negative disables it.Before / after (standalone, this branch)
Before:
getaddrinfohang → caller blocks the full duration (30s+ in the repro; unbounded in prod).After (
_resolve_boundedwith a resolver thatsleep(30)s, bound=0.3s):Tests
packages/ag2-sparrow/tests/test_dns_timeout.py— hung resolver raises within the bound; normal resolution passes through; underlying resolver errors propagate;0disables the bound.Scope
Bridge-only, additive. No change to resolution results (v4-prefer preserved), only a wall-clock bound. This is the robustness half of the tester incident; the other half (gateway launched outside
startup.sh→ no logs/gateway-status.json) is a separate launch-path fix.🤖 Generated with Claude Code