fix(gateway): route the hook event bridge through the leak-safe threadsafe helper - #85583
Open
briandevans wants to merge 3 commits into
Open
Conversation
…dsafe helper `TurnRunner._event_callback_sync` is the sync -> async bridge that carries lifecycle hook events from the agent thread onto the gateway's turn loop. It called `asyncio.run_coroutine_threadsafe` directly inside a swallowing `except Exception`, so when the loop was closing or gone the call raised before the loop ever took ownership of `hooks.emit(...)` -- the event was silently dropped and the coroutine was left created-but-never-awaited, leaking its frame and emitting `RuntimeWarning: coroutine ... was never awaited`. That is precisely the failure `agent.async_utils.safe_schedule_threadsafe` was added to absorb: commit 4e89c53, `fix(async): close unscheduled coroutines in all threadsafe bridges (NousResearch#26584)`, converted every threadsafe bridge in the repo on 2026-05-15. This bridge is not a site that sweep missed -- it did not exist yet. `git log -S'_hooks_ref.emit(event_type, context)'` pins its introduction to e76e7b5 (`feat(hooks): session:compress event_callback for MemPalace sync`), which landed on 2026-06-16, a month after the sweep; `git merge-base --is-ancestor 4e89c53 e76e7b5` confirms the ordering. So the invariant was already established repo-wide and this one bridge regressed it on arrival. The contrast is in-file and one commit wide: `_step_callback_sync`, twelve lines above, emits onto the same `ctx._loop_for_step` via the same `ctx._hooks_ref.emit(...)` and already goes through the helper with `logger=`/`log_message=`. `git blame` attributes every line of both method bodies to the same commit, 1a3a9de. This change gives the event bridge the same shape, with an event-specific `log_message` so a scheduling failure is still attributable. Why it matters at runtime rather than only on paper: unlike `step_callback`, which is attached only when hooks are loaded, `agent.event_callback = ctx._event_callback_sync` is wired unconditionally, so this bridge is on the production turn path for every gateway turn. Its real emitters are `agent/conversation_compression.py` and `agent/codex_runtime.py`, both firing `session:compress` -- so a user-authored compress hook (the introducing commit's own use case is memory sync) is dropped exactly when the gateway is shutting down, which is when persisting that state matters most. No behaviour change on the happy path: the helper returns the same future, which this bridge deliberately does not consume, matching the sibling.
…coroutine Regression coverage for the shutdown race on `TurnRunner._event_callback_sync`: a `session:compress` event fired from the agent thread while the gateway is tearing its turn loop down. The assertion is deliberately on the coroutine's *state*, not on the absence of an exception. `asyncio.run_coroutine_threadsafe` raises inside `loop.call_soon_threadsafe` before the loop adopts the coroutine, so a bridge that only catches the exception still leaves `hooks.emit(...)` in `CORO_CREATED` -- the hook silently unfired and its frame leaked. The test also asserts no `coroutine ... was never awaited` RuntimeWarning is recorded, which is the symptom that surfaces in a user's log. Verified fail-before/pass-after against unmodified origin/main: the state assertion fails with `CORO_CREATED != CORO_CLOSED` and pytest reports the RuntimeWarning at teardown. The harness fakes only the hook registry and the loop: `_RecordingHooks.emit` is a real coroutine function, so the bridge is handed a genuine coroutine object exactly as production does, and the runner is built through the same `TurnContext`/`TurnRunner` seam the existing tests/gateway/test_turn_context.py uses. No network, no gateway boot.
…py path
`safe_schedule_threadsafe` distinguishes two failure branches and the previous
commit only covered one. `ctx._loop_for_step` can be `None` outright -- it is
the `TurnContext` default -- and that path never reaches asyncio at all: the
helper closes the coroutine and returns immediately, where the bare bridge
dereferenced `None.call_soon_threadsafe` and leaked the coroutine on the way
out through the swallowing `except`. Reds independently on unmodified
origin/main with `CORO_CREATED != CORO_CLOSED`.
Two invariant guards accompany it, both of which pass on origin/main by
construction -- they exist to prove the change is a pure hardening of the
failure branches rather than a behaviour change:
* a live loop still receives and *runs* the emitted coroutine, so hook
delivery on the normal turn path is unaffected; and
* the bridge remains non-raising and still returns `None` on both failure
branches, i.e. the future is deliberately left unconsumed, matching the
sibling `_step_callback_sync`.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a shutdown-race leak in the gateway’s sync→async hook event bridge by routing TurnRunner._event_callback_sync through the existing leak-safe scheduling helper (agent.async_utils.safe_schedule_threadsafe). This aligns the event bridge with the repo-wide invariant established in the earlier threadsafe-bridge sweep and matches the sibling _step_callback_sync pattern in gateway/run.py.
Changes:
- Update
TurnRunner._event_callback_syncto schedule hook coroutines viasafe_schedule_threadsafe(...)(closing unscheduled coroutines on failure) instead of callingasyncio.run_coroutine_threadsafeinside a swallowingtry/except. - Add a focused gateway test suite that asserts the coroutine is closed (not leaked) when the loop is closed or missing, and preserves the non-raising/returns-
Nonecontract plus the happy-path delivery.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| gateway/run.py | Switch event hook scheduling to safe_schedule_threadsafe to prevent leaked, never-awaited coroutines during loop shutdown/unavailability. |
| tests/gateway/test_event_callback_bridge.py | New regression + invariant tests covering closed-loop and missing-loop failure branches and guarding happy-path behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
alt-glitch
added
type/bug
Contributor
fix(gateway): route the hook event bridge through the leak-safe threadsafe helper
|
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?
TurnRunner._event_callback_sync(gateway/run.py) is the sync → async bridge that carries lifecycle hook events from the agent thread onto the gateway's turn loop. It calledasyncio.run_coroutine_threadsafedirectly inside a swallowingexcept Exception:When the loop is closing or gone,
run_coroutine_threadsaferaises insideloop.call_soon_threadsafebefore the loop takes ownership of the coroutine. Catching the exception is not enough: thehooks.emit(...)coroutine is left inCORO_CREATED, so the hook silently never fires and its frame leaks withRuntimeWarning: coroutine ... was never awaited. This PR routes the bridge throughagent.async_utils.safe_schedule_threadsafe, which closes the coroutine on both failure branches.This is finishing a sweep that already merged, not a new opinion. Commit
4e89c53082b—fix(async): close unscheduled coroutines in all threadsafe bridges (#26584), 23 files, 2026-05-15 — introducedsafe_schedule_threadsafeand converted every threadsafe bridge in the repo. This bridge is not a site that sweep missed; it did not exist yet.git log -S'_hooks_ref.emit(event_type, context)' -- gateway/run.pypins its introduction toe76e7b50730(feat(hooks): session:compress event_callback for MemPalace sync), committed 2026-06-16 — a month after the sweep — andgit merge-base --is-ancestor 4e89c53082b e76e7b50730confirms the ordering. The invariant was already repo-wide; this one bridge regressed it on arrival.The contrast is in-file and one commit wide.
_step_callback_sync, twelve lines above, emits onto the samectx._loop_for_stepvia the samectx._hooks_ref.emit(...)and already goes through the helper withlogger=/log_message=.git blameattributes every line of both method bodies to the same commit,1a3a9de630a(refactor(gateway): extract run_sync onto TurnRunner). This change gives the event bridge the sibling's shape, with an event-specificlog_messageso a scheduling failure stays attributable.Why this is a live path, not a paper one. Unlike
step_callback, which is attached only when hooks are loaded —— the event bridge is wired unconditionally, so it is on the production turn path for every gateway turn. Its real emitters are
agent/conversation_compression.pyandagent/codex_runtime.py, both firingsession:compress. So a user-authored compress hook (the introducing commit's own use case is memory sync) is dropped exactly when the gateway is shutting down — the moment persisting that state matters most.Sibling sweep. This is the last raw
asyncio.run_coroutine_threadsafeleft in the Python core (cli.py,hermes_cli/,gateway/,tui_gateway/,utils.py,hermes_state*.py). The only other*_threadsafecall ingateway/run.pyisloop.call_soon_threadsafe(shutdown_handler, None), which passes a plain callable and constructs no coroutine, so it is outside this invariant. Nothing else to widen to.Rival disclosure, stated as a checked-N rather than an absence claim: I checked the diffs of 569 open PRs touching
gateway/run.py, enumerated in bothcreated-ascandcreated-descorder, and grepped each one's patch for_event_callback_sync/_step_callback_sync/agent.event_callback; 15 carried one of those symbols and every one of them is a hunk-header context anchor or an edit to the callback wiring block, not to this method body (7 PRs whose patch was too large for the API were checked withgh pr diff). The by-file census is a floor, not a ceiling — dense slices return exactly the API's cap of rows — so I am not claiming "no rivals", only that none of the 569 diffs I read modifies this method.Related Issue
No filed issue. This completes the merged sweep in #26584 (
4e89c53082b) on a bridge introduced after it; the ancestry is proved above rather than asserted.Type of Change
Changes Made
gateway/run.py—TurnRunner._event_callback_syncnow schedules throughsafe_schedule_threadsafe(..., logger=logger, log_message="event_callback hook scheduling error")instead of a rawasyncio.run_coroutine_threadsafein a swallowingexcept Exception. The returned future is deliberately left unconsumed, matching_step_callback_sync.tests/gateway/test_event_callback_bridge.py(new) — 5 tests: two regression tests for the failure branches, and three invariant guards.Three commits, each independently meaningful and green on its own:
fix(gateway):the production change.test(gateway):the loop-closed branch.test(gateway):the loop-missing branch plus the happy-path/contract guards.How to Test
Fail-before / pass-after, verified explicitly by restoring
gateway/run.pyfrom cleanorigin/mainwith the new tests in place:origin/maintest_closed_step_loop_closes_the_event_coroutineCORO_CREATED != CORO_CLOSEDtest_missing_step_loop_closes_the_event_coroutineCORO_CREATED != CORO_CLOSEDtest_live_step_loop_still_delivers_the_eventtest_bridge_stays_non_raising_and_returns_none[closed]test_bridge_stays_non_raising_and_returns_none[missing]The
origin/mainrun also emitsRuntimeWarning: coroutine '_RecordingHooks.emit' was never awaitedat teardown — the user-visible symptom — which disappears with the fix.The two green-before tests are there on purpose: they prove this is a pure hardening of the failure branches (a live loop still receives and runs the hook coroutine; the bridge stays non-raising and still returns
None), not a behaviour change.Adjacent suites run green:
tests/gateway/test_turn_context.py,tests/gateway/test_session_title_rename_lane.py,tests/gateway/test_skip_context_files_wiring.py,tests/gateway/test_fallback_chain_reload.py,tests/agent/test_async_utils.py,tests/gateway/test_shutdown_flush.py,tests/gateway/test_13121_shutdown_inflight_transcript_flush.py,tests/gateway/test_53175_cleanup_off_loop.py,tests/gateway/test_gateway_shutdown.py.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AContract Protected
Invariant: a coroutine handed to a threadsafe bridge is always disposed of — scheduled onto the loop, or closed. It is never left in
CORO_CREATED.Known-bad inputs (both now covered):
ctx._loop_for_stepis a closed loop;ctx._loop_for_stepisNone.Future-input coverage: the invariant is enforced by the shared helper rather than by this call site, so a later refactor of the emit payload, the event name, or the hook registry cannot silently reintroduce the leak.
Negative case:
test_live_step_loop_still_delivers_the_eventfails if the change ever suppresses delivery on a healthy loop, so the guard cannot be satisfied by simply not emitting.