Skip to content

fix(telegram): close reconnect races that leave adapter half-destroyed (salvage #56036) - #56224

Closed
kshitijk4poor wants to merge 2 commits into
NousResearch:mainfrom
kshitijk4poor:salvage-56036
Closed

fix(telegram): close reconnect races that leave adapter half-destroyed (salvage #56036)#56224
kshitijk4poor wants to merge 2 commits into
NousResearch:mainfrom
kshitijk4poor:salvage-56036

Conversation

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Summary

Salvage of #56036 by @JoaoMarcos44 (rebased onto current main). Closes the concurrency races that leave the Telegram adapter "half-destroyed" — the gateway logs ✓ telegram connected but silently stops processing messages after a network blip + self-restart, and can crash with 'NoneType' object has no attribute 'updater' (issue #55992).

Original PR was 127 commits behind main; both of @JoaoMarcos44's commits are cherry-picked here verbatim (authorship preserved).

The bug

After a network outage, multiple recovery paths (the PTB error callback, the heartbeat loop, the pending-updates probe, and the gateway's fatal-error handler) can race on the same outage:

  1. gateway/run.py::_handle_adapter_fatal_error TOCTOU — the adapter was removed from self.adapters in a finally block after await disconnect(). A second concurrent fatal-error notification for the same adapter still saw itself as the installed adapter during that await and called disconnect() on the same object twice — the concrete origin of the NoneType ... updater crash when teardown re-reads self._app.
  2. telegram/adapter.py::_handle_polling_network_error — read self._app repeatedly across await points; a concurrent disconnect() setting self._app = None mid-sequence swapped in None silently. The chained retry also didn't update self._polling_error_task, so the reentrancy guard went stale and each recovery path could start its own concurrent recovery for the same outage.

The fix

  • run.py: snapshot the platform-slot owner first and ignore a stale fatal notification from a superseded adapter instance (a background retry chain that lost to an already-succeeded reconnect); and claim the adapter (pop from self.adapters before awaiting disconnect()) so a concurrent notification can't double-disconnect.
  • adapter.py: capture a stable app = self._app local before the awaits and operate on it (fail fast with a RuntimeError if it was torn down, instead of an AttributeError on None.updater); and set self._polling_error_task = task for the chained retry so the reentrancy guard stays valid while recovery is in flight.

Review (this salvage)

Ran our review workflow — a concurrency/race pass, a correctness/scope pass, and my own trace. Clean, no findings.

  • Race analysis: no await sits between the pop and await disconnect() (no dropped-delivery window; strictly better than the old ordering, which kept a half-disconnected adapter routable for the whole disconnect). The stale-notification early-return uses an identity check (existing is not adapter) so it only suppresses superseded instances, never a legitimate same-object error. After app = self._app, there are no residual self._app reads before start_polling() in the method. The _polling_error_task handoff forms a proper chain — assigned synchronously with task creation (no yield between), cleared on teardown.
  • RuntimeError is caught: the new raise RuntimeError(...) before start_polling() is caught by the method's own except Exception handler, which re-enters the retry ladder — it never propagates uncaught to the direct-await or background-task callers.
  • Scope: clean — 2 code files + 2 test files, all on the reconnect-race fix.

Tests

pytest tests/gateway/test_runner_fatal_adapter.py tests/gateway/test_telegram_network_reconnect.py -q
27 passed

New tests cover: two concurrent fatal notifications → exactly one disconnect(); a stale notification from a superseded adapter is ignored (no disconnect of the new adapter, no re-queue, no shutdown); and the chained retry updating _polling_error_task. Adjacent gateway suites (delivery, runner-startup, notice-delivery) also pass — no regressions.

Supersedes #56036 and #56028 (@AlexFucuson9's narrower None-guard for the same #55992 symptom — this PR fixes the underlying races that produce the NoneType in the first place). Full credit to @JoaoMarcos44.

_handle_polling_network_error's chained retry never updated
self._polling_error_task, so the reentrancy guard shared with the
heartbeat loop and the pending-updates probe went stale mid-recovery,
letting more than one recovery attempt run concurrently against the
same adapter. Combined with a TOCTOU window in
_handle_adapter_fatal_error (the adapter was only removed from
self.adapters in a finally block after awaiting disconnect()), two
concurrent fatal notifications for the same adapter could both pass
the "still installed" check and call disconnect() twice, which is
where the reported "'NoneType' object has no attribute 'updater'"
originates once self._app is cleared by the first call.

- Reassign the chained retry task to self._polling_error_task so the
  guard reflects an in-flight recovery.
- Capture self._app in a local variable across the stop/start_polling
  sequence instead of re-reading self._app between awaits.
- Claim (pop) the adapter from self.adapters before awaiting
  disconnect() in _handle_adapter_fatal_error, not after, closing the
  TOCTOU window for a concurrent notification on the same adapter.
…adapters

A delayed fatal-error notification from an adapter instance that has
already been replaced by a successful reconnect (a different adapter
object now owns the platform slot) was still processed: it overwrote
the platform's runtime status back to retrying/fatal and could
re-queue an already-healthy platform for reconnection.

Snapshot the current owner of the platform slot at the top of
_handle_adapter_fatal_error and bail out before any side effect when
it belongs to a different, already-installed adapter.
@kshitijk4poor

Copy link
Copy Markdown
Collaborator Author

Ran the full hermes-pr-review Phase 2c (concurrency/race + 4-part structured review + hermes-agent-dev checks). 0 Critical, no actionable findings — both minor warnings (the _polling_error_task assignment ordering and the reconnect RuntimeError re-entry) were confirmed functionally safe: the assignment has no await gap, and the RuntimeError re-entry is bounded by the existing MAX_NETWORK_RETRIES (10) ladder that marks retryable-fatal, so no spin. No follow-up commit needed; carried as salvaged. 27 tests pass.

@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 platform/telegram Telegram bot adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 1, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.
Salvage of #56036 (@JoaoMarcos44). Note the canonical, most-comprehensive fix for #55992 already merged via #56200 (it adds the 409-conflict path guard this one lacks), so this rebased salvage is likely a close candidate. Related to #56036 / #56200 / #55992; not a duplicate.

@kshitijk4poor

Copy link
Copy Markdown
Collaborator Author

Both commits from this PR are already on main (rebase-merge landed them as a682091 close reconnect races that leave adapter half-destroyed and fb8efbb ignore stale fatal-error notifications from superseded adapters, authorship preserved). The PR record didn't flip to merged after the rebase re-attempt (GitHub detected the commits as already-present) — closing since there's nothing left to apply. Thanks @JoaoMarcos44!

@kshitijk4poor

Copy link
Copy Markdown
Collaborator Author

Closing — @JoaoMarcos44's #56036 fix has already landed on main (both the gateway/run.py TOCTOU fix and the adapter app-capture + _polling_error_task guard), and teknium1's 43edbae extended the same pattern to the 409-conflict retry path. Rebasing this branch onto current main yields an empty diff. Full credit to @JoaoMarcos44 — the fix is in.

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 platform/telegram Telegram bot adapter 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.

3 participants