fix: env-tag sweep catches setsid'd, FD-detached subprocess orphans (#817) - #827
Merged
Conversation
…ocess orphans (#817) The existing cleanup path has two passes: 1. terminate_process_group: kills the claude pgid via killpg 2. _kill_orphan_pipe_writers: scans /proc for processes holding the stdout pipe inode A subprocess that escapes BOTH passes — by calling setsid() (new pgid) AND redirecting stdin/stdout/stderr to /dev/null (no FDs back to our pipes) — survives indefinitely. In production this manifests as runaway CPU after a scheduled task ends in pending_retry, eventually starving the agent-server event loop, opening the agent_client CB, and cascading the scheduler CB into the dormant state described in the ticket. Fix: tag every Claude subprocess with TRINITY_EXECUTION_ID=<uuid> in its env. Env vars are inherited at every fork/exec/setsid AND survive double-fork daemonization (the new parent doesn't reset env), so the tag follows every descendant regardless of how it tries to detach. A new pass (kill_processes_by_env_tag) scans /proc/<pid>/environ and SIGKILLs every process carrying the matching tag. Wired into: - terminate_process_group (used by all timeout paths) - drain_reader_threads (runs on every task end — success or kill) - ProcessRegistry.terminate (the /api/cancel path triggered when the backend's task call times out) Critically, the sweep is local to the agent container — it does not depend on the backend reaching the agent. So even when the dormant CB has the agent isolated, the cleanup still fires when claude itself eventually exits. Test: tests/test_817_subprocess_leak.py engineers an agent template (config/agent-templates/test-leak-hook/) that injects a UserPromptSubmit hook spawning eight setsid'd, FD-detached CPU burners then sleeping 120s. The schedule's timeout_seconds=30 + max_retries=1 produces pending_retry deterministically. Pre-fix: 10 burner PIDs survive, test fails with full ps dump in the message. Post-fix: 0 surviving PIDs across 3 consecutive runs.
Gemini runtime spawned subprocesses without TRINITY_EXECUTION_ID env or any post-wait orphan cleanup, leaving the same leak class open as the pre-fix Claude path. Mirrors the #817 fix mechanism in both Gemini entry points (chat + headless task): - Inject TRINITY_EXECUTION_ID into Popen env so descendants are tagged - After process.wait() returns, call kill_processes_by_env_tag() to reap any setsid'd, FD-detached orphans - Error path in headless task also runs the sweep so a failing Gemini task does not leave leaked descendants Chat path generates execution_id if caller did not provide one (matching execute_claude_code's behavior). Production has not reported a Gemini-specific instance of #817 — this is defensive parity, not a fix for an observed bug. No test added; the mechanism is identical to the Claude path which the slow integration test already validates.
Adds TestKillProcessesByEnvTag covering branches the integration test (tests/test_817_subprocess_leak.py) does not exercise: - exact-match semantics (tag value 'abc' must not match 'abcdef') - name-scoped match (different env var with same value must not match) - exclude_pids parameter - calling-PID always excluded (guards against killing agent-server itself) - /proc unreadable returns 0 cleanly without raising - count returned matches number of distinct processes killed - non-tagged and differently-tagged processes left alive All tests are Linux-only (require /proc/<pid>/environ); marked @pytest.mark.skipif(sys.platform != "linux", ...) matching the existing convention for tests in this file. Verified all 9 pass on Linux via docker run python:3.11-slim.
obasilakis
marked this pull request as ready for review
May 13, 2026 07:32
vybe
approved these changes
May 13, 2026
vybe
left a comment
vybe
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Solid P1 fix — env-tag sweep is the right primitive here, strictly stronger than the PPid-walk alternative. Well-tested with an engineered repro and full parity across Claude and Gemini runtimes.
obasilakis
added a commit
that referenced
this pull request
May 15, 2026
The cgroup-walk refactor in the previous commit deleted three internal helpers from subprocess_pgroup.py: - kill_processes_by_env_tag (#827) - subsumed by cgroup sweep - _kill_orphan_pipe_writers (#618/#728) - subsumed by cgroup sweep - _set_idle_priority (#808) - only existed for _kill_orphan_pipe_writers The unit tests in tests/unit/test_subprocess_pgroup.py imported and exercised those symbols directly. With the symbols gone, pytest collection errored on every seed run, which the regression-diff CI job flagged as a new failure. Removes the four test classes that targeted deleted code: - TestDrainOrphanKillerTimeout (covered the _kill_orphan_pipe_writers daemon-thread budget inside drain_reader_threads) - TestKillOrphanPipeWriters (covered _kill_orphan_pipe_writers directly against engineered npx-style orphans) - TestSetIdlePriority (covered _set_idle_priority no-raise on every platform) - TestKillProcessesByEnvTag (covered kill_processes_by_env_tag's /proc/<pid>/environ scan) Retained tests that target still-existing symbols: - TestTerminateProcessGroup (pgid SIGTERM/SIGKILL of full tree) - TestDrainReaderThreads (reader unwind via pgid kill + natural drain) - TestSafeClosePipes (best-effort pipe close) - TestSignalProcessTree (signal propagation to group) Dedicated unit coverage for the new cgroup-walk modules (orphan_sweep, orphan_allowlist, orphan_sweeper) will land in a follow-up commit on this PR — kept separate so the deletion vs. the new-tests addition are reviewable independently.
vybe
pushed a commit
that referenced
this pull request
May 16, 2026
follow-up) (#857) * fix(agent-runtime): env-tag sweep catches setsid'd, FD-detached subprocess orphans (#817) The existing cleanup path has two passes: 1. terminate_process_group: kills the claude pgid via killpg 2. _kill_orphan_pipe_writers: scans /proc for processes holding the stdout pipe inode A subprocess that escapes BOTH passes — by calling setsid() (new pgid) AND redirecting stdin/stdout/stderr to /dev/null (no FDs back to our pipes) — survives indefinitely. In production this manifests as runaway CPU after a scheduled task ends in pending_retry, eventually starving the agent-server event loop, opening the agent_client CB, and cascading the scheduler CB into the dormant state described in the ticket. Fix: tag every Claude subprocess with TRINITY_EXECUTION_ID=<uuid> in its env. Env vars are inherited at every fork/exec/setsid AND survive double-fork daemonization (the new parent doesn't reset env), so the tag follows every descendant regardless of how it tries to detach. A new pass (kill_processes_by_env_tag) scans /proc/<pid>/environ and SIGKILLs every process carrying the matching tag. Wired into: - terminate_process_group (used by all timeout paths) - drain_reader_threads (runs on every task end — success or kill) - ProcessRegistry.terminate (the /api/cancel path triggered when the backend's task call times out) Critically, the sweep is local to the agent container — it does not depend on the backend reaching the agent. So even when the dormant CB has the agent isolated, the cleanup still fires when claude itself eventually exits. Test: tests/test_817_subprocess_leak.py engineers an agent template (config/agent-templates/test-leak-hook/) that injects a UserPromptSubmit hook spawning eight setsid'd, FD-detached CPU burners then sleeping 120s. The schedule's timeout_seconds=30 + max_retries=1 produces pending_retry deterministically. Pre-fix: 10 burner PIDs survive, test fails with full ps dump in the message. Post-fix: 0 surviving PIDs across 3 consecutive runs. * fix(agent-runtime): apply env-tag sweep parity to Gemini runtime (#817) Gemini runtime spawned subprocesses without TRINITY_EXECUTION_ID env or any post-wait orphan cleanup, leaving the same leak class open as the pre-fix Claude path. Mirrors the #817 fix mechanism in both Gemini entry points (chat + headless task): - Inject TRINITY_EXECUTION_ID into Popen env so descendants are tagged - After process.wait() returns, call kill_processes_by_env_tag() to reap any setsid'd, FD-detached orphans - Error path in headless task also runs the sweep so a failing Gemini task does not leave leaked descendants Chat path generates execution_id if caller did not provide one (matching execute_claude_code's behavior). Production has not reported a Gemini-specific instance of #817 — this is defensive parity, not a fix for an observed bug. No test added; the mechanism is identical to the Claude path which the slow integration test already validates. * test(subprocess): unit tests for env-tag sweep (#817) Adds TestKillProcessesByEnvTag covering branches the integration test (tests/test_817_subprocess_leak.py) does not exercise: - exact-match semantics (tag value 'abc' must not match 'abcdef') - name-scoped match (different env var with same value must not match) - exclude_pids parameter - calling-PID always excluded (guards against killing agent-server itself) - /proc unreadable returns 0 cleanly without raising - count returned matches number of distinct processes killed - non-tagged and differently-tagged processes left alive All tests are Linux-only (require /proc/<pid>/environ); marked @pytest.mark.skipif(sys.platform != "linux", ...) matching the existing convention for tests in this file. Verified all 9 pass on Linux via docker run python:3.11-slim. * fix(agent-runtime): cgroup-walk sweep catches env-stripped orphans (#817 follow-up) PR #827 added an env-tag sweep (kill_processes_by_env_tag) that caught subprocess orphans which escape both terminate_process_group (different pgid via setsid) AND _kill_orphan_pipe_writers (no shared pipe FDs). Eugene's 2026-05-13 production capture on cornelius-m proved that fix incomplete: the surviving orphan had no TRINITY_EXECUTION_ID anywhere in /proc/<pid>/environ, so the env-tag scan returned zero hits while the orphan burned 136-160% CPU for 100+ minutes and blocked every subsequent execution at the CB layer. The leak class is "process scrubs its environment after spawn" (env -i / sudo / re-exec into a clean shell). That defeats env-tag identification on principle — there is no signal we can plant in env that the orphan can't strip. Cgroup membership is the inescapable boundary. A process the kernel attributes to this container's CPU/memory is, by definition, in its cgroup. This change collapses the prior three reactive cleanup passes (pgid kill #407, pipe-writer sweep #618/#728/#808, env-tag sweep #827) into one exclusionary mechanism: read /sys/fs/cgroup/cgroup.procs (cgroup v2 unified) and SIGKILL every PID not on the allowlist. The allowlist composes three layers: - Hard-protected: PID 1, agent-server + parent chain, sweep caller, every sshd-named process (operator SSH sessions survive) - Platform essentials by cmdline pattern: tail -f /dev/null (startup.sh keep-alive), sudo+sshd wrapper, guardrail-config writer - Active execution descendants: ProcessRegistry-registered claude PIDs + pgids, expanded via ppid walk - User-configured persistent daemons: cmdline glob patterns from ~/.trinity/persistent-processes.allow (templates with deliberate long-lived daemons like cornelius-m's moltbook-http-mcp list them here) The sweep runs on three paths: 1. drain_reader_threads finally block (every execution end, success OR failure). Catches the production-success-path case where claude spawned a leaking grandchild then exited cleanly. 2. ProcessRegistry.terminate() after pgid SIGKILL. Catches the production-external-termination case where the backend CB or a user cancel kills the execution before drain_reader_threads can run. 3. Periodic background task in orphan_sweeper (Eugene's suggested fix #2). Default 90s startup grace + 30s interval. Catches the "no execution ever completes" production case where the CB fast-fails every subsequent task and no per-task cleanup runs. drain_reader_threads: 411 lines -> 138 lines subprocess_pgroup.py: 631 lines -> 331 lines (deleted env-tag and pipe-writer kernels) Validated end-to-end against the live cornelius-m repro from Eugene's ticket: - per-task path: orphan PID 734 (setsid + env -i + FD detached, no relation to claude) killed at exec end. Same shape orphan survived all three prior passes pre-fix. - external-termination path: orphan PID 668 injected mid-task, killed by ProcessRegistry.terminate's cgroup_orphans pass after SIGINT. Log attribution unambiguous: SIGINT -> 580ms graceful exit -> cgroup sweep -> orphan dead, all on the [ProcessRegistry] code path. - periodic-sweep path: orphan PID 311 injected with no execution running, killed at T+90s by the periodic sweeper. Eugene's exact 100-minute production scenario. CI: new pytest test_817_cgroup_sweep_catches_no_env_tag_orphan in tests/test_817_subprocess_leak.py covers the per-task path deterministically (PASS in 49s). External-termination and periodic- sweep paths validated manually here; can be added to CI as @slow tier in a follow-up. EXECUTION_TAG_NAME is retained on every Claude Popen for log identity and operator incident-response correlation (env grep on /proc), but cleanup no longer depends on it. Default allowlist file ships empty. Templates running deliberate long-lived daemons via SessionStart hooks must add their cmdline patterns to ~/.trinity/persistent-processes.allow or the periodic sweep will reap them on first iteration. This is intentional — a permissive default would defeat the point of the mechanism. Closes the regression Eugene captured in #817 (comment) * test: remove unit tests for cleanup symbols deleted in #817 follow-up The cgroup-walk refactor in the previous commit deleted three internal helpers from subprocess_pgroup.py: - kill_processes_by_env_tag (#827) - subsumed by cgroup sweep - _kill_orphan_pipe_writers (#618/#728) - subsumed by cgroup sweep - _set_idle_priority (#808) - only existed for _kill_orphan_pipe_writers The unit tests in tests/unit/test_subprocess_pgroup.py imported and exercised those symbols directly. With the symbols gone, pytest collection errored on every seed run, which the regression-diff CI job flagged as a new failure. Removes the four test classes that targeted deleted code: - TestDrainOrphanKillerTimeout (covered the _kill_orphan_pipe_writers daemon-thread budget inside drain_reader_threads) - TestKillOrphanPipeWriters (covered _kill_orphan_pipe_writers directly against engineered npx-style orphans) - TestSetIdlePriority (covered _set_idle_priority no-raise on every platform) - TestKillProcessesByEnvTag (covered kill_processes_by_env_tag's /proc/<pid>/environ scan) Retained tests that target still-existing symbols: - TestTerminateProcessGroup (pgid SIGTERM/SIGKILL of full tree) - TestDrainReaderThreads (reader unwind via pgid kill + natural drain) - TestSafeClosePipes (best-effort pipe close) - TestSignalProcessTree (signal propagation to group) Dedicated unit coverage for the new cgroup-walk modules (orphan_sweep, orphan_allowlist, orphan_sweeper) will land in a follow-up commit on this PR — kept separate so the deletion vs. the new-tests addition are reviewable independently. * fix(tests): preserve flat-import contract for subprocess_pgroup unit tests tests/unit/test_subprocess_pgroup.py imports subprocess_pgroup flat: it adds docker/base-image/agent_server/utils/ to sys.path then runs `import subprocess_pgroup` with no package context. The previous module body deliberately documented this contract: "This module is kept free of package-relative imports so it can be unit-tested without loading the rest of agent_server." The #817 cgroup-walk refactor broke that contract by adding: from .orphan_sweep import kill_cgroup_orphans which fails at collection time with: ImportError: attempted relative import with no known parent package That left head's pytest collection erroring on a single test, which the regression-diff CI job correctly flagged as a new failure under HEAD (1 collection error vs base's 1510 collected tests). Restores the contract with a try/except that preserves the package-relative form for normal use (inside agent_server) and falls back to flat name on ImportError (unit-test sys.path-injection path): try: from .orphan_sweep import kill_cgroup_orphans except ImportError: from orphan_sweep import kill_cgroup_orphans Same fix applied to orphan_sweep.py's import of orphan_allowlist — that module also needs to be flat-importable so the unit tests can load its dependency chain without the agent_server package. Verified locally: collect-only succeeds (11 tests), full run passes (11 passed, 2.87s).
6 tasks
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
2 participants
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.
Closes #817.
Summary
TRINITY_EXECUTION_ID=<uuid>so we can identify its descendants regardless of how they try to detach (setsid, FD redirect, double-fork).kill_processes_by_env_tagscans/proc/<pid>/environand SIGKILLs everything carrying the matching tag. Wired intoterminate_process_group,drain_reader_threads, andProcessRegistry.terminateso it fires on every task end (success, timeout, and/api/cancel).Root cause
The existing cleanup has two passes:
terminate_process_group— kills the Claude pgid viakillpg._kill_orphan_pipe_writers—/procscan for processes holding our stdout pipe inode.A subprocess that escapes BOTH —
setsid()(new pgid) AND</dev/null >/dev/null 2>&1(no FDs back to our pipes) — survives. Production observed this as a runaway-CPU agent that started up after a scheduled task ended inpending_retry, eventually starving the agent-server event loop, opening theagent_clientCB, and cascading the scheduler CB into the dormant state.Why env tag beats PPid-walk
Considered approach: walk
/procancestry fromclaude_pidbefore killing claude. Rejected because it misses double-fork daemonized processes (parent dies, child reparented to PID 1, ancestry chain broken). Env vars are inherited at every fork/exec/setsid and survive reparenting, so the tag is a strictly stronger ownership signal — no race window, no BFS, single/procscan.How the fix unwinds the cascade
The sweep runs at the end of every Claude task on the agent itself. Even at 160% CPU with a dormant CB, when Claude eventually finishes (naturally or via the internal
process.wait(timeout)budget),drain_reader_threadsruns, the sweep fires, the leaked processes die, CPU returns to baseline, the next CB probe succeeds, the cascade unwinds.Test plan
tests/test_817_subprocess_leak.py— engineered repro viaconfig/agent-templates/test-leak-hook/. A UserPromptSubmit hook spawns 8 setsid'd + FD-detached CPU burners then sleeps 120s. The schedule'stimeout_seconds=30, max_retries=1producespending_retrydeterministically.psdump in the message.The test is marked
@pytest.mark.slowand gated to manual/nightly runs.Out of scope
retry_scheduled_at = NULLbug (ticket's tertiary issue) is a separate follow-up.Production evidence ask
Asked Eugene on the ticket to capture
ps -eo pid,ppid,pgid,sid,cmd+ls -l /proc/<pid>/fd/next time the bug fires, to confirm the production leak shape matches the engineered repro (setsid + detached FDs). The fix should hold either way (env tag is independent of how the orphan escapes), but the evidence will tell us whether to expect any blind-spot follow-ups.