fix(agent-runtime): decouple headless executor from leaked reader threads (#970) - #980
Closed
AndriiPasternak31 wants to merge 2 commits into
Closed
fix(agent-runtime): decouple headless executor from leaked reader threads (#970)#980AndriiPasternak31 wants to merge 2 commits into
AndriiPasternak31 wants to merge 2 commits into
Conversation
…eads (#970) Phase 1 of #970. When an agent's headless task spawns grandchildren (stdio MCP servers, hooks, subagents) that hold the stdout pipe FD open after Claude exits, the reader thread wedges in readline(). The drain's force-close fallback could not unblock it, so the orchestrator blocked until the full task timeout (~2h), saturating execution slots and producing false "timed out after 7200s" failures even though Claude finished in seconds. - _drain_bounded returns a DrainOutcome literal ("completed"/"budget_exceeded"/"errored") instead of None, and captures+logs exceptions from the daemon thread instead of swallowing them (was the silent `except Exception: pass`). - _run_headless_subprocess records ctx.drain_budget_exceeded from the outcome and snapshots the PRE-drain stdout_exc, so a leaked reader appending late can't mask the #285 permission-mode fast-fail (D16). - _finalize_headless_result snapshots every field it reads (list copies + metadata.model_copy(deep=True)) on the budget-exceeded path so a still-leaked reader can't tear a read; _snapshot_for_finalize retries the deep-copy against the same race and falls back to the live ctx with a warning (D19). - Outer asyncio.wait_for margin widened from +60 to +_DRAIN_BUDGET_SECONDS+30 so the outer net can't fire mid-drain and mislabel a bounded drain (D6). - Per-process leaked-reader gauge (get_leaked_reader_thread_total) sourced from the real force-close leaked_count, lock-guarded (D21); Phase 1 forensic [Drain] logging to characterize the natural-drain overrun (D8). - Chat path calls _drain_bounded fire-and-forget; chat budget-exceeded recovery is a deferred follow-up (D18). Phase 2 (make safe_close_pipes abandonable + emit outcome=hard_timeout) is a follow-up ship. Tests: 36 new/updated unit tests; verified in the real agent base-image runtime; full unit suite green (the only failures are pre-existing/environmental, identical on origin/dev). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sync feature-flow docs with the #970 change (headless executor decoupled from leaked reader threads): - parallel-headless-execution.md: add a 2026-05-29 revision entry for the headless-executor-side changes — HeadlessRunContext.drain_budget_exceeded, pre-drain stdout_exc snapshot (D16), _finalize_headless_result snapshot via _snapshot_for_finalize on the budget-exceeded path (D19), and the widened outer asyncio.wait_for margin (D6). The drain-side _drain_bounded return contract was already documented in execution-termination.md by the #970 commit itself. - feature-flows.md: add a #970 changelog row, consistent with the sibling drain fixes (#586/#808/#912). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Resolve by running |
Contributor
|
Closing as superseded by #973 (merged to Why: This PR implements issue #970's suggested fix #1 — "decouple the orchestrator from the leaked reader so the platform-visible hang is bounded to the ~90s drain budget." But that decoupling and that 90s bound already exist on Your work is not lost. Two improvements here are genuinely valuable and orthogonal to #973 — the daemon-thread exception capture (D20, replacing the silent Branch |
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>
6 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.
Summary
Phase 1 fix for #970 (P1). When an agent's headless task spawns grandchild processes (stdio MCP servers, hooks, subagents) that hold the stdout pipe FD open after Claude exits, the executor's stdout reader thread wedges in
readline(). The drain's force-close fallback couldn't unblock it, so_run_headless_subprocessblocked until the full task timeout (~2h) — saturating execution slots and recording falsetimed out after 7200sfailures even though Claude finished in seconds (evidence: 24 leaked drains over 47h on one agent, each 1:1 with a 7200s false-timeout).This PR decouples the orchestrator from the leaked reader so the platform-visible hang is bounded to the drain budget (~90s).
Changes
subprocess_lifecycle.py—_drain_boundedreturns aDrainOutcomeliteral (completed/budget_exceeded/errored) instead ofNone, and captures + logs daemon-thread exceptions instead of the silentexcept Exception: pass(D20).headless_executor.pyHeadlessRunContext.drain_budget_exceededflag, set from the drain outcome at both callsites.stdout_excso a leaked reader can't mask the bug: expired subscription token causes hour-long zombie executions instead of fast failure #285 permission-mode fast-fail (D16)._finalize_headless_resultsnapshots every field it reads (list(...)+metadata.model_copy(deep=True)) on the budget-exceeded path;_snapshot_for_finalizeretries the deep-copy against the same race and falls back to the live ctx with a warning (D19). In-memoryresponse_partsprecedence over JSONL preserved (Async chat_with_agent: long execution silently fails with null response (reader-thread) #678 / D17).asyncio.wait_formargin widened+60→+_DRAIN_BUDGET_SECONDS+30so it can't fire mid-drain (D6).subprocess_pgroup.py— per-process leaked-reader gauge sourced from the real force-closeleaked_count, lock-guarded (D21); Phase 1 forensic[Drain]INFO logging (D8).claude_code.py— chat path calls_drain_boundedfire-and-forget; chat budget-exceeded recovery deferred (D18).Implementation matches the 25-decision
/autoplan+/plan-eng-review(incl. Codex outside-voice) plan. Phase 2 (makesafe_close_pipesabandonable + emitoutcome=hard_timeout) is a deferred follow-up ship.Test Coverage
36 new/updated unit tests covering every new path:
test_drain_bounded.py—completed/budget_exceeded/erroredoutcomes; errored is logged with traceback, not swallowed.test_headless_drain_budget.py— drain-outcome plumbing, pre-drainstdout_excprecedence, compound timeout+wedge bounding, full-field snapshot isolation, snapshot retry + fallback, end-to-end JSONL recovery within the outer margin.test_chat_drain_contract.py— pins the chat fire-and-forget contract (no binding/branching on the return).test_subprocess_pgroup.py— leaked gauge increments from the realleaked_count.Verification (Docker)
All runs in throwaway containers; the live stack was not touched.
trinity-backend:latest): 4 changed test files → 36 passed. Related-module regression sweep (15 files) → 143 passed, 0 failed.pytest unit/ -m "not slow", pytest-randomly): 1790 passed, 10 failed, 3 skipped. The 10 failures (test_git_pull_branch,test_orphaned_execution_recovery,test_reset_preserve_state_guardrails) are pre-existing/environmental (no Redis / git env in the throwaway container) — proven by running them on a cleanorigin/devworktree where they fail identically. Unrelated to the touched modules.trinity-agent-base:latest): smoke test confirms the fixed modules import and function in the actual agent runtime (leaked gauge,_drain_boundedliteral,drain_budget_exceededdefault, snapshot isolation,_DRAIN_BUDGET_SECONDS=90,claude_codeimports) — no boot-crash regression.tests/lint_sys_modules.py: PASS (no new violations; 7 retired).Pre-Landing Review
Self + adversarial review found no issues: the
errored/doneevent ordering is correct (errored fully settled once done is set), the pre-drainstdout_excsnapshot preserves the #285 fast-fail, the snapshot retry targets the right operation (model_copydict-iteration;list()of a list can't raise), and the widened outer margin keeps the inner machinery first-to-fire while remaining backend-bound.Plan Completion
Phase 1: all items DONE (D5, D6, D8, D16–D21 verified present in the diff). Phase 2 intentionally deferred to a follow-up ship per the plan.
Refs #970 — Phase 1 of 2; intentionally does not auto-close so #970 can track the Phase 2 follow-up (
safe_close_pipesabandonable +outcome=hard_timeout). The 2h false-timeout class is resolved by this phase.🤖 Generated with Claude Code