Skip to content

fix(gateway): heal a dead reconnect watcher when the platform is already queued - #90448

Open
jackulau wants to merge 1 commit into
NousResearch:mainfrom
jackulau:fix/90386-requeue-heals-dead-reconnect-watcher
Open

fix(gateway): heal a dead reconnect watcher when the platform is already queued#90448
jackulau wants to merge 1 commit into
NousResearch:mainfrom
jackulau:fix/90386-requeue-heals-dead-reconnect-watcher

Conversation

@jackulau

Copy link
Copy Markdown
Contributor

What does this PR do?

_ensure_reconnect_watcher_running() exists for exactly one situation: the reconnect watcher has exhausted _MAX_SUPERVISED_RESTARTS, so _spawn_supervised has logged giving up restarts and will never bring it back on its own. That is what #70344 added it for, and it is the gap _spawn_supervised's own auto-restart (the #71758 fix) explicitly does not cover, because the budget is finite.

It had exactly one call site:

def _queue_retryable_fatal_platform(self, adapter) -> bool:
    if not adapter.fatal_error_retryable:
        return False
    platform_config = self.config.platforms.get(adapter.platform)
    if not platform_config or adapter.platform in self._failed_platforms:
        return False          # <- silent return, before the ensure
    ...
    logger.info("%s queued for background reconnection", ...)
    self._ensure_reconnect_watcher_running()   # <- only here
    return True

The already in _failed_platforms early return skips it. But a platform that is already queued is the only kind of platform the watcher can have been retrying long enough to burn five rapid restarts on. The backstop could not fire in the one state it was written for.

Why the outage is silent rather than loud

This is the part that makes it expensive to diagnose. Every other guard in the fatal path is deliberately satisfied:

guard why it does not fire
logger.info("... queued for background reconnection") after the early return
stranded check in _handle_adapter_fatal_error_detached requires platform not in self._failed_platforms; it is in there, so the platform reads as safe and the gateway never exits for the service manager
No connected messaging platforms remain ... needs not self.adapters; with a second platform still connected, non-empty
... gateway staying alive, watcher will retry in background same condition, same reason

So a retryable fatal error can produce a single ERROR line and then nothing at all, while the platform sits in a queue nobody is draining. #90386 reports 4h17m of exactly that shape, with the cron scheduler running normally the whole time and recovery requiring a manual systemctl --user restart.

The change

Call the ensure on the already-queued path too. It is already idempotent and already cheap - it returns immediately unless the tracked task is done(), and it spawns through the same on_spawn handle tracking, so a live watcher is never duplicated.

The queue entry is deliberately not touched. Re-enqueueing would reset attempts and next_retry, restarting the backoff ladder on every fatal error and hammering a provider that is already refusing the connection. There is a test that fails if someone later "simplifies" it that way.

On the reported issue

This closes a real, reachable hole on that path, and it matches #90386's log signature exactly (one Fatal telegram adapter error line, then silence, gateway alive, cron unaffected). I want to be straight about what I have and have not proven: I do not have the reporter's _failed_platforms contents at 04:02:31, so I cannot prove from their logs alone that this is the only thing that went wrong in that outage. The report's own suggestion 3 (a state-file watchdog) would be a broader belt-and-braces change and is not attempted here. What I can show is that this branch is reachable, silent, terminal, and now covered.

Related Issue

Fixes #90386

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • gateway/run.py - _queue_retryable_fatal_platform: split the combined early return so the already queued case calls _ensure_reconnect_watcher_running() before returning False. The no platform_config case still returns immediately, since there is nothing to reconnect to.
  • gateway/run.py - _ensure_reconnect_watcher_running docstring: it is now called from both paths, and the docstring says which and why.
  • tests/gateway/test_platform_reconnect.py - new TestRequeueHealsDeadReconnectWatcher with three tests.

How to Test

  1. pytest tests/gateway/test_platform_reconnect.py -q - 24 passed.

  2. Sabotage proof. Revert only the gateway/run.py hunk and re-run:

    E  assert [] == ['platform_reconnect_watcher']
    E  a re-fatal on an already-queued platform must still heal a dead reconnect
       watcher -- it is the only remaining path back to the queue once
       _spawn_supervised has given up restarting (#90386)
    

    test_requeue_respawns_a_watcher_that_gave_up_restarting fails; the other two pass unpatched by design - they constrain the shape of the fix rather than detect the bug:

    • test_requeue_does_not_disturb_the_existing_queue_entry fails any fix that re-enqueues,
    • test_requeue_with_a_live_watcher_spawns_nothing fails any fix that spawns unconditionally.
  3. ruff check gateway/run.py tests/gateway/test_platform_reconnect.py - clean.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11 (Python 3.12, repo dev venv)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — docstring updated; no user-facing docs describe this internal path
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A, no config keys
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — pure asyncio bookkeeping, no paths, processes, or platform APIs; identical on all three

Duplicate check

Ran before writing any code, since this seam has been fixed several times:

…ady queued

_ensure_reconnect_watcher_running() exists for one situation: the reconnect
watcher has exhausted _MAX_SUPERVISED_RESTARTS, so _spawn_supervised has logged
"giving up restarts" and will never bring it back on its own (NousResearch#70344, and the
supervised-restart half of NousResearch#71758). It had exactly one call site, inside the
newly-queued branch of _queue_retryable_fatal_platform.

That branch is unreachable for a platform already in _failed_platforms, which
is the only kind of platform the watcher can have been retrying long enough to
burn five rapid restarts on. So the backstop could not fire in the one state it
was written for.

The failure is silent by construction. The early return logs nothing, so there
is no "queued for background reconnection" line. The stranded check in
_handle_adapter_fatal_error_detached deliberately treats a queued platform as
safe, so the gateway does not exit for the service manager either. With another
platform still connected, self.adapters is non-empty and the "gateway staying
alive, watcher will retry in background" branch is skipped too. A retryable
fatal error can therefore produce a single ERROR line and then nothing: the
platform sits in the queue that nobody is draining until someone restarts the
process by hand (NousResearch#90386 reports 4h17m of that, with cron unaffected throughout).

Call the ensure on the already-queued path as well. It is already idempotent
and already cheap: it returns immediately unless the tracked task is done, and
it routes through the same on_spawn handle tracking, so a live watcher is never
duplicated.

The queue entry itself is deliberately left untouched. Re-enqueueing would
reset attempts and next_retry, restarting the backoff ladder on every fatal
error and hammering a provider that is already refusing the connection.
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/gateway Gateway runner, session dispatch, delivery sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 20, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #71867 and #71177 covered watcher supervision/respawn, while this patch fixes the remaining already-queued early-return path after restart-budget exhaustion.

@jackulau

Copy link
Copy Markdown
Contributor Author

Agreed, and I checked both since #71177 is closed and I wanted to be sure this was not a rehash of something a maintainer had already declined.

It is not: #71177 is where _ensure_reconnect_watcher_running came from, and it was closed as superseded, not rejected. Per @teknium1 on that thread, the detach-on-timeout + _ensure_reconnect_watcher_running + faulthandler work landed via #70987, and the remaining _spawn_supervised delta landed via #72366 (salvage of #71867).

The useful detail is where #71177 put the call:

                 logger.info(
                     "%s queued for background reconnection",
                     adapter.platform.value,
                 )
+                self._ensure_reconnect_watcher_running()

Inside _handle_adapter_fatal_error_impl, immediately after the enqueue log. The later extraction into _queue_retryable_fatal_platform carried that placement over verbatim, so the already-queued gap is original to the design rather than something a refactor introduced - which is why grepping for a regression in the history does not turn it up. It has been unreachable since the day the backstop was added.

That also makes the two landed pieces complementary rather than overlapping with this one:

Full-suite note for reviewers: tests/gateway/ on this branch is 71 failed / 5762 passed against 72 failed / 5758 passed on pristine main at fab8479aa0 on the same Windows box, so the branch has strictly fewer failures than its base and none of them are in test_platform_reconnect.py (24/24 green). Linux CI is the authority here; that is just to say I checked rather than assumed.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head d6c333742d696d9dc8aa71dabe9a99392640f57d against base aebab05f9ec08c29672dd709db2e639d00a8544a; current main is a72c9ca248a051b8c7e8a69ff422c7be5066cdc4, four commits beyond the base with no overlap in gateway/run.py. Exact-head CI, Docker, and Nix are green, and there were no prior formal reviews on this head.

The local hunk is sensible defense-in-depth: if a retryable fatal callback arrives for an already-queued platform, checking watcher liveness without resetting attempts / next_retry is strictly better than silently returning, and the two guard tests correctly prevent re-enqueue/backoff reset and duplicate spawning.

I do not think the current evidence proves the P1 is closed, though. There are two architecture blockers.

1. The healer is still event-coupled after the event source is gone

The predecessor architecture says this explicitly. #72366 (salvage of #71867 by @ygd58) restored _spawn_supervised because _ensure_reconnect_watcher_running() was only reached from a new fatal-error arrival; if the watcher died while a platform was already queued and no later platform failed, nothing noticed it. Supervision fixed that by autonomously respawning the watcher.

This PR correctly observes that supervision is finite (_MAX_SUPERVISED_RESTARTS). But after that finite budget is exhausted, the system is back in the exact state #72366 described: queued work exists, the watcher is dead, and no independent owner is checking the invariant. Adding _ensure...() to the already-queued branch of another fatal callback is still another event-coupled check.

The core regression test currently manufactures the missing event:

  1. pre-populate _failed_platforms;
  2. mark _reconnect_watcher_task dead;
  3. synthesize a fresh fatal adapter;
  4. call _queue_retryable_fatal_platform().

That proves the new branch, but it does not prove the claimed self-heal topology. In production, #81036 moved queue publication before disconnect and removes the failed adapter from the live adapter map; once the reconnect watcher has burned through its own restart budget, there is no guarantee any adapter remains that can emit the second fatal callback the test injects. If no new fatal event happens, this patch never runs.

Required witness: put one platform in _failed_platforms, make platform_reconnect_watcher fail rapidly enough to exhaust the supervised restart budget, emit no further fatal callbacks, then prove the system recovers automatically once the underlying condition clears. The durable invariant should be something like:

while _running and _failed_platforms is non-empty, either a reconnect watcher is live, a bounded respawn is scheduled, or the gateway has requested supervisor restart.

That ownership belongs at the watcher/supervisor boundary, not at a future platform event. A reasonable shape is a critical-task on_give_up path that either schedules a slower bounded retry tier or requests process restart when queued work still exists; an independent housekeeping reconciliation would also close it. Please do not turn this into an unbounded tight restart loop.

2. #90386 is not current-main proof for this branch

The issue reporter is on Hermes v0.19.0 dated 2026-07-20. The directly preceding same-symptom repair, #81036, merged 2026-08-07 (salvage of #80700, preserving @HexLab98) and specifically changed the fatal path to:

  • queue retryable platforms before any disconnect await;
  • put an outer hard deadline around fatal handling;
  • best-effort queue on cancellation/exception;
  • harden Telegram disconnect steps.

That PR fixed #80598, whose report had the same signature: fatal line, then no queue/reconnect log, gateway still alive. The #90386 runtime therefore predates the current-main protection that materially changes how to interpret its log silence. Its logs cannot establish that adapter.platform in _failed_platforms was the surviving branch on current main; the old build could still have been hitting the already-fixed pre-queue disconnect wedge.

So I would not merge this with Fixes #90386 / P1 closure on the present evidence. Either reproduce the already-queued + dead-watcher state on a current-main build containing #81036, or reframe this PR as a narrow hardening (Refs #90386) and leave the incident open until a current-main witness exists.

Topology / credit

  • #70987 (@teknium1, based on work by @kshitijk4poor / @webtecnica) introduced the manual watcher-liveness backstop.
  • #72366 preserves @ygd58 from #71867 and owns supervised watcher restart + live-handle tracking. Its own rationale identifies the no-new-fatal-event state this PR still does not close after budget exhaustion.
  • #81036 preserves @HexLab98 from #80700 and is the current fatal-handler queue-before-disconnect / outer-timeout authority. It must be part of the acceptance witness because #90386 predates it.
  • #90448 is complementary hardening of the duplicate/re-fatal branch; it is not a duplicate of those predecessors, but it also cannot substitute for the critical-task exhaustion owner.

Re-review gate: autonomous no-new-event exhaustion witness; explicit disposition of Fixes #90386 versus current-main reproduction; preserve the predecessor credit above; rebase the four non-overlapping main commits and rerun exact-head CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P1 High — major feature broken, no workaround sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gateway self-heal wedges after Telegram polling network outage — reconnect watcher never takes over

3 participants