Repository navigation
fix(claude): prove a stopped chat's child processes gone when they exit with it - #22918
Conversation
…it with it Stopping a Claude chat snapshots its child processes, closes Claude, and then verifies each child is gone before the stop counts as proven. The verifier only accepted a child as gone after that child had appeared in one of its own process-table reads. When Claude exits gracefully it takes its short-lived children with it before the first read, so none of them was ever seen again. Every read confirmed them absent, and the verdict was still "unverifiable" after the full 3.5 s window. Measured live: 37 complete reads, target absent from all, verdict unverifiable, on every idle stop. The snapshot is itself a table read that saw each child alive, so it now counts as the first sighting. An absence counts only from a read that started after the child was last seen, which keeps what the old rule protected against: a shared or in-flight read begun before the snapshot cannot list a child forked since. Two such absences prove a child gone. Live, the same stop now proves the tree gone in about 150 ms. The daemon's terminal shutdown uses the same verifier and gets the same rule. Test reads that reused one capture stamped with the snapshot's own time now stamp each read when it starts, as real scans do.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughIdentity-required descendant verification now counts a missing process only when the process-table capture started after that descendant was last seen. It requires two qualifying absences per descendant for an Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The descendant-exit verification change is mergeable after normal checks; no actionable issue remains from this review. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and covers the change, rationale, testing, scope, and platform impact. However, the required Linked Issue entry is missing, and the required command-validation checklist remains unchecked.
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: c8f49318-b41f-4b26-864c-e1cedae5608e
📒 Files selected for processing (4)
src/main/daemon/terminal-descendant-shutdown.test.tssrc/main/pty-descendant-exit-verification.test.tssrc/main/pty-descendant-exit-verification.tssrc/main/pty-descendant-termination.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one non-blocking soundness note inline.
Reviewed changes
- Prove snapshot-only descendants gone —
terminateDescendantSnapshotWithVerdictnow seeds a per-PIDlastSeenAtMsfrom the snapshot's capture boundary and counts an absence only from a read whosecapturedAtMsis newer, so a child that exits with Claude before the verifier's first read can be proven gone. This is the only production change. - New regression test for the clean-exit path —
pty-descendant-exit-verification.test.tsadds a test that a snapshot-only child is provenexitedwithin 200 ms with no signal sent; it fails under the old rule (which waits out the window). - Rewritten stale-read test — the prior "omitted target stays unverifiable" test is replaced by one asserting a read whose
capturedAtMspredates the snapshot boundary is not counted; it pins the new timestamp guard. - Test mocks re-stamped after the snapshot —
pty-descendant-termination.test.tsanddaemon/terminal-descendant-shutdown.test.tsnow stamp verification reads after the walks that produced each snapshot, matching real scan timing; assertions (including the daemon's shared-read count) are unchanged.
I read the full diff, traced both identity-checked callers (claude-agent-sdk-exit-proof.ts and daemon/terminal-descendant-shutdown.ts), and ran the three changed test files: 57/57 pass. The logic is sound and the fix addresses the reported regression without weakening the identity checks.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| // The snapshot is itself a table read that saw each target alive. An absence is evidence only | ||
| // from a read that started after the target was last seen: a shared or in-flight read begun | ||
| // earlier can miss a descendant forked since, but a later full scan cannot miss a live one. | ||
| const lastSeenAtMs = new Map( | ||
| snapshot.descendants.map((row) => [ | ||
| row.pid, | ||
| snapshot.capturedAtMsByPid?.[String(row.pid)] ?? snapshot.capturedAtMs | ||
| ]) | ||
| ) |
There was a problem hiding this comment.
The freshness guard compares a read's scan start against the snapshot's scan start, but the snapshot observed each descendant at scan enumeration, which is at or after that start. A shared/in-flight read begun after the snapshot started but whose ps ran before a descendant was forked can therefore pass capture.capturedAtMs > lastSeenAtMs and be counted as a fresh absence. Today the daemon's 25 ms shared-capture linger versus the verifier's 50 ms poll cadence admits at most one such absence, so provenAbsent() (which requires two) cannot fire — but that safety comes from those two unrelated constants, not from this guard.
Technical details
# Freshness boundary uses scan start, not observation time
## Affected sites
- `src/main/pty-descendant-exit-verification.ts:130-135` — `lastSeenAtMs` seeded from `snapshot.capturedAtMs` (a scan start), so it under-states when the descendant was actually observed by up to the snapshot scan's duration.
- `src/main/pty-descendant-exit-verification.ts:165` — `capture.capturedAtMs > lastSeenAtMs` admits any read whose scan started after the snapshot started, including one whose `ps` enumerated before the descendant was forked.
- `src/main/pty-descendant-termination.ts:53` — `capturedAtMs = Date.now()` is stamped before `execFile('ps', ...)` spawns; no completion/enumeration boundary is recorded anywhere.
- `src/main/daemon/terminal-descendant-shutdown.ts:14-38` — the shared capture can be created by an earlier terminal's verifier before a later terminal's snapshot completes, so a later verifier can be served a read that predates its own snapshot's observation of a late-forked child.
## Required outcome
- A fresh absence should mean "this read could have listed the descendant and did not", i.e. its enumeration is after the snapshot's enumeration. The current check only guarantees the read started after the snapshot started.
## Suggested approach (optional)
- Record a scan completion boundary on `ProcessTableCapture`/`DescendantSnapshot` and seed `lastSeenAtMs` from it (an upper bound on enumeration), or have `captureDescendantSnapshot` pass its completion time into `collectDescendantRows`.
- At minimum, correct the comment at `:127-129`: the guard does not by itself exclude "a read begun earlier [than the child was forked]"; the shared-read slot's 25 ms linger is what currently bounds it.
## Open questions for the human (optional)
- Is the 25 ms linger in `readShutdownProcessTable` intended as a safety margin coupled to the 50 ms verifier poll, or incidental? If incidental, documenting it as load-bearing (or removing the dependence) would be worthwhile.…absence A matching read that started earlier but resolved later could move a target's last sighting back and let an older absence count; the sighting now only moves forward. The read after the deadline now records its absences the same way the polling loop does, so a second qualifying absence there proves the target gone.
There was a problem hiding this comment.
ℹ️ No new issues in the follow-up commits; the prior review's open soundness note is unaffected and remains non-blocking.
Reviewed changes
- Made a descendant's last sighting monotonic —
recordIdentityObservationadvanceslastSeenAtMswithMath.max, so a read that started earlier but resolves later can no longer move the sighting backward and reopen an absence window. - Counted the post-deadline read as an absence — the final read now flows through the same observation helper, letting it supply the second absence that proves a snapshot-only child gone instead of only deciding
live/unverifiable. - Extracted the shared observation loop — the polling loop and the final read now use one
recordIdentityObservationhelper rather than duplicated inline state updates. - Pinned both behaviors with tests — the latest-sighting-retention and final-read-absence tests both fail against
84f592cand pass at the head.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Review status: ready to merge (head b63384f)Summary: two review loops. The first made one follow-up fix; the second found nothing to change. Readiness checklist passed at the start and at the end. CI is green and the merge state is clean. Does it fix the problem? Yes. On the base code, a shell whose two children exit along with it came back "unverifiable" after about 4 s. With this PR, the same case comes back "exited" in 170–212 ms (real Is the approach right? Yes, keep it. The rule from #22232 caused the bug: it insisted on a sighting in the verifier's own reads, which a child that exited with Claude can never give. The PR replaces that rule with a direct check on the hazard it stood in for, a read that started before the child was last seen. It does not add a second guard beside the old one. Two absences are still required. Follow-up fix in this review (b63384f). From two CodeRabbit findings:
Each fix has a test, and each test fails when only its fix is reverted. Checked, not defects:
Open, not blocking:
Validation
Not covered:
|
Review status: ready to merge, no changes neededThis review pushed no commits. Head is still What was checked
Live Electron checkSetup: isolated background dev instances, real Claude Code, and the same steps on each build. The only difference between builds was the verifier file. A temporary log line showed the verdict (QA build only; removed afterwards). The idle-release timer was shortened to 10 s for the rig.
Verdict logs,
Verdict logs, this PR:
Chat with the background command running before the tab is closed (
Chat answering again after an idle stop and restart (
Existing issue seen during QA (not caused by this PR)After an idle stop and restart, the composer can show a red line: "Expected runtime fence 1; the session is at 3." The message is still delivered and answered, and the line stays until the next send. It comes from the chat screen's send path. The screen keeps the session's old fence number while you are away. Its first send after you return is refused, then resent with the current fence and accepted, but the error text is never cleared. It appears on Screenshot with the
Known limits, left unchanged on purpose
|
ELI5
When Orca stops a Claude chat's process, it also checks that every process Claude started is gone. That check was broken for the most common case: Claude exits cleanly and takes its short-lived helpers with it. Orca looked for them, found nothing every time, and still answered "can't tell", so every normal stop was reported as uncertain and took an extra 3.5 seconds. Now "not there on two fresh looks" counts as gone, and the check finishes in about 0.15 seconds.
What Changed
terminateDescendantSnapshotWithVerdictinpty-descendant-exit-verification.ts(identity-checked mode, used by Claude's stop and the daemon's terminal shutdown): a snapshotted child used to count as gone only after it had appeared in one of the verifier's own process-table reads and then been missing from two. A child that exited before the first read never appeared, so it could never be proven gone. Now the snapshot, which is itself a process-table read that saw the child alive, counts as the first sighting. An absence counts only from a read that started after the child was last seen. Two such absences prove it gone.Why
The old rule was guarding against a real hazard: an absence taken from a read that started before the child was seen, such as a shared or in-flight read in the daemon's batched shutdown, which cannot list a child forked after it began. Every capture already records when its scan started (
capturedAtMs), so the new rule guards that hazard directly. A read begun before the last sighting is ignored. A fullps -axoscan begun after it cannot omit a live process: a failed or truncated read is rejected, never parsed as a short table. That removes the "never seen again, so never provable" trap without weakening the protection.Alternatives considered:
Linked Issue
No separate issue. This is the follow-up from #22802's investigation. The rule came in with #22232 (daemon terminal-descendant reaping), which made every graceful Claude stop unverifiable. Before it, the identity-checked verifier accepted two consecutive empty reads.
Visual Proof
N/A: process-lifecycle behavior with no UI change. Live evidence instead (isolated dev instance, real Claude Code sessions, temporary logging in the verifier; QA build only):
mainunverifiable; ladderreaped:false, exited:true, treeVerdict:"unverifiable"exitedafter 150–166 ms (two fresh absences each); laddertreeVerdict:"exited"; returning to the chat restarts Claude and the next send is answeredKill path, this PR: closing a chat tab while a background
sleep 150ran. Claude did not exit on its own, so the ladder forced it (reaped:true). The runningsleep(in the snapshot) was gone 8 s later, and all 3 children were proven gone in 289 ms.Testing
pty-descendant-exit-verification.test.ts:unverifiablewith no signal.Existing tests reused one mocked capture stamped with the snapshot's own time for every read. A real scan is stamped when it starts, so the mocks in
pty-descendant-exit-verification.test.ts,pty-descendant-termination.test.ts(recycled pid) anddaemon/terminal-descendant-shutdown.test.tsnow stamp each read after the snapshot. The assertions are unchanged, including the daemon's shared-read count.126 tests across 9 suites (the verifier, PTY termination, daemon shutdown, daemon reaping leak, both Claude exit-proof suites, Claude close, stream-json close and Codex app-server teardown) pass, plus 69 in 5 more suites that reach the verifier.
pnpm tc:node, oxlint andcheck:code-quality:changedare clean.I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
ps -axoscan; SSH remotes are unaffected, since this runs on the execution host.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)