Repository navigation
fix(claude): run structured Claude under the POSIX provider supervisor - #23476
Conversation
…stopped A signalled supervisor forwards the signal to the provider group, escalates to SIGKILL after the grace, and exits only once the group is gone, so recovery's proof that the recorded pid is dead also proves the provider is. It refuses to spawn when its parent is already not the owner named in its spec, and watches that owner rather than whichever parent it first saw. The grace is a spec field. Recovery's SIGTERM stage now outlasts the supervisor's own stop, since a SIGKILL that lands first cannot be handled and leaves the group running.
…its provider group When Orca dies, the supervisor's stdout pipe has no reader. Provider output in the window before the parent-death watch fired raised an unhandled EPIPE that exited the supervisor with the provider group still running.
…ways covers it Recovery sized its SIGTERM stage from the default grace, so a launch with a longer grace would be SIGKILLed mid-stop and orphan its group with no test noticing. The spec now refuses any grace above one exported maximum, and recovery derives its SIGTERM stage from that maximum.
…ervisor' into brennanb2025/claude-under-supervisor
Owner death, stdin end after the grace, and a signal to the supervisor now all take one path: SIGTERM the provider group, SIGKILL it after the grace, and exit only once it is gone. The signal handlers are registered before the provider is spawned, so a stop that lands in the spawn window still reaps it. The longest stop grows to two graces plus the reap wait, and both recovery's SIGTERM stage and the connection's graceful close now wait that long before forcing, since forcing the supervisor sooner can orphan its group.
… supervisor A close now stops Claude with a SIGTERM through the supervisor instead of letting stdin end finish the turn, and Orca's death stops it through the supervisor.
…he supervised SIGTERM makes Claude emit
…ervisor' into brennanb2025/claude-under-supervisor
|
Status: ready for review. Stacked on #23466; merge that first. Review loops: clean. Automated tests cover the close, a Claude that ignores SIGTERM, an Orca force-quit, and a detached daemon surviving; each test fails when its core line is removed. Full-app QA (macOS, real Claude CLI): all five scenarios pass against the base; details and screenshots are in Visual Proof.
Not covered: Linux QA is blocked (the Linux QA host was unreachable). Windows is unchanged by construction and pinned by a unit test. There was one run per scenario. Known and not from this PR: after a force-quit the cut-off turn reads "Orca couldn't confirm" on both builds; #23456 changes that. |
![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 informational note on a downstream surface change worth confirming.
Reviewed changes
- POSIX supervision: structured Claude on macOS/Linux now spawns through
createProviderSpawnSpec, so the supervisor's pid becomes the owned root and the exit ladder learns it is supervised; Windows keeps a direct spawn. - Immediate supervised stop: the close ladder ends stdin then SIGTERMs the supervisor at once and waits
PROVIDER_SUPERVISOR_MAX_STOP_MS + 500before forcing, so closing a mid-turn chat stops Claude's own tools rather than letting end-of-input finish the turn. - Missing-binary diagnostic: the shared supervisor now writes the child spawn error to stderr before exiting 127, so a missing Claude binary still reports ENOENT.
- Tests: per-platform spawn shape, a timing-bound ladder test, a requested-stop turn-outcome test from recorded frames, an opt-in real-CLI stop suite, and real-process integration tests covering SIGTERM reap, supervisor escalation, owner-death reap, and detached-daemon survival.
ℹ️ The missing-CLI failure now reads differently on POSIX
On Windows a missing Claude binary still yields processless / AgentSessionPreSpawnError, but on macOS/Linux the supervised path makes it a first-hand root exit: the acquisition now throws AgentSessionAcquisitionRootExitObservedError (exitProof root-exit-observed) instead of AgentSessionPreSpawnError (processless). Lease release, owner verdict, and retry admission are equivalent, but the first-attempt surface changes (a raw thrown error becomes a structured refusal) and the durable deathEvidence.kind flips pid-absent → exit-observed. None of this is mentioned in the PR body and the chain is only pinned at the connection level, not at the adapter.
Technical details
# Missing-binary classification change
## Affected sites
- `src/main/claude/claude-agent-sdk-process-spawn.ts` — POSIX now spawns the supervisor, so `spawner.pid` exists even when Claude never starts.
- `src/main/claude/claude-stream-json-connection.ts:315` — supervised failure reports `exitVerdict.root === 'exited'` (test added at `claude-stream-json-connection.test.ts`, diff lines 652-673).
- `src/main/claude/claude-structured-session-close.ts:30-41` — `claudeAcquisitionCleanupError` maps a first-hand root exit to `AgentSessionAcquisitionRootExitObservedError`.
- `src/main/native-chat/agent-session-wire/structured-agent-session-failed-create-refusal.ts:30-59` — `root-exit-observed` → structured refusal; `AgentSessionPreSpawnError` is not handled and falls through to `classifyStoreFailure` (throws).
- `src/main/runtime/agent-session-acquisition-failure-settlement.ts:167-175` — `deathEvidence.kind` becomes `exit-observed` with detail "the provider process exited; its descendants were not proven gone".
## Required outcome
- Confirm the change from a thrown `AgentSessionPreSpawnError` to a structured `agent_session_operation_invalid` refusal (carrying `ownerVerdict: 'exited'`) is intended for a missing Claude binary, and that the `deathEvidence` copy is acceptable for a binary that never executed.
## Suggested approach
- If intended, state it in the PR body and add an adapter-level test that drives a missing `pathToClaudeCodeExecutable` end to end and asserts `AgentSessionAcquisitionRootExitObservedError` (the existing processless test is Windows-only, and the adapter coverage reaches this error only via `exitBeforeInit`).
## Open questions for the human
- Is the renderer's handling of a refusal vs a raw error acceptable here, or does it need the same diagnostic as before?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…adder cannot disagree with the spawn createProviderSpawnSpec now reports whether it wrapped the provider in the supervisor, and the Claude spawner reads that instead of repeating the platform check. The close ladder's SIGTERM follows the process actually spawned.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the single commit since the prior pullfrog review (297ea48cd0 → a6a1605fe7); the delta is a behavior-preserving refactor of how supervision is decided, plus a test that pins it.
- Supervision comes from the spawn spec:
createProviderSpawnSpecnow returns asupervised: boolean(its localsupervisedwas renamed tosupervisor), so the fact that a provider runs under the POSIX supervisor travels with the spec that actually built the spawn. - Claude reads that verdict:
createClaudeCodeProcessSpawnstoresspec.supervisedin a closure and exposes it through a getter, replacing the duplicatedplatform !== 'win32'computation. The close ladder can no longer disagree with the spawn that ran;falseuntil the SDK spawns matches the existing "null until the SDK spawns" contract forchild/pid. - Test: a new spawn-suite case drives both a
darwinspec reported unsupervised and awin32spec reported supervised, asserting the ladder SIGTERMs (or does not) from the spec value rather than the platform. It fails against the old platform-derived implementation, so it is real coverage. - Test alignment: the Codex supervisor shape expectation gains
supervised: falseforwin32, and the Claude spawn tests now pass an explicit platform.
No critical issues, no new regressions. The informational note from the prior review (the missing-CLI POSIX path now classifying as a root exit, with adapter-level coverage still absent) remains open and is unchanged by this commit.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Status: ready for review. Head Review: the first review loop was clean. After the readiness check, one fix: whether Claude runs under the supervisor is now decided in one place, the spawn spec's Readiness check: passed with no release-blocking findings. It ran at the end of the review only, not at the start as well; that's disclosed here. Minor findings, not fixed: the shared supervisor still lives in a Codex module (it moves once #23466 lands); a Claude binary that disappears just before spawn is now recorded as an exit rather than a pre-spawn failure (the chat still releases, and the error text is kept); and one integration test asserts a close under 1.25 s, which could flake on a loaded CI machine. Live QA, second full-app run on a remote Mac (a Claude worker ran it because the Grok quota was exhausted; results match the first run in Visual Proof):
Not covered: Linux live QA of this PR, and Windows (unchanged by construction and pinned by a unit test). |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughProvider spawn specifications now report whether a process is supervised, and the supervisor writes spawn errors to stderr. Claude launches through the provider spawn specification on POSIX and directly on Windows. Claude close passes the supervision state to exit proof, which sends SIGTERM and uses a longer graceful wait for supervised children. The child-eviction timeout now derives from snapshot-drain and provider stop limits. New tests cover platform-specific launches, shutdown paths, structured-session close behavior, and opt-in real-CLI scenarios. Priority: ➖ Normal Merge Risk: 🔵 Low · up to The change is mergeable with a narrow test improvement: require the eviction phase before asserting that it completes. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Supervised shutdown appears to reduce the time a Claude task can keep running after a chat closes or the application exits. The normal stop path is supported by tests, but exceptional cleanup and recovery timing are not fully established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const spec = createProviderSpawnSpec( | ||
| { | ||
| command: options.command, | ||
| args: [...options.args], | ||
| ...(options.cwd === undefined ? {} : { cwd: options.cwd }) | ||
| }, | ||
| definedEnv(options.env), | ||
| platform | ||
| ) |
There was a problem hiding this comment.
| afterEach(async () => { | ||
| for (const pid of recordedPids) { | ||
| if (alive(pid)) { | ||
| process.kill(pid, 'SIGKILL') | ||
| } | ||
| } |
There was a problem hiding this comment.
…supervised provider close Quit's child-eviction phase was a hand-picked 8 s. It is now the sink drain plus the longest supervised close over Claude and Codex plus a named 1 s margin, so a provider close that grows widens it instead of silently outrunning it. A close's tree-kill fallback stays outside the bound: once main exits, the supervisor stops its provider group on owner death, which a new test now proves for a clean owner exit, and next launch's recovery settles the lease.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.test.ts (1)
77-77: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the eviction phase exists before running it.
If
evict-owned-sessionsis removed or renamed,.find()returnsundefined. Optional chaining then skips eviction, and the test can still pass. Assert the phase exists before callingrun().Suggested fix
+ expect(evict).toBeDefined() const run = Promise.resolve(evict?.run()).then(() => {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 63b3671c-de25-4ebf-8f16-5d892909b1b5
📒 Files selected for processing (5)
src/main/claude/claude-child-exit-proof-ladder.tssrc/main/codex/codex-app-server-connection.tssrc/main/codex/codex-app-server-posix-supervisor.integration.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the single commit since the prior pullfrog review (05728a1f59 → 07d6876d5d), which replaces quit's hand-picked child-eviction bound with a derived one and adds coverage for the supervisor's clean-quit stop.
- Derived the eviction bound:
CHILD_EVICTION_TIMEOUT_MSis nowSNAPSHOT_DRAIN_TIMEOUT_MS + Math.max(SUPERVISED_GRACEFUL_EXIT_MS, PROVIDER_SUPERVISOR_MAX_STOP_MS) + EVICTION_MARGIN_MS(1_000 + 6_000 + 1_000 = 8_000 ms, unchanged from the old literal), so a supervised close that grows widens the bound instead of silently outrunning it. The two Windows close constants were exported for the test to measure. - Pinned the invariant with tests: the teardown suite now asserts the bound equals that formula, that every provider's supervised and Windows close fits inside it, and that eviction plus the resume-marker step stays under quit's 20 s deadline; a fake-timer case pins that eviction finishes when the chats close rather than at its bound.
- New clean-quit integration case: the supervisor is proven to reap the provider group and exit when its owner quits normally (exit 0, no stdin holder), complementing the existing owner-
SIGKILLcase. - Export/comment plumbing:
GRACEFUL_EXIT_MSis exported from the Claude ladder and the Codex connection, and the recovery stop-probe comment now reads "structured owner".
I ran the four changed unit suites (17 tests) — all pass. The process-tree integration suites still fail in this container for the known PID-1 non-reaping reason: the untouched codex-app-server-posix-supervisor.integration.test.ts fails identically ({code:1} instead of SIGTERM), including the new clean-quit case, so this is environmental rather than a regression. CHILD_EVICTION_TIMEOUT_MS is unchanged in value, and eviction runs over sessions via Promise.all, so a per-phase max over the single longest close is the right shape.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
1 participant
Follows #23466 (the provider supervisor hardening), which is now merged; this PR is based on main and uses its final timings: the provider gets 1 s after end-of-input, then 3 s after SIGTERM, and a supervisor takes at most 5.5 s to stop.
ELI5
When Orca crashed or quit with a Claude chat mid-task, the Claude process didn't stop. It saw its input close, took that as "no more messages", and finished the task on its own: running commands and editing files for up to minutes with nobody watching. Closing a chat mid-task did the same thing for a second and a half before Orca forcibly killed it. Now Claude runs under the same small watchdog process Codex already uses on macOS and Linux. When Orca goes away or closes the chat, the watchdog tells Claude to stop, and Claude stops its own running commands and exits, usually in under half a second.
What Changed
Before (macOS/Linux):
After (macOS/Linux):
createProviderSpawnSpec, the same inline supervisor the Codex app-server uses). Each chat has an owner record that names the one agent process allowed to write to its conversation; for Claude it now records the supervisor's pid, exactly as it already does for Codex.createProviderSpawnSpecreturnssupervised, and Claude's close reads it instead of repeating the platform check, so the stop cannot disagree with the process actually started.PROVIDER_SUPERVISOR_MAX_STOP_MS, 5.5 s), plus 0.5 s, so Orca never force-kills the supervisor mid-stop and orphans Claude. A Claude that ignores SIGTERM can therefore take up to about 6 s to close, up from 4.5 s before fix(codex): the provider supervisor outlives its provider group when stopped #23466's timings.ENOENT). Before, Orca got it directly as a spawn error with no process ID.Unchanged:
setsidor a double-fork) are not signalled, same as today. A test pins this.One visible difference in the chat: when a chat is closed mid-command, the running command's row now reads failed with "Exit code 137". That is the result Claude itself reports for the tool it killed on SIGTERM. Before, the row was left as running. The turn still reads interrupted, exactly as before, and nothing is reported as a crash. Recorded frames from the real CLI after the SIGTERM:
tool_result is_error "Exit code 137",system/status, and noresultframe.Why
A Claude that loses its input finishes its turn, so after an Orca crash its tools kept editing the workspace unwatched. The watcher is the one process that outlives Orca and still owns Claude, and SIGTERM is the only signal that lets Claude stop its own tool processes.
The goal is to close the window in which a Claude that Orca no longer watches keeps writing to the user's workspace and conversation. Otherwise recovery could also start a second Claude on the same conversation while the first is still writing to it.
--resumecontinues the conversation.Alternatives rejected:
nohupdev server should survive an Orca crash.Differences from the common pattern
Supervisor mechanism: forward the stop signal to the child's process group, SIGKILL after a grace, and exit only after the group is gone. That matches #23466's supervisor, which this PR reuses unchanged apart from the stderr line on spawn failure.
How a stop reaches Claude: the common pattern for stopping a Claude SDK child is to close stdin, then SIGTERM after about 2 s, then SIGKILL after 5 more.
Turn outcome of a requested stop: the turn reads interrupted on end-of-session in both. It matches.
Linked Issue
None. This is a maintainer follow-up to #23466; it closes the window in which an orphaned Claude keeps writing, for Claude structured chats on macOS and Linux.
Visual Proof
Full-app QA on macOS (an M4 MacBook Air) with the real Claude CLI, comparing the base (#23466 head
3b374050c6, where Claude is started directly) against this PR (297ea48cd0). That QA predates #23466's final timings; they apply only when Claude ignores SIGTERM, which no scenario below did. Every scenario started from a fresh, isolated profile; the build under test was confirmed by a marker string in the app bundle each time.sleep 120nohup sleep 300started firstAfter the force-quit, the chat relaunches and answers the next message on both builds. The cut-off turn reads "Orca couldn't confirm" on both, because this stack predates #23456, which changes that.
A chat closed mid-command and reopened from Agent Session History keeps its conversation: the follow-up question was answered with the codeword from before the close, on both builds. The stopped command's row reads "1 failed".
After the force-quit and relaunch (base, then this PR):
This PR, the next message after the relaunch is answered:
Closed mid-command, reopened, context kept (base, then this PR):
Testing
Quit's time limit (
structured-agent-session-host-teardown.test.ts):Clean quit (
codex-app-server-posix-supervisor.integration.test.ts): an owner that exits normally (exit code 0, not a kill) leaves the supervisor to stop the provider's group, including a child that ignores SIGTERM, and then exit. Ablation: with both the parent-death watch and the end-of-input trigger disabled, the group survives (red); with only the watch disabled, end-of-input alone still stops it.Real-process tests (bounded; cleanup only by recorded pid):
claude-supervised-stop.integration.test.tsuses a fake Claude that ignores end-of-input like a mid-turn Claude, runs a tool in its own process group, and double-forks a daemon. It pins four behaviours:ENOENTis lost.Unit tests:
claude-child-exit-proof-ladder.test.ts: a supervised child gets SIGTERM, and the close sequence waits out the supervisor's bound before forcing. Ablation: the fixed 1.5 s wait turns it red. An unsupervised (Windows) child is never sent SIGTERM.claude-structured-requested-stop.test.ts: adapter close with the frames recorded from the real CLI. The turn reads interrupted, andendedcarries nounexpected-exitcause. Ablation: the ended sweep removed turns it red.Opt-in real CLI:
claude-supervised-stop-real-cli.test.ts, gated byORCA_REAL_CLAUDE_SUPERVISED_STOP=1plusORCA_REAL_CLAUDE_BIN. Run on macOS (M4Air) with Claude 2.1.283, haiku, an isolated HOME and anorcashim that exits 97.--resumeansweredsuccess.Suites and checks:
src/main/claude,src/main/codex,src/main/native-chatandsrc/main/runtimepassed locally before main was merged: 13914 tests, excluding the two known local real-binary suites. On the merged head this PR's test files plus the supervisor, connection, teardown, eviction and journal-close tests pass (12 files, 105 tests).pnpm tc:nodeandpnpm tc:webpass. oxlint on the changed files,check:code-quality:changedandaudit:anti-slopare clean.Platforms:
macOS only.
Linux QA is BLOCKED: the Linux QA host's Orca runtime was unreachable. Nothing was substituted.
Windows was not run; it is unchanged by construction and pinned by a unit test.
Full-app QA on macOS, one run per scenario: see Visual Proof.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Written with AI assistance (Claude), and reviewed and validated by the author.
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
Known gap: Windows keeps the window in which an orphaned Claude keeps writing. It has no supervisor, and a Claude orphaned there finishes its turn as before.
Detached descendants: unchanged. Anything that leaves Claude's tree on purpose is not stopped.
Mixed versions:
Residuals:
unverifiable, neverexited, so the owner record is not released on it.Cross-platform, SSH, folder workspaces: the spawn runs on whichever host executes the chat (local or a remote Orca server), with that host's
process.execPath; this is the same path the Codex supervisor already takes there. Folder workspaces are unaffected.Author X handle: @BrennanKB5
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)