fix(test): fixture CLIs self-reap when orphaned and dispose kills the tree - #445
Merged
Conversation
… tree 30 orphaned fake-claude processes (aged 3-9 days) were found accumulated on the dev machine: any abnormally-terminated test run (vitest timeout SIGKILL, Ctrl-C, an interrupted agent run) skips teardown, and the spawned fixture CLI is left waiting on stdin forever. Two layers: 1. Fixture self-reap (packages/core/test/fixtures/fake-claude.ts): the fixture polls process.ppid and exits once it no longer matches the pid captured at start. A stdin end/close listener was considered first, but the real spawn protocol (@tanstack/ai-sandbox's spawnNdjson) writes the prompt and calls stdin.end() immediately after spawning, regardless of parent liveness, so stdin already closes seconds before any hang begins in every run - it cannot distinguish "orphaned" from "normal". A ppid poll is the correct signal here since the fixture only outlives its parent's process lifetime, never its stdin. 2. Spawn-side teardown hygiene (packages/harness-testkit/src/create-testkit.ts): testkit cleanup() previously only aborted attach-stream subscriptions and called app.dispose(), which drains in-flight runs on a 5s deadline and then just logs an error and moves on - it never aborted the run's own AbortController, so a test that throws before manually stopping a long-running session left its spawned child running past cleanup. cleanup() now also calls chat.stop for every live session (SIGTERM then SIGKILL escalation already lives in packages/core/src/chat/sandbox.ts), bounded to 3s so a test harness that doesn't honor abort (e.g. a deliberately held scripted turn) can't turn this into a hang. Regression test: packages/core/test/chat/fake-claude-orphan-reap.it.test.ts spawns fake-claude.ts through a real intermediate parent process (packages/core/test/fixtures/orphan-parent-sim.ts, pipe stdio + its own process group - the worst-case spawn shape, verified against @tanstack/ai-sandbox-local-process's handle.js which spawns the real "claude" child the same way: default 'pipe' stdio, detached: true on POSIX), SIGKILLs that parent, and asserts the fixture exits on its own using vi.waitFor. Verified failing against the pre-fix fixture (times out at 3s) and passing post-fix; verified empirically too, killing the actual process spawned via bootCoreApp() + local-process sandbox: pre-fix it survived 3+ seconds with ppid reparented to 1, post-fix it was gone within ~300ms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds parent-process monitoring to the fake Claude fixture, an integration test for orphan reaping, and bounded active-session cleanup in the harness testkit. ChangesOrphan Process Reaping
Bounded Testkit Cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OrphanReapTest
participant OrphanParentSim
participant FakeClaude
OrphanReapTest->>OrphanParentSim: spawn simulator
OrphanParentSim->>FakeClaude: spawn hanging child
FakeClaude-->>OrphanParentSim: signal readiness
OrphanParentSim-->>OrphanReapTest: report child PID
OrphanReapTest->>OrphanParentSim: SIGKILL parent
FakeClaude->>FakeClaude: detect changed process.ppid
FakeClaude-->>OrphanReapTest: terminate
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/test/chat/fake-claude-orphan-reap.it.test.ts`:
- Around line 22-40: Wrap the setup and assertions after parentPid validation in
a try/finally so failures during readiness parsing or liveness checks cannot
leak processes. In the finally block, kill orphan-parent-sim when parentPid is
still alive, and conditionally kill the parsed fixturePid when it is defined and
still alive; retain the existing fallback cleanup for the fixture.
In `@packages/harness-testkit/src/create-testkit.ts`:
- Around line 145-149: Bound session discovery within the cleanup timeout in the
surrounding testkit disposal flow: include rpc.sessions.list in the promise
passed to pTimeout rather than awaiting it beforehand, and provide the RPC
request with the available cancellation signal or timeout mechanism. Keep
stopping discovered sessions within the same deadline, and do not rely on
fallback to cancel the underlying request.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 38744310-2aee-4a4b-9949-2defce6fc7ce
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/core/test/chat/fake-claude-orphan-reap.it.test.tspackages/core/test/fixtures/fake-claude.tspackages/core/test/fixtures/orphan-parent-sim.tspackages/harness-testkit/package.jsonpackages/harness-testkit/src/create-testkit.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Improves test-process teardown so fixture CLIs terminate when orphaned and active sessions are stopped during cleanup.
Changes:
- Adds parent-PID monitoring to
fake-claude. - Stops running sessions with a bounded cleanup period.
- Adds an OS-level orphan-reaping regression test.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packages/core/test/fixtures/fake-claude.ts |
Adds orphan self-reaping. |
packages/core/test/fixtures/orphan-parent-sim.ts |
Simulates an orphaning parent process. |
packages/core/test/chat/fake-claude-orphan-reap.it.test.ts |
Tests fixture termination after parent death. |
packages/harness-testkit/src/create-testkit.ts |
Stops sessions during cleanup. |
packages/harness-testkit/package.json |
Adds p-timeout. |
pnpm-lock.yaml |
Records dependency resolution changes. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| '@tanstack/react-router': | ||
| specifier: latest | ||
| version: 1.170.23(react-dom@19.2.7(react@19.2.7))(react@19.2.7) | ||
| version: 1.170.25(react-dom@19.2.7(react@19.2.7))(react@19.2.7) |
Contributor
Author
There was a problem hiding this comment.
Attempted minimization (reset to main's lockfile + pnpm add p-timeout --filter @conciv/harness-testkit): the floating-latest TanStack graph still re-resolves on any resolve pass (113-line diff, same churn class). Declining per repo precedent (#433): in-range collateral from latest specifiers, CI-validated.
| }, | ||
| cleanup: async () => { | ||
| for (const abort of aborts) abort.abort() | ||
| const liveSessions = (await rpc.sessions.list(undefined).catch(() => [])) ?? [] |
Contributor
Author
There was a problem hiding this comment.
Confirmed against contract.ts:90 / rows.ts:8-19 (includeHidden input, hidden+running on SessionMeta) and fixed in cfff9ec — cleanup now lists {includeHidden: true} and stops only running sessions.
Comment on lines
+22
to
+40
| const parent = spawn(process.execPath, ['--import', tsxEntry, parentSimPath], {stdio: ['ignore', 'pipe', 'pipe']}) | ||
| const parentPid = parent.pid | ||
| if (parentPid === undefined) throw new Error('orphan-parent-sim did not spawn') | ||
|
|
||
| const [chunk] = await once(parent.stdout, 'data') | ||
| if (!(chunk instanceof Buffer)) throw new Error('expected a Buffer chunk from stdout') | ||
| const match = /READY (\d+)/.exec(chunk.toString()) | ||
| if (!match) throw new Error('orphan-parent-sim did not report READY') | ||
| const fixturePid = Number(match[1]) | ||
| expect(isAlive(fixturePid)).toBe(true) | ||
|
|
||
| process.kill(parentPid, 'SIGKILL') | ||
| await vi.waitFor(() => expect(isAlive(parentPid)).toBe(false), {timeout: 2000, interval: 50}) | ||
|
|
||
| try { | ||
| await vi.waitFor(() => expect(isAlive(fixturePid)).toBe(false), {timeout: 3000, interval: 50}) | ||
| } finally { | ||
| if (isAlive(fixturePid)) process.kill(fixturePid, 'SIGKILL') | ||
| } |
Replace the standalone ORPHAN_POLL_MS/reapWhenOrphaned() helper in fake-claude.ts with an inline ppid-poll block (interval now 1000ms, matching the LSP-watchdog convention), and drop create-testkit.ts's hand-rolled withTimeout in favor of p-timeout (matching browser-fixture.ts's existing usage). The cleanup's session discovery call is now folded inside the timed promise so a stalled rpc.sessions.list no longer blocks disposal past the 3s bound, and it lists with includeHidden so a deleted-but-still-running session isn't skipped; only sessions the API marks running get stopped. orphan-parent-sim.ts drops detached:true (SIGKILL never propagates to children, so orphaning works without it) and the stdio error swallowers; CONCIV_FAKE_HANG doesn't exit on stdin EOF (verified), so stdin can go fully to 'ignore'. To make the IT deterministic without a guessed delay, the sim now waits for fake-claude's own CONCIV_TEST_ARGV_FILE write (which happens right after the reap interval installs) before reporting READY, so the test's immediate SIGKILL can never race the reap installation. The IT test itself now reads READY via a single stdout 'data' event and drops the old speculative 300ms settle window + second liveness check; the whole run is now wrapped in try/finally from before the first await so a thrown READY-parse or assertion can't leak the parent-sim or fixture process into later tests. The final reap timeout widened to 5000ms to cover the slower 1s poll. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
omridevk
force-pushed
the
fix/fixture-orphan-reap
branch
from
August 12, 2026 22:23
fe60f0b to
cfff9ec
Compare
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
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
fake-claudeprocesses (aged 3-9 days) accumulated on the dev machine. Any abnormally-terminated test run (vitest timeout SIGKILL, Ctrl-C, an interrupted agent run) skips teardown, and the spawned fixture CLI is left waiting on stdin forever.packages/core/test/fixtures/fake-claude.ts): the fixture now pollsprocess.ppidand exits once it no longer matches the pid captured at start.end/closelistener was the first idea, but it's the wrong mechanism for this fixture's actual spawn protocol:@tanstack/ai-sandbox'sspawnNdjsonwrites the prompt and callsstdin.end()immediately after spawning (proc.stdin.write(input); await proc.stdin.end()), regardless of parent liveness. Stdin already closes within milliseconds of every run, hang or not — it cannot distinguish "orphaned" from "normal in-flight." A ppid poll is the correct signal here, since the fixture only ever needs to outlive its parent's process lifetime, not its stdin.bootCoreApp()→chat.send→@tanstack/ai-claude-code→@tanstack/ai-sandbox-local-process), then SIGKILLed the parent test process.1(ps -p <pid> -o pid,ppid,stat→Ss, alive at every 1s check).ps -p <pid>returned no such process within ~300ms of the kill.packages/harness-testkit/src/create-testkit.ts):cleanup()previously only aborted attach-stream subscriptions and calledapp.dispose(), which drains in-flight runs on a 5s deadline and then just logs an error and moves on — it never aborted the run's ownAbortController, so a test that throws before manually stopping a long-running session left its spawned child running past cleanup.cleanup()now also callschat.stopfor every live session (the existing SIGTERM→SIGKILL escalation inpackages/core/src/chat/sandbox.tshandles the actual kill), bounded to 3s so a test harness that doesn't honor abort (e.g. a deliberately held scripted turn, exercised bytranscript-durability.it.test.ts's T3) can't turn this into a hang.Regression test
packages/core/test/chat/fake-claude-orphan-reap.it.test.tsspawnsfake-claude.tsthrough a real intermediate parent process (packages/core/test/fixtures/orphan-parent-sim.ts) using pipe stdio and its own process group — the worst-case spawn shape, verified against@tanstack/ai-sandbox-local-process'shandle.js(the actual point that spawns the realclaudechild), which uses the same shape: default'pipe'stdio,detached: trueon POSIX. SIGKILLs that parent, and asserts the fixture exits on its own viavi.waitFor— no mocks, real OS-level process kill andprocess.kill(pid, 0)liveness checks.vi.waitFortimeout (orphan survives).Gates (all run in this worktree)
pnpm exec tsc -p packages/harness-testkit/tsconfig.json --noEmit— passpnpm turbo run typecheck --filter=@conciv/core --filter=@conciv/harness-testkit --filter=@conciv/harness --filter=@conciv/harness-init— passTURBO_CONCURRENCY=1 VITEST_MAX_FORKS=1 pnpm turbo run test --concurrency=1 --filter=@conciv/core --filter=@conciv/harness-testkit— pass (115 test files / 445 tests passed in@conciv/core, all@conciv/harness-testkitsuites green)pnpm lint(oxlint, scoped to touched files) — pass, 0 errorspnpm exec oxfmt --check(touched files) — passpnpm exec fallow audit --changed-since main --format json— verdictpass, 0 introduced findingspnpm exec fallow audit --format json --quiet --explain --gate-marker agent— verdictpassTest plan
@conciv/coreand@conciv/harness-testkit🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests