fix(agent-runtime): reap claude subprocess tree via process groups (#407) - #410
Merged
Conversation
) When claude-code spawned hook subprocesses (bash-guardrail, file-guardrail, output-scanner) that forked grandchildren, those grandchildren inherited our stdout/stderr pipe FDs. If a grandchild outlived claude itself, the inherited write end kept the pipe open — readline() never saw EOF, the executor thread wedged, claude became a <defunct> zombie, and agent-server spun at ~83% CPU until the container was restarted. Launch claude with start_new_session=True so it leads its own process group, capture the pgid at spawn time (before wait() reaps the parent), and reap the whole group on every exit path. Reader threads are now symmetric (both stdout and stderr in threads) and drained with a bounded grace period; if readers are still stuck after the direct child exits, the drain helper kills the group and force-closes the pipe FDs so readline() unwinds. Also updates ProcessRegistry.terminate() to signal the full tree via the stored pgid, preserving termination semantics even after the parent has been reaped. Process-group helpers extracted to docker/base-image/agent_server/utils/ subprocess_pgroup.py so they're unit-testable without loading the rest of agent_server. Regression test reproduces the #407 pattern end-to-end: a harness parent forks a grandchild that keeps stderr open, exits, and the helper must kill the grandchild via the captured pgid and unwind the stuck reader within a bounded time. Broader question — migrating this module to the Claude Agent SDK to remove the hand-rolled stream-json + subprocess-lifecycle plumbing entirely — is tracked separately in #409. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Updates execution-termination.md to reflect the new process-group based signal handling introduced in #410. Replaces the stale code snippet (that showed process.send_signal / process.kill on the single pid) with the _signal_process_tree + captured-pgid pattern, and adds a short section explaining why hook grandchildren require group-wide signaling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 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
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.
Summary
Fixes #407 — agent-server spinning at ~83% CPU after
claudeCLI subprocess becomes<defunct>and all HTTP endpoints hang until the container is restarted.claudewithstart_new_session=Trueand kill the entire process group on every exit path, so hook grandchildren (bash-guardrail, file-guardrail, output-scanner, …) can't outliveclaudeand wedgereadline()via inherited pipe FDs.readline()unwinds.wait()reaps the parent) and thread it through every cleanup callsite — after reap,os.getpgid(pid)raises, but the process group itself persists as long as any member does (the grandchildren we need to kill).ProcessRegistry.terminate()now signals the full tree via the stored pgid, preserving termination semantics even after the parent has been reaped.docker/base-image/agent_server/utils/subprocess_pgroup.pyso they're testable without loading the rest ofagent_server.Root cause
From the issue: parent
python3 /app/agent-server.pypinned at ~83% CPU with a[claude] <defunct>zombie beneath it. Last log line before the hang:[ProcessRegistry] Registered execution <id>. Claude CLI had already exited, but a hook grandchild kept the inherited stderr FD open — so ourreadline()never saw EOF, the executor thread wedged, and FastAPI stopped serving.Analysis and code read both point at the same fix shape: process-group lifecycle + bounded waits + thread drain. No code change is a regression from the last release; exposure grew after
max_turns_taskwas raised 20→50 (#361, 2d before the report) because tasks now do more hook-invoking tool calls before cap.Test plan
tests/unit/test_subprocess_pgroup.py(10 cases, all passing). Covers the production pattern end-to-end: harness parent forks a grandchild that keeps stderr open, exits, and the helper must kill the grandchild via the captured pgid and unwind the stuck reader within a bounded time. Also exercises idempotency, already-exited safety, andsignal_process_treefallback paths.test_error_classification,test_credential_sanitizer_agent,test_otel_trace_logging,test_orphaned_execution_recovery)../scripts/deploy/build-base-image.sh).python3 -c "from agent_server.utils.subprocess_pgroup import ..."andfrom agent_server.services.claude_code import execute_headless_task).Broader architectural question
The symptom class (#285 false-positive auth detection on stderr regex, #407 subprocess zombie/wedge) is downstream of running
claudeas a CLI subprocess and hand-parsingstream-jsonwith threads + asyncio bridges. Whether to migrate this module to the Claude Agent SDK instead is tracked separately in #409 — not in scope here.Closes #407
Related #409
🤖 Generated with Claude Code