Repository navigation
fix(native-chat): stop killing processes that only inherited a chat's spawn tag - #23460
Conversation
…wn token A spawn token is an environment variable, so every descendant of a provider child carries it. The Linux-only startup scan treated any carrier no lease claimed as a lost provider child and sent it SIGTERM, which also hit editors, tmux servers and nested Orca processes the agent had started. Remove that scan's killing consumer; the token scan stays for the reservation probe, and recorded owners are still stopped by identity during recovery.
Every descendant inherits the spawn token, so killing each pid that carries it can reach processes the agent started that are not the provider. Production never injected this path; teardown always uses the process-group and descendant-snapshot proof. Drop it, its deps, and the now-unused spawn-token argument.
|
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 (1)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change removes spawn-token-based orphan classification and cleanup from agent-session runtime code. It adds tests for descendant token scans and reservation reconciliation. Codex process teardown no longer discovers or signals processes by spawn token. POSIX snapshot and process-group teardown remain, and teardown tests now use the updated function signature. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change avoids stopping unrelated processes that inherited a spawn tag. No actionable issue remains from the supplied evidence; merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Removing the startup sweep prevents chat tags inherited by editors, development tools, or other processes from causing those processes to be killed. Recorded-owner checks and process-tree teardown remain, but unusual interruption and detached-process cases are not fully validated. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: fbdb6e59-3928-4294-a191-86478dcbe5c9
📒 Files selected for processing (15)
config/scripts/ci-shard-timings.jsonsrc/main/codex/codex-app-server-connection.tssrc/main/codex/codex-app-server-process-teardown.test.tssrc/main/codex/codex-app-server-process-teardown.tssrc/main/runtime/agent-session-orphan-child-reaper.test.tssrc/main/runtime/agent-session-orphan-child-reaper.tssrc/main/runtime/agent-session-record-store.test.tssrc/main/runtime/agent-session-record-store.tssrc/main/runtime/agent-session-spawn-token-process-scan.tssrc/main/runtime/structured-agent-session-runtime.test.tssrc/main/runtime/structured-agent-session-runtime.tssrc/main/runtime/structured-agent-session-spawn-token-descendants.test.tssrc/main/runtime/structured-agent-session-support-probe.test.tssrc/shared/agent-session-lease-adjudication.test.tssrc/shared/agent-session-lease-adjudication.ts
💤 Files with no reviewable changes (5)
- src/main/runtime/agent-session-orphan-child-reaper.test.ts
- config/scripts/ci-shard-timings.json
- src/main/runtime/agent-session-record-store.ts
- src/main/runtime/agent-session-orphan-child-reaper.ts
- src/main/runtime/structured-agent-session-runtime.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.
| const release = Promise.withResolvers<boolean>() | ||
|
|
||
| const teardown = terminateCodexAppServerProcessTree(target, undefined, { | ||
| const teardown = terminateCodexAppServerProcessTree(target, { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Inspect local and shared test setup for an intercept of process.kill.
ast-grep outline src/main/codex/codex-app-server-process-teardown.test.ts --view expanded
sed -n '1,65p' src/main/codex/codex-app-server-process-teardown.test.ts
rg -n -C 3 'process\.kill|signalProcessGroup|vi\.mock|vi\.spyOn|setupFiles' \
src/main/codex/codex-app-server-process-teardown.test.ts .Repository: stablyai/orca
Length of output: 45665
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- test ---'
sed -n '38,58p' src/main/codex/codex-app-server-process-teardown.test.ts
printf '%s\n' '--- implementation locations ---'
rg -n -C 8 'terminateCodexAppServerProcessTree|signalProcessGroup|process\.kill\(-' src/main/codex/codex-app-server-process-teardown.ts src/main/codex
printf '%s\n' '--- implementation source ---'
sed -n '1,260p' src/main/codex/codex-app-server-process-teardown.tsRepository: stablyai/orca
Length of output: 40219
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- vitest setup configuration ---'
sed -n '1,55p' config/vitest.config.ts
printf '%s\n' '--- configured setup files ---'
for f in config/scripts/happy-dom-offscreen-canvas.ts config/scripts/happy-dom-mutation-observer-retention.ts config/scripts/vitest-host-ports-setup.ts; do
if [ -f "$f" ]; then
printf '\n### %s\n' "$f"
rg -n -C 2 'process\.kill|spyOn|mockImplementation|vi\.' "$f" || true
fi
doneRepository: stablyai/orca
Length of output: 2454
Prevent the snapshot test from signaling a real process group.
The test passes rootPgid: 1234 without overriding signalProcessGroup. When descendant termination resolves to true, teardown calls the default process.kill(-1234, 'SIGKILL').
Suggested fix
const release = Promise.withResolvers<boolean>()
+ const signalProcessGroup = vi.fn()
const teardown = terminateCodexAppServerProcessTree(target, {
platform: 'darwin',
captureDescendants: async () => snapshot,
- terminateDescendants: () => release.promise
+ terminateDescendants: () => release.promise,
+ signalProcessGroup
})🧰 Tools
🪛 ast-grep (0.45.3)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import type { ChildProcess } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the full 15-file diff at d9b622a, verifying that deleting the token-based kill paths leaves the recovery-by-identity guarantee intact.
- Linux startup sweep removed —
agent-session-orphan-child-reaper.ts,AgentSessionRecordStore.listOrphanSpawnTokens,classifyObservedAgentSessionSpawnToken, and thereapOrphanChildrenfire-and-forget install call are all gone, so no startup path signals a process because it carries a spawn token. - Codex teardown narrowed —
diagnosticTokenFallbackand its injection deps (findSpawnTokenProcesses,signalPid,isPidPresent,wait,now) are removed andterminateCodexAppServerProcessTreedrops itsspawnTokenpositional argument. The only production caller never passedfindSpawnTokenProcesses, so that branch was unreachable in production; the dedicated-process-group and descendant-snapshot proofs are untouched. - Reservation proof kept —
scanAgentSessionSpawnTokenProcessesstill backsprobeAgentSessionReservation: no process carries the reserved token ⇒ released withreservation never spawnedevidence; one does ⇒ released without evidence and deliberately not signalled. - Recovery path verified — a crash with an intact lease still ends with the recorded owner SIGTERM/SIGKILLed by identity through
resolveStructuredSessionRecovery(structured-agent-session-recovery-resolution.ts:87-113); the deleted sweep never touched a lease-claimed token, so this is not a regression. - Tests — new
structured-agent-session-spawn-token-descendants.test.tsdrives the real install/reconcile/scan code against a fake/procand is falsifiable (restoring the sweep makes its first case SIGTERM both tagged processes); the obsolete reaper and adjudication tests are removed and the runtime/support-probe fixtures simplified.
Verified locally: pnpm tc:node clean, oxlint clean on the changed files, and the six affected suites (104 tests) pass, including the new file.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…2025/delete-orphan-reaper # Conflicts: # src/main/runtime/structured-agent-session-runtime.ts
|
Status: ready for review. Main is merged in (head The merge: one conflict. Main added a field right next to the one this PR deletes; main's field is kept and only the deleted one is dropped. Main added no new uses of anything this PR removes, and a search of source, tests and config finds none. Only this PR's changes remain: the changed-file list and every added and removed line match the PR's diff from before the merge, and the lockfiles match main. Checks: typecheck, lint, code-quality and anti-slop pass. The affected test files pass; a few unrelated timing tests flaked only under heavy machine load and pass when run alone. |
|
Status: ready for review. Head Review: two review loops (the first fixed wording in the description, the second was clean). The design was challenged by an adversarial review before it was built. 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. One minor finding: a new test waits a fixed 50 ms for background work, which could miss a slower future signaller. Live Linux QA: passed on a real Linux machine, with Orca built from source and run there (details in Visual Proof).
Not covered: real-Claude timing on Linux for a lost record. |
1 participant
ELI5
When Orca starts a Codex or Claude chat, it puts a secret tag in the agent's environment so it can recognise the agent later. Every program the agent starts copies that tag: an editor opened with
code ., a tmux server, a dev server, even a second copy of Orca. On Linux, whenever structured chat started up, Orca looked for every process on the computer wearing a tag no current chat claimed and told it to quit. So after a chat ended, the next start of structured chat could close the editor, dev server or Orca window the agent had opened for you. This PR removes that startup sweep. Orca now only stops agent processes it has on record, which is already how macOS and Windows behave.What Changed
Each chat has a record that names the one agent process allowed to write to its conversation (its pid, start time and tag). Before the agent process starts, the record holds only the tag Orca is about to give it; this PR calls that a reserved chat.
Before (Linux only): the first structured-chat use after Orca started ran a sweep over every process on the machine. Any process whose environment carried an Orca spawn tag that no chat record claimed received SIGTERM. When a chat ends normally its record gives up the tag, so anything the agent started that outlived the chat, and had left the agent's process group, was killed on the next startup. A development copy of Orca started from an agent's shell also carries the outer tag, so its first chat could kill the outer agent, the outer Codex process and itself.
After: nothing at startup signals a process because of the tag. A leftover agent process from a crash is still found and stopped the same way as before: the chat record names the exact process (pid, start time and tag), restart marks it as recovering, and recovery stops that one process by its recorded identity before a new agent is started. macOS and Windows are unchanged; the sweep never ran there because they cannot read another process's environment.
What was removed:
src/main/runtime/agent-session-orphan-child-reaper.tsand its tests: the sweep itself.AgentSessionRecordStore.listOrphanSpawnTokensandclassifyObservedAgentSessionSpawnToken: they turned "no record claims this tag" into "stop this process", and nothing else used them.reapOrphanChildrenoption on the structured-chat runtime and its fire-and-forget call at install.diagnosticTokenFallback, itsfindSpawnTokenProcessesoption and its tests): it SIGKILLed every process carrying the tag, the same flaw. No production caller ever enabled it. Teardown keeps its normal path: it freezes the process, lists its child processes, kills exactly those and checks they exited, then kills the process group.What stays: the tag scan itself. It still answers one question soundly after a crash: "did this reserved chat ever start an agent?" If no process carries the tag, the chat's claim is released with proof that nothing spawned; if one does, the claim is released without that proof and the process is left alone. Nothing else reads it.
Why
Orca should stop only a process it can name. The chat record states exactly which process is the agent, and recovery already stops that one after a crash. A tag that every descendant inherits can never tell the agent apart from the programs the agent started, so any sweep built on it will kill the user's programs sooner or later.
The mechanism was wrong at its root, not just too broad. The tag is an environment variable, so carrying it proves only "descended from something Orca spawned", never "is Orca's agent process". A normal chat end clears the record's tag on purpose, and a chat now ends when its main agent process exits even if something it started is still running. Together those make every long-lived program an agent starts look like a lost agent to the next sweep.
Alternatives considered:
Deleting it costs little. The crash case with an intact record never depended on it (recovery handles it by recorded identity when Orca restarts). Codex on macOS and Linux already runs under a helper that kills Codex when Orca dies. Claude has no such helper on any platform: when Orca crashes, the Claude process loses its input but finishes the turn it is on before exiting by itself. Measured on macOS, that took about 0.2 s when idle, 7 to 9 s mid-reply, and about 2 minutes while a shell tool ran, bounded by that tool's timeout; nothing it started was left running afterwards. So on Linux the sweep only mattered for a Claude process whose record was lost and that was still mid-turn when structured chat next started. That is now the same gap macOS and Windows have always had; a follow-up will run Claude under the same helper as Codex.
Differences from the common pattern
Comparable desktop hosts stop only the child processes they hold a handle or record for, and never infer ownership by scanning the whole machine. This PR moves Linux onto that model. Where Orca still differs: it keeps a machine-wide tag scan, used only to prove a reserved chat never spawned (read-only, it never signals), and Codex runs under a parent-death helper that is stricter than relying on the agent noticing its pipes closed.
Linked Issue
No issue filed. Found while reviewing the structured-chat store and the change that ends a Claude chat on its main process exit (#22946). Recovery by recorded identity is the path from #22820.
Visual Proof
Live QA on a real Linux machine (Ubuntu), with Orca built from source for each arm and run on that machine, because the sweep only ever ran on the host that owns the chat. Base = this PR's merge base (
6835b9b4e3), PR =85634ada3a. The build proof: the sweep's scan function is in the base bundle and absent from the PR bundle. Each arm ran inside its own process namespace, so the base sweep could only see and signal the test's own processes. The agent was a stand-inclaudelaunched by Orca's real spawn path with its real tag, because the real Claude could not sign in on that machine without copying credentials. Every signal was logged with the sender's pid.setsid nohup), the chat is closed, Orca is quit and relaunchedThe UI is identical on both arms (this PR has no UI change); the difference is only in which processes are left running:
Testing
New
structured-agent-session-spawn-token-descendants.test.ts, run as Linux with a fake process table:Existing recovery tests confirm a recorded owner proven alive is still stopped by identity after a restart. Deleting the stop call in recovery fails 4 of them.
Codex teardown, connection and supervisor tests pass after removing the tag-scan kill path.
Ran the new file plus the runtime, support-probe, record-store, owner-record adjudication, tag-scan, identity-probe, Codex teardown and recovery suites (12 files, 164 tests) passing;
pnpm tc:nodeandpnpm tc:web; oxlint on every changed file;pnpm run check:code-quality:changed;pnpm run audit:anti-slop.Live Linux QA: run on a real Linux machine; see Visual Proof. A real Claude wasn't run there (it couldn't sign in without copying credentials), so the agent was a stand-in launched through Orca's real spawn path. The real-Claude timing for a lost record on Linux isn't measured; the macOS measurement above is the reference.
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
Mixed versions: an older Orca build on the same Linux machine keeps running its sweep. It kills any tagged process its own records do not claim, so it can still kill processes started by a newer build's chats, including chats that belong to a different Orca on the machine (for example a dev build beside a packaged build, or desktop beside the background server). A newer build cannot prevent that; it goes away as those builds update. The tag format and the stored record format are unchanged, and nothing here is on the wire.
Descendants still inherit the tag. That no longer causes a kill, and the reserved-chat check only asks about a tag it minted for a chat that never recorded its process, so a descendant carrying it can only make that check more cautious (released without proof), never less.
SSH and folder workspaces: no change. The sweep only ever ran on the local Linux host, and recovery on remote hosts already refuses to judge a process it cannot see.
Author X handle: @BrennanKB5
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)