From 0b9d8586b33ac3c5b1afd8937fcc81e673453fa2 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:40:28 -0700 Subject: [PATCH 1/4] fix: show agent commands once without startup polling (#18729) * fix: wait for terminal line editor before agent startup input * fix: submit agent startup at the prompt without polling * test: update generated Bash prompt readiness contract --- .../daemon-bash-rcfile.txt | 9 +- ...nt-startup-prompt-latency.node-pty.test.ts | 123 ++++++++++++++++++ .../daemon/daemon-bash-shell-ready-rcfile.ts | 10 +- src/main/daemon/daemon-pty-adapter.test.ts | 35 +++-- src/main/daemon/daemon-pty-session-spawn.ts | 28 ++-- src/main/daemon/post-ready-flush-gate.test.ts | 8 ++ src/main/daemon/post-ready-flush-gate.ts | 31 ++--- .../pty-subprocess-managed-agent-env.test.ts | 5 +- .../pty-subprocess/shell-launch-plan.ts | 14 +- .../daemon/session-shell-ready-barrier.ts | 7 +- src/main/daemon/shell-ready.ts | 5 + ...ll-wrapper-generated-file-snapshot.test.ts | 1 + 12 files changed, 207 insertions(+), 69 deletions(-) create mode 100644 src/main/daemon/agent-startup-prompt-latency.node-pty.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/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/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', From 0a821e5bc8e66914df3de736f0eac06ae73a5401 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:42:42 -0700 Subject: [PATCH 2/4] fix(crash-reporting): make the own-Chromium gate a real choke point, and stop a refusal leaking the root (#18459) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(crash-reporting): make the own-Chromium gate a real choke point Round-3 review found the guard was not the choke point its own comments claimed: six pid-addressed `taskkill /pid /t /f` families in main were ungated and uninstrumented, so the stale-pid shape stayed producible and a `selfInitiatedTreeKillCount: 0` could read as exculpatory when it was not. - Gate the remaining main-process families: the git command-runner abort, the notebook-cell and automation-precheck timeouts. - Turn the `src/shared` seam into the gate itself (`process-tree-kill-gate`), so the runProcess choke point, the codex app-server deadline kill and the ephemeral-VM recipe kill ask the same decision. Those three are compiled into the CLI/relay too and cannot import main; main installs the guard at preflight. - Ratchet (`main-process-tree-kill-gate.test.ts`): a new pid-addressed taskkill in main that skips the gate fails, and the allowlist entries must still exist. - Give pid-addressed kills eviction priority in the 32-entry ring: 32 routine `win-pty-job` teardowns from a window-close burst no longer evict the one entry that discriminates a self-kill from an external one. - Correct the coverage doc, which described the uninstrumented Windows sites as POSIX `process.kill(-pid)` group kills and omitted the git and codex paths. * fix(crash-reporting): keep a refused tree-kill from leaking the root it owns A refusal must block the pid-addressed tree walk, not the termination. Five of the six gated sites returned on refusal with no fallback, so a refused `taskkill /pid /t /f` left git.exe, a timed-out notebook cell, an automation precheck or an ephemeral-VM recipe running while the caller reported it stopped. The root kill is addressed by the child handle, which cannot reach the recycled pid the refusal is about, so it stays correct and required on that path. Also fixes the ring eviction the scope preference introduced: with the ring saturated by pid-addressed kills, the only non-pid-addressed entry is the one just pushed, so the splice evicted itself and the detail came back `{}` -- byte-identical to the external-kill arm, in the window-close case the guard exists for. Eviction now excludes the newest entry and falls back to FIFO. Tests: refusal now asserts the root kill at all six sites, and the ring covers the saturated-pid ordering as well as round 3's group-burst ordering. * fix(crash-reporting): stop a refused tree-kill leaking the commit-message agent, and count call sites Two round-5 blocking findings, both open on main and on both branches. `killSourceControlAgentProcess` had no root-kill fallback on its win32 arm: the taskkill was the only termination, so once the own-Chromium gate could refuse it the promise resolved having killed nothing. Both callers do `terminationComplete ??= killSourceControlAgentProcess(child)` and then release the managed-home lock on that promise, so a refusal left the local Codex/Claude commit-message agent running while the caller reported it stopped -- the lock-contention failure the taskkill was added for. Same fix as the six sibling sites: the handle-addressed root kill cannot reach the recycled pid the refusal is about, so it stays correct and required on that path. The ratchet was file-granular, not call-site granular: one gate mention anywhere in a file exempted every taskkill in it, which left the six files that now ask the gate ratchet-blind -- the inverse of what it is for. It now counts `/pid` call sites against gate admissions per file, so a second ungated kill inside an existing family fails. Keying on the `/pid` argument rather than a quoted `taskkill` also catches a kill whose program name comes from a constant. The three comments that claimed more than the old scan enforced now state the rule and its two remaining blind spots. Also: the recording in `admitSelfInitiatedTreeKill` is now wrapped the way the `admitProcessTreeKill` seam already wraps it, with the refusal decision taken before anything that can throw so a diagnostics failure cannot flip it; and `orca-chromium-process-pids` documents the false-positive direction (a stale `getAppMetrics()` entry plus pid reuse refuses a live unrelated child), which is the mechanism the root-kill fallback exists to bound. Tests: refusal now asserts the root kill at all seven sites; the ratchet asserts call-site counting and the constant-program form. * test(crash-reporting): run the own-Chromium gate against real Windows trees Nothing on this branch had ever executed on Windows. The unit tests pin the gate's decision against a mocked taskkill, which cannot show that the decision does anything to a real process: that `/T /F` reaps a detached grandchild, that a refusal leaves that tree standing, or that the handle-addressed root kill the refusal path falls back to reaps the root while orphaning descendants. Adds a win32-gated live test covering all four, registered in both the `package_windows` CI lane and `WINDOWS_PACKAGE_TESTS` as `win32-test-lane-registration` requires. Also completes the coverage doc's "never instrumented" list, which omitted the macOS keyboard-input-source probe's POSIX group kill in `ipc/app.ts`. * fix(crash-reporting): pin the commit-message root kill on the Windows arm The first Windows run of this branch found nine failures the macOS suite cannot see: `commit-message-text-generation-test-harness` asserts `expect(child.kill).not.toHaveBeenCalled()` on `process.platform === 'win32'`, which is the contract the previous commit deliberately replaced — and it branches on the real platform, so it is dead code everywhere CI runs today. The harness now asserts the handle-addressed root kill on every platform. On win32 it lands after the tree walk, so the expectation waits rather than reading one tick early, and its ten call sites await it. Red against the pre-fix arm at all seven sites; the production code is unchanged. * test(crash-reporting): remove the Windows lane marker tree through the retrying helper The new win32 spec teardown used a raw rmSync, which the windows-lane-tree-removal boundary ratchet rejects — and which is exactly the EPERM the ratchet exists to prevent, since this spec's marker directory is written by processes it has just force-killed. * fix(crash-reporting): only refuse pid-addressed tree walks, disclose the handle-less codex site The own-Chromium gate refused the POSIX process-group arm of signalProcessTree as well, which was new macOS/Linux behaviour: a stale getAppMetrics() entry plus pid reuse would orphan a group that main reaps today. A POSIX group only holds what Orca put in it, so the refusal is now scoped to win-taskkill-tree and the POSIX arm is recorded and admitted like the other group kills in main. That also drops the synchronous getAppMetrics() read from every POSIX termination. codex-turn-added-roots kills roots found by a table walk, so a refusal has no handle to fall back to. Pin that the refusal is visible - crumb written, turn reported as not cancelled - rather than fixing what cannot be fixed. * test(crash-reporting): detach the Windows survival fixture and observe real spawns --- .github/workflows/pr.yml | 1 + config/scripts/pr-code-change-scope.mjs | 1 + src/main/automations/precheck-runner.ts | 18 +- src/main/codex/codex-app-server-session.ts | 13 ++ .../codex/codex-structured-turn-processes.ts | 2 + .../self-initiated-tree-kill-log.test.ts | 94 +++++++- .../self-initiated-tree-kill-log.ts | 79 ++++--- .../spawned-command-tree-kill.ts | 9 + src/main/ipc/notebook.ts | 16 +- src/main/main-process-tree-kill-gate.test.ts | 184 +++++++++++++++ src/main/orca-chromium-process-pids.ts | 6 + src/main/own-chromium-tree-kill-guard.test.ts | 41 +++- src/main/own-chromium-tree-kill-guard.ts | 46 +++- ...refused-tree-kill-root-termination.test.ts | 220 ++++++++++++++++++ src/main/startup/main-process-preflight.ts | 6 +- ...ssage-text-generation-cancellation.test.ts | 12 +- ...e-text-generation-local-subprocess.test.ts | 2 +- ...ge-text-generation-model-discovery.test.ts | 6 +- ...it-message-text-generation-test-harness.ts | 12 +- .../source-control-local-process.ts | 11 +- src/main/windows-live-tree-kill.win32.test.ts | 197 ++++++++++++++++ src/main/windows-process-tree-kill.ts | 5 +- .../child-process/process-tree-kill-gate.ts | 43 ++++ .../process-tree-kill-observer.ts | 37 --- .../process-tree-termination.test.ts | 9 +- .../child-process/process-tree-termination.ts | 36 ++- src/shared/ephemeral-vm-recipe-process.ts | 16 +- 27 files changed, 1001 insertions(+), 121 deletions(-) create mode 100644 src/main/main-process-tree-kill-gate.test.ts create mode 100644 src/main/refused-tree-kill-root-termination.test.ts create mode 100644 src/main/windows-live-tree-kill.win32.test.ts create mode 100644 src/shared/child-process/process-tree-kill-gate.ts delete mode 100644 src/shared/child-process/process-tree-kill-observer.ts diff --git a/.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/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/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/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/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' From 2e8097245029d2a398d40f81522826ac24e629ae Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Fri, 4 Sep 2026 17:03:01 -0700 Subject: [PATCH 3/4] chore: update in-app Android APK link (#18745) --- src/renderer/src/components/mobile/mobile-platform-copy.ts | 2 +- src/renderer/src/components/settings/MobileSettingsPane.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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) From 86cd327749dc624dd0181f18b4967fdbabbbac40 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 4 Sep 2026 17:11:41 -0700 Subject: [PATCH 4/4] Answer the structured-session support probe without installing the host (#18695) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Answer the structured-session support probe without installing the host `getStructuredAgentSessionCreateSupport` called `ensureStructuredAgentSessionHost()` before answering, so a read-only "can you create a Codex session here?" question performed the create route's lifecycle work: the first install opens the durable agent-session record store, attaches the PTY write-gate record lookup and starts the orphan-child reaper. Ask the pure predicate instead. `supportsCreate` on the installed host resolves to `adapterSupportsCreate`, which for the Codex adapter is exactly `agent === 'codex' && supportsCodexStructuredLocation(location)` — no adapter instance is needed to answer it. Nothing is lost: the create/attach route still installs via `ensureStructuredHostInstalled`, and startup restoration still installs and reconciles when a store is already persisted. * test(runtime): cover structured support probe parity --------- Co-authored-by: Merge Sim --- ...lve-recovered-structured-tui-transcript.ts | 8 +- ...ctured-agent-session-support-probe.test.ts | 174 ++++++++++++++++++ 2 files changed, 179 insertions(+), 3 deletions(-) create mode 100644 src/main/runtime/structured-agent-session-support-probe.test.ts 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) + }) +})