From 9db1b4ce6efb892dbdd8885cae6d05a5cbf5d7de Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 04:07:09 -0700 Subject: [PATCH] fix(crash-reporting): make the own-Chromium gate a real choke point Round-3 review found the guard was not the choke point its own comments claimed: six pid-addressed `taskkill /pid /t /f` families in main were ungated and uninstrumented, so the stale-pid shape stayed producible and a `selfInitiatedTreeKillCount: 0` could read as exculpatory when it was not. - Gate the remaining main-process families: the git command-runner abort, the notebook-cell and automation-precheck timeouts. - Turn the `src/shared` seam into the gate itself (`process-tree-kill-gate`), so the runProcess choke point, the codex app-server deadline kill and the ephemeral-VM recipe kill ask the same decision. Those three are compiled into the CLI/relay too and cannot import main; main installs the guard at preflight. - Ratchet (`main-process-tree-kill-gate.test.ts`): a new pid-addressed taskkill in main that skips the gate fails, and the allowlist entries must still exist. - Give pid-addressed kills eviction priority in the 32-entry ring: 32 routine `win-pty-job` teardowns from a window-close burst no longer evict the one entry that discriminates a self-kill from an external one. - Correct the coverage doc, which described the uninstrumented Windows sites as POSIX `process.kill(-pid)` group kills and omitted the git and codex paths. --- src/main/automations/precheck-runner.ts | 10 ++ src/main/codex/codex-app-server-session.ts | 10 ++ .../self-initiated-tree-kill-log.test.ts | 47 +++++-- .../self-initiated-tree-kill-log.ts | 58 ++++---- .../spawned-command-tree-kill.ts | 6 + src/main/ipc/notebook.ts | 10 ++ src/main/main-process-tree-kill-gate.test.ts | 128 ++++++++++++++++++ src/main/own-chromium-tree-kill-guard.test.ts | 41 +++++- src/main/own-chromium-tree-kill-guard.ts | 16 ++- src/main/startup/main-process-preflight.ts | 6 +- src/main/windows-process-tree-kill.ts | 5 +- .../child-process/process-tree-kill-gate.ts | 41 ++++++ .../process-tree-kill-observer.ts | 37 ----- .../process-tree-termination.test.ts | 9 +- .../child-process/process-tree-termination.ts | 27 +++- src/shared/ephemeral-vm-recipe-process.ts | 10 ++ 16 files changed, 371 insertions(+), 90 deletions(-) create mode 100644 src/main/main-process-tree-kill-gate.test.ts create mode 100644 src/shared/child-process/process-tree-kill-gate.ts delete mode 100644 src/shared/child-process/process-tree-kill-observer.ts diff --git a/src/main/automations/precheck-runner.ts b/src/main/automations/precheck-runner.ts index 753bd784b06..d5c2450fd72 100644 --- a/src/main/automations/precheck-runner.ts +++ b/src/main/automations/precheck-runner.ts @@ -4,6 +4,7 @@ import type { AutomationPrecheck, AutomationPrecheckResult } from '../../shared/ import { MAX_AUTOMATION_PRECHECK_OUTPUT_CHARS } from '../../shared/automation-precheck' import { getSshConnectionManager } from '../ipc/ssh' import { shellEscape } from '../ssh/ssh-connection-utils' +import { admitSelfInitiatedTreeKill } from '../own-chromium-tree-kill-guard' type AutomationPrecheckExecutionTarget = | { @@ -81,6 +82,15 @@ function killLocalPrecheckProcessTree(child: ChildProcess): ReturnType { expect(String(details.selfInitiatedKills)).toContain('more)') }) - it('records a kill issued through the shared runProcess choke point', () => { - installProcessTreeKillBreadcrumbObserver() + it('keeps the pid-addressed kill when a window-close burst overruns the ring', () => { + // Review probe: one taskkill, then 32 routine Job Object teardowns. Under + // plain FIFO the discriminating entry is evicted and the persisted detail + // becomes byte-identical to the external-kill arm. + const goneAt = 5_000_000 + recordSelfInitiatedTreeKill({ + pid: 4242, + site: 'pty-descendant-sweep', + scope: 'win-taskkill-tree', + at: goneAt - 4_000 + }) + for (let index = 0; index < 32; index += 1) { + recordSelfInitiatedTreeKill({ + pid: 6000 + index, + site: 'windows-pty-job-teardown', + scope: 'win-pty-job', + at: goneAt - 100 + }) + } - notifyProcessTreeKill({ pid: 3131, site: 'run-process-tree', scope: 'posix-process-group' }) + const details = selfInitiatedTreeKillDetails(goneAt) + + expect(details.selfInitiatedTreeKillCount).toBe(1) + expect(details.selfInitiatedGroupKillCount).toBe(31) + expect(String(details.selfInitiatedKills)).toMatch( + /^win-taskkill-tree\/pty-descendant-sweep\/pid4242 -4000ms/ + ) + }) + + it('records a kill issued through the shared runProcess choke point', () => { + installMainProcessTreeKillGate() + + expect( + admitProcessTreeKill({ pid: 3131, site: 'run-process-tree', scope: 'posix-process-group' }) + ).toBe(true) expect(findSelfInitiatedTreeKills(Date.now()).map((kill) => kill.pid)).toEqual([3131]) - setProcessTreeKillObserver(null) + setProcessTreeKillGate(null) }) }) diff --git a/src/main/crash-reporting/self-initiated-tree-kill-log.ts b/src/main/crash-reporting/self-initiated-tree-kill-log.ts index e819795074f..45223e4f6ce 100644 --- a/src/main/crash-reporting/self-initiated-tree-kill-log.ts +++ b/src/main/crash-reporting/self-initiated-tree-kill-log.ts @@ -1,8 +1,5 @@ import type { CrashReportDetailValue } from '../../shared/crash-reporting' -import { - setProcessTreeKillObserver, - type ProcessTreeKillScope -} from '../../shared/child-process/process-tree-kill-observer' +import type { ProcessTreeKillScope } from '../../shared/child-process/process-tree-kill-gate' import { recordCoalescedDurableCrashBreadcrumb } from './durable-crash-breadcrumb' /** @@ -19,24 +16,34 @@ import { recordCoalescedDurableCrashBreadcrumb } from './durable-crash-breadcrum * The ring is per-process and its only reader is `process-gone-recorder`, which * exists in Electron main. So a count reported on a `render-process-gone` covers * kills issued *from Electron main*, and nothing else: - * - Main only: the three `taskkill /T /F` families that gate on - * `admitSelfInitiatedTreeKill` (`terminateWindowsProcessTree` and the codex / - * claude account-login teardowns) and the codex app-server POSIX group + * - Main only: the families that import the gate directly — + * `terminateWindowsProcessTree`, the codex and claude account-login + * teardowns, the git command-runner abort, the notebook-cell and + * automation-precheck timeouts — plus the codex app-server POSIX group * teardowns. - * - Main *and* other hosts: `signalProcessTree` (the `runProcess` choke point, - * reached from the CLI, relay and daemon too — a fourth pid-addressed - * `taskkill` family, gated on the child not being reaped rather than on the - * Chromium set it cannot read), the POSIX PTY process-group sweep and the - * Windows PTY Job Object (relay `pty-handler`, daemon - * `subprocess-handle`). When those run outside main they record into that - * process's own ring, which nothing reads — no observer is installed there, - * and the tracer sink is a no-op. - * - Never instrumented: the direct `process.kill(-pid)` calls in the browser - * routes, notebooks, automation prechecks and ephemeral-VM recipes. + * - Main *and* other hosts, through the `process-tree-kill-gate` seam main + * installs the same guard into: `signalProcessTree` (the `runProcess` choke + * point, reached from the CLI, relay and daemon too), the codex app-server + * deadline kill (compiled into the CLI as well) and the ephemeral-VM recipe + * kill. Also host-spanning but recording directly: the POSIX PTY + * process-group sweep and the Windows PTY Job Object (relay `pty-handler`, + * daemon `subprocess-handle`). When any of these run outside main they record + * into that process's own ring, which nothing reads — no gate is installed + * there, and the tracer sink is a no-op. + * - Never instrumented, and none of them a pid-addressed kill issued from main: + * the POSIX `process.kill(-pid, …)` group arms of the notebook, precheck, + * browser-route and ephemeral-VM kills; the relay's own + * `subprocess-tree-termination` taskkill and the CLI's login-interruption + * taskkill (neither runs in main); and the browser-route Electron probes, + * which are reached only from `*.electron.test.ts`. + * + * `main-process-tree-kill-gate.test.ts` is the ratchet that keeps that list + * closed: a new `taskkill /pid` in main fails it. * * A daemon or relay kill missing from the count is a diagnostics gap, not a * missed suspect: those hosts cannot reach a Chromium pid in the first place - * (see `orca-chromium-process-pids.ts`). Absence is evidence, not proof. + * (see `orca-chromium-process-pids.ts`), and a group or Job-Object kill can + * only contain what Orca put in it. Absence is evidence, not proof. */ /** Which mechanism issued the kill; each has a different blast radius. */ @@ -96,8 +103,14 @@ export function recordSelfInitiatedTreeKill({ return } selfInitiatedKills.push({ pid, site, scope, at }) - if (selfInitiatedKills.length > MAX_TRACKED_SELF_KILLS) { - selfInitiatedKills = selfInitiatedKills.slice(-MAX_TRACKED_SELF_KILLS) + while (selfInitiatedKills.length > MAX_TRACKED_SELF_KILLS) { + // Evict routine group/job teardown before a pid-addressed kill: a window-close + // burst is 30+ group kills, and plain FIFO would drop the one entry that can + // explain the death, leaving a detail byte-identical to the external-kill arm. + const oldestGroupKill = selfInitiatedKills.findIndex( + (kill) => !isPidAddressedTreeKill(kill.scope) + ) + selfInitiatedKills.splice(Math.max(oldestGroupKill, 0), 1) } // Durable so it survives into the diagnostic bundle even when the kill takes // the reporting renderer with it; coalesced because the crash detail above is @@ -133,11 +146,6 @@ export function recordRefusedOwnChromiumTreeKill(target: { }) } -/** Routes the `runProcess` choke point's kills here; shared code cannot import us. */ -export function installProcessTreeKillBreadcrumbObserver(): void { - setProcessTreeKillObserver((kill) => recordSelfInitiatedTreeKill(kill)) -} - export function findSelfInitiatedTreeKills(at: number): SelfInitiatedTreeKill[] { return selfInitiatedKills.filter((kill) => { const offsetMs = kill.at - at diff --git a/src/main/git/command-runner/spawned-command-tree-kill.ts b/src/main/git/command-runner/spawned-command-tree-kill.ts index c2fefcba1bc..e97028eac1b 100644 --- a/src/main/git/command-runner/spawned-command-tree-kill.ts +++ b/src/main/git/command-runner/spawned-command-tree-kill.ts @@ -1,4 +1,5 @@ import { spawn, type ChildProcess } from 'node:child_process' +import { admitSelfInitiatedTreeKill } from '../../own-chromium-tree-kill-guard' const WINDOWS_TREE_KILL_WAIT_MS = 2_000 @@ -8,6 +9,11 @@ export function killSpawnedCommandTree(child: ChildProcess): Promise { child.kill() return Promise.resolve() } + if ( + !admitSelfInitiatedTreeKill({ pid, site: 'git-command-tree-kill', scope: 'win-taskkill-tree' }) + ) { + return Promise.resolve() + } return new Promise((resolve) => { let killer: ChildProcess try { diff --git a/src/main/ipc/notebook.ts b/src/main/ipc/notebook.ts index 9255ab7383d..a971a326f57 100644 --- a/src/main/ipc/notebook.ts +++ b/src/main/ipc/notebook.ts @@ -4,6 +4,7 @@ import { dirname } from 'node:path' import { ipcMain } from 'electron' import type { Store } from '../persistence' import { resolveAuthorizedPath } from './filesystem-auth' +import { admitSelfInitiatedTreeKill } from '../own-chromium-tree-kill-guard' export type NotebookRunResult = { stdout: string @@ -62,6 +63,15 @@ function terminateNotebookProcessTree( } if (process.platform === 'win32') { + if ( + !admitSelfInitiatedTreeKill({ + pid: child.pid, + site: 'notebook-cell-timeout', + scope: 'win-taskkill-tree' + }) + ) { + return null + } try { // Why: a timed-out cell can spawn descendants. taskkill /T is the // Windows equivalent of terminating the whole process group. diff --git a/src/main/main-process-tree-kill-gate.test.ts b/src/main/main-process-tree-kill-gate.test.ts new file mode 100644 index 00000000000..7d696a7316a --- /dev/null +++ b/src/main/main-process-tree-kill-gate.test.ts @@ -0,0 +1,128 @@ +import { readFileSync, readdirSync, statSync } from 'node:fs' +import { join, relative, resolve } from 'node:path' +import { describe, expect, it } from 'vitest' + +/** + * The ratchet behind the guard's claim to be a choke point. + * + * `admitSelfInitiatedTreeKill` is only "one decision" for as long as every + * pid-addressed `taskkill /pid /t /f` in Electron main asks it. Each such + * kill can land on a recycled pid that is now one of Orca's own Chromium + * processes (#10680), and an ungated one is also invisible to + * `selfInitiatedTreeKillCount`, which makes a zero read as exculpatory when it + * is not. A new family fails here rather than in the field. + */ +const REPOSITORY_ROOT = resolve(__dirname, '..', '..') +const MAIN_DIRECTORY = 'src/main/' +const SCANNED_EXTENSIONS = ['.ts', '.tsx'] +const IGNORED_DIRECTORIES = new Set([ + 'node_modules', + 'dist', + 'out', + 'build', + '.git', + '__fixtures__' +]) + +/** Pid-addressed: `/pid ` walks whatever tree owns that pid *now*. */ +const PID_ADDRESSED_TASKKILL = /['"]taskkill(?:\.exe)?['"][\s\S]{0,120}?['"]\/pid['"]/i + +const GATE = 'admitSelfInitiatedTreeKill' +/** The `src/shared` seam main installs the same gate into; shared code cannot import it directly. */ +const SEAM = 'admitProcessTreeKill' + +/** + * Only ever shrinks. Each entry states why the gate cannot reach it — never + * "not got to yet", which is what a new ungated family would also look like. + */ +const UNGATED_TASKKILL_ALLOWLIST = new Map([ + [ + 'src/main/browser/browser-route-egress-electron-launch.ts', + 'Electron probe reached only from *.electron.test.ts; kills the probe Electron it spawned' + ], + [ + 'src/main/browser/browser-route-persisted-worker-electron-process.ts', + 'Electron probe reached only from *.electron.test.ts; kills the probe Electron it spawned' + ], + [ + 'src/cli/handlers/interactive-login-interruption.ts', + 'CLI host: no Chromium pid on the machine to reach, and no reader for the ring' + ], + [ + 'src/relay/subprocess-tree-termination.ts', + 'Relay host: same, and the relay cannot import the main-process gate' + ] +]) + +function isTestFile(path: string): boolean { + return /\.(?:test|spec)\.tsx?$/.test(path) || /(?:test-harness|test-fixture|fixture)/.test(path) +} + +function scanSourceFiles(directory: string, found: string[] = []): string[] { + for (const entry of readdirSync(directory)) { + if (IGNORED_DIRECTORIES.has(entry)) { + continue + } + const path = join(directory, entry) + if (statSync(path).isDirectory()) { + scanSourceFiles(path, found) + continue + } + if (SCANNED_EXTENSIONS.some((extension) => entry.endsWith(extension)) && !isTestFile(path)) { + found.push(path) + } + } + return found +} + +// Only the Node-side hosts: a renderer or preload cannot spawn a process at all. +const SCANNED_HOSTS = ['src/main', 'src/shared', 'src/cli', 'src/relay'] + +/** Scanned once at import: 10k files is seconds, and every case below reuses it. */ +const PID_ADDRESSED_TASKKILL_FILES = SCANNED_HOSTS.flatMap((host) => + scanSourceFiles(join(REPOSITORY_ROOT, host)) + .map((path) => ({ + path: relative(REPOSITORY_ROOT, path).split('\\').join('/'), + source: readFileSync(path, 'utf8') + })) + .filter((file) => PID_ADDRESSED_TASKKILL.test(file.source)) +) + +function pidAddressedTaskkillFiles(): { path: string; source: string }[] { + return PID_ADDRESSED_TASKKILL_FILES +} + +describe('main-process tree-kill gate', () => { + it('finds the taskkill families it is meant to police', () => { + // Falsifiable: a scanner that matched nothing would pass every case below. + expect(pidAddressedTaskkillFiles().map((file) => file.path)).toContain( + 'src/main/windows-process-tree-kill.ts' + ) + }) + + it('routes every pid-addressed taskkill in Electron main through the gate', () => { + const ungated = pidAddressedTaskkillFiles() + .filter((file) => file.path.startsWith(MAIN_DIRECTORY)) + .filter((file) => !file.source.includes(GATE) && !file.source.includes(SEAM)) + .map((file) => file.path) + .filter((path) => !UNGATED_TASKKILL_ALLOWLIST.has(path)) + + expect(ungated).toEqual([]) + }) + + it('leaves no pid-addressed taskkill outside main unaccounted for', () => { + const unaccounted = pidAddressedTaskkillFiles() + .filter((file) => !file.path.startsWith(MAIN_DIRECTORY)) + .filter((file) => !file.source.includes(GATE) && !file.source.includes(SEAM)) + .map((file) => file.path) + .filter((path) => !UNGATED_TASKKILL_ALLOWLIST.has(path)) + + expect(unaccounted).toEqual([]) + }) + + it('keeps the allowlist honest: every entry still spawns a taskkill', () => { + const spawning = new Set(pidAddressedTaskkillFiles().map((file) => file.path)) + + expect([...UNGATED_TASKKILL_ALLOWLIST.keys()].filter((path) => !spawning.has(path))).toEqual([]) + }) +}) diff --git a/src/main/own-chromium-tree-kill-guard.test.ts b/src/main/own-chromium-tree-kill-guard.test.ts index 7e98661aca6..315ec4f1dc4 100644 --- a/src/main/own-chromium-tree-kill-guard.test.ts +++ b/src/main/own-chromium-tree-kill-guard.test.ts @@ -13,7 +13,12 @@ import { import { readOrcaChromiumProcessPids } from './orca-chromium-process-pids' import { classifyWindowsTreeKillTarget } from './windows-pty-root-identity' import { terminateWindowsProcessTree } from './windows-process-tree-kill' -import { admitSelfInitiatedTreeKill } from './own-chromium-tree-kill-guard' +import { + admitSelfInitiatedTreeKill, + installMainProcessTreeKillGate +} from './own-chromium-tree-kill-guard' +import { killCodexAppServerProcessTree } from './codex/codex-app-server-session' +import { setProcessTreeKillGate } from '../shared/child-process/process-tree-kill-gate' import { resetSelfInitiatedTreeKillLogForTest } from './crash-reporting/self-initiated-tree-kill-log' import { clearCrashBreadcrumbsForTest, @@ -57,6 +62,7 @@ beforeEach(() => { setActiveSink({ push: () => {}, flush: () => {}, close: () => {} }) clearCrashBreadcrumbsForTest() resetSelfInitiatedTreeKillLogForTest() + installMainProcessTreeKillGate() }) afterEach(() => { @@ -66,6 +72,7 @@ afterEach(() => { vi.restoreAllMocks() _resetTracerForTests() clearCrashBreadcrumbsForTest() + setProcessTreeKillGate(null) }) describe('refusing to tree-kill our own Chromium processes', () => { @@ -143,6 +150,38 @@ describe('refusing to tree-kill our own Chromium processes', () => { ) }) + it('refuses the codex app-server deadline kill against one of our own pids', () => { + const spawnImpl = vi.fn(() => ({ on: vi.fn(), unref: vi.fn() })) + const child = { pid: RENDERER_PID, kill: vi.fn() } + + killCodexAppServerProcessTree(child as never, { + platform: 'win32', + spawnImpl: spawnImpl as never + }) + + // The deadline timer fires on `child.pid` alone; a reaped-then-recycled pid + // is the stale-pid mechanism this gate exists to stop. + expect(spawnImpl).not.toHaveBeenCalled() + expect(getCrashBreadcrumbSnapshot()).toEqual([ + expect.objectContaining({ name: 'self_tree_kill_refused_own_chromium' }) + ]) + }) + + it('still lets the codex app-server deadline kill reach a foreign pid', () => { + const killer = { on: vi.fn(), unref: vi.fn() } + const spawnImpl = vi.fn(() => killer) + + killCodexAppServerProcessTree({ pid: 7777, kill: vi.fn() } as never, { + platform: 'win32', + spawnImpl: spawnImpl as never + }) + + expect(spawnImpl).toHaveBeenCalledWith('taskkill', ['/pid', '7777', '/t', '/f'], { + stdio: 'ignore', + windowsHide: true + }) + }) + it('refuses an own-Chromium pid at the gate the account teardowns share', () => { expect( admitSelfInitiatedTreeKill({ diff --git a/src/main/own-chromium-tree-kill-guard.ts b/src/main/own-chromium-tree-kill-guard.ts index 4daedf4faa4..75734c919ce 100644 --- a/src/main/own-chromium-tree-kill-guard.ts +++ b/src/main/own-chromium-tree-kill-guard.ts @@ -4,16 +4,19 @@ import { type SelfInitiatedTreeKillScope } from './crash-reporting/self-initiated-tree-kill-log' import { readOrcaChromiumProcessPids } from './orca-chromium-process-pids' +import { setProcessTreeKillGate } from '../shared/child-process/process-tree-kill-gate' /** * Gate every main-process tree-kill through one decision: refuse the pid when * Electron is currently accounting for it, otherwise put it on the record. * * Why a shared gate rather than a check inside `terminateWindowsProcessTree`: - * the codex and claude account-login teardowns run their own `taskkill /T /F` - * with different lifetimes (one sync, one with its own timeout ladder), so a - * guard that only lived in the tree-kill helper would cover one of three - * families. Returns false when the caller must not kill. + * five other families in main run their own `taskkill /T /F` with different + * lifetimes (sync, fire-and-forget, timeout ladder), and three more live in + * `src/shared` and reach this through `process-tree-kill-gate`, so a guard that + * only lived in the tree-kill helper would cover one of nine. + * `main-process-tree-kill-gate.test.ts` holds that set closed. Returns false + * when the caller must not kill. * * Electron main only, by construction. `terminateWindowsProcessTree` also runs * in the standalone daemon (the `pty-descendant-sweep` site), where @@ -38,3 +41,8 @@ export function admitSelfInitiatedTreeKill(target: { recordSelfInitiatedTreeKill(target) return true } + +/** Hands the gate to the shared choke points, which cannot import main. */ +export function installMainProcessTreeKillGate(): void { + setProcessTreeKillGate((kill) => admitSelfInitiatedTreeKill(kill)) +} diff --git a/src/main/startup/main-process-preflight.ts b/src/main/startup/main-process-preflight.ts index 269eb2212e7..177a2357441 100644 --- a/src/main/startup/main-process-preflight.ts +++ b/src/main/startup/main-process-preflight.ts @@ -48,7 +48,7 @@ import { } from './single-instance-lock' import { setAppEnvironment } from '../../shared/app-environment' import { ElectronAppEnvironment } from '../host/electron-app-environment' -import { installProcessTreeKillBreadcrumbObserver } from '../crash-reporting/self-initiated-tree-kill-log' +import { installMainProcessTreeKillGate } from '../own-chromium-tree-kill-guard' import { setSecretStore } from '../../shared/secret-store' import { ElectronSecretStore } from '../host/electron-secret-store' import { setPtyHostBindings } from '../ipc/pty-host-bindings' @@ -163,8 +163,8 @@ export function runMainProcessPreflight(options: MainProcessPreflightOptions): b }) } // Why before any spawn: `signalProcessTree` is shared with the CLI and relay, so - // it can only reach the main-process breadcrumb store through a registered observer. - installProcessTreeKillBreadcrumbObserver() + // it can only reach the main-process guard and breadcrumb store once this is registered. + installMainProcessTreeKillGate() const isDev = is.dev configureDevUserDataPath(isDev) configureOrcaUserDataPathEnv() diff --git a/src/main/windows-process-tree-kill.ts b/src/main/windows-process-tree-kill.ts index ea7b756ada4..31e6b18da2a 100644 --- a/src/main/windows-process-tree-kill.ts +++ b/src/main/windows-process-tree-kill.ts @@ -11,8 +11,9 @@ export const WINDOWS_PROCESS_TREE_KILL_TIMEOUT_MS = 5_000 * Best-effort: missing/already-dead roots still resolve so callers can finish * their own handle cleanup via killRoot. * - * Nearly every main-process taskkill runs through here; the two account-login - * teardowns keep their own spawn but share the same gate, so the refusal and the + * Most main-process taskkills run through here; the families that keep their own + * spawn (account-login teardowns, codex app-server deadline, git-command abort, + * notebook and precheck timeouts) share the same gate, so the refusal and the * breadcrumb live in `admitSelfInitiatedTreeKill` rather than in this function. */ export function terminateWindowsProcessTree( diff --git a/src/shared/child-process/process-tree-kill-gate.ts b/src/shared/child-process/process-tree-kill-gate.ts new file mode 100644 index 00000000000..4fa4e053a03 --- /dev/null +++ b/src/shared/child-process/process-tree-kill-gate.ts @@ -0,0 +1,41 @@ +/** + * Seam that lets the main process decide, and record, the tree-kills issued + * from code it does not own. + * + * Why a seam and not a direct call: `signalProcessTree` is the choke point every + * `runProcess` termination funnels through, and the codex app-server and + * ephemeral-VM kills are shared with the CLI — all of them live outside + * `src/main` and cannot import the own-Chromium guard or the crash breadcrumb + * store. Main registers the guard at startup; everywhere else this admits every + * kill and records nothing. + */ + +/** Blast radius, not mechanism: `win-taskkill-tree` is addressed by pid and walks + * whatever tree that pid has *now*, so it can land on a recycled pid that is + * since one of Orca's own Chromium processes. A process group can only contain + * processes Orca itself put there. */ +export type ProcessTreeKillScope = 'win-taskkill-tree' | 'posix-process-group' + +export type ProcessTreeKill = { + pid: number + site: string + scope: ProcessTreeKillScope +} + +/** False means the caller must not kill: main is currently accounting for that pid. */ +type ProcessTreeKillGate = (kill: ProcessTreeKill) => boolean + +let gate: ProcessTreeKillGate | null = null + +export function setProcessTreeKillGate(next: ProcessTreeKillGate | null): void { + gate = next +} + +export function admitProcessTreeKill(kill: ProcessTreeKill): boolean { + try { + return gate?.(kill) ?? true + } catch { + // Diagnostics must never turn a successful termination into a failed one. + return true + } +} diff --git a/src/shared/child-process/process-tree-kill-observer.ts b/src/shared/child-process/process-tree-kill-observer.ts deleted file mode 100644 index 8abf971798a..00000000000 --- a/src/shared/child-process/process-tree-kill-observer.ts +++ /dev/null @@ -1,37 +0,0 @@ -/** - * Seam that lets the main process record the tree-kills issued from here. - * - * Why a seam and not a direct call: `signalProcessTree` is the choke point every - * `runProcess` termination funnels through, but it lives in `src/shared` and so - * runs in the CLI and relay too — it cannot import the main-process crash - * breadcrumb store. Main registers the recorder at startup; everywhere else this - * stays a no-op. - */ - -/** Blast radius, not mechanism: `win-taskkill-tree` is addressed by pid and walks - * whatever tree that pid has *now*, so it can land on a recycled pid that is - * since one of Orca's own Chromium processes. A process group can only contain - * processes Orca itself put there. */ -export type ProcessTreeKillScope = 'win-taskkill-tree' | 'posix-process-group' - -export type ProcessTreeKill = { - pid: number - site: string - scope: ProcessTreeKillScope -} - -type ProcessTreeKillObserver = (kill: ProcessTreeKill) => void - -let observer: ProcessTreeKillObserver | null = null - -export function setProcessTreeKillObserver(next: ProcessTreeKillObserver | null): void { - observer = next -} - -export function notifyProcessTreeKill(kill: ProcessTreeKill): void { - try { - observer?.(kill) - } catch { - // Diagnostics must never turn a successful termination into a failed one. - } -} diff --git a/src/shared/child-process/process-tree-termination.test.ts b/src/shared/child-process/process-tree-termination.test.ts index cc3394fb5a3..10a4c725089 100644 --- a/src/shared/child-process/process-tree-termination.test.ts +++ b/src/shared/child-process/process-tree-termination.test.ts @@ -7,7 +7,7 @@ const { spawnMock } = vi.hoisted(() => ({ spawnMock: vi.fn() })) vi.mock('node:child_process', () => ({ spawn: spawnMock })) import { forceTerminateProcessTree, signalProcessTree } from './process-tree-termination' -import { setProcessTreeKillObserver, type ProcessTreeKill } from './process-tree-kill-observer' +import { setProcessTreeKillGate, type ProcessTreeKill } from './process-tree-kill-gate' function mockProcess(pid: number): ChildProcess { const child = new EventEmitter() as EventEmitter & { @@ -103,11 +103,14 @@ describe('process-tree-kill breadcrumb seam', () => { beforeEach(() => { observed.length = 0 - setProcessTreeKillObserver((kill) => observed.push(kill)) + setProcessTreeKillGate((kill) => { + observed.push(kill) + return true + }) }) afterEach(() => { - setProcessTreeKillObserver(null) + setProcessTreeKillGate(null) spawnMock.mockReset() vi.restoreAllMocks() }) diff --git a/src/shared/child-process/process-tree-termination.ts b/src/shared/child-process/process-tree-termination.ts index 15264b2f6ce..d508d743102 100644 --- a/src/shared/child-process/process-tree-termination.ts +++ b/src/shared/child-process/process-tree-termination.ts @@ -1,5 +1,5 @@ import { spawn as nodeSpawn, type ChildProcess } from 'node:child_process' -import { notifyProcessTreeKill } from './process-tree-kill-observer' +import { admitProcessTreeKill } from './process-tree-kill-gate' const PROBE_INTERVAL_MS = 25 const SUBPROCESS_TIMEOUT_MS = 2_000 @@ -13,8 +13,12 @@ const MAX_PS_OUTPUT_BYTES = 8 * 1024 * 1024 * leader would hand it to whatever group it inherited instead. * * Runs in every host — Electron main, the daemon, the relay, the CLI — so the - * main-process own-Chromium guard cannot reach here; the exit check below is - * what keeps the Windows branch off a pid that is no longer ours. + * own-Chromium guard arrives through `process-tree-kill-gate`, which main + * installs and every other host leaves admitting. The exit check below is what + * keeps the Windows branch off a pid that is no longer ours on those hosts. + * + * Both arms ask before killing, so a refused pid is never signalled; that also + * means the recorded crumb says "about to kill", not "killed". */ export function signalProcessTree(child: ChildProcess, signal?: NodeJS.Signals): Promise { if (!child.pid) { @@ -39,13 +43,17 @@ export function signalProcessTree(child: ChildProcess, signal?: NodeJS.Signals): } return taskkillTree(child, child.pid, signal) } - try { - process.kill(-child.pid, signal) - notifyProcessTreeKill({ + if ( + !admitProcessTreeKill({ pid: child.pid, site: 'run-process-tree', scope: 'posix-process-group' }) + ) { + return Promise.resolve(false) + } + try { + process.kill(-child.pid, signal) return Promise.resolve(true) } catch { return Promise.resolve(!processGroupExists(child.pid)) @@ -73,6 +81,12 @@ function taskkillTree( rootPid: number, signal?: NodeJS.Signals ): Promise { + // Asked before the spawn, not after: a refusal has to prevent the taskkill. + if ( + !admitProcessTreeKill({ pid: rootPid, site: 'run-process-tree', scope: 'win-taskkill-tree' }) + ) { + return Promise.resolve(false) + } return new Promise((resolve) => { let killer: ChildProcess try { @@ -86,7 +100,6 @@ function taskkillTree( resolve(false) return } - notifyProcessTreeKill({ pid: rootPid, site: 'run-process-tree', scope: 'win-taskkill-tree' }) let settled = false const finish = (fallback: boolean): void => { if (settled) { diff --git a/src/shared/ephemeral-vm-recipe-process.ts b/src/shared/ephemeral-vm-recipe-process.ts index b258a1b2704..08682256c18 100644 --- a/src/shared/ephemeral-vm-recipe-process.ts +++ b/src/shared/ephemeral-vm-recipe-process.ts @@ -1,5 +1,6 @@ import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process' import type { EphemeralVmRecipeContext } from './ephemeral-vm-recipe-runner' +import { admitProcessTreeKill } from './child-process/process-tree-kill-gate' const DEFAULT_MAX_CAPTURE_BYTES = 1024 * 1024 const CANCEL_FORCE_KILL_DELAY_MS = 5_000 @@ -139,6 +140,15 @@ function killRecipeProcess(child: ChildProcessWithoutNullStreams, force = false) // terminate the wrapper and orphan the actual recipe subprocess (e.g. a cloud // CLI mid-provision). taskkill /T walks and kills the whole tree. if (child.pid) { + if ( + !admitProcessTreeKill({ + pid: child.pid, + site: 'ephemeral-vm-recipe', + scope: 'win-taskkill-tree' + }) + ) { + return + } const killer = spawn('taskkill', ['/pid', String(child.pid), '/t', '/f'], { windowsHide: true, stdio: 'ignore'