Skip to content

fix(#970): bound headless wait on result message + tool stall - #973

Merged
vybe merged 1 commit into
devfrom
fix/970-headless-reader-thread-leak
Jun 2, 2026
Merged

fix(#970): bound headless wait on result message + tool stall#973
vybe merged 1 commit into
devfrom
fix/970-headless-reader-thread-leak

Conversation

@obasilakis

Copy link
Copy Markdown
Contributor

Summary

Fixes the #970 "2h false-timeout" class of failure: a scheduled task wedged for the full execution_timeout_seconds (up to 2h), got marked failed, and held its capacity slot the entire time — so later schedules failed with Agent at capacity (N/N parallel tasks running) even with no real work in flight.

The fix is ~30 functional LOC in _run_headless_subprocess, reusing existing infrastructure. No new thread, service, or file.

Root cause (two distinct defects, one symptom)

  1. The executor finalized on process EXIT, not on claude's response. read_stdout had no break on {"type":"result"} — it looped to pipe-EOF (process exit) while the main thread parked on process.wait(timeout=effective_timeout). When claude emitted its result (turn definitively over) but the process lingered in teardown (e.g. a stdio MCP child holding the pipe), the budget burned on already-complete work.

  2. Claude Code has no per-MCP-tool timeout. A hung stdio MCP tools/call blocks claude forever (only BASH_*/API_TIMEOUT_MS exist; MCP_TOOL_TIMEOUT is not a real env var). The open tool_use never gets a tool_result.

The fix

Replace the single blocking process.wait with a polling loop:

  • Early-completion — on result_seen, finalize with the captured result and force return_code=0. Genuine errors (max_turns, rate_limit, auth) are already classified into metadata.error_type from the stream and still surface in _finalize_headless_result (which runs its error_type checks before the return_code check). Converts a 2h false-FAILURE into an immediate SUCCESS. Non-heuristic — the result message is the end of a claude --print turn.
  • Stall watchdog_open_tool_exceeding() raises TimeoutExpired when an open tool_use has had no tool_result for >300s, reusing the stream parser's existing tool_start_times bookkeeping. Heuristic, generous limit (covers the MCP-hang case where no result is ever emitted).
  • Overall effective_timeout unchanged as the backstop.

Both reuse the existing _terminate_process_group kill and follow the file's existing detect-from-stream → kill-early pattern (auth-abort #285, permission-mode validation).


Why the issue's suggested fixes are wrong

The issue is titled a "reader thread leak" and proposes (1) decoupling the orchestrator from the leaked reader, and (2) wider pipe-holder discovery via lsof//proc. Both are wrong, and the issue's own evidence proves it.

The 2h is spent in process.wait, not the reader drain.

  • Reader threads live in the agent-server process and read claude's stdout pipe. They cannot keep the claude child alive, so they are mechanically incapable of extending process.wait(claude_pid). The only place a stuck reader costs wall-clock is the drain phase, which is hard-capped at 90s by _drain_bounded (bug: agent-server.py spins at 90% CPU on OAuth token auth failure, blocking CB recovery #728). A leaked reader therefore bounds to ≤90s — never 2h.
  • The issue's own number proves it: duration_ms ≈ 7,215,000 = 7,200,000 (budget) + ~15,000 (post-kill drain). That decomposition only fits "time spent in process.wait, then a short cleanup." If the drain were the hang, you would never see the full 7200s.

Suggested fix #1 (decouple from the leaked reader) is already implemented and cannot reduce a 2h hang. _drain_bounded (Issue #728) already runs the drain in a daemon thread and returns within _DRAIN_BUDGET_SECONDS = 90 regardless of whether a reader leaked. The orchestrator does not block on the leaked thread. Since the 2h is in process.wait (which runs before the drain), bounding the drain tighter saves nothing.

Suggested fix #2 (lsof//proc pipe-holder hunt) is dead weight and a regression risk. Issue #817 already replaced per-FD pipe-writer scanning with kill_cgroup_orphans() — killing on cgroup membership, "the inescapable boundary" that catches escapees regardless of setsid/FD-detachment/env-stripping. Re-introducing an lsof//proc/*/fd scan brings back exactly the os.stat D-state deadlock that #817 removed, for a case that's already covered.

Net: the "reader leaked" / outcome=leaked log lines the issue points at are emitted by the post-timeout cleanup (after the budget was already burned in process.wait), not the cause of the hang. The leak is a symptom; the wait-on-exit + missing tool timeout is the cause.


Reproduction evidence

  • Slot squat (issue symptom Feature/process engine #6) reproduced against a live agent with max_parallel_tasks=1: a wedged execution held the slot, and a second schedule failed at admission with the verbatim Agent at capacity (1/1 parallel tasks running) (duration_ms=74, never ran claude) via the scheduler's overflow_policy="reject" path (task_execution_service.py).
  • Live trace of the wedge showed the budget burned in process.wait on a hung stdio MCP tools/call, with the slot held the whole time.

Test plan

  • New unit tests: tests/unit/test_headless_executor_970_timeout.py (9 tests) — _open_tool_exceeding (5 pure cases) + the 4 wait-loop branches (early-completion forces return_code=0, stall raises, natural-exit regression, budget-timeout regression) driven by a fake Popen.
  • Adjacent subprocess/headless tests green (drain_bounded, subprocess_pgroup, pipe_drop, error_classification, jsonl_recovery, orphan_sweep, wallclock_timeout).
  • Full unit suite: 1790 passed, 4 skipped, 0 failed.

The headless executor finalized on process EXIT, not on claude's
response, and Claude Code has no per-MCP-tool timeout. So a hung stdio
MCP tools/call — or a process that lingers in teardown after emitting
its result — wedged process.wait for the full execution budget: the 2h
false-timeout, with the capacity slot held the entire time.

Replace the single blocking process.wait in _run_headless_subprocess
with a polling loop:

- early-completion: when claude emits {"type":"result"} (turn is
  definitively over) but the process won't exit, finalize with the
  captured result and force return_code=0. Genuine errors (max_turns,
  rate_limit, auth) were already classified into metadata.error_type
  from the stream and are still surfaced by _finalize_headless_result.
  Turns a 2h false-FAILURE into an immediate SUCCESS.

- stall watchdog: _open_tool_exceeding() raises when an open tool_use
  has had no tool_result for >300s, reusing the stream parser's
  existing tool_start_times bookkeeping.

- overall effective_timeout budget unchanged as the backstop.

Both reuse the existing _terminate_process_group kill and follow the
file's existing detect-from-stream/kill-early pattern (auth-abort #285,
permission-mode validation). No new thread, service, or file.
@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown

⚠️ 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.

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved via /validate-pr with a root-cause adjudication against current dev.

Verified in code that the 2h hang lives in process.wait(timeout=effective_timeout) (headless_executor.py:588), BEFORE the drain — and that the drain is already hard-bounded to 90s and already decoupled from the leaked reader (_drain_bounded daemon thread + done.wait(90) with no join, subprocess_lifecycle.py:79-87). The arithmetic (7,215,000ms ≈ 7,200,000 budget + ~15,000 drain) confirms the wait, not the drain, is the bottleneck. This PR's early-completion-on-result + stall-watchdog attacks the actual bottleneck; the early return_code=0 is safe because genuine errors are classified into metadata.error_type and surfaced in _finalize_headless_result before the return_code check (lines 633-648).

CI 14/14, security clean, 9 unit tests across all wait-loop branches.

@vybe
vybe merged commit b4c5bfd into dev Jun 2, 2026
14 checks passed
dolho added a commit that referenced this pull request Jun 12, 2026
…1025)

Salvages the two orthogonal robustness improvements from the closed PR #980
that the #973 fix did not include. Pure hardening of the already-bounded
drain/finalize path — defense-in-depth, not a behavioural change on the
clean path.

D20 — capture daemon-thread exceptions in `_drain_bounded`
  `subprocess_lifecycle._drain_bounded` swallowed every drain exception via
  `except Exception: pass`, making a drain that raised indistinguishable
  from a clean completion (and hiding the leaked reader). It now captures +
  logs the exception and returns a `DrainOutcome`
  (`completed` | `budget_exceeded` | `errored`) so the caller can tell the
  leaked-reader cases apart. Existing callers that ignore the return value
  are unaffected.

D19 — `_finalize_headless_result` snapshot isolation
  On the budget-exceeded / errored drain path a reader thread is leaked and
  may still be mutating ctx's shared buffers / metadata. The headless run
  context now carries `drain_budget_exceeded`; when set, finalize rebinds
  `ctx` to a deep snapshot (`list(...)` of each buffer +
  `metadata.model_copy(deep=True)`) via `_snapshot_for_finalize`, which is
  retry-guarded against the "changed size during iteration" race and falls
  back to the live ctx if every attempt loses. Clean drains keep the
  zero-copy fast path.

Out of scope (per the issue): the #980 asyncio.wait_for margin widening and
the #970 lsof/proc pipe-holder hunt — both correctly excluded.

Tests: `_drain_bounded` outcome matrix (completed/budget_exceeded/errored +
no-swallow logging) in test_drain_bounded.py; snapshot list/metadata
isolation, retry-exhaustion fallback, and default-off fast path in
test_headless_finalize_snapshot.py. Full related suite: 130 passed.

Related to #1025

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vybe pushed a commit that referenced this pull request Jun 16, 2026
…1025) (#1078)

* fix(headless): harden drain/finalize against leaked reader threads (#1025)

Salvages the two orthogonal robustness improvements from the closed PR #980
that the #973 fix did not include. Pure hardening of the already-bounded
drain/finalize path — defense-in-depth, not a behavioural change on the
clean path.

D20 — capture daemon-thread exceptions in `_drain_bounded`
  `subprocess_lifecycle._drain_bounded` swallowed every drain exception via
  `except Exception: pass`, making a drain that raised indistinguishable
  from a clean completion (and hiding the leaked reader). It now captures +
  logs the exception and returns a `DrainOutcome`
  (`completed` | `budget_exceeded` | `errored`) so the caller can tell the
  leaked-reader cases apart. Existing callers that ignore the return value
  are unaffected.

D19 — `_finalize_headless_result` snapshot isolation
  On the budget-exceeded / errored drain path a reader thread is leaked and
  may still be mutating ctx's shared buffers / metadata. The headless run
  context now carries `drain_budget_exceeded`; when set, finalize rebinds
  `ctx` to a deep snapshot (`list(...)` of each buffer +
  `metadata.model_copy(deep=True)`) via `_snapshot_for_finalize`, which is
  retry-guarded against the "changed size during iteration" race and falls
  back to the live ctx if every attempt loses. Clean drains keep the
  zero-copy fast path.

Out of scope (per the issue): the #980 asyncio.wait_for margin widening and
the #970 lsof/proc pipe-holder hunt — both correctly excluded.

Tests: `_drain_bounded` outcome matrix (completed/budget_exceeded/errored +
no-swallow logging) in test_drain_bounded.py; snapshot list/metadata
isolation, retry-exhaustion fallback, and default-off fast path in
test_headless_finalize_snapshot.py. Full related suite: 130 passed.

Related to #1025

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

* fix(headless): address review findings on the drain/finalize hardening (#1025)

Self-review of the #1025 PR surfaced two correctness gaps in the hardening
itself, plus a dead assignment and a redundant Event. Fixed:

1. Leaked-reader outcome was missed (strongest finding). drain_reader_threads
   force-closes and *returns normally* when a grandchild-held reader won't EOF
   (its own outcome=leaked METRIC, #586), so a within-budget, non-raising drain
   did NOT prove the readers were dead — _drain_bounded returned "completed",
   finalize skipped the snapshot, and read buffers a confirmed-alive reader was
   still mutating. _drain_bounded now adds a "leaked" DrainOutcome: after a
   within-budget return it checks whether any reader thread it was handed is
   still alive (no dependency on drain_reader_threads internals). The headless
   flag is renamed reader_may_be_live and set on `drain_outcome != "completed"`
   so budget_exceeded / errored / leaked all trigger the snapshot.

2. Snapshot didn't isolate the auth-abort signal. finalize reads
   auth_abort_event.is_set() / auth_abort_reason[0], but the stderr reader (the
   thread that leaks) mutates them — a late auth match could flip a success to a
   spurious 503 (→ SUB-003 auto-switch). _snapshot_for_finalize now freezes both
   (a fresh Event mirroring is_set() via _freeze_event + a list copy), so the
   snapshot's "every field finalize reads" claim holds.

3. Dropped the dead reader_may_be_live assignment on the TimeoutExpired path —
   that path re-raises to HTTP 504 without finalizing, so nothing reads it
   (replaced with a comment).

4. Collapsed the redundant `errored` Event in _drain_bounded to a single
   write-once `outcome` cell set by the daemon thread and read after the `done`
   barrier — removes the load-bearing two-Event ordering invariant.

Tests: new "leaked"-outcome case (within-budget drain leaving a live reader)
and auth-abort freeze isolation; renamed the flag test. 21 passed locally.
Re-verified against a live agent-server: clean task still 200s through the
fast path with zero leaked/snapshot warnings.

Related to #1025

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

2 participants