fix(agent-runtime): cgroup-walk sweep catches env-stripped orphans (#817 follow-up) - #857
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.
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)
obasilakis
marked this pull request as draft
May 15, 2026 17:56
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.
…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).
obasilakis
marked this pull request as ready for review
May 15, 2026 18:39
vybe
approved these changes
May 16, 2026
vybe
left a comment
vybe
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Cgroup boundary is the right primitive — env-tag can be stripped, cgroup membership can't. Three-layer allowlist is well-reasoned, race conditions handled correctly, all three sweep paths validated against the exact production failure shape. Net -300 lines with better coverage.
Two housekeeping items to do post-merge:
- Close #817 manually (the PR body header isn't a GitHub closing keyword so it won't auto-close)
- Track the slow-tier CI tests for external-termination and periodic-sweep paths as a follow-up issue
AndriiPasternak31
added a commit
that referenced
this pull request
May 17, 2026
Two conflicts:
- docker/base-image/agent_server/utils/subprocess_pgroup.py: dev's
PR #857 (cgroup-walk sweep, #817 follow-up) removed the explicit
`_kill_orphan_pipe_writers` /proc scan and replaced it with
`kill_cgroup_orphans()` called in `finally`. Took dev's structure
verbatim and re-added the #586 [METRIC] drain_outcome emissions at
the two reachable branches (natural-drain + force-close/leaked).
`orphan_kill_count` becomes vestigial (always 0) since the explicit
killer is gone — the actual orphan count is logged separately by
the cgroup sweep and can be correlated by pid. Tests assert
`orphan_kill_count=0` so the contract holds.
- docs/memory/feature-flows.md: both branches added entries to the
Recent Updates log. Kept all three in chronological order
(#862 → #586 → #831).
Adjacent updates (no conflict but stale after merge):
- tests/unit/test_subprocess_pgroup.py: monkeypatch target switched
from removed `_kill_orphan_pipe_writers` to `kill_cgroup_orphans`.
All 13 tests pass locally.
- docs/memory/feature-flows/execution-termination.md: Site B
description now points at `kill_cgroup_orphans()` instead of the
removed `_kill_orphan_pipe_writers`; `orphan_kill_count` description
notes its vestigial role since #817.
- docs/TRINITY_COMPATIBLE_AGENT_GUIDE.md: stop-hook authoring section
refers to the cgroup sweep instead of the removed function.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AndriiPasternak31
added a commit
that referenced
this pull request
May 17, 2026
…871) - agent-monitoring: fleet check covers stopped agents (#675/#853) + per-pass log - parallel-headless-execution: cgroup-walk sweep for env-stripped orphans (#857) - platform-settings: new Feature Flags section (session tab, voice, workspace #860) - public-agent-links: SITE-001 reverse-proxy retirement (#865) Co-Authored-By: Claude <noreply@anthropic.com>
2 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 follow-up
PR #827 caught one class of leak via an env-tag sweep. Eugene's 2026-05-13 production capture on
cornelius-mproved it incomplete:…while the orphan burned 136–160% CPU for 100+ minutes and blocked every subsequent execution at the CB layer. Total orphan survival: ~2.5h. Recovery: container restart.
The escape vector that defeated #827 is environment scrubbing (
env -i/sudo/ re-exec into a clean shell). There is no signal we can plant in env that the orphan can't strip.The new mechanism
Cgroup membership is the inescapable boundary. A process whose CPU the kernel attributes to this container is, by definition, in its cgroup. This PR collapses the prior three reactive cleanup passes into one exclusionary mechanism:
Allowlist composes:
sshd-named processtail -f /dev/null,sudo /sbin/sshd*, guardrail-config writerProcessRegistry+ ppid walk~/.trinity/persistent-processes.allow*moltbook-http-mcp*,*moltbook-mcp*When the sweep runs
drain_reader_threadsfinallyblockProcessRegistry.terminate()after pgid killorphan_sweeperservice (90s startup grace, 30s interval)What was deleted
kill_processes_by_env_tag(#827)_kill_orphan_pipe_writers(#618/#728/#808)</dev/nulldefeats it_set_idle_priority,_read_proc_field,_read_proc_cmdline_drain_reader_threads_innersplit (#817)_ORPHAN_LOG_DETAIL_CAP,_ORPHAN_SCAN_WALL_SECONDSEXECUTION_TAG_NAME/proc/<pid>/environcorrelation during incident response; no longer load-bearingsubprocess_pgroup.py: 631 → 331 lines (-300).drain_reader_threads: 411 → 138 lines (-273). Three new single-purpose modules add 788 lines but collectively replace the entire reactive-cleanup pyramid plus give us the periodic-sweep capability that wasn't possible before.Validation
Reproduced Eugene's exact failure shape locally against
cornelius-m:This orphan survived all three prior passes (pre-fix). Post-fix, validated end-to-end across all three sweep paths:
Per-task path (PID 734):
External-termination path (PID 668):
Periodic-sweep path (PID 311, no execution running):
68s from spawn → death, exactly
startup_delay (90s) + first_sweepafter container start. Eugene's 100-minute production scenario reduced to 30s upper bound (the periodic interval) for any future occurrence.Moltbook daemon (PID 301) was preserved by the allowlist across every test run.
CI
New pytest
test_817_cgroup_sweep_catches_no_env_tag_orphanintests/test_817_subprocess_leak.pyexercises the per-task path deterministically (PASS in 49s).External-termination and periodic-sweep paths validated manually but not in CI yet — adding them as
@pytest.mark.slowis the obvious follow-up; left out of this PR to keep the diff focused.Operator note
Templates that ship deliberate long-lived daemons (cornelius-m, future templates) MUST list their cmdline patterns in
~/.trinity/persistent-processes.allowor the periodic sweep will reap them on first iteration. Default file is empty — a permissive default would defeat the point. For cornelius-m, the patterns are:Files
docker/base-image/agent_server/utils/orphan_sweep.py(229 lines) —kill_cgroup_orphans,cgroup_availabledocker/base-image/agent_server/utils/orphan_allowlist.py(345 lines) — three-tierresolve_allowlistdocker/base-image/agent_server/services/orphan_sweeper.py(214 lines) — periodic background taskdocker/base-image/agent_server/utils/subprocess_pgroup.py— drop env-tag + pipe-writer kernels, simplifydrain_reader_threadsdocker/base-image/agent_server/services/process_registry.py— migrate to directkill_cgroup_orphanswith explicit preserve listdocker/base-image/agent_server/services/gemini_runtime.py— same migration (three call sites)docker/base-image/agent_server/main.py— wireschedule_orphan_sweeper(app)tests/test_817_subprocess_leak.py— addtest_817_cgroup_sweep_catches_no_env_tag_orphan🤖 Generated with Claude Code