diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index ca1651325c9..a749214e232 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -807,6 +807,7 @@ jobs: src/main/agent-hooks/windows-hook-payload-delivery.test.ts src/main/windows/windows-pty-job.win32.test.ts src/main/windows/windows-host-job.win32.test.ts + src/main/windows-live-tree-kill.win32.test.ts src/main/wsl/wsl-runner.test.ts src/main/wsl/wsl-guest-environment.test.ts src/main/wsl/wsl-invocation-boundary.test.ts diff --git a/config/scripts/pr-code-change-scope.mjs b/config/scripts/pr-code-change-scope.mjs index f5a73f6239f..15ded915c67 100644 --- a/config/scripts/pr-code-change-scope.mjs +++ b/config/scripts/pr-code-change-scope.mjs @@ -219,6 +219,7 @@ const WINDOWS_PACKAGE_TESTS = [ 'src/main/agent-hooks/windows-hook-payload-delivery.test.ts', 'src/main/windows/windows-pty-job.win32.test.ts', 'src/main/windows/windows-host-job.win32.test.ts', + 'src/main/windows-live-tree-kill.win32.test.ts', 'src/main/wsl/wsl-runner.test.ts', 'src/main/wsl/wsl-guest-environment.test.ts', 'src/main/wsl/wsl-invocation-boundary.test.ts', diff --git a/src/main/__fixtures__/shell-wrapper-snapshots/daemon-bash-rcfile.txt b/src/main/__fixtures__/shell-wrapper-snapshots/daemon-bash-rcfile.txt index 54837bf4d65..b79ed543494 100644 --- a/src/main/__fixtures__/shell-wrapper-snapshots/daemon-bash-rcfile.txt +++ b/src/main/__fixtures__/shell-wrapper-snapshots/daemon-bash-rcfile.txt @@ -127,10 +127,6 @@ __orca_osc133_precmd() { unset __orca_in_command fi printf "\033]133;A\007" - # Why: emit the shell-ready marker here (not a trailing PROMPT_COMMAND entry) - # so a framework that must be last in PROMPT_COMMAND — bash-preexec — is not - # displaced by one of Orca's own hooks. - [[ -n "$__orca_ready_marker" ]] && printf "\033]777;orca-shell-ready\007" return "$exit_code" } __orca_osc133_preexec() { @@ -188,6 +184,11 @@ __orca_osc133_epilogue() { unset __orca_in_prompt_command __orca_adopt_outer_debug_trap trap '__orca_osc133_preexec' DEBUG + # Readline renders PS1 after entering raw mode; prompt hooks still run in cooked mode. + if [[ -n "$__orca_ready_marker" ]]; then + PS1="${PS1-}"'\[\e]777;orca-shell-ready\a\]' + __orca_ready_marker="" + fi } __orca_normalize_prompt_command_part() { local __orca_value="$1" __orca_output_name="$2" __orca_character __orca_chunk diff --git a/src/main/automations/precheck-runner.ts b/src/main/automations/precheck-runner.ts index 753bd784b06..ab38fd42355 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 = | { @@ -73,7 +74,10 @@ function failedPrecheckResult( }) } -function killLocalPrecheckProcessTree(child: ChildProcess): ReturnType | null { +/** Exported for the refusal-fallback test; the timeout path is otherwise unreachable. */ +export function killLocalPrecheckProcessTree( + child: ChildProcess +): ReturnType | null { const pid = child.pid if (!pid) { child.kill() @@ -81,6 +85,18 @@ function killLocalPrecheckProcessTree(child: ChildProcess): ReturnType baseline.get(row.pid) !== windowsIdentity(row)) const addedPids = new Set(added.map((row) => row.pid)) const roots = added.filter((row) => !addedPids.has(row.ppid)) + // Added roots come from a table walk, not a spawn, so a refused tree walk has + // no handle to fall back to: the row stays in `remaining` and this reports false. await Promise.all( roots.map((row) => terminateWindowsProcessTree(row.pid, { site: 'codex-turn-added-roots' })) ) diff --git a/src/main/crash-reporting/self-initiated-tree-kill-log.test.ts b/src/main/crash-reporting/self-initiated-tree-kill-log.test.ts index c57c1796a35..e958dac42f8 100644 --- a/src/main/crash-reporting/self-initiated-tree-kill-log.test.ts +++ b/src/main/crash-reporting/self-initiated-tree-kill-log.test.ts @@ -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,90 @@ 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('keeps the newest teardown when a session has saturated the ring with pid kills', () => { + // Review probe, the mirror of the case above: 32 session-old taskkills (six + // routine families feed them) then the Job Object teardown 50ms before the + // death. A scope-preference eviction with no floor splices the entry it just + // pushed, and `{}` is byte-identical to the external-kill arm. + const goneAt = 5_000_000 + for (let index = 0; index < 32; index += 1) { + recordSelfInitiatedTreeKill({ + pid: 6000 + index, + site: 'pty-descendant-sweep', + scope: 'win-taskkill-tree', + at: goneAt - 600_000 + index * 1_000 + }) + } + recordSelfInitiatedTreeKill({ + pid: 7777, + site: 'windows-pty-job-teardown', + scope: 'win-pty-job', + at: goneAt - 50 + }) + + const details = selfInitiatedTreeKillDetails(goneAt) + + expect(details.selfInitiatedGroupKillCount).toBe(1) + expect(String(details.selfInitiatedKills)).toContain( + 'win-pty-job/windows-pty-job-teardown/pid7777 -50ms' + ) + }) + + it('evicts the oldest pid kill, not the newest, once every candidate is pid-addressed', () => { + const goneAt = 5_000_000 + for (let index = 0; index < 33; index += 1) { + recordSelfInitiatedTreeKill({ + pid: 6000 + index, + site: 'git-command-tree-kill', + scope: 'win-taskkill-tree', + at: goneAt - 1_000 + }) + } + + const pids = findSelfInitiatedTreeKills(goneAt).map((kill) => kill.pid) + + expect(pids).toHaveLength(32) + expect(pids).toContain(6032) + expect(pids).not.toContain(6000) + }) + + 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..809d421ac61 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,38 @@ 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, plus the macOS keyboard-input-source + * probe's group kill in `ipc/app.ts`; 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: it counts `/pid` call sites against gate admissions per file, so a new + * pid-addressed kill fails it whether it lands in a new file or inside a family + * that already asks the gate. It does not see a `/pid` argument built from a + * variable. * * 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. */ @@ -81,6 +92,25 @@ function isPidAddressedTreeKill(scope: SelfInitiatedTreeKillScope): boolean { return scope === 'win-taskkill-tree' } +/** + * Drop one entry, newest-first-preserving. + * + * Two rules, in order. The entry just recorded is never a candidate: it is the + * one closest to any death that follows, and evicting it leaves a detail + * byte-identical to the external-kill arm. Among the rest, routine group/job + * teardown goes 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 — + * falling back to plain FIFO once every candidate is pid-addressed, which is + * what an ordinary session saturates the ring with. + */ +function evictOneSelfInitiatedTreeKill(): void { + const lastCandidate = selfInitiatedKills.length - 1 + const oldestGroupKill = selfInitiatedKills.findIndex( + (kill, index) => index < lastCandidate && !isPidAddressedTreeKill(kill.scope) + ) + selfInitiatedKills.splice(Math.max(oldestGroupKill, 0), 1) +} + export function recordSelfInitiatedTreeKill({ pid, site, @@ -96,13 +126,15 @@ 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) { + evictOneSelfInitiatedTreeKill() } // 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 // the primary record and a teardown burst must not cost 30 ring slots plus a - // forced disk flush each. The newest pid still rides the emitted crumb. + // forced disk flush each. The retained ring crumb carries the newest pid, but + // the span trail emits only the first of a coalesced burst — read + // `selfInitiatedKills` for the rest. recordCoalescedDurableCrashBreadcrumb({ name: 'self_tree_kill', data: { pid, site, scope }, @@ -133,11 +165,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/daemon/agent-startup-prompt-latency.node-pty.test.ts b/src/main/daemon/agent-startup-prompt-latency.node-pty.test.ts new file mode 100644 index 00000000000..cfa01fed8e5 --- /dev/null +++ b/src/main/daemon/agent-startup-prompt-latency.node-pty.test.ts @@ -0,0 +1,123 @@ +import { appendFileSync, existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { createPtySubprocess } from './pty-subprocess' +import { Session } from './session' + +const SHELLS = process.platform === 'win32' ? [] : ['/bin/bash', '/bin/zsh'].filter(existsSync) +const COMMAND = "printf 'AGENT_%s\\n' STARTED" + +async function launch( + shell: string, + slow: boolean, + legacy = false +): Promise<{ output: string; ms: number }> { + const root = mkdtempSync(join(tmpdir(), 'orca-startup-latency-')) + const bash = shell.endsWith('bash') + const pause = slow ? 'sleep 0.6\n' : '' + const prompt = slow ? "PS1='$(sleep 0.3)prompt> '\n" : "PS1='prompt> '\n" + writeFileSync( + join(root, bash ? '.bash_profile' : '.zshrc'), + `${pause}${bash ? '' : 'setopt PROMPT_SUBST\n'}${prompt}` + ) + vi.stubEnv('HOME', root) + vi.stubEnv('ZDOTDIR', root) + vi.stubEnv('ORCA_ORIG_ZDOTDIR', root) + let session: Session | undefined + let timer: ReturnType | undefined + let legacyTimer: ReturnType | undefined + const readinessEvents: string[] = [] + const started = performance.now() + try { + const subprocess = await createPtySubprocess({ + sessionId: 'startup-latency', + cols: 120, + rows: 30, + cwd: root, + shellOverride: shell, + command: COMMAND, + env: { HOME: root, SHELL: shell, TERM: 'xterm-256color' } + }) + session = new Session({ + sessionId: 'startup-latency', + cols: 120, + rows: 30, + subprocess, + shellReadySupported: !legacy, + reportReadinessEvent: (event) => readinessEvents.push(event) + }) + const active = session + return await new Promise((resolve, reject) => { + let output = '' + timer = setTimeout( + () => reject(new Error(`Startup timed out: ${JSON.stringify(output)}`)), + 5000 + ) + active.attachClient({ + onExit: () => {}, + onData: (data) => { + output += data + if (output.includes('AGENT_STARTED')) { + resolve({ output, ms: performance.now() - started }) + } + } + }) + if (legacy) { + legacyTimer = setTimeout(() => active.write(`${COMMAND}\n`), 300) + } else { + active.write(`${COMMAND}\n`) + } + }) + } finally { + clearTimeout(timer) + clearTimeout(legacyTimer) + if (session) { + await session.forceKillAndWaitForExit(3000) + session.dispose() + } + vi.unstubAllEnvs() + rmSync(root, { recursive: true, force: true }) + expect(readinessEvents).toEqual([]) + } +} + +describe('agent startup at the rendered shell prompt', () => { + afterEach(() => vi.unstubAllEnvs()) + it.each(SHELLS)( + '%s displays the command once after slow startup and prompt expansion', + async (shell) => { + const before = await launch(shell, true, true) + expect(before.output.split(COMMAND)).toHaveLength(3) + const result = await launch(shell, true) + expect(result.output).not.toContain('orca-shell-ready') + expect(result.output.split(COMMAND)).toHaveLength(2) + expect(result.output.indexOf('prompt> ')).toBeLessThan(result.output.indexOf(COMMAND)) + } + ) + + it.skipIf(!process.env.ORCA_STARTUP_BENCH || SHELLS.length === 0)( + 'compares legacy input timing with prompt delivery', + async () => { + for (const shell of SHELLS) { + for (const slow of [false, true]) { + const legacy: number[] = [] + const current: number[] = [] + for (let i = 0; i < 5; i++) { + legacy.push((await launch(shell, slow, true)).ms) + const result = await launch(shell, slow) + expect(result.output).not.toContain('orca-shell-ready') + expect(result.output.split(COMMAND)).toHaveLength(2) + current.push(result.ms) + } + const result = JSON.stringify({ shell, slow, legacy, current }) + if (process.env.ORCA_STARTUP_BENCH_OUTPUT) { + appendFileSync(process.env.ORCA_STARTUP_BENCH_OUTPUT, `${result}\n`) + } + console.log(result) + } + } + }, + 60_000 + ) +}) diff --git a/src/main/daemon/daemon-bash-shell-ready-rcfile.ts b/src/main/daemon/daemon-bash-shell-ready-rcfile.ts index 142c0c131d9..f7834dba9d7 100644 --- a/src/main/daemon/daemon-bash-shell-ready-rcfile.ts +++ b/src/main/daemon/daemon-bash-shell-ready-rcfile.ts @@ -2,7 +2,6 @@ import { getPosixOmpShellWrapper } from '../pty/omp-shell-wrapper' import { getPosixCodexShellLaunchPreflight } from '../pty/codex-shell-launch-preflight' import { BASH_PROMPT_COMMAND_COMPOSITION_BLOCK } from '../bash-prompt-command-composition' import { BASH_FEATURE_CHANNEL_BLOCK, SHELL_STARTUP_IDENTITY_MARKER_BLOCK } from '../shell-templates' -import { SHELL_READY_MARKER } from './daemon-shell-ready-marker' export function getDaemonBashShellReadyRcfileContent(): string { return `# Orca daemon bash shell-ready wrapper @@ -56,10 +55,6 @@ __orca_osc133_precmd() { unset __orca_in_command fi printf "\\033]133;A\\007" - # Why: emit the shell-ready marker here (not a trailing PROMPT_COMMAND entry) - # so a framework that must be last in PROMPT_COMMAND — bash-preexec — is not - # displaced by one of Orca's own hooks. - [[ -n "$__orca_ready_marker" ]] && printf "${SHELL_READY_MARKER}" return "$exit_code" } __orca_osc133_preexec() { @@ -117,6 +112,11 @@ __orca_osc133_epilogue() { unset __orca_in_prompt_command __orca_adopt_outer_debug_trap trap '__orca_osc133_preexec' DEBUG + # Readline renders PS1 after entering raw mode; prompt hooks still run in cooked mode. + if [[ -n "$__orca_ready_marker" ]]; then + PS1="\${PS1-}"'\\[\\e]777;orca-shell-ready\\a\\]' + __orca_ready_marker="" + fi } ${BASH_PROMPT_COMMAND_COMPOSITION_BLOCK} __orca_prepend_prompt_command "__orca_osc133_precmd" diff --git a/src/main/daemon/daemon-pty-adapter.test.ts b/src/main/daemon/daemon-pty-adapter.test.ts index fc2f9365f23..d0c0198af65 100644 --- a/src/main/daemon/daemon-pty-adapter.test.ts +++ b/src/main/daemon/daemon-pty-adapter.test.ts @@ -344,35 +344,32 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { } }) - itOnPosix('keeps plain Codex startup on the short daemon shell-ready timeout', async () => { - await adapter.spawn({ - cols: 80, - rows: 24, - command: 'codex', - env: { SHELL: '/bin/zsh' } - }) - + itOnPosix('preserves the existing fast-start timing for fish', async () => { + await adapter.spawn({ cols: 80, rows: 24, command: 'codex', env: { SHELL: '/usr/bin/fish' } }) await waitFor(() => vi.mocked(lastSubprocess.write).mock.calls.length > 0) - expect(lastSubprocess.write).toHaveBeenCalledWith('codex\n') + expect(lastSubprocess.write).toHaveBeenCalledExactlyOnceWith('codex\n') + expect(lastSpawnOpts).not.toEqual( + expect.objectContaining({ startupCommandDelivery: 'shell-ready' }) + ) }) - itOnPosix('waits for shell-ready for delivery-hinted Codex startup', async () => { - await adapter.spawn({ - cols: 80, - rows: 24, - command: "codex 'linked issue context'", - startupCommandDelivery: 'shell-ready', - env: { SHELL: '/bin/zsh' } - }) + itOnPosix.each([ + { command: 'codex' }, + { command: 'codex', startupCommandDelivery: 'fast' as const }, + { command: "codex 'linked issue context'", startupCommandDelivery: 'shell-ready' as const } + ])('waits past 300ms and submits once after readiness: %j', async (startup) => { + await adapter.spawn({ cols: 80, rows: 24, ...startup, env: { SHELL: '/bin/zsh' } }) await new Promise((resolve) => setTimeout(resolve, 350)) expect(lastSubprocess.write).not.toHaveBeenCalled() - + expect(lastSpawnOpts).toEqual( + expect.objectContaining({ startupCommandDelivery: 'shell-ready' }) + ) lastSubprocess._simulateData('\x1b]777;orca-shell-ready\x07') lastSubprocess._simulateData('\r\nuser@host $ ') await waitFor(() => vi.mocked(lastSubprocess.write).mock.calls.length > 0) - expect(lastSubprocess.write).toHaveBeenCalledWith("codex 'linked issue context'\n") + expect(lastSubprocess.write).toHaveBeenCalledExactlyOnceWith(`${startup.command}\n`) }) }) diff --git a/src/main/daemon/daemon-pty-session-spawn.ts b/src/main/daemon/daemon-pty-session-spawn.ts index 235b286b0bc..00f19777878 100644 --- a/src/main/daemon/daemon-pty-session-spawn.ts +++ b/src/main/daemon/daemon-pty-session-spawn.ts @@ -1,5 +1,6 @@ import { recognizeAgentProcessFromCommandLine } from '../../shared/agent-process-recognition' import { shouldUseShellReadyStartupDelivery } from '../../shared/codex-startup-delivery' +import { CODEX_SHELL_READY_TIMEOUT_MS } from './session-shell-ready-barrier' import type { HistoryRecoveryContext, PendingDaemonSpawnOperation @@ -11,8 +12,11 @@ import { DaemonPtySpawnResult } from './daemon-pty-spawn-result' import type { DaemonPtySpawnContext } from './daemon-pty-spawn-request' import type { ColdRestoreInfo } from './history-reader' import { mintPtySessionId } from './pty-session-id' -import { CODEX_SHELL_READY_TIMEOUT_MS } from './session-shell-ready-barrier' -import { supportsPtyStartupBarrier } from './shell-ready' +import { + supportsPtyStartupBarrier, + shellReadyMarkerComesFromLineEditor, + resolvePtyShellPath +} from './shell-ready' import { getRecoveredHistorySeedSegments } from './terminal-history-seed-segments' import { AGENT_SESSION_CLAIM_DAEMON_PROTOCOL_VERSION, type CreateOrAttachResult } from './types' import { normalizeWslColdRestoreCwd } from './wsl-cold-restore-cwd' @@ -213,21 +217,23 @@ export abstract class DaemonPtySessionSpawn extends DaemonPtySpawnResult { let effectiveRows = restoreInfo?.rows ?? opts.rows const shellReadySupported = opts.command ? supportsPtyStartupBarrier(opts.env ?? {}) : false - const isCodexStartupCommand = - recognizeAgentProcessFromCommandLine(opts.command)?.agent === 'codex' - const shouldWaitForShellReady = - isCodexStartupCommand && - shouldUseShellReadyStartupDelivery({ + const immediateMarker = + process.platform !== 'win32' && + shellReadyMarkerComesFromLineEditor(opts.shellOverride || resolvePtyShellPath(opts.env ?? {})) + const shellReadyTimeoutMs = + shellReadySupported && + !immediateMarker && + recognizeAgentProcessFromCommandLine(opts.command)?.agent === 'codex' && + !shouldUseShellReadyStartupDelivery({ command: opts.command, startupCommandDelivery: opts.startupCommandDelivery }) - const shellReadyTimeoutMs = - shellReadySupported && isCodexStartupCommand && !shouldWaitForShellReady ? CODEX_SHELL_READY_TIMEOUT_MS : undefined - const context: DaemonPtySpawnContext = { - opts, + // Older daemons also need the existing hint to enable their ready marker. + opts: + opts.command && immediateMarker ? { ...opts, startupCommandDelivery: 'shell-ready' } : opts, operation, historyRecovery, requestedSessionId, diff --git a/src/main/daemon/post-ready-flush-gate.test.ts b/src/main/daemon/post-ready-flush-gate.test.ts index f8e58ba7c7d..06ff64db99f 100644 --- a/src/main/daemon/post-ready-flush-gate.test.ts +++ b/src/main/daemon/post-ready-flush-gate.test.ts @@ -20,6 +20,14 @@ describe('PostReadyFlushGate', () => { vi.useRealTimers() }) + it('flushes synchronously when the marker comes from the line editor', () => { + gate = new PostReadyFlushGate(onFlush, true) + gate.arm() + expect(onFlush).toHaveBeenCalledTimes(1) + expect(gate.isPending).toBe(false) + expect(vi.getTimerCount()).toBe(0) + }) + it('does not flush immediately when armed', () => { gate.arm() expect(onFlush).not.toHaveBeenCalled() diff --git a/src/main/daemon/post-ready-flush-gate.ts b/src/main/daemon/post-ready-flush-gate.ts index 199bb601024..f7f7a2f1869 100644 --- a/src/main/daemon/post-ready-flush-gate.ts +++ b/src/main/daemon/post-ready-flush-gate.ts @@ -1,23 +1,5 @@ -/** - * Defers a flush callback until after the shell has drawn its prompt and - * switched the PTY into raw mode. - * - * Why: the OSC 777 shell-ready marker fires from zsh's precmd_functions / - * bash's PROMPT_COMMAND — before the shell draws its prompt and before - * zle/readline flips the PTY into raw mode. Flushing queued input then lets - * the kernel (ECHO still on) echo the command once, and the line editor - * redraws it under the prompt — producing a visible duplicate (e.g. "claude" - * appears twice on agent launch). - * - * Strategy: after arm() is called, wait for prompt bytes plus a short delay - * for the tcsetattr() that enables raw mode. If the marker-completing scan - * already saw post-marker bytes, use that same short path immediately. - * A conservative wall-clock fallback covers ambiguous marker-only cases. - * - * Mirrors the gate in local-pty-shell-ready.ts::writeStartupCommandWhenShellReady, - * which solves the same race on the non-daemon path. - */ - +// Bash's prompt and zsh's line-init marker are ready for input immediately. +// Other shells retain the existing settling delay. export const POST_READY_FLUSH_DELAY_MS = 30 export const POST_READY_FLUSH_FALLBACK_MS = 200 @@ -26,7 +8,10 @@ export class PostReadyFlushGate { private postDataTimer: ReturnType | null = null private fallbackTimer: ReturnType | null = null - constructor(private readonly onFlush: () => void) {} + constructor( + private readonly onFlush: () => void, + private readonly markerIsLineEditorReady = false + ) {} /** True between arm() and the actual flush firing. Callers should treat * input as still-queued during this window to preserve ordering. */ @@ -38,6 +23,10 @@ export class PostReadyFlushGate { * wall-clock fallback unless the marker scan already observed post-marker * bytes, in which case the short post-data settle path is enough. */ arm(postMarkerBytesObserved = false): void { + if (this.markerIsLineEditorReady) { + this.onFlush() + return + } this.awaitingPromptDraw = true if (postMarkerBytesObserved) { this.notifyData() diff --git a/src/main/daemon/pty-subprocess-managed-agent-env.test.ts b/src/main/daemon/pty-subprocess-managed-agent-env.test.ts index 5267778088c..d5dbc33246c 100644 --- a/src/main/daemon/pty-subprocess-managed-agent-env.test.ts +++ b/src/main/daemon/pty-subprocess-managed-agent-env.test.ts @@ -261,7 +261,7 @@ describe('createPtySubprocess', () => { expect(lastCall[2].env.ORCA_SHELL_FEATURES).not.toContain('ready') }) - it('keeps plain Codex startup commands on the no-marker wrapper', async () => { + it('enables readiness and shell identity for plain Codex startup', async () => { const proc = mockPtyProcess() spawnMock.mockReturnValue(proc) const platform = Object.getOwnPropertyDescriptor(process, 'platform') @@ -285,7 +285,8 @@ describe('createPtySubprocess', () => { const lastCall = spawnMock.mock.calls.at(-1)! expect(lastCall[1]).toEqual(['-l']) expect(lastCall[2].env.ZDOTDIR).toMatch(ZSH_SHELL_READY_DIR) - expect(lastCall[2].env.ORCA_SHELL_FEATURES).not.toContain('ready') + expect(lastCall[2].env.ORCA_SHELL_FEATURES).toContain('ready') + expect(lastCall[2].env.ORCA_SHELL_FEATURES).toContain('identity') }) it('uses shell-ready wrapper for delivery-hinted Codex startup commands', async () => { diff --git a/src/main/daemon/pty-subprocess/shell-launch-plan.ts b/src/main/daemon/pty-subprocess/shell-launch-plan.ts index ed8cd5e6a22..12e049d9f72 100644 --- a/src/main/daemon/pty-subprocess/shell-launch-plan.ts +++ b/src/main/daemon/pty-subprocess/shell-launch-plan.ts @@ -1,3 +1,4 @@ +import { shouldUseShellReadyStartupDelivery } from '../../../shared/codex-startup-delivery' import { win32 as pathWin32 } from 'node:path' import { isWindowsGitBashShellPath, resolveWindowsGitBashShellPath } from '../../git-bash' import { isPwshAvailable } from '../../pwsh' @@ -32,10 +33,13 @@ import { recognizeAgentProcessFromCommandLine, type RecognizedAgentProcess } from '../../../shared/agent-process-recognition' -import { shouldUseShellReadyStartupDelivery } from '../../../shared/codex-startup-delivery' import { ORCA_HERMES_STARTUP_QUERY_ENV } from '../../../shared/hermes-startup-query' import { WINDOWS_GIT_BASH_SHELL } from '../../../shared/windows-terminal-shell' -import { getShellLaunchConfig, resolvePtyShellPath } from '../shell-ready' +import { + getShellLaunchConfig, + resolvePtyShellPath, + shellReadyMarkerComesFromLineEditor +} from '../shell-ready' import { resolveWslSessionContext } from '../wsl-session-context' import { finalizeDaemonPtyEnvironment, rescrubDaemonPtyEnvironment } from './spawn-environment' import type { PtySubprocessOptions } from '../pty-subprocess' @@ -60,7 +64,6 @@ export function createPtyShellLaunchPlan( let startupCommandDeliveredInShellArgs = false let windowsFallbackAttempts: WindowsShellSpawnAttempt[] = [] const startupAgentRecognition = recognizeAgentProcessFromCommandLine(opts.command) - const isCodexStartupCommand = startupAgentRecognition?.agent === 'codex' const requestedCwd = opts.cwd || resolveSafePtyDefaultCwd() if (opts.command && startupAgentRecognition) { assertSafeAgentStartupCwd(requestedCwd, opts.command) @@ -192,9 +195,10 @@ export function createPtyShellLaunchPlan( } const waitsForShellReady = Boolean(opts.command) && - (!isCodexStartupCommand || + (startupAgentRecognition?.agent !== 'codex' || + shellReadyMarkerComesFromLineEditor(shellPath) || shouldUseShellReadyStartupDelivery({ - command: opts.command as string, + command: opts.command, startupCommandDelivery: opts.startupCommandDelivery })) delete env.ORCA_SHELL_FEATURES diff --git a/src/main/daemon/session-shell-ready-barrier.ts b/src/main/daemon/session-shell-ready-barrier.ts index e7562e23224..538fd57a49a 100644 --- a/src/main/daemon/session-shell-ready-barrier.ts +++ b/src/main/daemon/session-shell-ready-barrier.ts @@ -1,3 +1,4 @@ +import { shellReadyMarkerComesFromLineEditor } from './shell-ready' import { installDeviceAttributesResponder, STARTUP_DA1_RESPONSE @@ -19,7 +20,6 @@ import { basename } from 'node:path' import type { ShellReadyState } from './types' const SHELL_READY_TIMEOUT_MS = 15_000 -// Why: Codex skips marker-gated command delivery; this only bounds older daemon/local paths that still report shell-ready for Codex. export const CODEX_SHELL_READY_TIMEOUT_MS = 300 export type SessionShellReadyBarrierDeps = { @@ -69,7 +69,10 @@ export class SessionShellReadyBarrier { this._state = 'unsupported' } - this.postReadyFlushGate = new PostReadyFlushGate(() => this.flushPreReadyQueue()) + this.postReadyFlushGate = new PostReadyFlushGate( + () => this.flushPreReadyQueue(), + shellReadyMarkerComesFromLineEditor(deps.subprocess.shellPath ?? '') + ) } get state(): ShellReadyState { diff --git a/src/main/daemon/shell-ready.ts b/src/main/daemon/shell-ready.ts index 9dc60b7c980..d51f6efb80a 100644 --- a/src/main/daemon/shell-ready.ts +++ b/src/main/daemon/shell-ready.ts @@ -101,6 +101,11 @@ export function resolvePtyShellPath(env: Record): string { return env.SHELL || process.env.SHELL || '/bin/zsh' } +export function shellReadyMarkerComesFromLineEditor(shellPath: string): boolean { + const shellName = pathWin32.basename(basename(shellPath)).toLowerCase() + return shellName === 'bash' || shellName === 'zsh' +} + export function shellPathSupportsPtyStartupBarrier(shellPath: string): boolean { const shellName = pathWin32.basename(basename(shellPath)).toLowerCase() // Why fish: markerless, its startup command is written before fish's reader owns 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..324e04f1db3 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,14 @@ export function killSpawnedCommandTree(child: ChildProcess): Promise { child.kill() return Promise.resolve() } + if ( + !admitSelfInitiatedTreeKill({ pid, site: 'git-command-tree-kill', scope: 'win-taskkill-tree' }) + ) { + // Refusal blocks the pid-addressed tree walk, never the termination: the + // handle-addressed root kill cannot reach a recycled pid. + child.kill() + 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..d90ab3616de 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 @@ -53,7 +54,8 @@ function appendBounded(capture: BoundedCapture, chunk: Buffer): void { capture.truncated = true } -function terminateNotebookProcessTree( +/** Exported for the refusal-fallback test; the timeout path is otherwise unreachable. */ +export function terminateNotebookProcessTree( child: ChildProcessWithoutNullStreams ): ReturnType | null { if (!child.pid) { @@ -62,6 +64,18 @@ function terminateNotebookProcessTree( } if (process.platform === 'win32') { + if ( + !admitSelfInitiatedTreeKill({ + pid: child.pid, + site: 'notebook-cell-timeout', + scope: 'win-taskkill-tree' + }) + ) { + // Refusal blocks the tree walk, not the termination: killing the root by + // handle cannot reach a recycled pid, and a timed-out cell must still stop. + child.kill() + 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..40b1421c32f --- /dev/null +++ b/src/main/main-process-tree-kill-gate.test.ts @@ -0,0 +1,184 @@ +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. + * + * Exactly what is enforced, so no comment elsewhere claims more: per file, the + * number of gate admissions must be at least the number of `/pid` call sites. + * Counting sites rather than files is the point — a file-granular scan would let + * a second, ungated taskkill land inside a family that already mentions the gate, + * which is the shape the six highest-risk files now have. What it still cannot + * see: a site that pairs an ungated kill with a second admission of an already + * gated one in the same file, and a kill whose `/pid` argument is itself built + * from a variable. + */ +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__' +]) + +/** + * One match per call site. Keyed on the `/pid` argument rather than the program + * name because `/pid ` is what makes the kill pid-addressed — it walks + * whatever tree owns that pid *now* — and because the literal survives a + * `taskkill` spawned through a constant or a variable, which a quoted-program + * pattern misses entirely. + */ +const PID_ADDRESSED_KILL_SITE = /['"]\/pid['"]/gi + +/** + * A call, not an import or a comment: `admitSelfInitiatedTreeKill` in main, and + * `admitProcessTreeKill` for the `src/shared` seam main installs the same gate + * into, which shared code cannot import directly. + */ +const GATE_ADMISSION = /\badmit(?:SelfInitiatedTreeKill|ProcessTreeKill)\s*\(/g + +function countMatches(source: string, pattern: RegExp): number { + return source.match(pattern)?.length ?? 0 +} + +/** Sites left over once each admission in the file has claimed one. */ +function ungatedKillSiteCount(source: string): number { + return Math.max( + countMatches(source, PID_ADDRESSED_KILL_SITE) - countMatches(source, GATE_ADMISSION), + 0 + ) +} + +/** + * 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_KILL_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) => countMatches(file.source, PID_ADDRESSED_KILL_SITE) > 0) +) + +function pidAddressedKillFiles(): { path: string; source: string }[] { + return PID_ADDRESSED_KILL_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(pidAddressedKillFiles().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 = pidAddressedKillFiles() + .filter((file) => file.path.startsWith(MAIN_DIRECTORY)) + .filter((file) => ungatedKillSiteCount(file.source) > 0) + .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 = pidAddressedKillFiles() + .filter((file) => !file.path.startsWith(MAIN_DIRECTORY)) + .filter((file) => ungatedKillSiteCount(file.source) > 0) + .map((file) => file.path) + .filter((path) => !UNGATED_TASKKILL_ALLOWLIST.has(path)) + + expect(unaccounted).toEqual([]) + }) + + it('counts call sites, not files: a second ungated kill in a gated file is caught', () => { + // The failure a file-granular scan let through: one gate mention exempting + // every taskkill in the file. + const gated = ` + import { admitSelfInitiatedTreeKill } from './own-chromium-tree-kill-guard' + if (admitSelfInitiatedTreeKill({ pid, site: 's', scope: 'win-taskkill-tree' })) { + spawn('taskkill', ['/pid', String(pid), '/t', '/f']) + } + ` + + expect(ungatedKillSiteCount(gated)).toBe(0) + expect( + ungatedKillSiteCount(`${gated}\nspawn('taskkill', ['/pid', String(other), '/t', '/f'])`) + ).toBe(1) + }) + + it('sees a kill whose program name comes from a constant', () => { + // A quoted-program pattern misses this shape; the `/pid` argument does not. + expect( + ungatedKillSiteCount(` + const KILLER = 'taskkill' + spawn(KILLER, ['/pid', String(pid), '/t', '/f']) + `) + ).toBe(1) + }) + + it('keeps the allowlist honest: every entry still spawns a taskkill', () => { + const spawning = new Set(pidAddressedKillFiles().map((file) => file.path)) + + expect([...UNGATED_TASKKILL_ALLOWLIST.keys()].filter((path) => !spawning.has(path))).toEqual([]) + }) +}) diff --git a/src/main/orca-chromium-process-pids.ts b/src/main/orca-chromium-process-pids.ts index b2613e42b79..3282f3a4d66 100644 --- a/src/main/orca-chromium-process-pids.ts +++ b/src/main/orca-chromium-process-pids.ts @@ -12,6 +12,12 @@ import { recordCoalescedDurableCrashBreadcrumb } from './crash-reporting/durable * Empty on a Node host and empty on failure: that is "no refusal proven", never * "safe to kill" — callers must keep every other guard they already have. * + * The other direction is real too, and bounded by design: `getAppMetrics()` can + * still list a renderer Electron has not finished reaping, so on Windows a pid + * already recycled onto an unrelated child of ours reads as `own` and its tree + * walk is refused. That is why a refusal only blocks the pid-addressed walk and + * every gated site still kills its own root through the child handle. + * * Why failure stays open rather than refusing everything: a refusal is not free. * `terminateWindowsProcessTree` resolves without killing, and * `killSourceControlAgentProcess` returns that straight to a caller that then diff --git a/src/main/own-chromium-tree-kill-guard.test.ts b/src/main/own-chromium-tree-kill-guard.test.ts index bd3b1674e18..7b9c30687bc 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 + }) + }) + /** * Fail-open is the deliberate choice — see `orca-chromium-process-pids.ts` for * why refusing everything is worse — so the crumb is the only thing that keeps diff --git a/src/main/own-chromium-tree-kill-guard.ts b/src/main/own-chromium-tree-kill-guard.ts index 4daedf4faa4..24b6a4b7327 100644 --- a/src/main/own-chromium-tree-kill-guard.ts +++ b/src/main/own-chromium-tree-kill-guard.ts @@ -4,16 +4,23 @@ 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. + * Gate every main-process tree-kill through one decision: refuse a pid-addressed + * walk when Electron is currently accounting for the pid, otherwise put the + * kill 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 by counting `/pid` + * call sites against gate admissions per file, not by file. Returns false + * when the caller must not walk that pid's tree; the caller still kills its own + * root through the child handle (`refused-tree-kill-root-termination.test.ts`), + * so a refusal is never a process leak. * * Electron main only, by construction. `terminateWindowsProcessTree` also runs * in the standalone daemon (the `pty-descendant-sweep` site), where @@ -30,11 +37,26 @@ export function admitSelfInitiatedTreeKill(target: { }): boolean { // Why: no PTY root, codex root or git child is ever one of our own Chromium // processes, so a pid that is means the caller is about to kill a renderer, - // the GPU or the browser itself (#10680). - if (readOrcaChromiumProcessPids().has(target.pid)) { - recordRefusedOwnChromiumTreeKill(target) - return false + // the GPU or the browser itself (#10680). Only the pid-addressed scope can + // land there: a POSIX group holds only what Orca put in it, so that arm is + // recorded and admitted like every other group kill in main, and a stale + // `getAppMetrics()` entry cannot orphan a macOS/Linux tree. + const isOwnChromiumPid = + target.scope === 'win-taskkill-tree' && readOrcaChromiumProcessPids().has(target.pid) + try { + if (isOwnChromiumPid) { + recordRefusedOwnChromiumTreeKill(target) + } else { + recordSelfInitiatedTreeKill(target) + } + } catch { + // Recording must never turn a successful termination into a failed one, and + // never flip the decision: it is taken above, before anything can throw. } - recordSelfInitiatedTreeKill(target) - return true + return !isOwnChromiumPid +} + +/** 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/refused-tree-kill-root-termination.test.ts b/src/main/refused-tree-kill-root-termination.test.ts new file mode 100644 index 00000000000..ada4b5942a9 --- /dev/null +++ b/src/main/refused-tree-kill-root-termination.test.ts @@ -0,0 +1,220 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { spawnMock, execFileMock, queryWindowsProcessDescendantsMock } = vi.hoisted(() => ({ + spawnMock: vi.fn(), + execFileMock: vi.fn(), + queryWindowsProcessDescendantsMock: vi.fn() +})) + +vi.mock('node:child_process', async (importOriginal) => ({ + ...(await importOriginal>()), + spawn: spawnMock, + execFile: execFileMock +})) +vi.mock('electron', () => ({ ipcMain: { handle: vi.fn(), on: vi.fn() } })) +vi.mock('./providers/windows-foreground-process-rows', () => ({ + queryWindowsProcessDescendants: queryWindowsProcessDescendantsMock +})) + +import { + getAppEnvironment, + hasAppEnvironment, + setAppEnvironment, + type AppEnvironment +} from '../shared/app-environment' +import { installMainProcessTreeKillGate } from './own-chromium-tree-kill-guard' +import { setProcessTreeKillGate } from '../shared/child-process/process-tree-kill-gate' +import { resetSelfInitiatedTreeKillLogForTest } from './crash-reporting/self-initiated-tree-kill-log' +import { + clearCrashBreadcrumbsForTest, + getCrashBreadcrumbSnapshot +} from './crash-reporting/crash-breadcrumb-store' +import { _resetTracerForTests, setActiveSink } from './observability/tracer' +import { terminateNotebookProcessTree } from './ipc/notebook' +import { killLocalPrecheckProcessTree } from './automations/precheck-runner' +import { killRecipeProcess } from '../shared/ephemeral-vm-recipe-process' +import { killSpawnedCommandTree } from './git/command-runner/spawned-command-tree-kill' +import { killCodexAppServerProcessTree } from './codex/codex-app-server-session' +import { signalProcessTree } from '../shared/child-process/process-tree-termination' +import { killSourceControlAgentProcess } from './text-generation/source-control-local-process' +import { terminateCodexTurnProcesses } from './codex/codex-structured-turn-processes' + +/** A pid Electron reports as one of ours: every gate below must refuse it. */ +const RENDERER_PID = 1001 + +function appEnvironment(): AppEnvironment { + return { + getPath: () => process.cwd(), + getAppPath: () => process.cwd(), + getVersion: () => '0.0.0-test', + isPackaged: () => false, + onWillQuit: () => {}, + exit: () => {}, + getAppMetrics: (() => [ + { pid: RENDERER_PID, type: 'Tab' } + ]) as unknown as AppEnvironment['getAppMetrics'] + } +} + +let previousEnvironment: AppEnvironment | null = null +let previousPlatform: PropertyDescriptor | undefined + +function setPlatform(platform: NodeJS.Platform): void { + Object.defineProperty(process, 'platform', { value: platform, configurable: true }) +} + +beforeEach(() => { + previousEnvironment = hasAppEnvironment() ? getAppEnvironment() : null + previousPlatform = Object.getOwnPropertyDescriptor(process, 'platform') + setAppEnvironment(appEnvironment()) + setActiveSink(null) + clearCrashBreadcrumbsForTest() + resetSelfInitiatedTreeKillLogForTest() + installMainProcessTreeKillGate() + spawnMock.mockReset() + execFileMock.mockReset() + queryWindowsProcessDescendantsMock.mockReset() + spawnMock.mockReturnValue({ on: vi.fn(), once: vi.fn(), unref: vi.fn(), kill: vi.fn() }) +}) + +afterEach(() => { + setProcessTreeKillGate(null) + if (previousPlatform) { + Object.defineProperty(process, 'platform', previousPlatform) + } + if (previousEnvironment) { + setAppEnvironment(previousEnvironment) + } + _resetTracerForTests() +}) + +/** + * A refusal must never become a process leak. The gate only blocks the + * pid-addressed tree walk; the root kill is addressed by the child handle, so it + * cannot reach the recycled pid we refused, and skipping it would report a + * timed-out command as stopped while its tree keeps running. + */ +describe('a refused tree-kill still terminates the root it owns', () => { + it('kills the git command root when the tree walk is refused', async () => { + setPlatform('win32') + const child = { pid: RENDERER_PID, kill: vi.fn() } + + await killSpawnedCommandTree(child as never) + + expect(spawnMock).not.toHaveBeenCalled() + expect(child.kill).toHaveBeenCalledTimes(1) + }) + + it('kills the notebook cell root when the tree walk is refused', () => { + setPlatform('win32') + const child = { pid: RENDERER_PID, kill: vi.fn() } + + expect(terminateNotebookProcessTree(child as never)).toBeNull() + + expect(spawnMock).not.toHaveBeenCalled() + expect(child.kill).toHaveBeenCalledTimes(1) + }) + + it('kills the automation precheck root when the tree walk is refused', () => { + setPlatform('win32') + const child = { pid: RENDERER_PID, kill: vi.fn() } + + expect(killLocalPrecheckProcessTree(child as never)).toBeNull() + + expect(spawnMock).not.toHaveBeenCalled() + expect(child.kill).toHaveBeenCalledTimes(1) + }) + + it('kills the ephemeral-VM recipe root when the tree walk is refused', () => { + setPlatform('win32') + const child = { pid: RENDERER_PID, kill: vi.fn() } + + killRecipeProcess(child as never, true) + + expect(spawnMock).not.toHaveBeenCalled() + expect(child.kill).toHaveBeenCalledWith('SIGKILL') + }) + + it('kills the codex app-server root when the deadline tree walk is refused', () => { + const child = { pid: RENDERER_PID, kill: vi.fn() } + + killCodexAppServerProcessTree(child as never, { + platform: 'win32', + spawnImpl: spawnMock as never + }) + + expect(spawnMock).not.toHaveBeenCalled() + expect(child.kill).toHaveBeenCalledWith('SIGKILL') + }) + + it('kills the commit-message agent root when the tree walk is refused', async () => { + setPlatform('win32') + const child = { pid: RENDERER_PID, kill: vi.fn() } + + await killSourceControlAgentProcess(child as never) + + expect(execFileMock).not.toHaveBeenCalled() + expect(child.kill).toHaveBeenCalledWith('SIGKILL') + }) + + it('kills the runProcess root when the Windows arm of the shared choke point is refused', async () => { + setPlatform('win32') + const windowsChild = { pid: RENDERER_PID, kill: vi.fn(), exitCode: null, signalCode: null } + + await expect(signalProcessTree(windowsChild as never, 'SIGKILL')).resolves.toBe(false) + expect(spawnMock).not.toHaveBeenCalled() + expect(windowsChild.kill).toHaveBeenCalledWith('SIGKILL') + }) + + it('still signals the POSIX process group: a group only holds what Orca put in it', async () => { + // Same contract as main and as the other three POSIX group arms in main + // (claude-login, codex teardown, PTY sweep): record, never refuse. A stale + // `getAppMetrics()` entry must not orphan a macOS/Linux tree. + setPlatform('linux') + const posixChild = { pid: RENDERER_PID, kill: vi.fn(), exitCode: null, signalCode: null } + const processKill = vi.spyOn(process, 'kill').mockImplementation(() => true) + + await expect(signalProcessTree(posixChild as never, 'SIGKILL')).resolves.toBe(true) + expect(processKill).toHaveBeenCalledWith(-RENDERER_PID, 'SIGKILL') + expect(posixChild.kill).not.toHaveBeenCalled() + expect(getCrashBreadcrumbSnapshot()).toEqual([ + expect.objectContaining({ + name: 'self_tree_kill', + data: expect.objectContaining({ pid: RENDERER_PID, scope: 'posix-process-group' }) + }) + ]) + processKill.mockRestore() + }) +}) + +/** + * The one gated site with nothing to fall back to: the roots it kills are found + * by a process-table walk, not spawned here, so there is no child handle. A + * refusal must then be visible — the refusal crumb is written and the turn is + * reported as not cancelled — rather than resolving as if the tree had gone. + */ +describe('a refused tree-kill with no handle to fall back to', () => { + it('reports the codex turn as not cancelled and records the refused added root', async () => { + const appServerPid = 500 + const addedRoot = { + pid: RENDERER_PID, + ppid: appServerPid, + name: 'node.exe', + command: 'node', + depth: 1 + } + queryWindowsProcessDescendantsMock.mockResolvedValue([addedRoot]) + + await expect( + terminateCodexTurnProcesses(appServerPid, { platform: 'win32', identities: new Map() }) + ).resolves.toBe(false) + + expect(execFileMock).not.toHaveBeenCalled() + expect(getCrashBreadcrumbSnapshot()).toEqual([ + expect.objectContaining({ + name: 'self_tree_kill_refused_own_chromium', + data: expect.objectContaining({ pid: RENDERER_PID, site: 'codex-turn-added-roots' }) + }) + ]) + }) +}) diff --git a/src/main/runtime/orca-runtime-resolve-recovered-structured-tui-transcript.ts b/src/main/runtime/orca-runtime-resolve-recovered-structured-tui-transcript.ts index cfb8b421966..aef04bde6bc 100644 --- a/src/main/runtime/orca-runtime-resolve-recovered-structured-tui-transcript.ts +++ b/src/main/runtime/orca-runtime-resolve-recovered-structured-tui-transcript.ts @@ -3,6 +3,8 @@ import { OrcaRuntimeWithStopStructuredSessionProcess } from './orca-runtime-stop import type { AgentSessionOwnerBinding } from '../../shared/agent-session-host-authority' import { agentSessionOwnerBindingsEqual } from '../../shared/claimed-agent-pty-owner-snapshot' import { resolvePinnedCodexRolloutProof } from '../codex/codex-tui-rollout-proof' +import { supportsCodexStructuredLocation } from '../codex/codex-structured-location-support' +import { supportsClaudeStructuredLocation } from '../claude/claude-structured-location-support' import { getStructuredAgentSessionHost } from '../native-chat/agent-session-wire/structured-agent-session-registry' import { resolveStructuredAgentSessionCreateSupport } from '../native-chat/structured-agent-session-create-support' import { LOCAL_EXECUTION_HOST_ID } from '../../shared/execution-host' @@ -51,13 +53,13 @@ export class OrcaRuntimeWithResolveRecoveredStructuredTuiTranscript extends Orca agent: 'claude' | 'codex' ): Promise<{ supported: boolean; reason?: 'agent' | 'remote' | 'wsl' }> { const location = await this.resolveStructuredAgentSessionLocation(worktreeSelector) - await this.ensureStructuredAgentSessionHost() - // The verdict lives in a typechecked module; this file is @ts-nocheck. return resolveStructuredAgentSessionCreateSupport({ agent, location, adapterSupportsCreate: - getStructuredAgentSessionHost()?.supportsCreate(location, agent) === true, + agent === 'claude' + ? supportsClaudeStructuredLocation(location) + : supportsCodexStructuredLocation(location), getSettings: () => this.requireStore().getSettings() }) } diff --git a/src/main/runtime/structured-agent-session-support-probe.test.ts b/src/main/runtime/structured-agent-session-support-probe.test.ts new file mode 100644 index 00000000000..e393e41f3a4 --- /dev/null +++ b/src/main/runtime/structured-agent-session-support-probe.test.ts @@ -0,0 +1,174 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { OrcaRuntimeService } from './orca-runtime' +import { + getStructuredAgentSessionHost, + setStructuredAgentSessionHost +} from '../native-chat/agent-session-wire/structured-agent-session-registry' +import { agentSessionPtyWriteGate } from './agent-session-pty-write-gate' + +type InstallEffects = { + storeOpened: boolean + writeGateAttached: boolean + reaperStarted: boolean +} + +/** Stands in for `install()` by performing the three effects it performs, so a probe that + * reinstalls the host is caught by what the install *does*, not by a call count alone. */ +function stubStructuredHostInstall(runtime: OrcaRuntimeService): { + effects: InstallEffects + ensure: ReturnType +} { + const effects: InstallEffects = { + storeOpened: false, + writeGateAttached: false, + reaperStarted: false + } + // `supportsCreate` answers as the real Codex adapter would, so a probe that reinstalls the host + // still returns the right answer and fails on the install effects alone. + const host = { + reconcileRestartLeases: vi.fn(async () => {}), + supportsCreate: (location: { executionHostId: string; wslDistro: string | null }) => + location.executionHostId === 'local' && location.wslDistro === null + } + const ensure = vi.fn(async () => { + effects.storeOpened = true + effects.reaperStarted = true + agentSessionPtyWriteGate.attachRecordLookup(() => null) + effects.writeGateAttached = true + setStructuredAgentSessionHost(host as never) + }) + vi.spyOn(runtime, 'ensureStructuredAgentSessionHost').mockImplementation(ensure) + return { effects, ensure } +} + +type TestLocation = { + executionHostId: string + wslDistro: string | null + workspaceKind?: 'folder' | 'git-worktree' +} + +type SupportResult = { + supported: boolean + reason?: 'agent' | 'remote' | 'wsl' +} + +function createRuntime(location: TestLocation): OrcaRuntimeService { + const runtime = new OrcaRuntimeService({ getSettings: () => ({}) } as never) + const internal = runtime as unknown as { + resolveStructuredAgentSessionLocation: () => Promise + } + internal.resolveStructuredAgentSessionLocation = vi.fn(async () => ({ + executionHostId: location.executionHostId, + wslDistro: location.wslDistro, + workspaceId: 'workspace-1', + workspaceKind: location.workspaceKind ?? 'git-worktree' + })) + return runtime +} + +async function expectSupportWithoutInstall(input: { + agent: 'claude' | 'codex' + location: TestLocation + expected: SupportResult + repetitions?: number +}): Promise { + const runtime = createRuntime(input.location) + const { effects, ensure } = stubStructuredHostInstall(runtime) + + const answers: SupportResult[] = [] + for (let index = 0; index < (input.repetitions ?? 1); index += 1) { + answers.push( + await runtime.getStructuredAgentSessionCreateSupport('id:workspace-1', input.agent) + ) + } + + expect(answers).toEqual(Array(input.repetitions ?? 1).fill(input.expected)) + expect(ensure).not.toHaveBeenCalled() + expect(effects).toEqual({ + storeOpened: false, + writeGateAttached: false, + reaperStarted: false + }) + expect(getStructuredAgentSessionHost()).toBeNull() +} + +describe('structured agent-session create-support probe', () => { + afterEach(() => { + setStructuredAgentSessionHost(null) + agentSessionPtyWriteGate.detachRecordLookup() + vi.restoreAllMocks() + }) + + it.each(['codex', 'claude'] as const)( + 'answers %s support repeatedly without installing the host', + async (agent) => { + await expectSupportWithoutInstall({ + agent, + location: { executionHostId: 'local', wslDistro: null }, + expected: { supported: true }, + repetitions: 3 + }) + } + ) + + it.each(['codex', 'claude'] as const)( + 'still reports an unsupported remote %s location without installing the host', + async (agent) => { + await expectSupportWithoutInstall({ + agent, + location: { executionHostId: 'ssh-host-1', wslDistro: null }, + expected: { supported: false, reason: 'remote' } + }) + } + ) + + it.each(['codex', 'claude'] as const)( + 'still reports an unsupported WSL %s location without installing the host', + async (agent) => { + await expectSupportWithoutInstall({ + agent, + location: { executionHostId: 'local', wslDistro: 'Ubuntu' }, + expected: { supported: false, reason: 'wsl' } + }) + } + ) + + it.each(['codex', 'claude'] as const)( + 'supports a local folder workspace for %s without installing the host', + async (agent) => { + await expectSupportWithoutInstall({ + agent, + location: { + executionHostId: 'local', + wslDistro: null, + workspaceKind: 'folder' + }, + expected: { supported: true } + }) + } + ) + + it('still installs and reconciles on startup when a store is already persisted', async () => { + const runtime = createRuntime({ executionHostId: 'local', wslDistro: null }) + const { effects, ensure } = stubStructuredHostInstall(runtime) + const internal = runtime as unknown as { + hasPersistedStructuredAgentSessionStore: () => boolean + refreshMobileSessionPtyRecords: () => Promise + } + internal.hasPersistedStructuredAgentSessionStore = () => true + internal.refreshMobileSessionPtyRecords = vi.fn(async () => {}) + + await runtime.prepareStructuredAgentSessionStartupRestoration() + + expect(ensure).toHaveBeenCalledTimes(1) + expect(effects).toEqual({ + storeOpened: true, + writeGateAttached: true, + reaperStarted: true + }) + expect( + (getStructuredAgentSessionHost() as unknown as { reconcileRestartLeases: () => void }) + .reconcileRestartLeases + ).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/main/shell-wrapper-generated-file-snapshot.test.ts b/src/main/shell-wrapper-generated-file-snapshot.test.ts index ddbf1537984..36cd837e4fd 100644 --- a/src/main/shell-wrapper-generated-file-snapshot.test.ts +++ b/src/main/shell-wrapper-generated-file-snapshot.test.ts @@ -73,6 +73,7 @@ const CONTRACT_GLOBALS = new Set([ 'OPENCODE_CONFIG_DIR', 'PATH', 'PROMPT_COMMAND', + 'PS1', // Bash appends its non-printing Readline readiness marker. 'CURSOR', 'ZDOTDIR', 'precmd_functions', 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/text-generation/commit-message-text-generation-cancellation.test.ts b/src/main/text-generation/commit-message-text-generation-cancellation.test.ts index 9cfde774384..820af710fb9 100644 --- a/src/main/text-generation/commit-message-text-generation-cancellation.test.ts +++ b/src/main/text-generation/commit-message-text-generation-cancellation.test.ts @@ -101,7 +101,7 @@ describe('generateCommitMessageFromContext', () => { cancelGenerateCommitMessageLocal('/repo') - expectChildTerminated(children[0]!) + await expectChildTerminated(children[0]!) expect(children[1]?.kill).not.toHaveBeenCalled() children[0]?.listeners.get('close')?.(null) @@ -192,7 +192,7 @@ describe('generateCommitMessageFromContext', () => { cancelGeneratePullRequestFieldsLocal('/repo') expect(children[0]?.kill).not.toHaveBeenCalled() - expectChildTerminated(children[1]!) + await expectChildTerminated(children[1]!) const commitStdout = children[0]?.listeners.get('stdout:data') commitStdout?.(Buffer.from('Update README\n')) @@ -250,7 +250,7 @@ describe('generateCommitMessageFromContext', () => { cancelGeneratePullRequestFieldsLocal('/repo') listeners.get('close')?.(null) - expectChildTerminated(child) + await expectChildTerminated(child) await expect(pullRequest).resolves.toEqual({ success: false, error: 'Generation canceled.', @@ -306,7 +306,7 @@ describe('generateCommitMessageFromContext', () => { ) cancelGenerateCommitMessageLocal('/repo') - expectChildTerminated(child) + await expectChildTerminated(child) await Promise.resolve() await Promise.resolve() await Promise.resolve() @@ -344,7 +344,7 @@ describe('generateCommitMessageFromContext', () => { error: 'Generation canceled.', canceled: true }) - expectChildTerminated(firstChild) + await expectChildTerminated(firstChild) const second = generateCommitMessageFromContext(context, params, { kind: 'local', @@ -377,7 +377,7 @@ describe('generateCommitMessageFromContext', () => { await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(1)) cancelGenerateCommitMessageLocal('/descendant-repo') await expect(first).resolves.toMatchObject({ canceled: true }) - expectChildTerminated(firstChild) + await expectChildTerminated(firstChild) // SIGKILL reaches the codex process but not a grandchild that inherited its // stdout, so 'exit' arrives and 'close' never does. diff --git a/src/main/text-generation/commit-message-text-generation-local-subprocess.test.ts b/src/main/text-generation/commit-message-text-generation-local-subprocess.test.ts index c124cb85779..f73152255da 100644 --- a/src/main/text-generation/commit-message-text-generation-local-subprocess.test.ts +++ b/src/main/text-generation/commit-message-text-generation-local-subprocess.test.ts @@ -71,7 +71,7 @@ describe('generateCommitMessageFromContext', () => { error: 'agent CLI command produced too much output. Check the agent CLI configuration and try again.' }) - expectChildTerminated(child) + await expectChildTerminated(child) }) it('passes prepared provider environment to local agent subprocesses', async () => { diff --git a/src/main/text-generation/commit-message-text-generation-model-discovery.test.ts b/src/main/text-generation/commit-message-text-generation-model-discovery.test.ts index 4bc1a720f05..9a1f932f0a0 100644 --- a/src/main/text-generation/commit-message-text-generation-model-discovery.test.ts +++ b/src/main/text-generation/commit-message-text-generation-model-discovery.test.ts @@ -325,7 +325,7 @@ describe('discoverCommitMessageModelsLocal', () => { await vi.advanceTimersByTimeAsync(60_000) await assertion - expectChildTerminated(child) + await expectChildTerminated(child) expect(child.stdout.listenerCount('data')).toBe(0) expect(child.stderr.listenerCount('data')).toBe(0) expect(child.listenerCount('error')).toBe(0) @@ -352,7 +352,7 @@ describe('discoverCommitMessageModelsLocal', () => { success: false, error: 'Codex model discovery timed out after 60s.' }) - expectChildTerminated(firstChild) + await expectChildTerminated(firstChild) expect(spawnMock).toHaveBeenCalledTimes(1) firstChild.emit('close', null) @@ -413,7 +413,7 @@ describe('discoverCommitMessageModelsLocal', () => { success: false, error: 'Cursor returned too much model data.' }) - expectChildTerminated(child) + await expectChildTerminated(child) expect(child.stdout.listenerCount('data')).toBe(0) expect(child.stderr.listenerCount('data')).toBe(0) expect(child.listenerCount('error')).toBe(0) diff --git a/src/main/text-generation/commit-message-text-generation-test-harness.ts b/src/main/text-generation/commit-message-text-generation-test-harness.ts index 21103d71f40..8ba925ef087 100644 --- a/src/main/text-generation/commit-message-text-generation-test-harness.ts +++ b/src/main/text-generation/commit-message-text-generation-test-harness.ts @@ -33,15 +33,17 @@ export function withPlatform(platform: NodeJS.Platform, fn: () => T): T { // expectChildTerminated(child) with no extra argument. export function createChildTerminationExpectation( terminateWindowsProcessTreeMock: ReturnType -): (child: { pid: number; kill: ReturnType }) => void { - return (child) => { +): (child: { pid: number; kill: ReturnType }) => Promise { + return async (child) => { if (process.platform === 'win32') { expect(terminateWindowsProcessTreeMock).toHaveBeenCalledWith(child.pid, { site: 'source-control-text-generation' }) - expect(child.kill).not.toHaveBeenCalled() - return } - expect(child.kill).toHaveBeenCalledWith('SIGKILL') + // Every platform kills the root by its own handle. On win32 that is not a + // duplicate of the tree walk: it is what keeps a refused walk from resolving + // having killed nothing while the caller releases the managed-home lock. It + // runs after the walk there, so it can be a tick behind the caller. + await vi.waitFor(() => expect(child.kill).toHaveBeenCalledWith('SIGKILL')) } } diff --git a/src/main/text-generation/source-control-local-process.ts b/src/main/text-generation/source-control-local-process.ts index 170dead6b52..3f046494f0f 100644 --- a/src/main/text-generation/source-control-local-process.ts +++ b/src/main/text-generation/source-control-local-process.ts @@ -22,22 +22,25 @@ import type { TextGenerationOperation } from './source-control-text-generation-types' -export function killSourceControlAgentProcess( +export async function killSourceControlAgentProcess( child: SpawnedSourceControlAgentProcess ): Promise { const pid = child.pid if (!pid) { - return Promise.resolve() + return } if (process.platform === 'win32') { - return terminateWindowsProcessTree(pid, { site: 'source-control-text-generation' }) + // taskkill owns the tree, but the own-Chromium gate can refuse the + // pid-addressed walk; the handle-addressed root kill below cannot reach the + // recycled pid it refused, and callers release the managed-home lock on this + // promise, so it must not resolve having killed nothing. + await terminateWindowsProcessTree(pid, { site: 'source-control-text-generation' }) } try { child.kill('SIGKILL') } catch { // The process may exit between the PID check and kill. } - return Promise.resolve() } export function runLocalSourceControlPlan(input: { diff --git a/src/main/windows-live-tree-kill.win32.test.ts b/src/main/windows-live-tree-kill.win32.test.ts new file mode 100644 index 00000000000..45563e82c89 --- /dev/null +++ b/src/main/windows-live-tree-kill.win32.test.ts @@ -0,0 +1,197 @@ +import { existsSync, mkdtempSync, readFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { spawn, ChildProcess } from 'node:child_process' +import { subscribe, unsubscribe } from 'node:diagnostics_channel' +import { afterAll, afterEach, beforeEach, describe, expect, it } from 'vitest' +import { setAppEnvironment, type AppEnvironment } from '../shared/app-environment' +import { setProcessTreeKillGate } from '../shared/child-process/process-tree-kill-gate' +import { signalProcessTree } from '../shared/child-process/process-tree-termination' +import { removeTreeSync } from '../shared/windows-transient-lock-removal' +import { + findSelfInitiatedTreeKills, + resetSelfInitiatedTreeKillLogForTest +} from './crash-reporting/self-initiated-tree-kill-log' +import { installMainProcessTreeKillGate } from './own-chromium-tree-kill-guard' +import { terminateWindowsProcessTree } from './windows-process-tree-kill' + +/** + * The unit tests pin the gate's decision against a mocked `taskkill`; this pins + * what that decision does to real Windows processes. + * + * Both are needed. Every claim the gate makes is about a mechanism the mocks + * cannot show: that `taskkill /T /F` actually reaps a detached grandchild, that + * a refusal actually leaves that tree standing, and that the handle-addressed + * root kill the refusal path falls back to actually reaps the root while + * orphaning its descendants — the asymmetry the PR discloses rather than fixes. + * + * Runs only on win32; skipped elsewhere. + */ +const describeOnWindows = process.platform === 'win32' ? describe : describe.skip + +/** Read live by the guard on every kill, so a case can flip it mid-test. */ +let orcaChromiumPids: number[] = [] + +function appEnvironment(): AppEnvironment { + return { + getPath: () => process.cwd(), + getAppPath: () => process.cwd(), + getVersion: () => '0.0.0-live', + isPackaged: () => false, + onWillQuit: () => {}, + exit: () => {}, + getAppMetrics: (() => + orcaChromiumPids.map((pid) => ({ + pid, + type: 'Tab' + }))) as unknown as AppEnvironment['getAppMetrics'] + } +} + +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0) + return true + } catch { + return false + } +} + +const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)) + +async function waitFor(predicate: () => boolean, timeoutMs = 10_000): Promise { + const deadline = Date.now() + timeoutMs + while (Date.now() < deadline && !predicate()) { + await sleep(100) + } + return predicate() +} + +let markerDirectory = '' +let markerSequence = 0 +const spawnedRoots: ChildProcess[] = [] +const spawnedLeaves: number[] = [] +const observedSpawns: ChildProcess[] = [] + +function observeSpawn(message: unknown): void { + if ( + typeof message === 'object' && + message !== null && + 'process' in message && + message.process instanceof ChildProcess + ) { + observedSpawns.push(message.process) + } +} + +/** A real root with a real grandchild; the grandchild reports its pid on disk. */ +async function spawnLiveTree(): Promise<{ + child: ChildProcess + rootPid: number + leafPid: number +}> { + const marker = join(markerDirectory, `leaf-${markerSequence++}.pid`) + const leafSource = `require('node:fs').writeFileSync(${JSON.stringify(marker)}, String(process.pid)); setTimeout(() => {}, 600000)` + // Non-detached Windows children can die with the root's libuv Job Object. + const rootSource = `require('node:child_process').spawn(process.execPath, ['-e', ${JSON.stringify(leafSource)}], { stdio: 'ignore', detached: true, windowsHide: true }); setTimeout(() => {}, 600000)` + const child = spawn(process.execPath, ['-e', rootSource], { + stdio: 'ignore', + windowsHide: true + }) + spawnedRoots.push(child) + const rootPid = child.pid as number + expect(rootPid).toBeGreaterThan(0) + expect(await waitFor(() => existsSync(marker))).toBe(true) + const leafPid = Number(readFileSync(marker, 'utf8')) + spawnedLeaves.push(leafPid) + expect(await waitFor(() => isAlive(leafPid))).toBe(true) + return { child, rootPid, leafPid } +} + +describeOnWindows('own-Chromium gate against real Windows process trees', () => { + beforeEach(() => { + markerDirectory ||= mkdtempSync(join(tmpdir(), 'orca-live-tree-kill-')) + resetSelfInitiatedTreeKillLogForTest() + orcaChromiumPids = [] + setAppEnvironment(appEnvironment()) + installMainProcessTreeKillGate() + observedSpawns.length = 0 + subscribe('child_process', observeSpawn) + }) + + afterEach(async () => { + unsubscribe('child_process', observeSpawn) + orcaChromiumPids = [] + for (const leafPid of spawnedLeaves.splice(0)) { + await terminateWindowsProcessTree(leafPid, { site: 'live-tree-kill-cleanup' }) + } + for (const root of spawnedRoots.splice(0)) { + root.kill('SIGKILL') + } + setProcessTreeKillGate(null) + }) + + afterAll(() => { + if (markerDirectory) { + removeTreeSync(markerDirectory) + } + }) + + it('admitted: taskkill reaps the root and its detached grandchild, and the kill is recorded', async () => { + const { rootPid, leafPid } = await spawnLiveTree() + + await terminateWindowsProcessTree(rootPid, { site: 'live-tree-kill-admit' }) + + expect(await waitFor(() => !isAlive(rootPid))).toBe(true) + expect(await waitFor(() => !isAlive(leafPid))).toBe(true) + expect( + findSelfInitiatedTreeKills(Date.now()).some( + (kill) => kill.pid === rootPid && kill.site === 'live-tree-kill-admit' + ) + ).toBe(true) + }) + + it('refused: the tree survives, nothing is recorded, and the handle kill still reaps the root', async () => { + const { child, rootPid, leafPid } = await spawnLiveTree() + orcaChromiumPids = [rootPid] + + await terminateWindowsProcessTree(rootPid, { site: 'live-tree-kill-refuse' }) + + await sleep(1_000) + expect(isAlive(rootPid)).toBe(true) + expect(isAlive(leafPid)).toBe(true) + expect(findSelfInitiatedTreeKills(Date.now())).toEqual([]) + + // The fallback every gated site runs after a refusal. + child.kill('SIGKILL') + expect(await waitFor(() => !isAlive(rootPid))).toBe(true) + // Let root-owned job cleanup finish before asserting independent survival. + await sleep(250) + // Disclosed asymmetry: a refusal orphans descendants rather than reaping them. + expect(isAlive(leafPid)).toBe(true) + }) + + it('signalProcessTree refused: the root goes by handle and the barrier reports unverified', async () => { + const { child, rootPid, leafPid } = await spawnLiveTree() + orcaChromiumPids = [rootPid] + observedSpawns.length = 0 + + await expect(signalProcessTree(child, 'SIGKILL')).resolves.toBe(false) + + expect(observedSpawns).toHaveLength(0) + expect(await waitFor(() => !isAlive(rootPid))).toBe(true) + await sleep(250) + expect(isAlive(leafPid)).toBe(true) + }) + + it('signalProcessTree admitted: the whole tree goes and the barrier reports verified', async () => { + const { child, rootPid, leafPid } = await spawnLiveTree() + observedSpawns.length = 0 + + await expect(signalProcessTree(child, 'SIGKILL')).resolves.toBe(true) + + expect(observedSpawns.map((child) => child.spawnfile)).toEqual(['taskkill']) + expect(await waitFor(() => !isAlive(rootPid))).toBe(true) + expect(await waitFor(() => !isAlive(leafPid))).toBe(true) + }) +}) 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/renderer/src/components/mobile/mobile-platform-copy.ts b/src/renderer/src/components/mobile/mobile-platform-copy.ts index fd70176609d..cd6669891a3 100644 --- a/src/renderer/src/components/mobile/mobile-platform-copy.ts +++ b/src/renderer/src/components/mobile/mobile-platform-copy.ts @@ -22,7 +22,7 @@ const IOS_CHANNEL_COPY: Record = { const ANDROID_COPY: InstallCopy = { ctaLabel: 'Download APK', - url: 'https://github.com/stablyai/orca/releases/download/mobile-android-v0.0.46/app-release.apk' + url: 'https://github.com/stablyai/orca/releases/download/mobile-android-v0.0.47/app-release.apk' } export function getInstallCopy(platform: Platform, iosChannel: IosChannel): InstallCopy { diff --git a/src/renderer/src/components/settings/MobileSettingsPane.tsx b/src/renderer/src/components/settings/MobileSettingsPane.tsx index 2e98a5ccb43..ff8de2b16e5 100644 --- a/src/renderer/src/components/settings/MobileSettingsPane.tsx +++ b/src/renderer/src/components/settings/MobileSettingsPane.tsx @@ -13,7 +13,7 @@ export { getMobileSettingsPaneSearchEntries } const ORCA_IOS_APP_STORE_URL = 'https://apps.apple.com/app/orca-ide/id6766130217' const ORCA_ANDROID_APK_URL = - 'https://github.com/stablyai/orca/releases/download/mobile-android-v0.0.46/app-release.apk' + 'https://github.com/stablyai/orca/releases/download/mobile-android-v0.0.47/app-release.apk' export function MobileSettingsPane(): React.JSX.Element { const showMobileButton = useAppStore((s) => s.settings?.showMobileButton !== false) 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..420fbfeda9b --- /dev/null +++ b/src/shared/child-process/process-tree-kill-gate.ts @@ -0,0 +1,43 @@ +/** + * 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 walk that pid's tree — main is accounting for + * it. Killing the root through its own child handle stays correct and required: + * a handle cannot land on the recycled pid the refusal is about. */ +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..ffcb93b2245 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,17 @@ 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 walking the tree, so a refused pid never gets a + * pid-addressed kill; that also means the recorded crumb says "about to kill", + * not "killed". The root is still killed through its handle, which cannot reach + * the recycled pid the refusal was about, so a refusal is never a leak. Main's + * gate only ever refuses the `win-taskkill-tree` scope — a POSIX group holds + * only what Orca put in it — so the POSIX refusal arm is the seam's contract, + * not something any installed gate exercises today. */ export function signalProcessTree(child: ChildProcess, signal?: NodeJS.Signals): Promise { if (!child.pid) { @@ -39,13 +48,20 @@ 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' }) + ) { + // Same shape as the reaped-pid skip above: refuse the group, still kill the + // root by handle, and report unverified. + killRoot(child, signal) + return Promise.resolve(false) + } + try { + process.kill(-child.pid, signal) return Promise.resolve(true) } catch { return Promise.resolve(!processGroupExists(child.pid)) @@ -73,6 +89,13 @@ 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' }) + ) { + killRoot(child, signal) + return Promise.resolve(false) + } return new Promise((resolve) => { let killer: ChildProcess try { @@ -86,7 +109,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..30d44ac1dbc 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 @@ -132,13 +133,26 @@ export async function runRecipeCommand(args: { }) } -function killRecipeProcess(child: ChildProcessWithoutNullStreams, force = false): void { +/** Exported for the refusal-fallback test; the abort path is otherwise unreachable. */ +export function killRecipeProcess(child: ChildProcessWithoutNullStreams, force = false): void { const signal = force ? 'SIGKILL' : 'SIGTERM' if (process.platform === 'win32') { // Recipes run through `cmd.exe /c` (shell: true), so child.kill() would only // 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' + }) + ) { + // Refusal blocks the tree walk, not the termination: the root kill is + // handle-addressed, so it cannot reach the recycled pid we refused. + child.kill(signal) + return + } const killer = spawn('taskkill', ['/pid', String(child.pid), '/t', '/f'], { windowsHide: true, stdio: 'ignore'