Reap leaked Claude subagent watchers with an idle ceiling - #517
Conversation
A Claude subagent watcher's only exits were the SubagentStop-driven StopWatcher signal and the parent-exit watchdog, so a missed SubagentStop leaked the watcher for the entire life of the parent session. Observed on a 13-day-old session: 8 watchers still alive against subagents that had finished between 1 and 12 days earlier, ~40 MB RSS each. ShouldEndOnIdle now admits Claude CHILD watchers. Its session watcher is deliberately excluded — that one has a working parent-exit watchdog and a sessionEnd hook, so a ceiling there could end a live session out from under a thinking user. Transcript silence alone is not enough to reap: a subagent running a long build writes nothing between its tool_use and the matching tool_result. UpdateClaudePendingToolCalls tracks those ids so an in-flight tool suppresses the ceiling, and the window itself is a generous 6h (KCAP_CLAUDE_SUBAGENT_IDLE_MINUTES) because this is a leak backstop rather than an end-of-conversation detector. Nothing depends on reaping promptly — a child watcher posts no session-end — whereas reaping a live subagent would drop the rest of its transcript. The per-vendor idle window moves out of RunWatch into ResolveIdleWindow so the Claude child mapping is covered, and the drain-loop gate becomes TracksClaudeToolCalls so it cannot silently drift out of step with the watchers the ceiling applies to. Closes #514 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR Summary by QodoReap leaked Claude subagent watchers via idle ceiling + tool-in-flight guard
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc1042cf9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| foreach (var line in newLines) { | ||
| UpdateClaudePendingToolCalls(state.PendingClaudeToolCalls, line); |
There was a problem hiding this comment.
Track Claude tool calls from the raw drainRead.Lines, not newLines: newLines has already passed through SecretRedactor.RedactLine, which replaces every line over 64 KiB with OversizeLinePlaceholder. Consequently, an oversized tool_result never removes its ID and permanently suppresses this new idle ceiling, so a missed SubagentStop still leaks the watcher indefinitely; conversely, an oversized tool_use is never added and a genuinely running tool can be reaped after the ceiling.
Useful? React with 👍 / 👎.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Both bugs were real and are fixed in 2ca06b5. Thanks — the first one defeated the entire feature. Tool ids tracked from redacted lines (codex P1) — confirmed, fixed. Regression test Tool state lost on resume (qodo) — confirmed, fixed. I took the full-scan option rather than a bounded window — a Verbose comments (qodo) — fair, trimmed. One thing I did not change: |
Two defects in the idle ceiling, both found by the automated reviewers. The tracker was fed the redacted line list. RedactLine swaps any line over 64 KiB for a placeholder carrying no tool ids, and oversized tool_results are routine — a big file read or a build log. That id was then never cleared, toolInFlight stayed true forever, and the ceiling never fired: the leak silently reinstated on exactly the busy sessions that leak most. Feed it drainRead.Lines instead. Only ids are read; no raw content leaves the process. A watcher that reconnects mid-tool resumes at the server's line cursor and never sees the tool_use that opened before it, so the pending set came up empty and a live subagent was eligible for reaping — the one outcome the generous window exists to prevent. BackfillClaudePendingToolCallsAsync rebuilds the set from [0, cursor) once at startup, opened FileShare.ReadWrite because the agent is still writing the file, and failing soft: losing the backstop beats failing a watcher's startup. Also trims the comments added in the previous commit, per the repo's keep-comments-minimal rule. Note the same redaction exposure exists for UpdateCodexPendingToolCalls, which also reads the redacted list. Left alone here — pre-existing and outside this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2ca06b5 to
cf26c0b
Compare
|
Code review by qodo was updated up to the latest commit 2ca06b5 |
…le-ceiling # Conflicts: # src/Capacitor.Cli/Commands/WatchCommand.cs
…le-ceiling # Conflicts: # src/Capacitor.Cli.Core/Models.cs # src/Capacitor.Cli/Commands/WatchCommand.cs
Review follow-ups. The backfill parsed every line from the file start to the resume cursor, putting O(N) JSON parsing on the watcher's startup path for what is a best-effort backstop. It now parses only the last 512 lines before the cursor. That window is sound rather than arbitrary: an unfinished tool sits at the tail by construction, since nothing is appended between its tool_use and the matching tool_result. It also stops a stale unmatched tool_use from an interrupted turn stranding an id and suppressing the ceiling forever — the same failure shape as the redaction bug. Lines before the window are still read (there is no cheap seek to line N) but never parsed. Cancellation is no longer reported as a failure: the seed-quarantine path cancels and then calls straight into the backfill, so an expected shutdown logged a misleading "backfill skipped". Returns early when already cancelled and catches OperationCanceledException separately. The oversize test payload is now sized off SecretRedactor.MaxRedactableLineChars instead of a hard-coded 70 KiB, so changing the threshold cannot quietly turn those into tests of the small-line path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All three addressed in 50e5fb0, plus the merge with #526. 1. Unbounded backfill scan — fixed, and it was hiding a correctness bug. So the window isn't just cheaper, it's more correct. A full scan would keep any stale unmatched 2. Backfill logs on cancel — fixed. Early return when the token is already cancelled, and 3. Hard-coded oversize payload — fixed. Now One thing I checked and deliberately did not build. The above made me suspect a broader hole: an Perfectly balanced in every case — the subagents finished their tools and it was the stop signal Merge with #526. Both conflicts were additive collisions, kept both sides: |
) * Feed the Codex watcher state trackers raw lines, not redacted ones RedactLine replaces any line over 64 KiB with a placeholder carrying no call_id and a type neither tracker recognises, and an oversized function_call_output — a build log, a large file read, a verbose test run — is routine rather than exceptional. UpdateCodexPendingToolCalls therefore never removed the call_id, leaving PendingCodexToolCalls non-empty for the life of the watcher. toolInFlight was then permanently true and ShouldEndOnIdle could never fire. For Codex that is not a degraded backstop: the desktop app's shared app-server never exits per conversation, so the idle timeout is the ONLY per-conversation session-end path. The session stays Active in the read model indefinitely and the watcher never exits. The same stranded call_id also permanently blocks the live subagent-stop added in #526, which requires no tool call in flight — reinstating the symptom it fixed, a finished child's chat card spinning for the parent's whole lifetime. CodexSubagentTurn.Observe read the same list and fails in the other direction: it treats any response_item as the turn re-opening, so an oversized one is not recognised and a child that re-engaged after task_complete could be reported stopped while still working. Both now read drainRead.Lines, matching what SeedCodexSubagentTurnState already does when it folds this state from disk — the seed was correct and only the live drain path was inconsistent. Neither tracker emits anything; they read call_id and type only, so no unredacted content leaves the process and redaction still governs everything sent to the server. Same defect and same fix as the Claude tracker in #517. Closes #528 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Gate the Codex tool tracker on vendor now that it reads raw lines Both reviewers caught the same regression. The tracker's loop was never gated on vendor — it ran for every watcher on the grounds that a non-Codex line has no response_item and so is a cheap no-op. That held while it read the redacted list, where any oversized line arrived as a ~90-byte placeholder. Reading raw lines removes that bound, so a Claude or Gemini watcher would JsonDocument.Parse a multi-megabyte tool output on every drain to discover, again, that it is not a Codex record. TracksCodexToolCalls mirrors TracksClaudeToolCalls and makes the decision testable rather than an inline condition. Not gated on watcher role: a collab child needs the tracking for ShouldPostSubagentStop. Also trims the comments added in the previous commit, per the repo's keep-comments-minimal rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
Closes #514 (AI-1844).
The leak
A Claude subagent watcher had exactly two exit paths: the
SubagentStop-drivenStopWatchersignal, and the parent-exit watchdog.
ShouldEndOnIdlewas gated tocodex/antigravity/cursor,so there was no third. Miss the
SubagentStopand the watcher survives until the entire parentclaudeprocess quits.Found while diagnosing ~110 live
kcapprocesses on one machine. A single 13-day-old session owned9 watchers: 1 legitimate session watcher plus 8 whose subagents had finished between 1 and 12 days
earlier (~40 MB RSS each). Parent PID alive in every case, so the watchdog correctly never fired —
the parent being alive is precisely why they were stuck.
#140 / AI-820 fixed the dominant cause of a missed
SubagentStop(fd inheritance hanging thehook). This adds the missing backstop for when one is missed for any other reason — roughly 5% of
that session's ~170 subagents.
The fix
ShouldEndOnIdleadmits Claude child watchers. The vendor gate became a switch that states eachvendor's eligible role rather than a flat vendor list:
sessionEndhookClaude's session watcher is deliberately excluded — a ceiling there could end a live session out
from under a user who is just thinking.
Transcript silence alone does not reap. A subagent running a long build writes nothing between
its
tool_useand the matchingtool_result, andtoolInFlightwas Codex/Antigravity-only(
UpdateCodexPendingToolCallsparses Codex'sresponse_itemshape — a permanent no-op for Claude).So this adds
UpdateClaudePendingToolCalls, trackingtool_use.id→tool_result.tool_use_idinside
message.content[], shapes taken verbatim from a real subagent transcript.Two layers, because either alone is insufficient:
KCAP_CLAUDE_SUBAGENT_IDLE_MINUTES) — so a parser mis-read cannot reapa live subagent, and a subagent quiet for a reason the tracker cannot see is still safe.
6h rather than the 60m used elsewhere because this is a leak backstop, not an end-of-conversation
detector. Nothing depends on reaping promptly: a child watcher posts no
session-end(theagentId is nullgate onPostSessionEndOnParentExitAsync), so reaping early costs only the tailof that transcript, while the leak it fixes lasts days.
Two extractions so the wiring is covered rather than buried in
RunWatch:ResolveIdleWindow(vendor, isSessionWatcher, env)— the per-vendor window switch, with the envlookup injected. Each vendor keeps reading its own knob; a Claude subagent is not retunable via
the Codex knob.
TracksClaudeToolCalls(vendor, isSessionWatcher)— the drain-loop gate. If this drifts out ofstep with the watchers the ceiling applies to,
PendingClaudeToolCallsstays empty,toolInFlightis permanently false, and the suppression silently vanishes. That is the dangerous regression, so
it is a predicate with its own test rather than an inline condition.
Testing
TDD throughout — the eligibility test was watched failing with
Expected to be true but found Falseagainst the vendor gate before the gate changed.
WatchCommandTests77/77,UpdateClaudePendingToolCallsTests7/7.watcher staying ineligible, the composed policy at its real 6h default, per-vendor knob isolation,
and the tracker against multi-tool messages, string content, a Codex line, and malformed JSON.
CursorTailingWatcherTestsintegration 5/5.AgentOrchestratorVendorTestsdaemon-teardown tests; that class runs 228/228 in isolation on thisbranch, and fails an identical 5 under load at baseline. Pre-existing load flakiness, not
regressions.
Not addressed
This bounds the damage; it does not fix why
SubagentStopgets missed. That root cause is stillopen.
🤖 Generated with Claude Code