mirror of
https://github.com/stablyai/orca.git
synced 2026-09-29 16:02:50 +00:00
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 <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.
This commit is contained in:
@@ -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<typeof se
|
||||
}
|
||||
|
||||
if (process.platform === 'win32') {
|
||||
if (
|
||||
!admitSelfInitiatedTreeKill({
|
||||
pid,
|
||||
site: 'automation-precheck-timeout',
|
||||
scope: 'win-taskkill-tree'
|
||||
})
|
||||
) {
|
||||
return null
|
||||
}
|
||||
try {
|
||||
// Why: shell prechecks can launch child processes; taskkill walks the
|
||||
// Windows process tree so timeout means the command is actually stopped.
|
||||
|
||||
@@ -2,6 +2,7 @@ import { spawn, type ChildProcess, type ChildProcessWithoutNullStreams } from 'n
|
||||
import { waitForProcessExitUntil } from './codex-process-exit-deadline'
|
||||
import { stderrIndicatesMissingAppServer } from './codex-app-server-capability-signal'
|
||||
import { withCliRuntimeOnPath } from '../../shared/node-cli-command-resolution'
|
||||
import { admitProcessTreeKill } from '../../shared/child-process/process-tree-kill-gate'
|
||||
|
||||
// Why: `codex app-server` is Orca's sanctioned RPC surface into Codex-owned
|
||||
// state (hook trust hashes, the sqlite thread index). This module owns the
|
||||
@@ -74,6 +75,15 @@ export function killCodexAppServerProcessTree(
|
||||
const platform = options.platform ?? process.platform
|
||||
const spawnImpl = options.spawnImpl ?? spawn
|
||||
if (platform === 'win32' && child.pid) {
|
||||
if (
|
||||
!admitProcessTreeKill({
|
||||
pid: child.pid,
|
||||
site: 'codex-app-server-session-deadline',
|
||||
scope: 'win-taskkill-tree'
|
||||
})
|
||||
) {
|
||||
return
|
||||
}
|
||||
try {
|
||||
// Why: npm-installed Codex runs behind cmd.exe; killing only that wrapper
|
||||
// leaves the app-server child alive after a timeout or failed shutdown.
|
||||
|
||||
@@ -17,17 +17,17 @@ import { recordProcessGoneCrash, type ProcessGoneCrashEvent } from './process-go
|
||||
import { resetProcessGoneSiblingCorrelationForTest } from './process-gone-sibling-correlation'
|
||||
import {
|
||||
findSelfInitiatedTreeKills,
|
||||
installProcessTreeKillBreadcrumbObserver,
|
||||
recordRefusedOwnChromiumTreeKill,
|
||||
recordSelfInitiatedTreeKill,
|
||||
resetSelfInitiatedTreeKillLogForTest,
|
||||
selfInitiatedTreeKillDetails
|
||||
} from './self-initiated-tree-kill-log'
|
||||
import {
|
||||
notifyProcessTreeKill,
|
||||
setProcessTreeKillObserver
|
||||
} from '../../shared/child-process/process-tree-kill-observer'
|
||||
admitProcessTreeKill,
|
||||
setProcessTreeKillGate
|
||||
} from '../../shared/child-process/process-tree-kill-gate'
|
||||
import { terminateWindowsProcessTree } from '../windows-process-tree-kill'
|
||||
import { installMainProcessTreeKillGate } from '../own-chromium-tree-kill-guard'
|
||||
import { _resetTracerForTests, setActiveSink } from '../observability/tracer'
|
||||
|
||||
/** The field shape: renderer, `reason=killed exitCode=1`, win32 (#G2). */
|
||||
@@ -252,12 +252,43 @@ describe('self-initiated tree kill breadcrumb', () => {
|
||||
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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<void> {
|
||||
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 {
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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 <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 <n>` 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<string, string>([
|
||||
[
|
||||
'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([])
|
||||
})
|
||||
})
|
||||
@@ -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({
|
||||
|
||||
@@ -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))
|
||||
}
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
@@ -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.
|
||||
}
|
||||
}
|
||||
@@ -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()
|
||||
})
|
||||
|
||||
@@ -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<boolean> {
|
||||
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<boolean> {
|
||||
// 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) {
|
||||
|
||||
@@ -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'
|
||||
|
||||
Reference in New Issue
Block a user