Repository navigation
fix(gateway): keep a strong reference to the SIGTERM/SIGINT shutdown task - #83864
Open
briandevans wants to merge 7 commits into
Open
briandevans wants to merge 7 commits into
briandevans wants to merge 7 commits into
Conversation
`shutdown_signal_handler` ended with a bare `asyncio.create_task(runner.stop())` and discarded the handle. The event loop keeps only a weak reference to a task, so a still-pending task can be garbage-collected mid-flight. This file already states that hazard, and already fixes it — on the other signal path. `request_restart()` (SIGUSR1) creates a task with the same shape and deliberately keeps it in `self._restart_task`, with the reason written out inline: "a bare asyncio.create_task() keeps only a weak reference, so the event loop may garbage-collect a still-pending task mid-flight." The path that every `systemctl stop`, `hermes gateway stop` and interactive Ctrl+C takes was left bare. The damaging window is precise: `stop()` only becomes self-anchoring once it reaches `self._stop_task = asyncio.create_task(_stop_impl())`. A collection before that point means `_stop_impl` is never created at all, so the gateway does not tear down — it keeps running until systemd's TimeoutStopSec escalates to SIGKILL, with in-flight turns undrained and sessions never finalized. A collection after that point is harmless, because the inner task is strongly referenced by `self._stop_task`. Adds `_shutdown_task` alongside `_stop_task` / `_restart_task` at both existing declaration sites (class annotation and `__init__`), so bare runners built via `object.__new__` in the shutdown-path tests get the same `None` default the other two have.
`shutdown_signal_handler` is not a once-only path, so assigning `runner._shutdown_task` unconditionally would let a second invocation overwrite the reference to the task that is actually performing the teardown, re-opening the collection window the anchor exists to close. Three ordinary ways it fires twice: - a second Ctrl+C while the first drain is still running; - an interactive SIGINT followed by the service manager's SIGTERM; - `_run_planned_stop_watcher`, which drives the same callable from its polling thread via `loop.call_soon_threadsafe(shutdown_handler, None)`. Its `_draining` gate does not close until `_stop_impl` is already running, and its own docstring records that on POSIX "the signal handler always races us to consuming the marker file". Re-point the anchor only when the previous task is absent or done. This is behaviour-preserving for the duplicate call: `stop()` short-circuits on `self._stop_task` and awaits the first teardown, so the second task never did work of its own — it only existed to await the first.
`_stop_impl` cancels every entry in `self._background_tasks` and skips exactly two handles: `_stop_task`, and `_restart_task` because it "is awaiting _stop_task right now; cancelling it would propagate CancelledError into this _stop_impl and skip _shutdown_event.set() / _exit_code = 75 (NousResearch#12875)". `_shutdown_task` has that same relationship — it holds the outer `runner.stop()` coroutine, which is parked in `await self._stop_task` while this loop runs. It is the reason the naive fix for the missing anchor is wrong: parking the handle in `_background_tasks` would trade a garbage-collection bug for a cancellation bug, with `_stop_impl` cancelling the very task waiting on it. Like `_restart_task`, `_shutdown_task` is deliberately kept out of `_background_tasks`, so this branch does not fire on any current path. It is recorded here because this loop is the single place that enforces the "never cancel a task awaiting `_stop_task`" invariant, and the two handles that satisfy that description should not be treated differently by it.
…mption Five regression tests, all red on the unfixed file and green after, each pinned to one of the three production hunks: - `_shutdown_task` exists as a class-level default beside `_stop_task` and `_restart_task`, so the bare runners the shutdown-path tests build via `object.__new__` inherit `None`; - `shutdown_signal_handler` contains no `asyncio.create_task(...)` whose result is discarded; - the task it creates is anchored on the runner as `_shutdown_task`; - that assignment is guarded by a `done()` check, so a repeat signal cannot replace the handle to a still-running teardown; - `_stop_impl`'s cancel sweep leaves `_shutdown_task` alone while still cancelling an ordinary background task. The last one runs against a real `stop()` on the bare-runner harness in `restart_test_helpers`. The first four are asserted by parsing `gateway/run.py` with `ast`, because `shutdown_signal_handler` is a closure built inside the gateway start path and cannot be imported — the same technique `test_adapter_connect_is_reconnect_contract.py` already uses for a contract that is likewise unreachable by import. Lives in its own file rather than extending `test_gateway_shutdown.py` so it does not collide with the other open changes in this area.
There was a problem hiding this comment.
Pull request overview
This PR hardens the gateway shutdown path by keeping a strong reference to the runner.stop() task created by the SIGINT/SIGTERM shutdown handler, mirroring the existing _restart_task pattern used for SIGUSR1 restart. This prevents a narrow but real failure window where the outer stop() task could be garbage-collected before it becomes self-anchoring, leading to hung shutdowns until forced SIGKILL.
Changes:
- Add
GatewayRunner._shutdown_taskand anchor the SIGINT/SIGTERM shutdown task to it (with adone()-guard to avoid overwriting a live shutdown). - Ensure
_stop_impl’s background-task cancel sweep skips_shutdown_task(like_stop_task/_restart_task). - Add a regression test file covering the anchoring and cancel-sweep behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
gateway/run.py |
Anchors SIGINT/SIGTERM shutdown task on the runner and exempts it from the shutdown cancel sweep. |
tests/gateway/test_shutdown_task_anchor.py |
Adds regression coverage for shutdown-task anchoring and cancel-sweep behavior (includes source-structure assertions). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+42
to
+45
| def _shutdown_signal_handler_node() -> ast.FunctionDef: | ||
| """The single ``shutdown_signal_handler`` definition in ``gateway/run.py``.""" | ||
| tree = ast.parse(RUN_PY.read_text(encoding="utf-8")) | ||
| matches = [ |
Author
There was a problem hiding this comment.
Agreed, and fixed — I checked AGENTS.md and "Never read source code in tests" is a hard ban, so this was a real defect in the PR rather than a style preference. Reading the source was a workaround for shutdown_signal_handler being a closure inside the gateway start path; the right move is to make the behaviour importable, not to assert on text.
Done in 101874c80aa (refactor) + 4a24ede20f4 (tests), current head 4a24ede20f4:
GatewayRunner._schedule_shutdown_task()now owns the create-and-anchor and the "don't re-point a live shutdown" check, and returns the task that owns the shutdown. The handler is a one-line call to it. No behaviour change.- All four source-parsing assertions are gone. The replacements call the method:
test_scheduling_shutdown_anchors_the_task_on_the_runner,test_a_repeat_signal_does_not_replace_the_in_flight_shutdown_task,test_a_later_signal_schedules_again_once_the_previous_one_finished, plus the already-behaviouraltest_stop_does_not_cancel_the_anchored_shutdown_task— all intests/gateway/test_shutdown_task_anchor.py.
Each production hunk is mutation-covered so none of these can pass against a broken implementation: returning the task without storing it fails 3 tests, dropping the in-flight guard fails the repeat-signal test, and deleting the _shutdown_task branch from _stop_impl's cancel sweep fails the sweep test.
Author
|
CI audit — the single failure on this branch is a pre-existing baseline on clean
The slice that actually runs this PR's code is 8/12, and it is green: |
…ndler
Move the anchoring and the repeat-signal guard out of the
`shutdown_signal_handler` closure and onto `GatewayRunner`, so the
behaviour is reachable from a test.
The handler is built inside the gateway start path and cannot be
imported, which had pushed its regression tests into asserting the shape
of `gateway/run.py`'s source. AGENTS.md bans that outright ("Never read
source code in tests" — it passes when the implementation is subtly
broken and fails on correct refactors), so the fix is to make the
behaviour importable rather than to assert on text. The following commit
replaces those tests with behavioural ones against this method.
No behaviour change: the handler now calls
`runner._schedule_shutdown_task()`, which performs the same
create-and-anchor and the same "don't re-point a live shutdown" check,
and additionally returns the task that owns the shutdown so callers can
await or inspect it.
The previous revision of this file asserted structural properties by
reading and `ast.parse`-ing `gateway/run.py`, because the behaviour lived
in a closure that could not be imported. AGENTS.md bans that outright
("Never read source code in tests"): such a test passes when the
implementation is subtly broken and fails on a correct refactor.
The preceding commit made the behaviour importable as
`GatewayRunner._schedule_shutdown_task`, so all four structural
assertions are replaced by tests that call it:
- scheduling a shutdown leaves the task reachable from the runner, not
only from the event loop, which is what stops it being collected while
still pending;
- a repeat signal reuses the in-flight task and does not start a second
`stop()`;
- once the previous shutdown has finished, a later signal does schedule
again, so the guard cannot wedge the gateway;
- `_stop_impl`'s cancel sweep leaves `_shutdown_task` alone while still
cancelling an ordinary background task (unchanged, already behavioural).
Each production hunk is mutation-covered: dropping the anchor fails 3
tests, dropping the repeat-signal guard fails the repeat test, and
removing the cancel-sweep exemption fails the sweep test.
alt-glitch
added
type/bug
fix(gateway): keep a strong reference to the SIGTERM/SIGINT shutdown task
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
shutdown_signal_handleringateway/run.pyis installed for both SIGINT and SIGTERM, and it ended by scheduling the shutdown and throwing the handle away:This repo already argues that this is a bug — on the other signal path.
request_restart()(the SIGUSR1 restart handler) creates a task of exactly the same shape, deliberately keeps a reference to it, and writes out why:One signal path was hardened against that hazard and the comment explaining why is sitting in the file. The other — the path every
systemctl stop, everyhermes gateway stopand every interactive Ctrl+C takes — was left bare. This PR completes that hardening by mirroring_restart_task.The failure window, stated precisely
stop()is two-stage and only the inner stage is self-anchoring:self._stop_task = asyncio.create_task(_stop_impl())and awaited, so it is strongly referenced;runner.stop()coroutine the signal handler wraps — had no reference at all.So the damaging window is narrow and specific: if the outer task is collected before
stop()reachesself._stop_task = asyncio.create_task(_stop_impl()),_stop_implis never created and the gateway never tears down. It keeps running until systemd'sTimeoutStopSecescalates to SIGKILL — in-flight turns are never drained, sessions are never finalized, and the operator sees "I sent stop and it hung, then it got killed and I lost the turn."A collection after that point is harmless: the inner task survives on
self._stop_taskand shutdown completes normally. I am not claiming that every collection loses the shutdown, and this is a latent-hazard fix in the same sense the_restart_taskone was — the asyncio docs and this file's own comment both say the handle must be kept.Why the obvious fix is the wrong one
The naive version — parking the task in
self._background_tasks— is self-cancelling._stop_impl's teardown loop cancels every member of that set and skips exactly two handles,_stop_taskand_restart_task. Putting the shutdown handle there would have_stop_implcancel the very task that is awaiting it, trading a garbage-collection bug for a cancellation bug. That is why this uses a dedicated_shutdown_taskand adds the matching exemption instead.Why the assignment is guarded
shutdown_signal_handleris not a once-only path — a second Ctrl+C, a SIGINT followed by the service manager's SIGTERM, or_run_planned_stop_watcherdriving the same callable from its polling thread vialoop.call_soon_threadsafe(shutdown_handler, None)all re-enter it. (That watcher's own docstring notes that on POSIX "the signal handler always races us to consuming the marker file", and its_draininggate does not close until_stop_implis already running.) An unconditional assignment would let the second call overwrite the reference to the task performing the live teardown, putting us straight back in the window above. Skipping the duplicate is behaviour-preserving:stop()short-circuits onself._stop_task, so the second task only ever existed to await the first one's work.Scope
Deliberately fenced to the signal-handler statement only. There are two other open PRs anchoring
create_tasksites in this file — #17966 and #45372, both on the watcher sites (_run_process_watcher,_session_expiry_watcher,_platform_reconnect_watcherand friends insidestart()). Neither touches the signal handler, and this PR does not touch any watcher site, so all three apply in any order. The regression tests live in a new file for the same reason: #41642 and #41690 are both adding their own test files in this area, and a shared test file would be a needless conflict.Related Issue
No filed issue — found by auditing the two signal paths in
gateway/run.pyagainst each other, after the SIGUSR1 path's comment made the invariant explicit.Type of Change
Changes Made
Six atomic commits:
gateway/run.py— anchor the task the SIGINT/SIGTERM handler creates inrunner._shutdown_task, and declare the attribute at both existing sites (the class annotation beside_stop_task/_restart_task, and__init__) so bare runners built viaobject.__new__inherit the sameNonedefault.gateway/run.py— only re-point that anchor when the previous shutdown task is absent ordone(), so a repeat signal cannot drop the handle to the live teardown.gateway/run.py— add_shutdown_taskto_stop_impl's cancel-sweep skip list, beside_stop_taskand_restart_task. Like_restart_task, the handle is deliberately kept out of_background_tasks, so this branch does not fire on any current path; it is recorded there because that loop is the single place enforcing "never cancel a task that is awaiting_stop_task", and the handles satisfying that description should not be treated differently by it.tests/gateway/test_shutdown_task_anchor.py— regression tests.gateway/run.py— extract the anchoring and the guard out of the handler closure intoGatewayRunner._schedule_shutdown_task(), no behaviour change. The handler is built inside the gateway start path and cannot be imported, which had pushed the tests into asserting the shape of the source; AGENTS.md bans that ("Never read source code in tests"), so the behaviour is made importable instead.tests/gateway/test_shutdown_task_anchor.py— replace the four source-parsing assertions with behavioural tests against that method.How to Test
Five tests, all behavioural — no source text is read. Every production hunk is mutation-covered, i.e. each one is independently load-bearing:
gateway/run.py_shutdown_task..._repeat_signal_does_not_replace_the_in_flight_shutdown_task)_shutdown_taskbranch from_stop_impl's cancel sweeptest_stop_does_not_cancel_the_anchored_shutdown_task)What they assert:
stop();_stop_impl's cancel sweep leaves_shutdown_taskalone while still cancelling an ordinary background task — driven against a realrunner.stop()on therestart_test_helpersbare-runner harness.Adjacent suites run green on this branch:
test_gateway_shutdown.py,test_planned_stop_watcher.py,test_restart_drain.py,test_clean_shutdown_marker.py,test_shutdown_cache_cleanup.py,test_startup_restart_race.py,test_restart_resume_pending.py,test_background_command.py,test_api_server_active_work_drain.py,test_cron_active_work_drain.py,test_session_race_guard.py,test_max_concurrent_sessions.py,test_gateway_process_exit.py,test_external_drain_control.py— 121 passed, 2 skipped.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — ran the focused + adjacent suites listed above, not the full suiteDocumentation & Housekeeping
docs/, docstrings) — or N/A (inline comments only; no user-facing docs affected)cli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/Aadd_signal_handlerraisesNotImplementedErrorthere), but_run_planned_stop_watcherinvokes the same callable on every platform, so the anchor and the repeat-call guard apply to the Windows fallback path too. Verified on macOS only.