diff --git a/src/main/codex/codex-app-server-process-teardown.test.ts b/src/main/codex/codex-app-server-process-teardown.test.ts index 013ee183d12..1ddb672a531 100644 --- a/src/main/codex/codex-app-server-process-teardown.test.ts +++ b/src/main/codex/codex-app-server-process-teardown.test.ts @@ -23,7 +23,7 @@ describe('terminateCodexAppServerProcessTree', () => { release.resolve() await teardown - expect(terminateWindowsTree).toHaveBeenCalledWith(1234) + expect(terminateWindowsTree).toHaveBeenCalledWith(1234, { site: 'codex-app-server-teardown' }) expect(target.kill).toHaveBeenCalledWith('SIGKILL') }) diff --git a/src/main/codex/codex-app-server-process-teardown.ts b/src/main/codex/codex-app-server-process-teardown.ts index cc7ea8c8d17..4ac18b9ef4f 100644 --- a/src/main/codex/codex-app-server-process-teardown.ts +++ b/src/main/codex/codex-app-server-process-teardown.ts @@ -151,7 +151,7 @@ async function terminateOnce( } if ((deps.platform ?? process.platform) === 'win32') { const terminate = deps.terminateWindowsTree ?? terminateWindowsProcessTree - await terminate(rootPid) + await terminate(rootPid, { site: 'codex-app-server-teardown' }) // taskkill owns the tree; this preserves the prior direct-child fallback when it fails. child.kill('SIGKILL') return true diff --git a/src/main/codex/codex-structured-turn-processes.ts b/src/main/codex/codex-structured-turn-processes.ts index f69c0455a2e..6bb4b960a1b 100644 --- a/src/main/codex/codex-structured-turn-processes.ts +++ b/src/main/codex/codex-structured-turn-processes.ts @@ -57,7 +57,9 @@ async function terminateWindowsAddedProcesses( const added = current.filter((row) => baseline.get(row.pid) !== windowsIdentity(row)) const addedPids = new Set(added.map((row) => row.pid)) const roots = added.filter((row) => !addedPids.has(row.ppid)) - await Promise.all(roots.map((row) => terminateWindowsProcessTree(row.pid))) + await Promise.all( + roots.map((row) => terminateWindowsProcessTree(row.pid, { site: 'codex-turn-added-roots' })) + ) const targetIdentities = new Map(added.map((row) => [row.pid, windowsIdentity(row)])) const remaining = await queryWindowsProcessDescendants(rootPid, { fresh: true }) return ( diff --git a/src/main/crash-reporting/process-gone-recorder.ts b/src/main/crash-reporting/process-gone-recorder.ts index acb33c71b7e..267a0669b90 100644 --- a/src/main/crash-reporting/process-gone-recorder.ts +++ b/src/main/crash-reporting/process-gone-recorder.ts @@ -34,6 +34,7 @@ import { findSiblingChildDeaths, siblingProcessDeathDetails } from './process-gone-sibling-correlation' +import { selfInitiatedTreeKillDetails } from './self-initiated-tree-kill-log' import { getMainProcessLifecycleIdentity } from './main-process-lifecycle-identity' import { captureMinidumpSignature, @@ -247,7 +248,10 @@ export function recordProcessGoneCrash( { ...event.details, ...mainProcessLifecycle, - ...siblingDetails + ...siblingDetails, + // Why: an Orca-issued kill and an external one are identical in every other + // recorded field, so this is what answers "did we do this to ourselves?" + ...selfInitiatedTreeKillDetails(goneAt) }, event.processType ) 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 new file mode 100644 index 00000000000..e1f9d350c3c --- /dev/null +++ b/src/main/crash-reporting/self-initiated-tree-kill-log.test.ts @@ -0,0 +1,165 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('electron', () => ({ + app: { + getVersion: () => '1.4.194-test', + getAppMetrics: () => [] + } +})) + +import { clearCrashBreadcrumbsForTest, getCrashBreadcrumbSnapshot } from './crash-breadcrumb-store' +import { ProcessGoneDedupe } from './process-gone-dedupe' +import { recordProcessGoneCrash, type ProcessGoneCrashEvent } from './process-gone-recorder' +import { resetProcessGoneSiblingCorrelationForTest } from './process-gone-sibling-correlation' +import { + findSelfInitiatedTreeKills, + recordSelfInitiatedTreeKill, + resetSelfInitiatedTreeKillLogForTest, + selfInitiatedTreeKillDetails +} from './self-initiated-tree-kill-log' +import { terminateWindowsProcessTree } from '../windows-process-tree-kill' +import { _resetTracerForTests, setActiveSink } from '../observability/tracer' + +/** The field shape: renderer, `reason=killed exitCode=1`, win32 (#G2). */ +function killedRendererEvent(): ProcessGoneCrashEvent { + return { + source: 'renderer', + processType: 'renderer', + reason: 'killed', + exitCode: 1, + expectedTeardown: 'none', + details: { processType: 'renderer' } + } +} + +type RecordedReport = { details: Record } + +function capturingStore(recorded: RecordedReport[]) { + return { + record: async (report: RecordedReport) => { + recorded.push(report) + return { id: 'report-1' } + }, + attachDetails: async () => null + } +} + +/** Drives one crash through the recorder and returns the persisted details. */ +async function recordKilledRenderer(): Promise> { + const recorded: RecordedReport[] = [] + recordProcessGoneCrash( + capturingStore(recorded) as never, + killedRendererEvent(), + new ProcessGoneDedupe(), + async () => null + ) + await vi.waitFor(() => expect(recorded).toHaveLength(1)) + return recorded[0]!.details +} + +beforeEach(() => { + setActiveSink({ push: () => {}, flush: () => {}, close: () => {} }) + clearCrashBreadcrumbsForTest() + resetProcessGoneSiblingCorrelationForTest() + resetSelfInitiatedTreeKillLogForTest() +}) + +afterEach(() => { + vi.restoreAllMocks() + _resetTracerForTests() + clearCrashBreadcrumbsForTest() + resetProcessGoneSiblingCorrelationForTest() + resetSelfInitiatedTreeKillLogForTest() +}) + +describe('self-initiated tree kill breadcrumb', () => { + it('separates an Orca-issued tree kill from an external kill of the same shape', async () => { + // Arm A — Orca issues the kill through its own taskkill choke point. + await terminateWindowsProcessTree(4242, { + execFileImpl: ((_program, _args, _options, done) => { + ;(done as () => void)() + return undefined as never + }) as never, + site: 'pty-descendant-sweep' + }) + const selfKilled = await recordKilledRenderer() + + resetSelfInitiatedTreeKillLogForTest() + clearCrashBreadcrumbsForTest() + + // Arm B — identical crash, nobody inside Orca issued a kill. + const externallyKilled = await recordKilledRenderer() + + expect(selfKilled.selfInitiatedTreeKills).toMatch(/^pty-descendant-sweep\/pid4242 [+-]\d+ms$/) + expect(selfKilled.selfInitiatedTreeKillCount).toBe(1) + expect(externallyKilled.selfInitiatedTreeKills).toBeUndefined() + expect(externallyKilled.selfInitiatedTreeKillCount).toBeUndefined() + // Every other recorded field is identical — that is why the breadcrumb exists. + expect({ + ...selfKilled, + selfInitiatedTreeKills: null, + selfInitiatedTreeKillCount: null + }).toEqual({ + ...externallyKilled, + selfInitiatedTreeKills: null, + selfInitiatedTreeKillCount: null + }) + }) + + it('records a durable breadcrumb so the kill survives into the diagnostic bundle', async () => { + await terminateWindowsProcessTree(777, { + execFileImpl: ((_program, _args, _options, done) => { + ;(done as () => void)() + return undefined as never + }) as never, + site: 'codex-turn-added-roots' + }) + + expect(getCrashBreadcrumbSnapshot()).toEqual([ + expect.objectContaining({ + name: 'self_tree_kill', + data: expect.objectContaining({ + pid: 777, + site: 'codex-turn-added-roots', + scope: 'win-taskkill-tree' + }) + }) + ]) + }) + + it('keeps only kills near the death and drops the rest of the ring', () => { + const goneAt = 1_000_000 + recordSelfInitiatedTreeKill({ + pid: 1, + site: 'a', + scope: 'win-taskkill-tree', + at: goneAt - 6_000 + }) + recordSelfInitiatedTreeKill({ + pid: 2, + site: 'b', + scope: 'posix-process-group', + at: goneAt - 90 + }) + recordSelfInitiatedTreeKill({ pid: 3, site: 'c', scope: 'win-pty-job', at: goneAt + 500 }) + + const nearby = findSelfInitiatedTreeKills(goneAt) + + expect(nearby.map((kill) => kill.pid)).toEqual([2]) + expect(selfInitiatedTreeKillDetails(goneAt).selfInitiatedTreeKills).toBe('b/pid2 -90ms') + }) + + it('bounds the ring at 32 entries', () => { + const goneAt = 2_000_000 + for (let index = 0; index < 40; index += 1) { + recordSelfInitiatedTreeKill({ + pid: index + 1, + site: 'sweep', + scope: 'win-taskkill-tree', + at: goneAt - 10 + }) + } + + expect(findSelfInitiatedTreeKills(goneAt)).toHaveLength(32) + }) +}) diff --git a/src/main/crash-reporting/self-initiated-tree-kill-log.ts b/src/main/crash-reporting/self-initiated-tree-kill-log.ts new file mode 100644 index 00000000000..cb0ffbc8082 --- /dev/null +++ b/src/main/crash-reporting/self-initiated-tree-kill-log.ts @@ -0,0 +1,117 @@ +import type { CrashReportDetailValue } from '../../shared/crash-reporting' +import { recordDurableCrashBreadcrumb } from './durable-crash-breadcrumb' + +/** + * Records the force-kills Orca itself issues, so a later `render-process-gone` + * can say whether we were holding the knife. + * + * Why: on Windows a `taskkill /T /F` we issue and an external one produce the + * identical `reason=killed exitCode=1` plus the identical concurrent sibling + * deaths — reproduced side by side on Windows 11 / Electron 43.4.1, differing in + * zero recorded fields. This is the field that separates them. + */ + +/** Which mechanism issued the kill; each has a different blast radius. */ +export type SelfInitiatedTreeKillScope = 'win-taskkill-tree' | 'posix-process-group' | 'win-pty-job' + +export type SelfInitiatedTreeKill = { + pid: number + site: string + scope: SelfInitiatedTreeKillScope + at: number +} + +// Why 32 and not the sibling ring's 16: one teardown fans out over every root of +// a codex turn, so a single incident can spend a dozen entries on its own. +const MAX_TRACKED_SELF_KILLS = 32 + +// Why asymmetric: a kill older than this cannot plausibly explain the death, +// while the forward edge mirrors SIBLING_DEATH_LOOKAHEAD_MS — a kill issued just +// after the renderer died is at least as likely to be teardown reacting to it. +export const SELF_TREE_KILL_LOOKBACK_MS = 5_000 +export const SELF_TREE_KILL_LOOKAHEAD_MS = 250 + +// Same truncation rule as MAX_SIBLING_DEATHS_DETAIL_LENGTH: drop whole entries +// rather than let sanitizeCrashReportDetails cut the list mid-token. +const MAX_SELF_TREE_KILLS_DETAIL_LENGTH = 200 + +let selfInitiatedKills: SelfInitiatedTreeKill[] = [] + +export function recordSelfInitiatedTreeKill({ + pid, + site, + scope, + at = Date.now() +}: { + pid: number + site: string + scope: SelfInitiatedTreeKillScope + at?: number +}): void { + if (!Number.isInteger(pid) || pid <= 0) { + return + } + selfInitiatedKills.push({ pid, site, scope, at }) + if (selfInitiatedKills.length > MAX_TRACKED_SELF_KILLS) { + selfInitiatedKills = selfInitiatedKills.slice(-MAX_TRACKED_SELF_KILLS) + } + // Durable so it survives into the diagnostic bundle even when the kill takes + // the reporting renderer with it; durable breadcrumbs flush immediately. + recordDurableCrashBreadcrumb('self_tree_kill', { pid, site, scope }) +} + +/** + * A tree-kill we refused because the target is one of our own Chromium + * processes. Falsifiable on purpose: this crumb appearing in a field bundle is + * direct proof that Orca was about to kill its own renderer. + */ +export function recordRefusedOwnChromiumTreeKill(target: { + pid: number + site: string + scope: SelfInitiatedTreeKillScope +}): void { + recordDurableCrashBreadcrumb('self_tree_kill_refused_own_chromium', target) +} + +export function findSelfInitiatedTreeKills(at: number): SelfInitiatedTreeKill[] { + return selfInitiatedKills.filter((kill) => { + const offsetMs = kill.at - at + return offsetMs >= -SELF_TREE_KILL_LOOKBACK_MS && offsetMs <= SELF_TREE_KILL_LOOKAHEAD_MS + }) +} + +// Why not `site:pid@offset`: sanitizeCrashReportString reads `word:word@` as a +// credential URL and redacts the whole token. Mirror describeChildDeath instead. +function describeSelfInitiatedTreeKill(kill: SelfInitiatedTreeKill, goneAt: number): string { + const offsetMs = kill.at - goneAt + return `${kill.site}/pid${kill.pid} ${offsetMs >= 0 ? '+' : ''}${offsetMs}ms` +} + +/** Empty when Orca issued no nearby kill — absence is the discriminating half. */ +export function selfInitiatedTreeKillDetails( + goneAt: number +): Record { + const kills = findSelfInitiatedTreeKills(goneAt) + if (kills.length === 0) { + return {} + } + const described = [...kills] + .sort((a, b) => Math.abs(a.at - goneAt) - Math.abs(b.at - goneAt)) + .map((kill) => describeSelfInitiatedTreeKill(kill, goneAt)) + const kept: string[] = [] + for (const entry of described) { + if (kept.length > 0 && [...kept, entry].join(', ').length > MAX_SELF_TREE_KILLS_DETAIL_LENGTH) { + break + } + kept.push(entry) + } + const dropped = described.length - kept.length + return { + selfInitiatedTreeKillCount: kills.length, + selfInitiatedTreeKills: dropped > 0 ? `${kept.join(', ')} (+${dropped} more)` : kept.join(', ') + } +} + +export function resetSelfInitiatedTreeKillLogForTest(): void { + selfInitiatedKills = [] +} diff --git a/src/main/orca-chromium-process-pids.ts b/src/main/orca-chromium-process-pids.ts new file mode 100644 index 00000000000..c262d2d8e49 --- /dev/null +++ b/src/main/orca-chromium-process-pids.ts @@ -0,0 +1,27 @@ +import { getAppEnvironment, hasAppEnvironment } from '../shared/app-environment' + +/** + * PIDs of Orca's own Chromium processes — browser, renderers, GPU, utilities. + * + * Why: `taskkill /T /F` aimed at one of these kills a renderer we depend on, and + * the `render-process-gone` it produces is indistinguishable from an external + * kill in every field Orca records (#10680). A pid in this set is proof the + * target is ours to keep, not ours to tear down. + * + * 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. + */ +export function readOrcaChromiumProcessPids(): ReadonlySet { + if (!hasAppEnvironment()) { + return new Set() + } + try { + const pids = getAppEnvironment() + .getAppMetrics() + .map((metric) => metric.pid) + .filter((pid) => Number.isInteger(pid) && pid > 0) + return new Set(pids) + } catch { + return new Set() + } +} diff --git a/src/main/own-chromium-tree-kill-refusal.test.ts b/src/main/own-chromium-tree-kill-refusal.test.ts new file mode 100644 index 00000000000..4e787f7f17f --- /dev/null +++ b/src/main/own-chromium-tree-kill-refusal.test.ts @@ -0,0 +1,115 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { appMetricsMock } = vi.hoisted(() => ({ + appMetricsMock: vi.fn((): { pid: number; type?: string }[] => []) +})) + +import { + getAppEnvironment, + hasAppEnvironment, + setAppEnvironment, + type AppEnvironment +} from '../shared/app-environment' +import { readOrcaChromiumProcessPids } from './orca-chromium-process-pids' +import { classifyWindowsTreeKillTarget } from './windows-pty-root-identity' +import { terminateWindowsProcessTree } from './windows-process-tree-kill' +import { + clearCrashBreadcrumbsForTest, + getCrashBreadcrumbSnapshot +} from './crash-reporting/crash-breadcrumb-store' +import { _resetTracerForTests, setActiveSink } from './observability/tracer' + +const ORCA_MAIN_PID = 1000 +const RENDERER_PID = 1001 + +/** Orca's renderer is a direct child of the main process, so the ppid walk says `own`. */ +const PROCESS_ROWS = [ + { pid: RENDERER_PID, ppid: ORCA_MAIN_PID }, + { pid: ORCA_MAIN_PID, ppid: 900 } +] + +function appEnvironment(): AppEnvironment { + return { + getPath: () => process.cwd(), + getAppPath: () => process.cwd(), + getVersion: () => '0.0.0-test', + isPackaged: () => false, + onWillQuit: () => {}, + exit: () => {}, + getAppMetrics: appMetricsMock as unknown as AppEnvironment['getAppMetrics'] + } +} + +let previousEnvironment: AppEnvironment | null = null + +beforeEach(() => { + previousEnvironment = hasAppEnvironment() ? getAppEnvironment() : null + setAppEnvironment(appEnvironment()) + appMetricsMock.mockReturnValue([ + { pid: ORCA_MAIN_PID, type: 'Browser' }, + { pid: RENDERER_PID, type: 'Tab' }, + { pid: 1002, type: 'GPU' } + ]) + setActiveSink({ push: () => {}, flush: () => {}, close: () => {} }) + clearCrashBreadcrumbsForTest() +}) + +afterEach(() => { + if (previousEnvironment) { + setAppEnvironment(previousEnvironment) + } + vi.restoreAllMocks() + _resetTracerForTests() + clearCrashBreadcrumbsForTest() +}) + +describe('refusing to tree-kill our own Chromium processes', () => { + it('reads the live Chromium pid set from the app environment', () => { + expect([...readOrcaChromiumProcessPids()]).toEqual([ORCA_MAIN_PID, RENDERER_PID, 1002]) + }) + + it('classifies a live renderer as foreign even though its ancestry reaches us', () => { + expect(classifyWindowsTreeKillTarget(RENDERER_PID, PROCESS_ROWS, ORCA_MAIN_PID)).toBe('foreign') + }) + + it('still classifies a real PTY child of ours as own', () => { + const rows = [...PROCESS_ROWS, { pid: 7777, ppid: ORCA_MAIN_PID }] + + expect(classifyWindowsTreeKillTarget(7777, rows, ORCA_MAIN_PID)).toBe('own') + }) + + it('never spawns taskkill against one of our own Chromium pids', async () => { + const execFileImpl = vi.fn() + + await terminateWindowsProcessTree(RENDERER_PID, { + execFileImpl: execFileImpl as never, + site: 'pty-descendant-sweep' + }) + + expect(execFileImpl).not.toHaveBeenCalled() + expect(getCrashBreadcrumbSnapshot()).toEqual([ + expect.objectContaining({ + name: 'self_tree_kill_refused_own_chromium', + data: expect.objectContaining({ pid: RENDERER_PID, site: 'pty-descendant-sweep' }) + }) + ]) + }) + + it('still taskkills a pid that is not one of ours', async () => { + const execFileImpl = vi.fn((_program, _args, _options, done: () => void) => { + done() + }) + + await terminateWindowsProcessTree(7777, { + execFileImpl: execFileImpl as never, + site: 'pty-descendant-sweep' + }) + + expect(execFileImpl).toHaveBeenCalledWith( + 'taskkill', + ['/pid', '7777', '/T', '/F'], + expect.anything(), + expect.any(Function) + ) + }) +}) diff --git a/src/main/pty-descendant-termination.ts b/src/main/pty-descendant-termination.ts index c91bbdd9a20..bf254d03b56 100644 --- a/src/main/pty-descendant-termination.ts +++ b/src/main/pty-descendant-termination.ts @@ -275,7 +275,9 @@ export async function killWithDescendantSweep( const target = await verify(rootPid).catch((): WindowsTreeKillTarget => 'unknown') // Re-check ownership: the identity query awaits, so exit can land meanwhile. if (target === 'own' && (deps.ownsRoot?.() ?? true)) { - const killTree = deps.killWindowsTree ?? terminateWindowsProcessTree + const killTree = + deps.killWindowsTree ?? + ((pid: number) => terminateWindowsProcessTree(pid, { site: 'pty-descendant-sweep' })) // Why: taskkill may race an already-exited tree; never block killRoot on that. await killTree(rootPid).catch(() => {}) } diff --git a/src/main/pty/posix-pty-process-groups.ts b/src/main/pty/posix-pty-process-groups.ts index 21b18808d6d..424ca6eac6a 100644 --- a/src/main/pty/posix-pty-process-groups.ts +++ b/src/main/pty/posix-pty-process-groups.ts @@ -1,4 +1,5 @@ import { execFileSync } from 'node:child_process' +import { recordSelfInitiatedTreeKill } from '../crash-reporting/self-initiated-tree-kill-log' const PROCESS_TABLE_TIMEOUT_MS = 1_000 const PROCESS_TABLE_MAX_BYTES = 1024 * 1024 @@ -116,6 +117,11 @@ export function forceKillPosixPtyProcessGroups( for (const pgid of groups) { try { signalProcessGroup(pgid) + recordSelfInitiatedTreeKill({ + pid: pgid, + site: 'posix-pty-process-group-sweep', + scope: 'posix-process-group' + }) } catch (error) { // Why: the PTY exit callback may reap a group between `ps` and killpg. // ESRCH is proof that this captured owner is already gone, not failure. diff --git a/src/main/rate-limits/codex-probe-termination.test.ts b/src/main/rate-limits/codex-probe-termination.test.ts index 4fb029e01a9..8a8148eb848 100644 --- a/src/main/rate-limits/codex-probe-termination.test.ts +++ b/src/main/rate-limits/codex-probe-termination.test.ts @@ -97,7 +97,9 @@ describe('terminateCodexProbeChild', () => { expect(child.kill).not.toHaveBeenCalled() await vi.advanceTimersByTimeAsync(CODEX_PROBE_SHUTDOWN_DRAIN_MS) - expect(killWindowsProcessTree).toHaveBeenCalledWith(child.pid) + expect(killWindowsProcessTree).toHaveBeenCalledWith(child.pid, { + site: 'codex-rate-limit-probe' + }) expect(child.kill).toHaveBeenCalledTimes(1) expect(child.kill).toHaveBeenCalledWith() @@ -122,7 +124,9 @@ describe('terminateCodexProbeChild', () => { }) await vi.advanceTimersByTimeAsync(0) - expect(killWindowsProcessTree).toHaveBeenCalledWith(child.pid) + expect(killWindowsProcessTree).toHaveBeenCalledWith(child.pid, { + site: 'codex-rate-limit-probe' + }) child.exit() await Promise.resolve() expect(settled).toBe(false) diff --git a/src/main/rate-limits/codex-probe-termination.ts b/src/main/rate-limits/codex-probe-termination.ts index e5a2f966542..9491bb35f7b 100644 --- a/src/main/rate-limits/codex-probe-termination.ts +++ b/src/main/rate-limits/codex-probe-termination.ts @@ -90,7 +90,9 @@ export async function terminateCodexProbeChild( try { // npm-installed Codex runs beneath cmd.exe; killing only that wrapper can // leave app-server alive after the credential-home lock is released. - await (options?.killWindowsProcessTree ?? terminateWindowsProcessTree)(child.pid) + await (options?.killWindowsProcessTree ?? terminateWindowsProcessTree)(child.pid, { + site: 'codex-rate-limit-probe' + }) } catch { // The direct-child fallback still applies if an injected killer rejects. } 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 dd0ae06e1ec..21103d71f40 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 @@ -36,7 +36,9 @@ export function createChildTerminationExpectation( ): (child: { pid: number; kill: ReturnType }) => void { return (child) => { if (process.platform === 'win32') { - expect(terminateWindowsProcessTreeMock).toHaveBeenCalledWith(child.pid) + expect(terminateWindowsProcessTreeMock).toHaveBeenCalledWith(child.pid, { + site: 'source-control-text-generation' + }) expect(child.kill).not.toHaveBeenCalled() return } diff --git a/src/main/text-generation/source-control-local-process.ts b/src/main/text-generation/source-control-local-process.ts index e999ee4c8ee..170dead6b52 100644 --- a/src/main/text-generation/source-control-local-process.ts +++ b/src/main/text-generation/source-control-local-process.ts @@ -30,7 +30,7 @@ export function killSourceControlAgentProcess( return Promise.resolve() } if (process.platform === 'win32') { - return terminateWindowsProcessTree(pid) + return terminateWindowsProcessTree(pid, { site: 'source-control-text-generation' }) } try { child.kill('SIGKILL') diff --git a/src/main/windows-process-tree-kill.ts b/src/main/windows-process-tree-kill.ts index 44085692d08..196ef4988da 100644 --- a/src/main/windows-process-tree-kill.ts +++ b/src/main/windows-process-tree-kill.ts @@ -1,6 +1,11 @@ import { execFile } from 'node:child_process' +import { + recordRefusedOwnChromiumTreeKill, + recordSelfInitiatedTreeKill +} from './crash-reporting/self-initiated-tree-kill-log' +import { readOrcaChromiumProcessPids } from './orca-chromium-process-pids' -export type WindowsTreeKiller = (rootPid: number) => Promise +export type WindowsTreeKiller = (rootPid: number, deps?: { site?: string }) => Promise /** Bound hung taskkill so killRoot still runs in killWithDescendantSweep. */ export const WINDOWS_PROCESS_TREE_KILL_TIMEOUT_MS = 5_000 @@ -9,14 +14,27 @@ export const WINDOWS_PROCESS_TREE_KILL_TIMEOUT_MS = 5_000 * Force-kill a Windows process and every descendant (`taskkill /T /F`). * Best-effort: missing/already-dead roots still resolve so callers can finish * their own handle cleanup via killRoot. + * + * This is the main process's single taskkill choke point, so it is also where + * the self-kill breadcrumb and the own-Chromium refusal live — instrumenting + * callers instead would rot the first time one is added. */ export function terminateWindowsProcessTree( rootPid: number, - deps: { execFileImpl?: typeof execFile } = {} + deps: { execFileImpl?: typeof execFile; site?: string } = {} ): Promise { if (!Number.isInteger(rootPid) || rootPid <= 0) { return Promise.resolve() } + const site = deps.site ?? 'windows-process-tree-kill' + // 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(rootPid)) { + recordRefusedOwnChromiumTreeKill({ pid: rootPid, site, scope: 'win-taskkill-tree' }) + return Promise.resolve() + } + recordSelfInitiatedTreeKill({ pid: rootPid, site, scope: 'win-taskkill-tree' }) const run = deps.execFileImpl ?? execFile return new Promise((resolve) => { run( diff --git a/src/main/windows-pty-root-identity.ts b/src/main/windows-pty-root-identity.ts index b2f6ba43e81..c99224cb72a 100644 --- a/src/main/windows-pty-root-identity.ts +++ b/src/main/windows-pty-root-identity.ts @@ -1,4 +1,5 @@ import { queryWindowsProcessRowsFresh } from './providers/windows-foreground-process-rows' +import { readOrcaChromiumProcessPids } from './orca-chromium-process-pids' /** * Whether a PID still sits inside this process's own subtree. Note this is @@ -33,12 +34,14 @@ export type WindowsProcessLinkReader = () => Promise = readOrcaChromiumProcessPids() ): WindowsTreeKillTarget { // Why: our own pid is never a PTY root, so reading it here means the pid is // corrupt. `foreign` is the refusing verdict, which is what that must get — @@ -46,6 +49,12 @@ export function classifyWindowsTreeKillTarget( if (!Number.isInteger(rootPid) || rootPid <= 0 || rootPid === ownerPid) { return 'foreign' } + // Same reasoning one hop out: our renderer, GPU and utility children are all + // direct children of ownerPid, so the ancestry walk below calls them `own` and + // hands teardown a licence to taskkill /T /F Orca's own UI (#10680). + if (ownChromiumPids.has(rootPid)) { + return 'foreign' + } const parentByPid = new Map() for (const row of rows) { // Duplicate PID rows make ancestry ambiguous, so they never prove ownership. @@ -118,6 +127,7 @@ export async function verifyWindowsTreeKillTarget( deps: { readRows?: WindowsProcessLinkReader ownerPid?: number + ownChromiumPids?: ReadonlySet platform?: NodeJS.Platform timeoutMs?: number } = {} @@ -134,5 +144,10 @@ export async function verifyWindowsTreeKillTarget( if (!rows) { return 'unknown' } - return classifyWindowsTreeKillTarget(rootPid, rows, deps.ownerPid ?? process.pid) + return classifyWindowsTreeKillTarget( + rootPid, + rows, + deps.ownerPid ?? process.pid, + deps.ownChromiumPids ?? readOrcaChromiumProcessPids() + ) } diff --git a/src/main/windows/windows-pty-job.ts b/src/main/windows/windows-pty-job.ts index bebf72bb75e..5a63263dff3 100644 --- a/src/main/windows/windows-pty-job.ts +++ b/src/main/windows/windows-pty-job.ts @@ -1,5 +1,6 @@ import type { IPty } from 'node-pty' import { createRequire } from 'node:module' +import { recordSelfInitiatedTreeKill } from '../crash-reporting/self-initiated-tree-kill-log' /** * Job-object ownership for a ConPTY's process tree. @@ -94,7 +95,15 @@ export function terminatePtyJob(proc: IPty): JobTerminationOutcome { return 'unavailable' } try { - return native.terminateJob(target.id, target.shellPid) ? 'terminated' : 'unavailable' + if (!native.terminateJob(target.id, target.shellPid)) { + return 'unavailable' + } + recordSelfInitiatedTreeKill({ + pid: target.shellPid, + site: 'windows-pty-job-teardown', + scope: 'win-pty-job' + }) + return 'terminated' } catch { return 'unavailable' }