fix(gateway): reap only the background processes an abandoned turn created - #76188
fix(gateway): reap only the background processes an abandoned turn created#76188JoaoMarcos44 wants to merge 3 commits into
Conversation
…eated An agent turn can spawn a long-running background subprocess (e.g. `next build`) and later be abandoned via inactivity timeout, /stop, /new, or a client disconnect. Before this fix the gateway interrupted the agent loop but never touched the subprocess: it kept running inside the gateway's cgroup, unbounded, until memory pressure starved the event loop and made every platform/cron look hung (NousResearch#76115). The process registry already knew how to kill a process tree — the missing piece was per-turn ownership: nothing distinguished a process that predates the turn (must survive), a process the turn started and finished successfully (must survive), and a process an abandoned turn left running (must be reaped). - tools/process_registry.py: snapshot_running_ids() captures a turn's starting baseline; kill_started_since() reaps only IDs created after it, scoped to one task_id. - gateway/turn_context.py: TurnContext carries process_task_id + process_baseline so the timeout/interrupt paths can reach them. - gateway/run.py: baseline is snapshotted right before the turn's executor task starts; the inactivity-timeout path and the explicit /stop|/new|disconnect interrupt path both reap via the same helper. A daemon-thread watchdog backs up the asyncio-based timeout poll, since a starved event loop is exactly the failure mode this bug causes. The turn's own worker clears its ownership markers the instant it finishes, closing a race where a /stop landing right after normal completion could reap a background process the turn deliberately left running. Related but insufficient on their own: NousResearch#37454 (cgroup ExecStopPost reaper only fires on service restart) and NousResearch#68915 (orphaned-pipe grandchild detection, a registry bug not a turn-lifecycle gap). Neither ties process cleanup to turn abandonment.
Related: #76172/#76183 use a global age-based sweep. This PR instead reaps only processes created by an abandoned gateway turn and explicitly preserves successful-turn background work. Both address #76115, but the lifecycle policies differ and need a maintainer choice. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for preserving successful-turn background work rather than using a global sweep. The abandoned-turn cleanup gap is present on current main: gateway/run.py:22077-22080 and gateway/run.py:24159-24186 only interrupt the agent.
Problems
- The reaper is asynchronous but its ownership scope is session-wide. Gateway turns pass
task_id=ctx.session_id(gateway/run.py:5096-5108), and the PR starts the reaper before_interrupt_and_clear_sessionreleases the session. A replacement turn can create a process before the old reaper enumerates targets; that process is absent from the old baseline and is killed incorrectly. - The stated client-disconnect coverage is incomplete: API-server SSE disconnect handlers call
agent.interrupt()directly atgateway/platforms/api_server.py:4231-4238andgateway/platforms/api_server.py:4810-4818, outside this diff.
Suggested changes
- Associate background processes with a unique logical-turn ownership token, not a session-wide task ID, and reap by that token.
- Cover the API-server disconnect path or narrow the claimed lifecycle scope.
This is an automated hermes-sweeper review.
Addresses the hermes-sweeper review on NousResearch#76188: 1. task_id is session-scoped (task_id == session_id), not turn-scoped, and the reap runs on a detached thread. A replacement turn could claim the same session and spawn a legitimate process before the previous turn's reaper thread actually enumerates its targets, killing that new process by mistake. Fixed by gating the reap on the existing run_generation mechanism (_is_session_run_current) instead of inventing a new ownership token: the timeout path captures its own run_generation at turn start, the interrupt path captures the generation immediately after invalidating it. If a newer turn has since claimed the session, the reap is skipped — that newer turn owns its own baseline, so nothing is left permanently unreaped. 2. gateway/platforms/api_server.py's SSE handlers for chat-completions and the /api/sessions responses endpoint run their own agent lifecycle via _run_agent() and never passed through TurnRunner, so client-disconnect abandonment there had no baseline and no reap — contradicting the PR's stated disconnect coverage. Both disconnect handlers now snapshot/reap through the same tools.process_registry primitives, via a small _reap_disconnected_agent_processes() helper shared by both call sites.
|
Addressed both points from the hermes-sweeper review in dbbb10d:
Tests, ruff, and py_compile all still green; no regressions in the existing suite. |
…ct reap dbbb10d shipped without direct test coverage for its own new logic — the same gap teknium's review flagged on the competing PR. Close it: - _reap_gateway_turn_processes: skips when is_still_current() is False, proceeds when True, fails open (reaps) if the check itself raises rather than silently disabling the leak fix. - _abandon_timed_out_gateway_turn: still marks the turn abandoned (interrupt fires) even when the reap itself is skipped. - api_server._reap_disconnected_agent_processes: reaps the baseline-diff for an owned turn, no-ops when the agent never recorded ownership markers. - APIServerAdapter._run_agent: markers are populated with the right task_id/baseline during the turn and cleared once it completes, closing the same race window fixed in gateway/run.py for this separate agent-lifecycle surface.
Addresses the hermes-sweeper review on #76188: 1. task_id is session-scoped (task_id == session_id), not turn-scoped, and the reap runs on a detached thread. A replacement turn could claim the same session and spawn a legitimate process before the previous turn's reaper thread actually enumerates its targets, killing that new process by mistake. Fixed by gating the reap on the existing run_generation mechanism (_is_session_run_current) instead of inventing a new ownership token: the timeout path captures its own run_generation at turn start, the interrupt path captures the generation immediately after invalidating it. If a newer turn has since claimed the session, the reap is skipped — that newer turn owns its own baseline, so nothing is left permanently unreaped. 2. gateway/platforms/api_server.py's SSE handlers for chat-completions and the /api/sessions responses endpoint run their own agent lifecycle via _run_agent() and never passed through TurnRunner, so client-disconnect abandonment there had no baseline and no reap — contradicting the PR's stated disconnect coverage. Both disconnect handlers now snapshot/reap through the same tools.process_registry primitives, via a small _reap_disconnected_agent_processes() helper shared by both call sites.
|
Merged via #76687 — your three commits were cherry-picked with authorship preserved (rebase-merge), so they land on main under your name: 80e4fb5, a356917, 1b88682. Excellent work on this one: the baseline-diff ownership model, the daemon watchdog rationale (the failure mode starves the exact event loop the normal timeout poll depends on), and the run_generation race fix in your second commit were all verified end-to-end with real subprocesses during review. We added three follow-up commits on top: an epoch gate for the API-server disconnect reap (concurrent runs can share a client session_id, so a stale reaper needed the same gate you added on the gateway path), coverage for the /v1/runs sibling surface, an empty-task_id guard, and a dedup of kill_started_since into kill_all. Closing this PR in favor of the merged salvage. Thanks for the contribution! |
Addresses the hermes-sweeper review on NousResearch#76188: 1. task_id is session-scoped (task_id == session_id), not turn-scoped, and the reap runs on a detached thread. A replacement turn could claim the same session and spawn a legitimate process before the previous turn's reaper thread actually enumerates its targets, killing that new process by mistake. Fixed by gating the reap on the existing run_generation mechanism (_is_session_run_current) instead of inventing a new ownership token: the timeout path captures its own run_generation at turn start, the interrupt path captures the generation immediately after invalidating it. If a newer turn has since claimed the session, the reap is skipped — that newer turn owns its own baseline, so nothing is left permanently unreaped. 2. gateway/platforms/api_server.py's SSE handlers for chat-completions and the /api/sessions responses endpoint run their own agent lifecycle via _run_agent() and never passed through TurnRunner, so client-disconnect abandonment there had no baseline and no reap — contradicting the PR's stated disconnect coverage. Both disconnect handlers now snapshot/reap through the same tools.process_registry primitives, via a small _reap_disconnected_agent_processes() helper shared by both call sites.
Addresses the hermes-sweeper review on NousResearch#76188: 1. task_id is session-scoped (task_id == session_id), not turn-scoped, and the reap runs on a detached thread. A replacement turn could claim the same session and spawn a legitimate process before the previous turn's reaper thread actually enumerates its targets, killing that new process by mistake. Fixed by gating the reap on the existing run_generation mechanism (_is_session_run_current) instead of inventing a new ownership token: the timeout path captures its own run_generation at turn start, the interrupt path captures the generation immediately after invalidating it. If a newer turn has since claimed the session, the reap is skipped — that newer turn owns its own baseline, so nothing is left permanently unreaped. 2. gateway/platforms/api_server.py's SSE handlers for chat-completions and the /api/sessions responses endpoint run their own agent lifecycle via _run_agent() and never passed through TurnRunner, so client-disconnect abandonment there had no baseline and no reap — contradicting the PR's stated disconnect coverage. Both disconnect handlers now snapshot/reap through the same tools.process_registry primitives, via a small _reap_disconnected_agent_processes() helper shared by both call sites.
What does this PR do?
Fixes #76115. When a gateway turn spawns a background subprocess (
background=true, e.g.next build) and that turn is later abandoned — inactivity timeout,/stop,/new, or a client disconnect — the subprocess was never reaped. It kept running inside the gateway's cgroup indefinitely. Enough memory growth pushes the cgroup pastMemoryHigh, the kernel throttles/swap-thrashes the gateway process, its asyncio event loop starves, and every platform connection and cron job looks hung even though nothing upstream is actually broken.The process registry already had a working tree-kill primitive. What was missing was per-turn ownership: nothing distinguished a process that predates the turn (must survive), a process the turn started and finished successfully (must survive), and a process an abandoned turn left running (must be reaped).
Root cause → fix
%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#00f0ff', 'mainBkg': '#0a0a16', 'primaryTextColor': '#ffffff', 'primaryBorderColor': '#ff007f', 'lineColor': '#00f0ff'}}}%% flowchart TD A[🔒 Turn Starts] -->|Snapshot Baseline IDs| B[⚡ process_registry.snapshot_running_ids] B --> C{Agent Runs Tool} C -->|background=true| D[🚀 Subprocess Spawned] C -->|Turn Finishes Normally| E[✅ Ownership Markers Cleared] D --> F{Turn Abandoned?} F -->|Inactivity Timeout| G[🧵 Daemon Watchdog Thread] F -->|/stop, /new, Disconnect| H[⚔️ Explicit Interrupt Path] F -->|No — Completed| E G --> I[🔥 kill_started_since: Reap IDs Not in Baseline] H --> I I --> J[🩸 Prior + Legit Processes Preserved] E -.->|Stale Reference Guard| HInfographic :
Timeline: everything before the Turn Baseline Snapshot is preserved; the selective reaper only terminates processes created after that snapshot by a turn that was abandoned (timeout /
/stop//new/ disconnect). Matchessnapshot_running_ids()→kill_started_since()intools/process_registry.py.tools/process_registry.py:snapshot_running_ids(task_id)captures the IDs already running for a turn'stask_idat start;kill_started_since(task_id, baseline, source=...)reaps only the IDs created after that baseline. Pure additive methods — no existing method touched.gateway/turn_context.py:TurnContextgainsprocess_task_id+process_baselineso both cleanup paths can reach the same ownership data.gateway/run.py:_turn_task_id = session_id, matching the sametask_idthe terminal tool already threads througheffective_task_idwhen it spawns a process — verified end to end, not assumed)./stop//new/disconnect interrupt path both reap through the same_reap_gateway_turn_processeshelper, so the two call sites can't drift._watch_gateway_turn_inactivity) backs up the asyncio-based timeout poll — the failure mode this bug causes is exactly "event loop too starved to run its own timeout check," so the detector can't depend solely on that loop._gateway_turn_process_task_id/_gateway_turn_process_baseline) on the agent instance the instantrun_sync()returns. Without this,.turn.agentstays reachable until the next turn is claimed, so a/stoplanding right after a turn finished normally could still reap a background process that turn deliberately left running — violating the exact invariant this fix exists to protect.What it does NOT change
No new core tool, no new
HERMES_*env var, no prompt change, no cache-key change, no change to any pre-existingProcessRegistrymethod. Processes that predate a turn, and processes left running by a turn that completed successfully, are untouched — verified by dedicated regression tests, not just claimed.Related issues — checked, not assumed related
ExecStopPostcgroup reaperAIAgent.close()never closes_codex_sessionNone of the above ties process cleanup to gateway turn abandonment specifically. This PR is scoped to that one lifecycle.
Test plan
tests/gateway/test_abandoned_turn_process_cleanup.py(new) — watchdog reaps only baseline-diff IDs; completed-worker wins the race; timeout cleanup is idempotenttests/tools/test_process_registry.py—snapshot_running_ids/kill_started_sincecontractstests/gateway/test_tool_response_drop_recovery.py— existing interrupt/recovery behavior unaffectedtests/gateway/test_turn_context.py—TurnContextfield additionsruff check,python -m py_compile— cleantest_process_registry.pyfailures on Windows (os.getpgidunavailable, PTY EOF semantics) are unrelated to this patch