Skip to content

fix(yuanbao): anchor the reconnect task and drain background tasks on disconnect - #83418

Open
briandevans wants to merge 5 commits into
NousResearch:mainfrom
briandevans:fix/yuanbao-reconnect-anchor-background-drain
Open

fix(yuanbao): anchor the reconnect task and drain background tasks on disconnect#83418
briandevans wants to merge 5 commits into
NousResearch:mainfrom
briandevans:fix/yuanbao-reconnect-anchor-background-drain

Conversation

@briandevans

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes two defects in the Yuanbao adapter's fire-and-forget task lifecycle. Both leave a task unowned at a lifecycle boundary, and the second one is created by fixing the first — which is why they belong in one PR.

Symptom: the bot goes silently offline and never comes back. ConnectionManager.schedule_reconnect launched the backoff loop with a bare asyncio.create_task(...) and dropped the handle:

def schedule_reconnect(self) -> None:
    """Schedule a reconnect only if running and not already reconnecting."""
    if self._adapter._running and not self._reconnecting:
        asyncio.create_task(self._reconnect_with_backoff())

The event loop keeps only a weak reference to a task nobody else holds, so the still-pending reconnect can be garbage-collected mid-flight. When that happens there is no retry left to fire — the retry was the task that vanished — and the adapter stays down until the gateway is restarted. _reconnect_with_backoff sleeps up to 60s between attempts, so the window in which the task is pending and unreferenced is a long one.

This is the repo's own stated invariant, in two places:

  • gateway/platforms/yuanbao.py_track_task's docstring: "Register a fire-and-forget task so it won't be GC'd prematurely."
  • gateway/run.py"a bare asyncio.create_task() keeps only a weak reference, so the event loop may garbage-collect a still-pending task mid-flight."

And the correct idiom is already used in this same file, 40 lines above at the _flush_inbound_buffer dispatch:

adapter._track_task(asyncio.create_task(
    adapter._inbound_pipeline.execute(ctx),
    name=f"yuanbao-pipeline-{key}",
))

The second half — teardown. YuanbaoAdapter.disconnect() is documented as "Cancel background tasks and close the WebSocket connection", but it only ever cancelled _inbound_tasks. Everything registered through _track_task lands in _background_tasks, and that set had no teardown path at all — its only removal path is the per-task done callback. So the inbound pipeline and the recall-redaction job were abandoned mid-flight at shutdown, with their except/finally cleanup never running.

That also closes the ordering hole the first fix would otherwise open: once the reconnect is anchored into _background_tasks, an in-flight one could outlive disconnect() and go on trying to revive an adapter that was deliberately stopped. disconnect() clears _running early, so the schedule_reconnect guard covers new schedules — it does nothing for a reconnect already past that check. Fixing the GC bug alone would trade it for a shutdown-ordering bug.

Cancel-then-gather mirrors ConnectionManager.close(), which already cancels and awaits _heartbeat_task / _recv_task. Three details worth calling out:

  • the set is snapshotted before cancelling, because cancelling mutates it through that same done callback;
  • return_exceptions=True stops one task raising from its cleanup path from aborting teardown for the rest;
  • asyncio.current_task() is excluded, so a shutdown driven from inside a tracked task cannot await itself.

_inbound_tasks handling is left byte-identical.

Sibling-site sweep

Every task-creation site in gateway/platforms/yuanbao.py was enumerated (create_task / _track_task / _background_tasks), and schedule_reconnect is the only one missing a strong reference. The rest are already anchored and are deliberately untouched:

Site Owner Verdict
:1160, :1825, :3602 added to _background_tasks / _track_task already anchored
:3002, :3009 added to _inbound_tasks already anchored
:3212, :3215, :3701, :3708 assigned to _heartbeat_task / _recv_task attribute = strong ref
:4225, :4305 stored in _reply_heartbeat_tasks[chat_id] / _tasks[chat_id] dict = strong ref
:3644 schedule_reconnect nothing the defect

The same construct appears in other platform adapters (weixin.py, qqbot/adapter.py, bluebubbles.py) and in gateway/run.py. Those are deliberately out of scope here — each already has an open PR against it (#65966/#26049 for weixin, #38848/#74775 for qqbot, #18395 for bluebubbles, #45372 for run.py), and this PR stays inside one file and one adapter's lifecycle.

Duplicate check

Searched open PRs for schedule_reconnect, yuanbao reconnect, yuanbao disconnect, yuanbao background tasks, yuanbao GC task, and _track_task: no open PR touches Yuanbao's reconnect or disconnect path. The _track_task / _background_tasks matches are all other files — weixin.py (#65966, #26049), qqbot/adapter.py (#41298), gateway/run.py (#17966, #6790, #45372), gateway/platforms/base.py (#29216, #43903) — verified by reading each one's changed-file list. The nearest open PRs that do touch gateway/platforms/yuanbao.py sit in unrelated regions (#74860 around _sender_may_designate_home, #65112, #29077, #62031, #78166, #55782), none within schedule_reconnect or disconnect().

Related Issue

No filed issue — found by auditing task lifecycle at shutdown/reconnect boundaries in gateway/platforms/yuanbao.py.

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/platforms/yuanbao.pyConnectionManager.schedule_reconnect routes its task through self._adapter._track_task(...) and names it yuanbao-reconnect. The _running / _reconnecting guards are unchanged; only ownership of the reference changes.
  • gateway/platforms/yuanbao.pyYuanbaoAdapter.disconnect cancels and awaits the tasks in _background_tasks alongside the existing _inbound_tasks cancellation, then clears the set.
  • tests/test_yuanbao_task_lifecycle.py — new; 7 regression tests.

How to Test

  1. Run the new tests: pytest tests/test_yuanbao_task_lifecycle.py -q → 7 passed.
  2. Confirm they actually pin the fix. Revert only the schedule_reconnect hunk to its previous form and re-run: test_schedule_reconnect_anchors_task_against_gc fails with assert 0 == 1 — the task is not in _background_tasks. Nothing else fails.
  3. Revert only the disconnect() hunk and re-run: test_disconnect_drains_in_flight_background_tasks and test_disconnect_survives_a_background_task_that_raises fail, and pytest additionally logs unhandled exception during asyncio.run() shutdown for the abandoned task. Nothing else fails.
  4. Adjacent suites, all green: pytest tests/test_yuanbao_task_lifecycle.py tests/test_yuanbao_shutdown.py tests/test_yuanbao_reconnect_set_active.py tests/test_yuanbao_pipeline.py tests/test_yuanbao_integration.py tests/test_yuanbao_proto.py tests/test_yuanbao_markdown.py tests/gateway/test_yuanbao_forwarded_heartbeat.py tests/gateway/test_yuanbao_media_ssrf.py tests/gateway/platforms/test_yuanbao_recall_db_only.py tests/gateway/platforms/test_yuanbao_state_cleanup.py -q → 147 passed.

Each of the four commits was also checked out on its own and its touched suite run, so the history bisects cleanly.

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 ran the full Yuanbao surface plus the reconnect/shutdown suites (147 tests, listed above), not the whole repo suite
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (Darwin 25.4), Python 3.11.15

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A (disconnect()'s existing docstring already described the behaviour this PR makes true)
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — pure asyncio task bookkeeping, no platform-specific behaviour
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

`ConnectionManager.schedule_reconnect` launched the backoff loop with a bare
`asyncio.create_task(...)` and dropped the handle. The event loop holds only a
weak reference to a task nobody else references, so a still-pending reconnect
can be garbage-collected mid-flight. When that happens the Yuanbao adapter
never reconnects and the bot is silently offline until the gateway is
restarted — there is no retry left to fire, because the retry *was* the task
that vanished.

`YuanbaoAdapter._track_task` exists for exactly this case ("Register a
fire-and-forget task so it won't be GC'd prematurely") and is already used for
the inbound pipeline 40 lines above, at the `_flush_inbound_buffer` dispatch.
Route the reconnect through it and give the task a `name=`, matching every
other named task in this file.

The `_running` / `_reconnecting` guards are unchanged: this only fixes who
holds the reference, not when a reconnect is scheduled.
Asserts that a scheduled reconnect is reachable from the adapter's
`_background_tasks` set, which is what stops the event loop from collecting it
while it is still pending. The test fails against a bare `asyncio.create_task`
(the set stays empty) and passes once the task is routed through
`_track_task`.

Also pins the two guards the fix runs through, so a later refactor cannot
quietly start scheduling reconnects on a stopped adapter or on top of an
in-progress one.
`disconnect()` is documented as "Cancel background tasks and close the
WebSocket connection", but it only ever cancelled `_inbound_tasks`. Everything
registered through `_track_task` lands in `_background_tasks` — the inbound
pipeline, the recall-redaction job, and the reconnect — and that set has no
teardown path at all: its only removal path is the per-task done callback. So
every one of those jobs was abandoned mid-flight when the adapter shut down,
with its `except`/`finally` cleanup never running.

It also closes the ordering hole the anchoring fix would otherwise open. Now
that a reconnect is held in `_background_tasks`, an in-flight one could
outlive `disconnect()` and go on trying to revive an adapter that was
deliberately stopped. `disconnect()` clears `_running` early, so the
`schedule_reconnect` guard covers *new* schedules — it does nothing for a
reconnect already past that check.

Cancel-then-gather mirrors `ConnectionManager.close()`, which already cancels
and awaits `_heartbeat_task` / `_recv_task`. Details:

- The set is snapshotted first, because cancelling mutates it through the same
  done callback.
- `return_exceptions=True` keeps one task raising from its cleanup path from
  aborting teardown for the rest.
- `asyncio.current_task()` is excluded so a shutdown driven from inside a
  tracked task cannot await itself.

`_inbound_tasks` handling is left byte-identical.
Four cases against the teardown path:

- an in-flight `_track_task` job is cancelled *and* awaited by `disconnect()`,
  proven by asserting its `except asyncio.CancelledError` arm actually ran
  before `disconnect()` returned — a cancel without the await would leave the
  task not yet unwound;
- a task whose cleanup raises does not abort teardown for the others;
- a shutdown driven from inside a tracked task completes instead of awaiting
  itself;
- the pre-existing `_inbound_tasks` cancellation is unchanged.

The first two fail against the old `disconnect()`, which left
`_background_tasks` untouched.
Copilot AI lite review requested due to automatic review settings August 10, 2026 19:45

Copilot AI 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.

Pull request overview

This PR fixes background task lifecycle handling in the Yuanbao gateway adapter by ensuring reconnect tasks are strongly referenced (preventing GC mid-flight) and by draining tracked fire-and-forget tasks during adapter disconnect, with regression tests added to lock in the behavior.

Changes:

  • Anchor ConnectionManager.schedule_reconnect()’s backoff loop task via YuanbaoAdapter._track_task(...) (named yuanbao-reconnect) to prevent premature garbage collection.
  • Extend YuanbaoAdapter.disconnect() to cancel and await in-flight _background_tasks (tracked via _track_task) so teardown doesn’t leave tasks running post-disconnect.
  • Add a new focused test suite validating reconnect anchoring and disconnect task draining semantics.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
gateway/platforms/yuanbao.py Tracks reconnect backoff task via _track_task and drains _background_tasks during disconnect().
tests/test_yuanbao_task_lifecycle.py Adds regression coverage for task anchoring and disconnect draining behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +5083 to +5087
current = asyncio.current_task()
background = [
task for task in self._background_tasks
if task is not current and not task.done()
]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The snapshot is the list(...) call:

background = [
    task for task in list(self._background_tasks)
    if task is not current and not task.done()
]

list() materialises a copy of the set before the comprehension starts walking it, so the sequence being iterated is that copy, not _background_tasks. The _track_task done-callback can only discard() from the underlying set, which is no longer what the loop is reading.

The same holds for both later uses — the for task in background cancel loop and asyncio.gather(*background, return_exceptions=True) — since both take the list. That gather is precisely where the callbacks fire (it suspends while the cancellations land), and it cannot raise RuntimeError: Set changed size during iteration for the same reason. The trailing self._background_tasks.clear() is what reconciles the set afterwards.

tests/test_yuanbao_task_lifecycle.py::test_disconnect_drains_in_flight_background_tasks and ::test_disconnect_survives_a_background_task_that_raises both exercise this path with callbacks firing mid-drain and assert _background_tasks == set() on the way out.

The drain built its work list straight off `self._background_tasks` while the
comment above it claimed to snapshot first. The comprehension is in fact a
snapshot and cannot be interrupted — asyncio schedules done callbacks through
`loop.call_soon`, so none of them can run partway through synchronous code —
but the code did not say so, and the `_inbound_tasks` loop directly above
already spells the snapshot out as `list(...)`.

Make it explicit and match that neighbour, and correct the comment to state
the real reason the snapshot matters: the done callbacks mutate
`_background_tasks` while the `gather` is suspended.

No behaviour change.
@briandevans

Copy link
Copy Markdown
Contributor Author

@copilot Addressed in c629a7ccd90 (current head of this branch).

  • disconnect() snapshot — the work list is now built from list(self._background_tasks), and the comment above it has been corrected. On the RuntimeError itself: it is not reachable as written. Future.__schedule_callbacks dispatches every done callback through loop.call_soon, so _background_tasks.discard cannot run partway through a synchronous comprehension on a single-threaded loop. What the comment got wrong was the reason the snapshot matters — it is the await asyncio.gather(...) two lines below that suspends and lets those callbacks mutate the set, not the iteration. The explicit list(...) also matches the for task in list(self._inbound_tasks) loop immediately above it, so the two teardown blocks now read the same way.

No behaviour change, and the drain is still covered by test_disconnect_drains_in_flight_background_tasks, test_disconnect_survives_a_background_task_that_raises, and test_disconnect_from_inside_a_tracked_task_does_not_await_itself in tests/test_yuanbao_task_lifecycle.py (lines 100, 152 and 186).

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 10, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(yuanbao): anchor the reconnect task and drain background tasks on disconnect

  1. gateway/platforms/yuanbao.py disconnect(): the drain cancels/awaits a snapshot of _background_tasks and then calls self._background_tasks.clear(). Any task tracked during the drain (e.g. a cancellation handler that schedules a new reconnect) lands in the set after the snapshot and is dropped by clear() without being cancelled — a live task could outlive teardown. Consider looping until the set is empty after the gather, or asserting it is.
  2. await asyncio.gather(*background, return_exceptions=True) has no timeout. If a tracked task swallows CancelledError (a reconnect loop with a broad except, for instance), disconnect() blocks forever. Worth confirming _reconnect_with_backoff is cancellation-safe, or bounding the gather with asyncio.wait(..., timeout=…).
  3. The current_task() exclusion and the not task.done() filter in the snapshot read correctly, and test_disconnect_from_inside_a_tracked_task_does_not_await_itself covers the self-shutdown case well. No issue there.

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.

4 participants