mirror of
https://github.com/stablyai/orca.git
synced 2026-09-30 08:03:12 +00:00
fix(windows): refuse tree-kills of Orca's own Chromium pids and record the rest
G2 is 20 field reports that share only a symptom. It is at least four fingerprints: ~15 Windows `reason=killed exitCode=1`, 3 POSIX SIGKILL under memory pressure (G4-oom), 2 duplicate reports of one macOS V8 Proxy Resolver SIGKILL, and 1 `0x80000003` install-dir ACL crash (G1; #17740 ships in v1.4.196 only, not 1.4.195). Nothing here claims to fix all of them. Two changes: 1. Behaviour. `classifyWindowsTreeKillTarget` returns `own` for any direct child of the main process — which our renderer, GPU and network-service utility all are — so PTY teardown could `taskkill /T /F` Orca's own UI (#10680). Both that classifier and `terminateWindowsProcessTree` now refuse any pid Electron is currently accounting for in `getAppMetrics()`. 2. Diagnosis. An Orca-issued kill and an external one are byte-identical in every field the crash report records today, so the cluster is undecidable. Every main-process force-kill choke point now records a durable `self_tree_kill` breadcrumb, and `process_gone` reports carry `selfInitiatedTreeKills` naming the pid and its offset from the death. A refused kill records `self_tree_kill_refused_own_chromium`, which is falsifiable: if it ever shows up in the field, we were the killer.
This commit is contained in:
@@ -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')
|
||||
})
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 (
|
||||
|
||||
@@ -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
|
||||
)
|
||||
|
||||
@@ -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<string, unknown> }
|
||||
|
||||
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<Record<string, unknown>> {
|
||||
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)
|
||||
})
|
||||
})
|
||||
@@ -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<string, CrashReportDetailValue> {
|
||||
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 = []
|
||||
}
|
||||
@@ -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<number> {
|
||||
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()
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
)
|
||||
})
|
||||
})
|
||||
@@ -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(() => {})
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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.
|
||||
}
|
||||
|
||||
@@ -36,7 +36,9 @@ export function createChildTerminationExpectation(
|
||||
): (child: { pid: number; kill: ReturnType<typeof vi.fn> }) => 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
|
||||
}
|
||||
|
||||
@@ -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')
|
||||
|
||||
@@ -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<void>
|
||||
export type WindowsTreeKiller = (rootPid: number, deps?: { site?: string }) => Promise<void>
|
||||
|
||||
/** 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<void> {
|
||||
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(
|
||||
|
||||
@@ -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<readonly ProcessLink[] | nu
|
||||
* `own`. That is not remote during teardown, when Orca is itself the process
|
||||
* allocating pids. Closing it needs real identity (a `Win32_Process.CreationDate`
|
||||
* baseline, the analogue of the POSIX `lstart` check, or an inherited handle /
|
||||
* Job Object).
|
||||
* Job Object). The Chromium-process half of it IS closed: `ownChromiumPids`
|
||||
* refuses any pid Electron is currently accounting for.
|
||||
*/
|
||||
export function classifyWindowsTreeKillTarget(
|
||||
rootPid: number,
|
||||
rows: readonly ProcessLink[],
|
||||
ownerPid: number
|
||||
ownerPid: number,
|
||||
ownChromiumPids: ReadonlySet<number> = 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<number, number | null>()
|
||||
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<number>
|
||||
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()
|
||||
)
|
||||
}
|
||||
|
||||
@@ -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'
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user