fix(agent-server): resolve #728 drain-deadlock — sweep orphans before pipe close, bound all close paths

[nyasour]

Problem

When an agent has a persistent MCP connection (a remote streamable-HTTP MCP server, native type:http or via an npx mcp-remote stdio bridge), claude --print completes its work — the MCP tool calls succeed (the remote returns HTTP 200s) — but a setsid'd grandchild (or the live connection) keeps claude's stdout write-end open past SIGKILL. Every such execution then hangs the drain, gets SIGKILLed after the 90s budget, discards the model's already-produced output (empty result), and trips the agent circuit breaker. With an empty .mcp.json it works fine, which is what pins the trigger to a lingering pipe holder.

Observed signature (base image 0.8.0):

[Subprocess] Reader thread(s) still busy after process exit ... killing process group
[Subprocess] Drain budget (90s) exceeded — safe_close_pipes may have deadlocked with reader thread's TextIOWrapper lock ... Issue #728.
[Subprocess] #1502: SIGKILLed the process group ...
[METRIC] drain_outcome ... outcome=leaked ... leaked_count=1

Root cause

A lock-ordering deadlock in drain_reader_threads:

1. The stdout reader thread is parked in a blocking readline(), holding CPython's TextIOWrapper buffer lock.
2. safe_close_pipes() → TextIOWrapper.close() was called synchronously in the async coroutine; close() needs that same lock → deadlock.
3. kill_cgroup_orphans() — the only thing that can SIGKILL the pipe-holder so the reader hits EOF and releases the lock — ran in a finally after the deadlocked close, so it never fired.

Fix

• drain_reader_threads: run the cgroup orphan sweep before the force-close (holder dies → reader EOFs → lock released → close is a fast no-op), and bound the close itself via _bounded_safe_close_pipes (asyncio.wait_for + asyncio.to_thread). The finally backstop sweep and the bug: drain_reader_threads closes stdout pipe before reader can drain backlog — silently loses final result line on long agentic tasks #531 natural-drain-before-kill ordering are preserved.
• HeadlessRunContext.terminate() (second deadlock-exposed site): switched from asyncio.run(_bounded_safe_close_pipes(...)) to a new sync _bounded_safe_close_pipes_sync (daemon thread + join(timeout)). asyncio.run()'s loop teardown joins the leaked non-daemon to_thread worker for up to THREAD_JOIN_TIMEOUT (300s on CPython 3.13), silently defeating the 5s bound and pinning a shared executor thread — a daemon thread is never joined at teardown, so the true bound holds.
• claude_code.py / codex_runtime.py: their await loop.run_in_executor(None, _safe_close_pipes, process) calls were fully unbounded (never resolve if the escapee is alive). Routed through the bounded async helper (5s).

Tests

tests/unit/test_subprocess_pgroup.py:

• A fork-based test where a setsid'd grandchild holds stdout open for 20s (past post_kill_grace) — the exact condition existing tests never covered. FAILS on pre-fix code (drain took 20.01s), PASSES post-fix (~1s).
• TestBoundedSafeClosePipes — the sync + async bounded helpers each return within their timeout under a genuinely-wedged safe_close_pipes.

pytest tests/unit/test_subprocess_pgroup.py tests/unit/test_drain_bounded.py tests/unit/test_headless_executor_pipe_drop.py tests/unit/test_headless_executor_970_timeout.py tests/unit/test_codex_runtime.py -q → 167 passed, 1 skipped (pre-existing Linux-only skip; ran on macOS).

Context

Closes the gap behind the #1502 band-aid; addresses the deadlock tracked as #728 and reported in discussion #1696.

Deferred follow-ups (kept out to stay focused)

• Add the cgroup sweep at the claude_code/codex_runtime close sites (they currently only pgid-kill before closing).
• A JSONL-recovery safety net so output is never lost even on a pathological drain.

🤖 Generated with Claude Code

[vybe]

Retargeted `main` → `dev` (per SDLC, feature/fix PRs land on `dev`; `main` is release-cut only).

That retarget surfaces a conflict, but only from branch history — the branch was cut from `main`, which has diverged from `dev` (124 commits ahead / 11 behind). The change itself applies cleanly:

• `git cherry-pick` of 5c8b684 onto `dev` → clean, zero conflicts, 7 files +482/-42 (identical stat).
• Only headless_executor.py differs between the branches, and in a different region: `dev` carries the bug: agent-server masks error_during_execution as a successful context: fork placeholder — failed session resumes recorded as success, Sessions-tab fallback never fires #1673 `error_type == "execution_error"` hunk at ~L1047, this PR touches L54/L357/L374. Both coexist after the rebase.
• `pytest tests/unit/test_subprocess_pgroup.py tests/unit/test_codex_runtime.py` on the rebased tree → 105 passed, 1 skipped.

Could you rebase onto `dev` and force-push? I could not do it for you — the fork is org-owned, so GitHub's maintainer-push does not apply and I get a 403.

```bash
git fetch upstream dev && git rebase upstream/dev && git push --force-with-lease
```

Everything else validated: required checks green, security scan clean, closing keyword present (`resolve #728` → auto-promotes on merge), tests cover the non-happy paths.

[github-actions]

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

[AndriiPasternak31]

Validated via /validate-pr. The engineering here is the strongest thing I've reviewed this week — the root-cause analysis is correct and the test is a real reproduction, not a mock. Requesting changes on process only; I have no objection to the fix itself.

What holds up

The lock-ordering diagnosis is right. safe_close_pipes() needs the same TextIOWrapper buffer lock the reader thread holds while parked in a blocking read, so calling it synchronously while the pipe-holder is still alive deadlocks the coroutine — and a deadlocked coroutine never reaches the trailing finally, which is why the #817 sweep that could have freed the lock never fired. Moving _sweep_cgroup_orphans() ahead of the force-close in subprocess_pgroup.py attacks the actual causal chain rather than widening a timeout.

The asyncio.run() catch in HeadlessRunContext.terminate() is the kind of thing that normally ships undetected: loop.shutdown_default_executor() joins the leaked non-daemon to_thread worker for up to THREAD_JOIN_TIMEOUT (300s on CPython 3.13), so the "5s bound" would have been a 5-minute bound in exactly the pathological case it exists for. Using a daemon thread in _bounded_safe_close_pipes_sync is the correct escape.

test_drained_via_sweep_before_close_when_grandchild_survives_entire_window is the test the previous fixes in this lineage were missing — a real setsid()'d grandchild outliving the entire drain window, failing pre-fix at 20.01s and passing post-fix in ~1s, with the sweep stubbed only to stay cross-platform while still performing a real SIGKILL of the real holder. The #1502 and #1661 follow-ups exist because the earlier tests couldn't reach this branch.

Blocking

1. The PR closes no open issue. The closing keyword targets #728, which is closed and is a different bug (agent-server.py spins at 90% CPU on OAuth token auth failure). The whole lineage is closed — #728 → #1502 → #1661 — and #1661 is titled "#728 fix incomplete", which is what this PR is actually resolving. As written the keyword is a no-op and the work traces to nothing open. Please reopen #1661 (or file a fresh bug for this deadlock) and re-point the reference at it, so the status automation and the release notes pick it up.

2. No CI has run. gh pr checks reports no checks at all on this branch — this is a fork PR, so the workflows need a maintainer to approve the run. A change to the agent base-image runtime can't merge on a local test run alone. A maintainer will need to approve the workflow; given docker/base-image/** is touched, this also wants a /verify-local pass with the agent stage (base image rebuild + real agent boot), not just the backend stages.

Non-blocking

3. The async helper still pins a shared pool thread. _bounded_safe_close_pipes uses asyncio.to_thread, so on timeout it abandons a non-daemon worker from the loop's default ThreadPoolExecutor — the exact hazard you correctly avoided in the sync variant. The inline comment argues it's harmless because the app loop is long-lived and never torn down, and that's true for the bound; the cost is different. That pool is capped at min(32, cpu_count + 4) and is shared process-wide with every other to_thread/run_in_executor(None, ...) caller in the agent server — the git auto-sync maintenance cycle (#1595) among them. So each wedge permanently consumes a slot from a small shared pool, and the failure mode isn't a leaked thread, it's unrelated subsystems stalling once the pool is exhausted. A leaked reader thread is a plain thread and costs nothing shared; this one isn't. Routing both helpers through the same daemon-thread primitive would make the async path leak as cheaply as the sync one.

4. Diff display is misleading. GitHub renders this as 32 files / +2035, including the .claude and src/backend/enterprise submodule pointers and two research PDFs. That's a stale merge-base artifact from the branch being cut off main before those commits reached dev — verified locally, the true delta against current dev is 7 files / +482. A rebase onto dev would make the diff reviewable at a glance and is worth doing before this gets more eyes.

Happy to re-review as soon as the issue reference points somewhere open and CI has actually run.