fix(gateway): spawn platform reconnect watcher with task-level supervision - #71867
Closed
ygd58 wants to merge 1 commit into
Closed
fix(gateway): spawn platform reconnect watcher with task-level supervision#71867ygd58 wants to merge 1 commit into
ygd58 wants to merge 1 commit into
Conversation
…ision Fixes NousResearch#71758. A platform adapter that dies on a transient upstream failure (marked retryable=True, e.g. photon's sidecar exiting when its upstream gRPC/CDN returns errors) is correctly queued into _failed_platforms for background reconnection. But the reconnect watcher task itself was spawned via a bare asyncio.create_task -- if an exception ever escaped its OUTER while-loop (not just the per-platform inner try/except), the watcher died silently: no log, no restart. _ensure_reconnect_watcher_running() already existed to respawn a dead watcher, but it's only called from _handle_adapter_fatal_error_impl() when a NEW platform's fatal error arrives. If the watcher dies while a platform is already sitting in the queue and no OTHER platform ever fails afterward, nothing ever notices the watcher is dead -- exactly matching the reported symptom: photon queued for reconnect, the gateway itself healthy (other platforms kept working, so nothing re-triggered the ensure-alive check), and the platform stayed dead for 17.5h until a manual restart, well after the transient upstream outage had recovered. Fix: spawn the reconnect watcher via the existing _spawn_supervised() task-level supervisor (already used for kanban_dispatcher_watcher, handoff_watcher, etc.) instead of a bare asyncio.create_task, at both the initial startup spawn and the manual-respawn path in _ensure_reconnect_watcher_running(). _spawn_supervised already provides exactly what's missing here: catches and logs any exception escaping the task, and auto-restarts with capped exponential backoff (healthy-run counter resets so a daemon that crashes occasionally over days is never permanently abandoned) -- self-healing independent of any new fatal-error event. Also hardened a related race: the watcher's per-platform loop looked up self._failed_platforms[platform] via direct indexing after snapshotting the keys with list(...). A platform removed concurrently between the snapshot and the lookup (e.g. a manual /platform resume, or a reconnect that succeeded via a different path) would raise an uncaught KeyError -- exactly the class of bug this fix's supervision now catches, but avoiding the crash-and-restart cycle entirely is better than relying on it. Changed to .get() with a skip-if-missing guard. 6 new tests pass (initial spawn uses _spawn_supervised, manual respawn uses _spawn_supervised, the core regression -- watcher self-heals after an uncaught exception with no new fatal-error event -- and the race-guard scenario); 52/52 in the full tests/gateway/test_platform_reconnect.py file (including the 4 pre-existing _ensure_reconnect_watcher_running tests, confirming no regression to that respawn-when-dead-or-missing behavior).
alt-glitch
added
type/bug
Collaborator
Related to #71177: both repair reconnect-watcher liveness. This focused patch additionally guards the failed-platform snapshot/lookup race; please choose or consolidate the overlapping watcher-supervision changes. |
teknium1
added a commit
that referenced
this pull request
Jul 27, 2026
…er a supervised respawn Follow-up to the salvaged #71867 supervision fix. _spawn_supervised's own backoff respawn created a new task without updating the external handle self._reconnect_watcher_task, so after the reconnect watcher crashed and self-restarted, _ensure_reconnect_watcher_running() saw the stale handle as done() and spawned a SECOND concurrent watcher (double reconnect attempts). Add an optional on_spawn callback to _spawn_supervised, fired with the live task on every spawn INCLUDING internal respawns, and pass it at both reconnect- watcher spawn sites so the tracked handle always advances. The two supervision mechanisms (supervisor auto-restart + ensure-respawn) now compose instead of racing. Regression test sabotage-verified.
teknium1
added a commit
that referenced
this pull request
Jul 27, 2026
…er a supervised respawn Follow-up to the salvaged #71867 supervision fix. _spawn_supervised's own backoff respawn created a new task without updating the external handle self._reconnect_watcher_task, so after the reconnect watcher crashed and self-restarted, _ensure_reconnect_watcher_running() saw the stale handle as done() and spawned a SECOND concurrent watcher (double reconnect attempts). Add an optional on_spawn callback to _spawn_supervised, fired with the live task on every spawn INCLUDING internal respawns, and pass it at both reconnect- watcher spawn sites so the tracked handle always advances. The two supervision mechanisms (supervisor auto-restart + ensure-respawn) now compose instead of racing. Regression test sabotage-verified.
Contributor
|
Merged via #72366 with your commit cherry-picked onto current main — authorship preserved. The reconnect watcher is spawned with task-level supervision again, plus a follow-up fix so a supervised respawn keeps _reconnect_watcher_task pointing at the live task (prevents a duplicate watcher). Closes #71758. Thanks! |
randlee
pushed a commit
to randlee/hermes-agent
that referenced
this pull request
Aug 11, 2026
…er a supervised respawn Follow-up to the salvaged NousResearch#71867 supervision fix. _spawn_supervised's own backoff respawn created a new task without updating the external handle self._reconnect_watcher_task, so after the reconnect watcher crashed and self-restarted, _ensure_reconnect_watcher_running() saw the stale handle as done() and spawned a SECOND concurrent watcher (double reconnect attempts). Add an optional on_spawn callback to _spawn_supervised, fired with the live task on every spawn INCLUDING internal respawns, and pass it at both reconnect- watcher spawn sites so the tracked handle always advances. The two supervision mechanisms (supervisor auto-restart + ensure-respawn) now compose instead of racing. Regression test sabotage-verified.
18 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Fixes #71758.
Problem
A platform adapter that dies on a transient upstream failure (marked
retryable=True) is correctly queued for background reconnection. But the reconnect watcher task itself was spawned via a bareasyncio.create_task-- if an exception ever escaped its outer while-loop, the watcher died silently: no log, no restart._ensure_reconnect_watcher_running()already existed to respawn a dead watcher, but it's only called when a NEW platform's fatal error arrives. If the watcher dies while a platform is already queued and no other platform ever fails afterward, nothing notices -- matching the reported symptom of a platform staying dead for 17.5h after a transient outage recovered.Fix
Spawn the reconnect watcher via the existing
_spawn_supervised()task-level supervisor (already used for other long-lived background tasks) instead of a bareasyncio.create_task, at both the initial startup spawn and the manual-respawn path. This catches and logs any exception escaping the task, and auto-restarts with capped exponential backoff -- self-healing independent of any new fatal-error event.Also hardened a related race: a platform removed from
_failed_platformsconcurrently between the watcher's keys-snapshot and its lookup could raise an uncaughtKeyError. Changed to.get()with a skip-if-missing guard.Verification
6 new tests pass (spawn paths use
_spawn_supervised, the core self-heals-without-new-fatal-error regression, and the race-guard scenario); 52/52 in the fulltests/gateway/test_platform_reconnect.pyfile (no regression to existing respawn behavior).