Skip to content

fix(gateway): run adapter connect detach-on-timeout so a swallowed cancel can't wedge reconnect - #70345

Closed
VaitaR wants to merge 1 commit into
NousResearch:mainfrom
VaitaR:fix/connect-timeout-detach
Closed

fix(gateway): run adapter connect detach-on-timeout so a swallowed cancel can't wedge reconnect#70345
VaitaR wants to merge 1 commit into
NousResearch:mainfrom
VaitaR:fix/connect-timeout-detach

Conversation

@VaitaR

@VaitaR VaitaR commented Jul 23, 2026

Copy link
Copy Markdown

What does this PR do?

Hardens GatewayRunner._connect_adapter_with_timeout against a connect() that
swallows CancelledError.

Every other bounded teardown/cleanup await in the gateway goes through
_await_adapter_cleanup_with_timeout (gateway/run.py:3796), which runs the child as a
detached task so that when the deadline fires the runner is released even if the child
traps its cancellation
. The connect path was the one exception: it used plain
asyncio.wait_for(adapter.connect(...)). wait_for cancels an overdue child and then
awaits it — so a connect() whose teardown swallows CancelledError would keep the
background reconnect watcher blocked indefinitely, leaving the gateway alive but never
rebuilding a deaf adapter.

This converts connect to the same detach-on-timeout pattern: on deadline the connect task
is cancelled and detached (drained via consume_detached_task_result), and the call
raises TimeoutError exactly as before, so the watcher's existing failure/backoff path is
unchanged.

Honesty note: this was found while investigating a ~22h silent-deaf wedge on 0.19.0
(issue linked below). That incident's freeze happened before the reconnect watcher ever
ran, so this is not a proven fix for it — it closes a real, separate
plain-wait_for inconsistency (defense-in-depth). The incident itself needs a stack dump
to localize, as described in the issue.

Related Issue

Fixes #70344

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • gateway/run.py_connect_adapter_with_timeout now runs adapter.connect() as a
    detached task via asyncio.wait({task}, timeout=...), returning task.result() on
    success (same result/exception surface as the old await wait_for) and, on deadline,
    cancelling + detaching the task and raising TimeoutError. A timed-out connect is never
    installed in self.adapters, so a late-completing swallowed-cancel connect stays
    unowned and is disposed by the existing failure path rather than leaving a
    half-registered adapter (the race class addressed by Telegram polling silently dies after network error + self-restart #55992/fix(telegram): close reconnect races that leave the adapter half-destroyed (#55992) #56200).
  • tests/gateway/test_platform_reconnect.py — regression test
    test_connect_timeout_releases_when_connect_swallows_cancel: a connect() that traps
    CancelledError and keeps running. It hangs the old plain-wait_for implementation
    (verified: the 2s outer bound trips with a bare asyncio.TimeoutError) and returns
    promptly with the bounded "... connect timed out" error after this change.

How to Test

  1. Check out this branch.
  2. pytest tests/gateway/test_platform_reconnect.py -q — 39 passed.
  3. To confirm it's a real regression test: git stash the gateway/run.py change and
    re-run just test_connect_timeout_releases_when_connect_swallows_cancel — it fails
    (inner call hangs, outer 2s guard trips); restore and it passes.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(gateway): ...)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run pytest tests/ -q and all tests pass — ran the affected gateway suite
    (tests/gateway/test_platform_reconnect.py, 39 passed); did not run the full
    tests/ (heavy optional deps, e.g. torch, not installed in my env)
  • I've added tests for my changes
  • I've tested on my platform: macOS 15 (Darwin 25.5), Python 3.13

Documentation & Housekeeping

  • Docstring updated on the changed method — no other docs affected — N/A elsewhere
  • No config keys changed — N/A
  • No architecture/workflow change — N/A
  • Cross-platform: pure asyncio, no platform-specific primitives — N/A
  • No tool descriptions/schemas changed — N/A

…ncel can't wedge reconnect

_connect_adapter_with_timeout used a plain asyncio.wait_for(adapter.connect()),
which cancels an overdue child and then awaits it. A connect() whose teardown
swallows CancelledError would keep the background reconnect watcher blocked
indefinitely, leaving the gateway alive but never rebuilding a deaf adapter.

Run connect as a detached task (the pattern already used by
_await_adapter_cleanup_with_timeout): on deadline the task is cancelled and
drained via consume_detached_task_result and TimeoutError is raised, so the
watcher's existing failure/backoff path is unchanged. A timed-out connect is
never installed in self.adapters, so a late-completing swallowed-cancel connect
stays unowned and is disposed rather than leaving a half-registered adapter.
@VaitaR
VaitaR force-pushed the fix/connect-timeout-detach branch from cdcaae3 to 9cc85ae Compare July 23, 2026 22:32
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 23, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #70344: the live diff hardens a swallowed-cancellation connect timeout, while the reported incident froze before the reconnect watcher started. This is not yet a confirmed fix for that incident.

@egilewski

Copy link
Copy Markdown
Contributor

suggesting changes

The detach-on-timeout path releases the watcher, but it also drops lifecycle
ownership of a connect() that is still running. After
_connect_adapter_with_timeout cancels and detaches the task, each timeout
caller immediately runs _dispose_unused_adapter or
_safe_adapter_disconnect. If connect() swallows cancellation, that cleanup
can finish first and the detached connect can then finish successfully,
reactivating polling/socket resources on an adapter no registry owns.
consume_detached_task_result only observes the result; it does not clean up a
late success.

On a clean replay onto current main, a lifecycle adapter that waits after
catching CancelledError produced this sequence for both cleanup paths:

TIMEOUT=telegram connect timed out after 0.01s
AFTER_DISCONNECT=active:False,disconnect_calls:1
AFTER_LATE_CONNECT=active:True,disconnect_calls:1

Current main hangs in the corresponding anti-wedge probe, so the patch does
fix that failure mode. The focused file also passes all 39 tests on the replay,
but the new regression checks registry ownership before releasing the detached
task and never asserts the adapter's state after late completion.

Security evidence:

  • trust boundary: the timeout returns control to the gateway's registry and
    cleanup code while platform-controlled connect() code may still be running
    and mutating network resources.
  • source/sink/invariant: the source is a connect() that catches
    CancelledError; the sink is late polling/socket activation after the
    caller's one-time cleanup. A timed-out, unowned adapter must not become active
    again after disposal.
  • current-main reproduction: the swallowed-cancellation anti-wedge probe did
    not return on current main and hit its 3-second process bound, reproducing
    the existing wedge.
  • PR-head or patch-replay validation: the patch returns the documented
    telegram connect timed out after 0.01s error on both the PR head and a clean
    replay onto current main, but the replay also reproduced late reactivation
    after cleanup.
  • positive/negative cases: the focused gateway reconnect file passes all 39
    tests on the PR head and current-main replay; both primary
    _dispose_unused_adapter and secondary/startup _safe_adapter_disconnect
    paths changed from inactive after cleanup to active after the detached
    connect completed.
  • residual bypass search: all three production caller families perform only a
    one-time cleanup after the helper raises, and no completion-triggered owner or
    final disposal handles a late successful connect.
  • reviewer validation: a lifecycle adapter that catches cancellation, waits,
    and then activates deterministically produced
    active:False,disconnect_calls:1 after cleanup followed by
    active:True,disconnect_calls:1 after late completion.

Please retain ownership of the timed-out connect long enough to arrange a final
cleanup when it eventually completes (or enforce an equivalent close/generation
invariant), and extend the regression to release the child after the caller's
first cleanup and assert that the adapter remains inactive and receives the
required final disposal.

Signed: GPT-5.6-sol-xhigh in Codex

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Merged via #71176 — your commit was cherry-picked with authorship preserved (rebase-merge). Thanks for the clean single-concern fix!

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 P2 Medium — degraded but workaround exists 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 silently deaf after generic network loss — reconnect watcher never starts post-escalation (0.19.0)

4 participants