Skip to content

fix(native-chat): stop killing processes that only inherited a chat's spawn tag #23460

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
brennanb2025 merged 3 commits into from
Sep 28, 2026
Merged

fix(native-chat): stop killing processes that only inherited a chat's spawn tag #23460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
879a1e7
fix(native-chat): stop signalling processes that only inherited a spa…
brennanb2025 Sep 27, 2026
d9b622a
fix(codex): remove the token-scan kill path from app-server teardown
brennanb2025 Sep 27, 2026
85634ad
Merge commit '6835b9b4e3ea256e30ee5fe3ed663ae99326bb72' into brennanb…
brennanb2025 Sep 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion config/scripts/ci-shard-timings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3241,7 +3241,6 @@
"src/main/runtime/agent-session-launch-env-backfill.test.ts": 264,
"src/main/runtime/agent-session-lease-renewal.test.ts": 242,
"src/main/runtime/agent-session-operation-admission.test.ts": 165,
"src/main/runtime/agent-session-orphan-child-reaper.test.ts": 90,
"src/main/runtime/agent-session-process-identity-probe-windows-batch.test.ts": 47,
"src/main/runtime/agent-session-process-identity-probe.test.ts": 78,
"src/main/runtime/agent-session-provider-handle-transition.test.ts": 104,
Expand Down
4 changes: 1 addition & 3 deletions src/main/codex/codex-app-server-connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { buildCodexAppServerExitError } from './codex-app-server-exit-error'
import { initializeCodexAppServerConnection } from './codex-app-server-handshake'
import { CodexAppServerHandshakeExitUnprovenError } from './codex-app-server-handshake-exit-proof'
import { terminateCodexAppServerProcessTree } from './codex-app-server-process-teardown'
import { CODEX_SPAWN_TOKEN_ENV } from './codex-structured-owner-identity'
import { waitForProcessExitUntil } from './codex-process-exit-deadline'
import {
CodexAppServerTimeoutError,
Expand Down Expand Up @@ -64,12 +63,11 @@ export async function openCodexAppServerConnection(
}
const spawnSpec = createProviderSpawnSpec(launch, childEnv, process.platform)
const child = spawnImpl(spawnSpec)
const spawnToken = launch.env?.[CODEX_SPAWN_TOKEN_ENV]

function terminateProcessTree(): Promise<boolean> {
// The supervisor and provider own separate POSIX groups so the supervisor can prove the
// provider group empty before relaying its exit. Forced wrapper teardown uses descendant proof.
return terminateCodexAppServerProcessTree(child, spawnToken)
return terminateCodexAppServerProcessTree(child)
}

let stderrTail = ''
Expand Down
54 changes: 7 additions & 47 deletions src/main/codex/codex-app-server-process-teardown.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ describe('terminateCodexAppServerProcessTree', () => {
const release = Promise.withResolvers<void>()
const terminateWindowsTree = vi.fn(() => release.promise)

const teardown = terminateCodexAppServerProcessTree(target, undefined, {
const teardown = terminateCodexAppServerProcessTree(target, {
platform: 'win32',
terminateWindowsTree
})
Expand All @@ -38,52 +38,12 @@ describe('terminateCodexAppServerProcessTree', () => {
expect(target.kill).toHaveBeenCalledWith('SIGKILL')
})

it('kills exact Linux spawn-token PIDs before the recorded wrapper', async () => {
const target = child()
const findSpawnTokenProcesses = vi
.fn<() => Promise<number[] | null>>()
.mockResolvedValueOnce([1234, 2345, 3456])
.mockResolvedValueOnce([1234])
.mockResolvedValueOnce([1234])
const signalPid = vi.fn()

await expect(
terminateCodexAppServerProcessTree(target, 'spawn-1', {
platform: 'linux',
findSpawnTokenProcesses,
signalPid,
isPidPresent: () => false,
wait: async () => undefined
})
).resolves.toBe(true)

expect(signalPid.mock.calls).toEqual([
[2345, 'SIGKILL'],
[3456, 'SIGKILL']
])
expect(target.kill).toHaveBeenCalledTimes(1)
expect(target.kill).toHaveBeenCalledWith('SIGKILL')
})

it('keeps the wrapper reachable when Linux cannot prove descendant exit', async () => {
const target = child()

await expect(
terminateCodexAppServerProcessTree(target, 'spawn-1', {
platform: 'linux',
findSpawnTokenProcesses: async () => null
})
).resolves.toBe(false)

expect(target.kill).not.toHaveBeenCalled()
})

it('waits for an owned POSIX snapshot before killing the wrapper', async () => {
const target = child()
const snapshot = { rootPgid: 1234, descendants: [], capturedAtMs: 1 }
const release = Promise.withResolvers<boolean>()

const teardown = terminateCodexAppServerProcessTree(target, undefined, {
const teardown = terminateCodexAppServerProcessTree(target, {

@coderabbitai coderabbitai Bot Sep 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.ts

Repository: 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
done

Repository: 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)

platform: 'darwin',
captureDescendants: async () => snapshot,
terminateDescendants: () => release.promise
Expand All @@ -102,7 +62,7 @@ describe('terminateCodexAppServerProcessTree', () => {
const signalProcessGroup = vi.fn()

await expect(
terminateCodexAppServerProcessTree(target, undefined, {
terminateCodexAppServerProcessTree(target, {
platform: 'darwin',
dedicatedProcessGroup: true,
captureDescendants,
Expand All @@ -119,7 +79,7 @@ describe('terminateCodexAppServerProcessTree', () => {
const target = child()

await expect(
terminateCodexAppServerProcessTree(target, undefined, {
terminateCodexAppServerProcessTree(target, {
platform: 'linux',
dedicatedProcessGroup: true,
signalProcessGroup: () => {
Expand All @@ -142,7 +102,7 @@ describe('terminateCodexAppServerProcessTree', () => {
const target = { pid: UNREACHABLE_PGID, kill: vi.fn(() => true) as ChildProcess['kill'] }

await expect(
terminateCodexAppServerProcessTree(target, undefined, {
terminateCodexAppServerProcessTree(target, {
platform: 'darwin',
captureDescendants: async () => ({
rootPgid: UNREACHABLE_PGID,
Expand All @@ -162,7 +122,7 @@ describe('terminateCodexAppServerProcessTree', () => {
const signalProcessGroup = vi.fn()

await expect(
terminateCodexAppServerProcessTree(target, undefined, {
terminateCodexAppServerProcessTree(target, {
platform: 'darwin',
captureDescendants: async () => ({ rootPgid: 1234, descendants: [], capturedAtMs: 1 }),
terminateDescendants: async () => true,
Expand Down Expand Up @@ -191,7 +151,7 @@ describe('terminateCodexAppServerProcessTree', () => {

const results = await Promise.all(
targets.map((target) =>
terminateCodexAppServerProcessTree(target, undefined, {
terminateCodexAppServerProcessTree(target, {
platform: 'linux',
dedicatedProcessGroup: true,
captureDescendants,
Expand Down
74 changes: 2 additions & 72 deletions src/main/codex/codex-app-server-process-teardown.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,19 @@ import type { ChildProcessHandle } from '../../shared/child-process/run-process'
import { captureDescendantSnapshot, type DescendantSnapshot } from '../pty-descendant-termination'
import { terminateDescendantSnapshotAndWait } from '../pty-descendant-exit-verification'
import { terminateWindowsProcessTree } from '../windows-process-tree-kill'
import { findAgentSessionSpawnTokenProcesses } from '../runtime/agent-session-spawn-token-process-scan'
import { recordSelfInitiatedTreeKill } from '../crash-reporting/self-initiated-tree-kill-log'

const TOKEN_PROCESS_EXIT_TIMEOUT_MS = 3_500
const TOKEN_PROCESS_POLL_MS = 25
const activeTeardowns = new WeakMap<object, Promise<boolean>>()

type TeardownChild = Pick<ChildProcessHandle, 'pid' | 'kill'>

export type CodexAppServerProcessTeardownDeps = {
platform?: NodeJS.Platform
dedicatedProcessGroup?: boolean
/** Diagnostic/recovery injection only; never used by the primary teardown. */
findSpawnTokenProcesses?: (spawnToken: string) => Promise<number[] | null>
captureDescendants?: (rootPid: number) => Promise<DescendantSnapshot | null>
terminateDescendants?: (snapshot: DescendantSnapshot) => Promise<boolean>
terminateWindowsTree?: (rootPid: number, deps?: { site?: string }) => Promise<void>
signalPid?: (pid: number, signal: NodeJS.Signals) => void
signalProcessGroup?: (pgid: number, signal: NodeJS.Signals) => void
isPidPresent?: (pid: number) => boolean
wait?: (ms: number) => Promise<void>
now?: () => number
}

function terminateDedicatedPosixGroup(
Expand All @@ -47,70 +38,11 @@ function terminateDedicatedPosixGroup(
return true
}

function sendSignal(pid: number, signal: NodeJS.Signals): void {
try {
process.kill(pid, signal)
} catch {
// An already-gone exact PID is the desired outcome.
}
}

function isPidPresent(pid: number): boolean {
try {
process.kill(pid, 0)
return true
} catch (error) {
return (error as NodeJS.ErrnoException).code !== 'ESRCH'
}
}

async function diagnosticTokenFallback(
rootPid: number,
spawnToken: string,
deps: CodexAppServerProcessTeardownDeps
): Promise<boolean> {
const find = deps.findSpawnTokenProcesses ?? findAgentSessionSpawnTokenProcesses
const signal = deps.signalPid ?? sendSignal
const pidPresent = deps.isPidPresent ?? isPidPresent
const delay =
deps.wait ?? ((ms: number) => new Promise<void>((resolve) => setTimeout(resolve, ms)))
const now = deps.now ?? Date.now
const deadline = now() + TOKEN_PROCESS_EXIT_TIMEOUT_MS
const signalled = new Set<number>()
while (now() < deadline) {
const pids = await find(spawnToken).catch(() => null)
if (pids === null) {
return false
}
for (const pid of pids.filter((candidate) => candidate !== rootPid)) {
signalled.add(pid)
signal(pid, 'SIGKILL')
}
if ([...signalled].every((pid) => !pidPresent(pid))) {
return true
}
await delay(TOKEN_PROCESS_POLL_MS)
}
return false
}

async function terminatePosixTree(
child: TeardownChild,
rootPid: number,
_spawnToken: string | undefined,
deps: CodexAppServerProcessTeardownDeps
): Promise<boolean> {
// Kept only for explicit recovery callers/tests. Production always follows
// the dedicated process-group path below; token enumeration is evidence,
// never the owner of orphan-reaping decisions.
if (_spawnToken && deps.findSpawnTokenProcesses) {
const reaped = await diagnosticTokenFallback(rootPid, _spawnToken, deps)
if (reaped) {
child.kill('SIGKILL')
return true
}
return false
}
child.kill('SIGSTOP')
const capture = deps.captureDescendants ?? captureDescendantSnapshot
const snapshot = await capture(rootPid).catch(() => null)
Expand Down Expand Up @@ -158,7 +90,6 @@ async function terminatePosixTree(
/** Stops every process owned by one app-server launch before releasing its wrapper. */
async function terminateOnce(
child: TeardownChild,
spawnToken: string | undefined,
deps: CodexAppServerProcessTeardownDeps
): Promise<boolean> {
const rootPid = child.pid
Expand All @@ -176,20 +107,19 @@ async function terminateOnce(
if (deps.dedicatedProcessGroup) {
return terminateDedicatedPosixGroup(rootPid, deps)
}
return terminatePosixTree(child, rootPid, spawnToken, deps)
return terminatePosixTree(child, rootPid, deps)
}

export function terminateCodexAppServerProcessTree(
child: TeardownChild,
spawnToken?: string,
deps: CodexAppServerProcessTeardownDeps = {}
): Promise<boolean> {
const key = child as object
const active = activeTeardowns.get(key)
if (active) {
return active
}
const attempt = terminateOnce(child, spawnToken, deps).catch(() => false)
const attempt = terminateOnce(child, deps).catch(() => false)
activeTeardowns.set(key, attempt)
void attempt.then(() => {
if (activeTeardowns.get(key) === attempt) {
Expand Down
68 changes: 0 additions & 68 deletions src/main/runtime/agent-session-orphan-child-reaper.test.ts
View file
Open in desktop

This file was deleted.

48 changes: 0 additions & 48 deletions src/main/runtime/agent-session-orphan-child-reaper.ts
View file
Open in desktop

This file was deleted.

Loading
Loading