Skip to content

fix(agent-server): kill setsid escapees holding pipe write-end (#548) - #619

Closed
dolho wants to merge 2 commits into
devfrom
feature/548-pipe-write-holder-kill
Closed

fix(agent-server): kill setsid escapees holding pipe write-end (#548)#619
dolho wants to merge 2 commits into
devfrom
feature/548-pipe-write-holder-kill

Conversation

@dolho

@dolho dolho commented May 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds _kill_pipe_write_holders() to subprocess_pgroup.py — a Linux-only /proc/*/fd scan that SIGKILLs processes holding the write end of our subprocess pipes
  • Wires it into drain_reader_threads() after the existing terminate_process_group(), before the post-kill natural-drain window
  • Closes the path where async Stop hooks (e.g. git pushssh calling setsid()) escape the process group, keep stdout/stderr open, and cause executions to be recorded as failed with "0 tool calls" even when tools ran

Root cause

killpg(pgid, SIGKILL) only reaches members of the captured process group. Async hooks and some MCP launchers setsid() their children into a brand-new session, invisible to that signal. Those orphans keep our pipe write-ends open; drain_reader_threads()'s 30s natural-drain timer expires without seeing EOF, the force-close path runs, and the final {"type":"result"} JSON line is discarded from the kernel buffer.

Fix

Inode-keyed /proc/*/fd scan: collect the inode(s) of process.stdout / process.stderr (we own the read ends, so fstat is reliable), walk every PID, look for a matching pipe:[N] symlink whose fdinfo flags indicate a writer (access_mode != O_RDONLY), SIGKILL it. Inode keying makes this race-safe across concurrent executions in the same container — each Popen gets a fresh pipe with a unique inode, so a stuck execution never kills writers belonging to another execution.

Behaviour on macOS / non-Linux: helper returns 0 immediately (no /proc).

Test plan

  • tests/unit/test_subprocess_pgroup.py::TestKillPipeWriteHolders::test_kills_setsid_escapee_holding_pipe — full repro: parent forks grandchild that calls os.setsid() and writes heartbeats to stdout; asserts drain_reader_threads completes in <3s (proving kernel EOF via helper, not the force-close fallback)
  • test_returns_zero_on_no_writers — read-end-only pipe → 0 kills
  • test_handles_none_and_closed_pipes — None / closed handles skipped silently
  • All 11 existing tests in the file still pass (additive change; helper returns 0 in the in-group-grandchild scenarios they cover)
  • Full file: 14 passed in 3.30s

Fixes #548

vybe and others added 2 commits May 1, 2026 09:57
The canvas is inside v-if="voice.isActive.value" so canvasEl.value is
null when onMounted fires. renderFrame() exits early without scheduling
the next frame, killing the loop permanently.

Replace onMounted initialization with watch(canvasEl) so the RAF loop
starts when the canvas enters the DOM and stops when it leaves.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Async Stop hooks (e.g. git-push spawning ssh) and some MCP launchers fork
detached children via setsid(), placing them in a brand-new session that
killpg(pgid) cannot reach. Those children keep our stdout/stderr pipe
write-ends open, so drain_reader_threads' post-kill natural-drain window
expires without seeing EOF and the final result JSON line is dropped.
Executions are recorded as failed with "0 tool calls" even when tools ran.

Add _kill_pipe_write_holders(): a Linux-only /proc/*/fd scan keyed on the
pipe inode of our subprocess.Popen pipes. After the group kill in
drain_reader_threads, SIGKILL any non-self process holding a writable
handle to one of our pipes. Inode keying makes this race-safe across
concurrent executions in the same container — every Popen allocates a
fresh pipe with a unique inode.

Regression test test_kills_setsid_escapee_holding_pipe spawns a parent
that forks a grandchild which os.setsid()'s and then writes heartbeats to
stdout. Asserts drain completes in <3s, proving the kernel EOF'd via the
helper instead of falling through to the force-close path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dolho

dolho commented May 1, 2026

Copy link
Copy Markdown
Contributor Author

End-to-end verification in production base image

Built trinity-agent-base:latest from this branch and ran the full code path against a real setsid escapee — the actual bug pathology, not a mock. Test child writes a sentinel to stdout, then forks a grandchild that calls os.setsid() (escaping the process group, exactly like ssh spawned from a git push async Stop hook) and keeps stdout open. Then invokes drain_reader_threads() from the installed production module.

Result

[WARNING] [Subprocess] Reader thread(s) still busy after process exit
         (pid=7, stuck_count=1) — killing process group, then waiting 8s for natural drain
[WARNING] [Subprocess] Killed 1 setsid-escapee process(es) holding pipe
         write-end after group kill (pid=7)

After parent exit: reader_alive=True lines_captured=3
                   sample=['CLAUDE_OUTPUT_LINE', 'escapee_heartbeat', 'escapee_heartbeat']
Drain elapsed: 0.00s
Reader alive after drain: False
Sentinel captured: True
Total lines captured: 3
RESULT: PASS — fix works in production base image runtime

What this proves

Check Result
Bug pathology reproduced (reader stuck after parent exit) reader_alive=True before drain
New helper warning surfaces in real runtime Killed 1 setsid-escapee process(es) log fires
Kernel EOF'd via helper, not via force-close fallback Drain elapsed 0.00s (would be 8s+ if it fell through to safe_close_pipes)
Pre-fork data preserved (regression for #531) CLAUDE_OUTPUT_LINE captured + 2 escapee heartbeats
/proc/*/fd enumeration works in container Yes
os.kill() permission works under CAP_DROP: ALL + NET_BIND_SERVICE Yes (process is owned by same uid, no extra caps needed for self-uid kill)

Coverage summary

  1. Host unit tests — 14/14 pass (incl. new test_kills_setsid_escapee_holding_pipe, test_returns_zero_on_no_writers, test_handles_none_and_closed_pipes)
  2. Same unit tests run inside trinity-agent-base:latest — 14/14 pass (Python 3.11, container /proc)
  3. End-to-end — production module + real os.fork() + real os.setsid() + real drain_reader_threads() call, all inside the new base image — pass with the new WARNING log line firing exactly once

@dolho
dolho requested a review from vybe May 1, 2026 09:38
@vybe

vybe commented May 1, 2026

Copy link
Copy Markdown
Contributor

Base branch needs to be retargeted before merge.

This PR targets `main` directly. Per the Trinity SDLC, all feature branches must land through `dev` first:

`feature/*` → `dev` → `main`

Please change the base branch to `dev`:

```bash
gh pr edit 619 --base dev
```

The inode-keyed /proc scan is solid and the Linux-only guard is correct — this is the only change needed before merge.

Reminder for whoever merges this: run `./scripts/deploy/build-base-image.sh` after merge — the fix lives in `docker/base-image/` and won't reach running agents until the base image is rebuilt.

@dolho
dolho changed the base branch from main to dev May 4, 2026 07:17
@vybe

vybe commented May 4, 2026

Copy link
Copy Markdown
Contributor

Closing as superseded. Both changes in this PR are already in dev:

No code from this PR was lost. Safe to close.

@vybe vybe closed this May 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: child processes inherit agent-server stdout pipe → reader thread stuck, false '0 tool calls' failures

2 participants