Conversation
After reaching the maximum number of reconnection attempts, instead of calling _mark_disconnected() to exit silently, set a retryable fatal error and schedule a notification task. This leaves takeover to the gateway reconnection monitor, preventing platform hang caused by unexpected listener task termination.
…ection attempts are exhausted.
alt-glitch
added
type/bug
Related: #17814 handles the same exhaustion paths but awaits fatal notification in the listener; this PR schedules it separately to avoid disconnect cancellation. #19414 is a broader repair. Please choose or consolidate the lifecycle behavior; this is not a duplicate. |
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for covering the three QQBot reconnect-exhaustion exits. The silent-exit premise is present on current main at gateway/platforms/qqbot/adapter.py:590-592, 644-647, and 656-659.
Problems
- The sibling retryable fatal at
gateway/platforms/qqbot/adapter.py:539-544(qq_quick_disconnect) still returns without notifying the gateway. It can leave the same retryable platform unqueued. - The new tests mock
_notify_fatal_errorattests/gateway/test_qqbot.py:2290, so they do not exercise the real runner handoff or assert QQBot reaches_failed_platforms. CurrentGatewayRunner._handle_adapter_fatal_error()already detaches and shields this lifecycle atgateway/run.py:6843-6968(commit2ab153218).
Suggested changes
- Apply the handoff to the quick-disconnect terminal branch and cover it.
- Add a QQBot +
GatewayRunnerasync regression test asserting teardown completes and QQBot is queued for reconnect, analogous totests/gateway/test_telegram_network_reconnect.py:103-133.
Automated hermes-sweeper review.
| task (a harmless no-op), and the handler runs to completion and | ||
| enqueues us for the reconnect watcher. | ||
| """ | ||
| self._fatal_notify_task = asyncio.create_task(self._notify_fatal_error()) |
There was a problem hiding this comment.
Please apply this handoff to the existing retryable qq_quick_disconnect return at current-main adapter.py:539-544 as well; that branch sets a retryable fatal error but does not notify the runner, so it can still strand QQBot outside _failed_platforms.
| adapter._fail_pending = mock.Mock() | ||
| adapter._mark_disconnected = mock.Mock() | ||
| adapter._set_fatal_error = mock.Mock() | ||
| adapter._notify_fatal_error = mock.AsyncMock() |
There was a problem hiding this comment.
These mocks prove only local scheduling. Add a regression that binds a real GatewayRunner._handle_adapter_fatal_error, triggers the listener exhaustion, and asserts QQBot enters runner._failed_platforms; that is the cancellation-sensitive behavior this PR is intended to preserve.
teknium1
added
the
sweeper:blast-moderate
The quick-disconnect branch in `_listen_loop` called `_set_fatal_error` and returned without invoking `_schedule_fatal_notify`, so the retryable fatal never reached the gateway's reconnect watcher and the bot died silently on permission/config errors. Schedule the notify so all four exhaustion exits hand off to the gateway consistently, and add adapter- and runner-level regression tests covering the quick-disconnect exit.
|
Both addressed.
One note: test_telegram_network_reconnect.py:103-133 is adapter-level and doesn't spin up a GatewayRunner, so I patterned on test_runner_fatal_adapter.py:98-130 instead as the closer analogue. Happy to restructure if you'd prefer. |
|
Hi @cadezhou — hat tip first: you diagnosed this silent-death root cause a month before I did, and the core mechanism we both landed on (detached fatal-notify so the gateway watcher takes over, avoiding the Technical deltas I found while comparing (all verifiable in your diff):
#97857 covers all of the above and has absorbed your richer |
|
One correction to my comparison above, in fairness: alongside the mocked unit tests, this PR does include a mock-free end-to-end handoff test ( |
…oing zombie
When _listen_loop exhausts MAX_RECONNECT_ATTEMPTS (e.g. a network/DNS
outage longer than the backoff sequence), it called
_mark_disconnected() and returned silently. No fatal error was set, so
the gateway never learned the adapter died: the zombie stayed installed
in self.adapters with is_connected=False forever. Inbound events were
gone (no WebSocket), every send waited 15s in _wait_for_reconnection()
then failed with "Not connected", and the _failed_platforms watcher
never picked it up because it only covers platforms that failed at
startup — only a gateway restart could recover the platform.
The same silent exit applied to the quick-disconnect and fatal
close-code branches, which did call _set_fatal_error() but never
notified the gateway (nothing polls installed adapters post-startup),
and to the rate-limit max-attempts exit.
Now every terminal listen-loop exit goes through _give_up_and_notify():
set an appropriate fatal error, mark disconnected, and schedule
_notify_fatal_error() on a detached task, so the gateway's
_handle_adapter_fatal_error disconnects the zombie and requeues the
platform for background reconnection (retryable) or marks it fatal
(non-retryable). The notification must be detached because the gateway
handler calls adapter.disconnect(), which awaits self._listen_task —
awaiting the listen task from inside itself would deadlock. The
detached wrapper swallows handler exceptions (best-effort notify) so a
gateway-side bug cannot leave an unawaited task error.
Two secondary fixes folded in:
- The quick-disconnect exit returns before the shared
_fail_pending("Connection closed") — it now fails pending response
futures itself, matching the sibling-adapter teardown contract (defensive
parity: nothing populates QQ's pending-response dict today, but if a
correlation mechanism is added, a handler-less standalone adapter would
otherwise leave futures unsettled).
- Fatal close-code error codes are slugified (qq_invalid_opcode,
qq_offline_sandbox_only, qq_invalid_api_version) since they now persist in
runtime-status
error_code fields; the human-readable message is unchanged.
Regression tests cover the max-reconnect exit (retryable, notified, no
deadlock when the handler disconnects the adapter), the fatal
close-code exit (non-retryable, notified), the quick-disconnect exit
(pending futures settled), and handler-exception swallowing.
Error-code context (last close code / trailing exception / rate-limit
marker) in the qq_max_reconnect messages absorbed from NousResearch#72673 by
@cadezhou — credit to that PR for the richer diagnostics; it independently
diagnosed the same silent-death root cause a month earlier.
The mock-free GatewayRunner handoff e2e test
(TestQQBotGatewayHandoffEndToEnd) is adapted from NousResearch#72673 by @cadezhou with
error codes adjusted — it proves a retryable fatal reaches the real
_handle_adapter_fatal_error and lands in _failed_platforms with attempts=0,
complementing the adapter-side tests which stop at the handler contract.
4 participants
The QQ (
qqbot) adapter died silently once its WebSocket reconnect attempts were exhausted (it just called_mark_disconnected()), leaving the listener task dead with nothing watching it — the platform never recovered.This PR makes all three exhaustion paths in
_listen_loop(rate-limited 4008,QQCloseErrorbackoff, genericException):qq_reconnect_exhaustedfatal error (retryable=True) so the gateway reconnect watcher takes over;_schedule_fatal_notify()instead of awaiting inline — the notify chain callsdisconnect(), which cancels the running_listen_task, so an inline await would abort the gateway handler before it re-enqueues the adapter.Tests
TestReconnectExhaustionHandoffcovering all three paths, asserting a retryableqq_reconnect_exhaustedis set,_mark_disconnected()is no longer called, and the notify is scheduled as a detached task.tests/gateway/test_qqbot.pypasses 169/169.Closes #29005