fix(homeassistant): bound every wedgeable await and watchdog the listener - #68540
fix(homeassistant): bound every wedgeable await and watchdog the listener#68540Sora-bluesky wants to merge 2 commits into
Conversation
2060d27 to
2b91cdb
Compare
|
suggesting changes
The 56 focused Home Assistant and Telegram reconnect tests otherwise passed, including bounded handshake and teardown, healthy-listener ping handling, failed-ping recovery, and cancellation-resistant task abandonment. Security evidence:
Signed: GPT-5.6-sol-xhigh in Codex |
|
Good catch. CancelledError is a BaseException, so the While validating it I found the naive handler still leaks if a second cancellation lands while the close is in flight (the watchdog respawn and disconnect() can both cancel the same listen task). Wrapped the close in a tracked task + asyncio.shield so it runs to completion instead of getting interrupted, and kept a strong ref so asyncio doesn't GC the shielded task mid-close. The same gap was in _cleanup_ws() and disconnect(), so those close as one shielded unit now too. Added regressions that cancel _ws_connect()/_cleanup_ws()/disconnect() mid-teardown and assert close() is still awaited. They fail on the current head (close awaited 0 times) and pass after the fix. |
|
suggesting changes
With Please observe the close task with a deadline mechanism that does not wait for cancellation completion (for example, a separately tracked task plus Security evidence:
Signed: GPT-5.6-sol-xhigh in Codex |
|
Thanks for catching that, it turned out to be bigger than the wait_for patch. Went through the whole close path and the cancellation-suppression bug was one case of a wider pattern. The bounded close no longer uses Also fixed while in there:
One thing to be upfront about: if close() is genuinely broken and never returns, no wrapper can prove the socket actually closed. This bounds how long the caller waits and keeps the orphaned close from getting lost, but it's not a guarantee close() itself works. That was already true before, just flagging it's not something this fix claims to solve. |
|
too large to review safely This PR changes 699 production lines before tests and docs. Please split it or add a focused justification if it should stay together. Signed: GPT-5.6-terra-low in Codex |
bc9ddb5 to
57d0220
Compare
|
You're right, it grew past what's reviewable in one pass. I've split it. This PR is now just the #67470 fix: bound the wedgeable teardown awaits, watchdog the listen loop, and close the WS session when a connect is cancelled. That's the reported "goes silently deaf after a network blip" bug on its own. The bounded-abandon rework I described above -- making the teardown bound hold even when a close() suppresses its own cancellation (your finding), plus the connect/disconnect generation races that surfaced while fixing it -- moves to a follow-up PR. I'll open that against the merged result once this one lands, so each is a focused diff instead of one ~700-line pile. The follow-up work is already on a branch so nothing's lost; I just didn't want to hand you all of it to review at once. |
|
suggesting changes The smaller split restores head With Security evidence:
Signed: GPT-5.6-sol-xhigh in Codex |
|
Good call, this belongs here. Put the bounded-abandon back.
Added three regressions for that: the cancellation-suppressing abandon, plus the session and rest-session closes still running after a ws close that raises CancelledError on its own. Left the connect/disconnect generation-race work out of this one, since you said that part can be a follow-up. |
…ener The HA gateway adapter could go silently deaf for hours after a single transient network failure (NousResearch#67470), the same class Telegram fixed in 3391e63 / c2cb375: - _ws_connect assigned self._session before ws_connect() awaited, so a raised connect left the just-created session dangling. Build into locals, close on raise, and wire self._session/_ws only once usable. - Every teardown await (ws/session/REST close) is now bounded via _bounded_close (asyncio.wait_for + _DRAIN_TIMEOUT) so a wedged CLOSE-WAIT socket can't stall the reconnect ladder or disconnect(). - The auth handshake's receive_json()/send_json() calls are bounded (_HANDSHAKE_TIMEOUT); any handshake exception — timeout, client error, or cancellation — tears the connection down in place instead of leaking it to a later loop pass. - A cause-agnostic watchdog task checks a _last_progress heartbeat (bumped per listen-loop pass and per received frame) and force-cancels + respawns a listener that has made no progress for _LISTEN_STUCK_TIMEOUT. Before respawning it sends an HA-protocol ping: aiohttp answers WS heartbeat PINGs internally, so a healthy-but-quiet HA produces no reader frames — the app-level pong arrives as a normal frame, bumps progress via the reader (single-reader invariant), and a live listener is spared the spurious reconnect. - Task cancellation waits use asyncio.wait (observe with deadline), not wait_for: wait_for's timeout path awaits the cancellation completing, so a task that swallows CancelledError would hang it — the very stall being fixed. A zombie that won't die is logged and abandoned; disconnect() and the watchdog always return. Tests (tests/gateway/test_homeassistant_network_reconnect.py, mirroring the Telegram reconnect suite): session-leak on failed connect, bounded hanging closes in cleanup/disconnect, bounded auth handshake, handshake send-failure cleanup, watchdog respawn of a wedged listener, ping-probe sparing a quiet-but-healthy listener, ping-probe failure respawning, bounded abandonment of an uncancellable task, and clean watchdog exit. All fail against the previous adapter except the exit test. Fixes NousResearch#67470 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ba613b9 to
2eb7202
Compare
|
too large to review safely This PR changes 524 production lines before tests and docs. Please split it or add a focused justification if it should stay together. Signed: GPT-5.6-terra-low in Codex |
|
Fair call on the numbers: 474 changed production lines in adapter.py plus 976 of tests. Why it is one piece: the bug class is any await on the HA websocket that can wedge the adapter, and the fix bounds all of them (send, receive, auth, subscribe, reconnect backoff). A partial split would land some bounds while the remaining awaits still wedge the adapter, so the user-visible stall survives until the last piece merges. Most of the line count is tests covering each timeout path. The size predates the discipline my recent PRs follow (150-250 lines). If one review-sized unit is not worth the cost here, deprioritizing it is a fair outcome. Taking your second option: justification over split. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused recovery coverage; the current main adapter still has the unbounded websocket lifecycle paths this PR targets (plugins/platforms/homeassistant/adapter.py:147-195, 219-247).
Problems
plugins/platforms/homeassistant/adapter.py:632-640abandons a cancellation-resistant listener, calls_cleanup_ws(), and overwritesself._listen_task. The abandoned_listen_loop()can later resume and follow its existing reconnect path (adapter.py:578-591), which operates on the sameself._wsandself._sessionas the replacement. No generation/ownership guard prevents that old task from closing or reconnecting the replacement's connection.- The generic abandonment test in
tests/gateway/test_homeassistant_network_reconnect.py:393-424proves the helper returns, but does not exercise a resumed abandoned_listen_loop()after replacement.
Suggested changes
- Add a listener generation/ownership guard around cleanup and reconnect, and retain abandoned listeners until completion.
- Add a regression where an old read suppresses cancellation then returns after the watchdog replacement; verify it cannot mutate the new connection.
Automated hermes-sweeper review.
| return | ||
|
|
||
| self._last_progress = time.monotonic() | ||
| self._listen_task = asyncio.create_task(self._listen_loop()) |
There was a problem hiding this comment.
If _cancel_task_bounded() returned with the previous listener still pending, this overwrites the only active-task reference while that old _listen_loop() can later resume and run its own _cleanup_ws()/_ws_connect() against shared adapter fields. Please add generation ownership (and a resumed-zombie-listener regression) so an abandoned incarnation cannot touch the replacement connection.
…tions When the watchdog detects a wedged _listen_loop and respawns it, the old (abandoned) listener instance can still be scheduled to run if it was blocked on a swallow-cancellation read. The generation guard prevents that abandoned listener from interfering with the new generation''s connection or reconnect state. The fix: - Add self._listen_gen counter, incremented on each listener spawn (initial in connect(), respawn in _watchdog_loop()). - Each _listen_loop captures its generation at entry and compares it to the current counter at two guard points: (1) loop entry, (2) before reconnect. - If the counter has advanced (listener was respawned), the abandoned loop exits cleanly. Fixes sweeper review finding: abandoned listener calling _cleanup_ws / _ws_connect after being replaced, potentially corrupting the new connection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both paths were real — verified before fixing. The abandoned loop could resume once its swallowed cancel consumed the one Fixed in f862558:
On the retained-until-completion suggestion: I kept abandonment (the #67470 design — staying deaf is worse than leaking one stuck task) and made the abandoned instance inert instead, which I think is the property the retention idea was after. |
|
too large to review safely This PR changes 573 production lines before tests and docs. Please split it or add a focused justification if it should stay together. Signed: GPT-5.6-terra-low in Codex |
|
Fair, and the number checks out: 520 added and 53 removed in the adapter across 7 hunks, with 13 new functions. That is not a small review. Two mechanisms are bundled here. Bounding the awaits — the close and teardown helpers plus their call sites — is the actual fix for the wedge that was reported. The watchdog, the listener respawn and the generation guard are a second mechanism layered on top; they share the bounded helpers but nothing forces them into the same change. They landed together because I wrote them in one sitting, which is not a reason for you to review them in one sitting. I'll split it so the bounded-await hardening stands alone here and the watchdog goes to a follow-up that builds on it. I would rather do that than ask you to carry 573 lines at once. |
SummaryFifty-four PRs address or reference this reliability complex, spanning Telegram connection retries, polling-conflict recovery, dead-poller detection, startup deadlines, and the Home Assistant listener wedge targeted by #68540. For #67470, #68540 is the only listed HA implementation: its diff bounds connection lifecycle awaits and adds listener watchdog/generation ownership, but its review size remains blocking. Related pull requests
DuplicatesSalvage/duplicate chains are #1527→#1535, #2297→#2312 with #2298 narrower, #2477→#2517, #3177→#3268, #18088→#18751, #25630→#28486 with #23806/#27099 superseded, #55789→#55905/#55921, #56036→#56200/#56224, #58250→#58293, #63345→#64370, and #64506→#64574 with #64639 duplicate. #75073 and #75096 address #75017 through materially different mechanisms; #75073 is withdrawn, while #75096 preserves the queue contract. Suggested consolidationDespite the keep_open review on #68540: the latest contributor reviews identify its 573-line production scope as too large to review safely, and the author explicitly agreed that bounded-await hardening and watchdog/generation recovery are separable. Author action: split out the bounded-await/session-leak fix that can merge, then carry the watchdog and generation-ownership machinery in a focused follow-up; keep #75096 open for separate review as the recorded best fix for #75017, with #75073 already closed. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I66377(["issue #66377 (closed)"])
I67470(["issue #67470 (open)"])
P68540["PR #68540 (open)"]
P68540 -->|fixes| I66377
P68540 -->|fixes| I67470
class I66377 closed
class I67470 open
class P68540 open
class P68540 target
click I66377 "https://github.com/NousResearch/hermes-agent/issues/66377"
click I67470 "https://github.com/NousResearch/hermes-agent/issues/67470"
click P68540 "https://github.com/NousResearch/hermes-agent/pull/68540"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 54 pull requests and 36 issues in this complex. Diffs were read for 3 of 54 PRs (rest unavailable); Assessment working set: 89 kB of PR diffs, 285 kB of issue/PR text, 121 kB of discussion (155 comments), 5 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
|
Split done. The bounded-await half is up as #80372, rebased on current main with all 15 of its tests carried over and mutation-checked. The watchdog + stale-generation half is ready on a follow-up branch. It builds on the teardown primitives, so it goes up once the first half lands. I'll keep this PR open for reference until both halves are in, unless you'd rather close it sooner. |
What does this PR do?
The Home Assistant gateway adapter could go silently deaf for hours after a single transient network failure (#67470): the reconnect ladder stalls, the process stays
active(running), and nothing ever notices. This is the same bug class the Telegram adapter fixed in3391e639f(bounded drain) andc2cb37532(cause-agnostic watchdog); this PR ports that pattern to the HA adapter's session/WS pair, plus one additional wedge point found while verifying the report.Four defects fixed in
plugins/platforms/homeassistant/adapter.py:self._sessionwas assigned beforews_connect()awaited, so a raised connect left the just-created session dangling. The connect path now builds into locals, closes on raise, and wiresself._session/self._wsonly once the socket is usable.ws.close()/session.close()(anddisconnect()'s REST close) could block forever on a wedged CLOSE-WAIT socket, hanging the reconnect ladder. Every teardown await is bounded via_bounded_close(asyncio.wait_for+ 5s_DRAIN_TIMEOUT), per-step, so one wedged close can't skip the other resource.receive_json()/send_json()calls in the auth ladder had no timeout — a server that accepts the socket but never responds froze_ws_connect()forever (this matches the reported "storm stops with no further log lines"). Each is bounded by_HANDSHAKE_TIMEOUT, and any handshake failure — timeout, client error, or cancellation — tears the connection down in place._watchdog_looptask checks a_last_progressheartbeat (bumped per listen-loop pass and per received frame) and force-cancels + respawns a listener that has made no progress for_LISTEN_STUCK_TIMEOUT(300s).Two subtleties worth calling out for review:
_read_events'async for— so a healthy-but-quiet HA (no state changes) is indistinguishable from a wedged socket by progress alone. Before respawning, the watchdog sends an HA-protocolping: thepongarrives as a normal frame, bumps progress via the reader (preserving the single-reader invariant), and a live listener is spared the spurious reconnect.asyncio.wait, notwait_for, for task cancellation.wait_for's timeout path awaits the cancellation completing, so a task that swallowsCancelledErrorwould hang it — the very stall being fixed._cancel_task_boundedobserves with a deadline viaasyncio.waitand abandons (with an error log) a zombie that won't die;disconnect()and the watchdog always return.Related Issue
Fixes #67470
Type of Change
Changes Made
plugins/platforms/homeassistant/adapter.py— the four fixes above; watchdog started alongside the listener inconnect()and cancelled first indisconnect()(so it can't respawn mid-teardown).tests/gateway/test_homeassistant_network_reconnect.py— new, mirroringtest_telegram_network_reconnect.pyconventions: session-leak on failed connect, bounded hanging closes in cleanup/disconnect(), bounded auth handshake, handshake send-failure cleanup, watchdog respawn of a wedged listener, ping-probe sparing a quiet-but-healthy listener, ping-probe failure respawning, bounded abandonment of an uncancellable task, and clean watchdog exit.How to Test
scripts/run_tests.sh tests/gateway/test_homeassistant_network_reconnect.py tests/gateway/test_telegram_network_reconnect.py -qactive(running). With this patch every such await is bounded and the watchdog recovers the listener within_LISTEN_STUCK_TIMEOUT+ one interval, with an explicit error log naming the stall.Checklist
Code
mainDocumentation & Housekeeping
cli-config.yaml.example— N/A (no config surface added)CONTRIBUTING.md/AGENTS.md— N/Ascripts/check-windows-footguns.py --diffis clean.Screenshots / Logs
🤖 Generated with Claude Code