fix(gateway): store asyncio.create_task refs to prevent GC of background watchers - #65417
fix(gateway): store asyncio.create_task refs to prevent GC of background watchers#65417x7peeps wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the unretained watcher-task pattern. The ownership needs a separate lifecycle lane rather than reusing _background_tasks.
Problems
gateway/run.py:21229adds the signal-handler task that awaitsrunner.stop()to_background_tasks. The shutdown loop cancels tracked tasks atgateway/run.py:8427-8437; this wrapper is neither_stop_tasknor_restart_task. Commit1ce5d6d97documents that cancelling a task awaiting_stop_taskpropagatesCancelledErrorinto_stop_impland skips cleanup.- Adding permanent watcher loops to
_background_tasksalso blocks scale-to-zero:gateway/run.py:4208treats any unfinished tracked task as active work, while_session_expiry_watcherruns forself._running(gateway/run.py:7766-7779).
Suggested changes
- Keep the signal-handler stop wrapper out of
_background_tasks. - Use a distinct strong-reference collection for service watchers, with explicit shutdown handling, and add regressions for signal shutdown and scale-to-zero idle detection.
Automated hermes-sweeper review.
| # Start background session expiry watcher to finalize expired sessions | ||
| asyncio.create_task(self._session_expiry_watcher()) | ||
| task = asyncio.create_task(self._session_expiry_watcher()) | ||
| self._background_tasks.add(task) |
There was a problem hiding this comment.
_background_tasks is also the scale-to-zero active-work signal (gateway/run.py:4208). This watcher runs for the lifetime of self._running, so tracking it here makes an otherwise idle scale-to-zero gateway permanently non-idle. Please use a separate strong-reference collection for service loops, or exclude those loops from that predicate.
| logger.debug("spawn_async_diagnostic failed: %s", _e) | ||
| asyncio.create_task(runner.stop()) | ||
| task = asyncio.create_task(runner.stop()) | ||
| runner._background_tasks.add(task) |
There was a problem hiding this comment.
Do not add the outer runner.stop() task to this cancellation set. _stop_impl cancels every member except _stop_task and _restart_task (gateway/run.py:8427-8437), so this task is cancelled while awaiting _stop_task; commit 1ce5d6d97 documents that this propagation aborts shutdown cleanup.
Related to #45372 and #6790. This direct-registration implementation overlaps their watcher-task retention work, but also covers additional active watcher and shutdown call sites. Please consolidate the competing implementations rather than treating either as a duplicate. |
|
Closed as duplicate per maintainer feedback. |
Summary
Register 11 fire-and-forget
asyncio.create_task()calls ingateway/run.pywith the existing_background_tasksset to prevent garbage collection of critical background watchers.Problem
asyncio.create_task()only keeps a weak reference to the created task. If no strong reference is held, the task can be garbage-collected before it completes, causing the coroutine to silently disappear.In
gateway/run.py, 11 background watchers were created withasyncio.create_task()but never stored anywhere:_run_process_watcher(crash-recovery + mid-run batch, 2 sites)_session_expiry_watcher_kanban_notifier_watcher_kanban_dispatcher_watcher_platform_reconnect_watcher_handoff_watcher_async_delegation_watcher_scale_to_zero_watcher_drain_control_watcherrunner.stop()in signal handlerAny of these being GC'd would cause the corresponding gateway functionality to silently stop working with no error logged.
Fix
Use the existing
_background_tasksset (already present at line 3177) with the established pattern: