mirror of
https://github.com/stablyai/orca.git
synced 2026-10-30 16:47:34 +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. * fix(crash-reporting): coalesce self-kill breadcrumbs and scope the discriminator Round-1 review remediation. Three blocking findings, all accepted. 1. Breadcrumb flood (accepted). recordSelfInitiatedTreeKill wrote an uncoalesced durable crumb from two routine teardown paths, and the reviewer reproduced 12 terminal closes x 3 process groups completely evicting the 30-slot ring — including this PR's own refusal crumb — plus a forced writeSync per killed group. It now uses the existing recordCoalescedDurableCrashBreadcrumb (5s window for pid-addressed taskkills, 60s for routine group/job teardown), so a burst costs one ring slot and one flush. The refusal crumb is coalesced per victim pid, so a retry loop cannot flood while a distinct pid always gets its own crumb. Regression test replays the reviewer's exact 12x3 reproduction and asserts the refusal crumb and a pre-existing gpu_process_crashed both survive. 2. Undifferentiated count (accepted). posix-process-group and win-pty-job are structurally incapable of reaching a Chromium process, and scope was absent from the persisted string. Scope is now in every entry (`<scope>/<site>/pid<N> +Nms`), and the count is split: selfInitiatedTreeKillCount now counts only pid-addressed taskkills — the kills that can land on a recycled pid that is now our renderer — with pty-scoped sweeps in selfInitiatedGroupKillCount. The list is renamed selfInitiatedKills because it carries both, and sorts pid-addressed kills first so truncation never drops the discriminating ones for teardown noise. The reviewer's repro (routine macOS terminal close + unrelated exit-133 crash) now yields selfInitiatedTreeKillCount undefined. 3. Recording gaps and a false comment (accepted). New admitSelfInitiatedTreeKill gate: it refuses own-Chromium pids and records the rest, and all three main-process taskkill families now go through it — terminateWindowsProcessTree plus codex-accounts/service.ts and claude-accounts (which keep their own spawn lifetimes). The false "single taskkill choke point" comment is gone. The runProcess choke point the investigation asked for is instrumented via a setProcessTreeKillObserver seam in src/shared/child-process — shared code runs in the CLI and relay so it cannot import the main breadcrumb store — registered in main preflight. The codex app-server POSIX group teardowns and the claude POSIX branch record too. The module doc no longer claims absence is discriminating: it enumerates what is instrumented and names the direct process.kill(-pid) sites that are not. Non-blocking, also fixed: - Breadcrumb calls moved out of the try blocks whose catch is the ESRCH contract (posix-pty-process-groups, codex teardown, claude POSIX), so a throw from the diagnostic path can never be reported as a failed kill. - Detail truncation now bounds the first entry too, matching its comment. - own-chromium-tree-kill-refusal.test.ts renamed to own-chromium-tree-kill-guard.test.ts, colocated with the module it tests. Not changed, with reasons: - Date.now() vs performance.now(): kept. Offsets are computed against goneAt = Date.now() in process-gone-recorder; a monotonic clock here would make the offsets meaningless. The reviewer verified this and agreed it is not a defect. - app.getAppMetrics() per force-kill remains unbenchmarked. It reads in-process browser state rather than enumerating the OS process table, and a TTL cache would let a recycled pid slip past the refusal, so it stays uncached. - The ~15 remaining direct process.kill(-pid) sites (browser routes, notebooks, automation prechecks, ephemeral VM recipes) are not instrumented. Rather than claim coverage this PR does not have, the module doc names them. claude-command-process.ts crossed the 300-line cap, so terminateClaudeProcess moved to claude-login-process-termination.ts. No max-lines suppression added. * fix(crash-reporting): scope the self-kill guard to its real host topology Round-2 review findings on the own-Chromium tree-kill guard. BLOCKING 1 — "the own-Chromium refusal is a no-op in the process that issues the pty-descendant-sweep taskkill". Correct on the mechanism, wrong on the consequence; REBUTTED in part and documented in full. Confirmed: the only non-test `setAppEnvironment` installs are main-process-preflight.ts:177 (Electron) and orcad-entry.ts:84 (Node, whose `getAppMetrics()` is `[]`); daemon-init-fresh-import.ts is a test harness. So in the standalone daemon `readOrcaChromiumProcessPids()` is empty and `admitSelfInitiatedTreeKill` always admits. But that is not a live hazard. `killWithDescendantSweep` reaches `terminateWindowsProcessTree` only when `verifyWindowsTreeKillTarget` returns `own`, and that walks ancestry back to `deps.ownerPid ?? process.pid` — the KILLING process's pid. In the daemon that is the daemon's pid. Orca's Chromium processes are children of Electron main, a sibling of the daemon, so their chain never reaches it: hop 0 lands on main, and within MAX_ANCESTOR_HOPS the walk dead-ends and returns `foreign`. The reviewer's probe passes `ownerPid: 1000` with the renderer as a direct child of 1000 — that is the Electron-main topology, where the AppEnvironment IS installed and the guard DOES fire, not the daemon's. On an orcad/SSH host there is no Chromium on the box at all, so `[]` is accurate rather than degraded. Locked in as tests rather than prose (own-chromium-tree-kill-guard.test.ts): a renderer classifies `foreign` from a daemon ownerPid with an empty pid set, and `own` from main's ownerPid with an empty set — the falsifiable pair showing the pid set is load-bearing in main and nowhere else. Documented the host coverage in orca-chromium-process-pids.ts and own-chromium-tree-kill-guard.ts. One genuine hole the finding exposes: `signalProcessTree`'s `taskkillTree` is a fourth pid-addressed taskkill family (non-blocking item 2), it runs in the daemon/relay/CLI where the guard cannot run, and it guarded only on `!child.pid`. Reusing the predicate the codex login teardown already uses, the win32 branch now refuses a reaped child and falls back to `killRoot` — the same shape as the existing `!child.pid` branch. That closes the reaped-then-recycled pid path in every host. BLOCKING 2 — module doc overstates coverage. Rewritten: the ring is per-process and its only reader lives in Electron main, so a count on a `render-process-gone` covers main-issued kills only. Sites are now split into main-only, main-and- other-hosts (runProcess choke point, POSIX PTY group sweep, Windows Job Object — which record into a ring nothing reads when they run in the daemon or relay), and never-instrumented, with the note that a daemon/relay omission is a diagnostics gap, not a missed suspect, per the topology argument above. BLOCKING 3 — the three out-of-main instrumentation sites were untested. Added regression coverage: the runProcess seam on both branches plus the reaped-child refusal (process-tree-termination.test.ts), the group sweep recording only groups it actually signalled and skipping an ESRCH group (posix-pty-process-groups.test.ts), and the Job Object recording the shell pid only on `terminated` (windows-pty-job.test.ts). Verified red: reverting the three production files to origin/main fails 7 of the new tests. BLOCKING 4 — the Windows evidence validates a single-process model. Accepted. The main2.js arms exercise `pty-descendant-sweep` inside one Electron process; that models the in-process/degraded daemon and the local PTY provider, not the standalone daemon. Arm C's "the 449351d6 shape is not producible with the guard" holds for main-issued kills only. In the daemon the shape is blocked one layer earlier, by the ancestry check, which the arms do not exercise. NON-BLOCKING taken: `recordSelfInitiatedTreeKill` moved outside the native `terminateJob` try in windows-pty-job.ts, so a diagnostics throw can no longer downgrade a real termination to `unavailable` and escalate callers to a broader kill; covered by a test. The "all three families" parenthetical is gone with the doc rewrite. `pnpm build:relay` run: exit 0, all seven targets built. NON-BLOCKING declined: codex-accounts/service.ts records before the spawn because a refusal must prevent the spawn — the crumb means "we were about to kill this pid", which is the artifact worth having; the existing comment already says so. `app.getAppMetrics()` perf is unbenchmarked and unchanged by this round. Verification: pnpm tc clean; oxlint clean on touched paths; check:code-quality:changed 0 new findings; oxfmt applied. 730 tests pass across shared/child-process, main/crash-reporting, main/pty, main/windows and the guard and descendant-sweep suites. The 4 failures in providers/git/codex-integration reproduce on HEAD without these changes. * fix(crash-reporting): keep the reaped-pid skip from flipping the termination barrier The win32 hasExited short-circuit correctly avoids taskkill on a pid Windows may have reissued, but it resolved `true` — verified tree termination. A taskkill against a reaped pid already resolved `false`, and run-process turns `true` into barrierTerminationVerified + terminationReporter.report(), which releases the git admission grant on root exit instead of on `close`. That admits the next git command while a descendant holding the inherited pipes is still writing the repo. Resolve `false` so the skip changes only which process we refuse to signal, not what the barrier claims.
196 lines
6.6 KiB
TypeScript
196 lines
6.6 KiB
TypeScript
import type { ChildProcessHandle } from '../../shared/child-process/run-process'
|
|
import { captureDescendantSnapshot, type DescendantSnapshot } from '../pty-descendant-termination'
|
|
import { terminateDescendantSnapshotAndWait } from '../pty-descendant-exit-verification'
|
|
import { terminateWindowsProcessTree } from '../windows-process-tree-kill'
|
|
import { findAgentSessionSpawnTokenProcesses } from '../runtime/agent-session-spawn-token-process-scan'
|
|
import { recordSelfInitiatedTreeKill } from '../crash-reporting/self-initiated-tree-kill-log'
|
|
|
|
const TOKEN_PROCESS_EXIT_TIMEOUT_MS = 3_500
|
|
const TOKEN_PROCESS_POLL_MS = 25
|
|
const activeTeardowns = new WeakMap<object, Promise<boolean>>()
|
|
|
|
type TeardownChild = Pick<ChildProcessHandle, 'pid' | 'kill'>
|
|
|
|
export type CodexAppServerProcessTeardownDeps = {
|
|
platform?: NodeJS.Platform
|
|
dedicatedProcessGroup?: boolean
|
|
/** Diagnostic/recovery injection only; never used by the primary teardown. */
|
|
findSpawnTokenProcesses?: (spawnToken: string) => Promise<number[] | null>
|
|
captureDescendants?: (rootPid: number) => Promise<DescendantSnapshot | null>
|
|
terminateDescendants?: (snapshot: DescendantSnapshot) => Promise<boolean>
|
|
terminateWindowsTree?: (rootPid: number, deps?: { site?: string }) => Promise<void>
|
|
signalPid?: (pid: number, signal: NodeJS.Signals) => void
|
|
signalProcessGroup?: (pgid: number, signal: NodeJS.Signals) => void
|
|
isPidPresent?: (pid: number) => boolean
|
|
wait?: (ms: number) => Promise<void>
|
|
now?: () => number
|
|
}
|
|
|
|
function terminateDedicatedPosixGroup(
|
|
rootPid: number,
|
|
deps: CodexAppServerProcessTeardownDeps
|
|
): boolean {
|
|
const signalGroup =
|
|
deps.signalProcessGroup ??
|
|
((pgid: number, signal: NodeJS.Signals) => process.kill(-pgid, signal))
|
|
try {
|
|
signalGroup(rootPid, 'SIGKILL')
|
|
} catch (error) {
|
|
return (error as NodeJS.ErrnoException).code === 'ESRCH'
|
|
}
|
|
// Outside the try: that catch is the ESRCH contract, not a breadcrumb handler.
|
|
recordSelfInitiatedTreeKill({
|
|
pid: rootPid,
|
|
site: 'codex-app-server-teardown',
|
|
scope: 'posix-process-group'
|
|
})
|
|
return true
|
|
}
|
|
|
|
function sendSignal(pid: number, signal: NodeJS.Signals): void {
|
|
try {
|
|
process.kill(pid, signal)
|
|
} catch {
|
|
// An already-gone exact PID is the desired outcome.
|
|
}
|
|
}
|
|
|
|
function isPidPresent(pid: number): boolean {
|
|
try {
|
|
process.kill(pid, 0)
|
|
return true
|
|
} catch (error) {
|
|
return (error as NodeJS.ErrnoException).code !== 'ESRCH'
|
|
}
|
|
}
|
|
|
|
async function diagnosticTokenFallback(
|
|
rootPid: number,
|
|
spawnToken: string,
|
|
deps: CodexAppServerProcessTeardownDeps
|
|
): Promise<boolean> {
|
|
const find = deps.findSpawnTokenProcesses ?? findAgentSessionSpawnTokenProcesses
|
|
const signal = deps.signalPid ?? sendSignal
|
|
const pidPresent = deps.isPidPresent ?? isPidPresent
|
|
const delay =
|
|
deps.wait ?? ((ms: number) => new Promise<void>((resolve) => setTimeout(resolve, ms)))
|
|
const now = deps.now ?? Date.now
|
|
const deadline = now() + TOKEN_PROCESS_EXIT_TIMEOUT_MS
|
|
const signalled = new Set<number>()
|
|
while (now() < deadline) {
|
|
const pids = await find(spawnToken).catch(() => null)
|
|
if (pids === null) {
|
|
return false
|
|
}
|
|
for (const pid of pids.filter((candidate) => candidate !== rootPid)) {
|
|
signalled.add(pid)
|
|
signal(pid, 'SIGKILL')
|
|
}
|
|
if ([...signalled].every((pid) => !pidPresent(pid))) {
|
|
return true
|
|
}
|
|
await delay(TOKEN_PROCESS_POLL_MS)
|
|
}
|
|
return false
|
|
}
|
|
|
|
async function terminatePosixTree(
|
|
child: TeardownChild,
|
|
rootPid: number,
|
|
_spawnToken: string | undefined,
|
|
deps: CodexAppServerProcessTeardownDeps
|
|
): Promise<boolean> {
|
|
// Kept only for explicit recovery callers/tests. Production always follows
|
|
// the dedicated process-group path below; token enumeration is evidence,
|
|
// never the owner of orphan-reaping decisions.
|
|
if (_spawnToken && deps.findSpawnTokenProcesses) {
|
|
const reaped = await diagnosticTokenFallback(rootPid, _spawnToken, deps)
|
|
if (reaped) {
|
|
child.kill('SIGKILL')
|
|
return true
|
|
}
|
|
return false
|
|
}
|
|
child.kill('SIGSTOP')
|
|
const capture = deps.captureDescendants ?? captureDescendantSnapshot
|
|
const snapshot = await capture(rootPid).catch(() => null)
|
|
if (!snapshot) {
|
|
child.kill('SIGKILL')
|
|
return true
|
|
}
|
|
const terminate = deps.terminateDescendants ?? terminateDescendantSnapshotAndWait
|
|
const descendantsExited = await terminate(snapshot)
|
|
// A detached POSIX launch is the leader of its own process group. Group
|
|
// signalling reaches grandchildren even after they daemonise/reparent,
|
|
// while the stopped root and captured pgid make the ownership proof exact.
|
|
// The identity-gated descendant sweep remains the fallback for older hosts
|
|
// or launches that could not establish a dedicated group.
|
|
if (descendantsExited && snapshot.rootPgid === rootPid) {
|
|
const signalGroup =
|
|
deps.signalProcessGroup ??
|
|
((pgid: number, signal: NodeJS.Signals) => {
|
|
try {
|
|
process.kill(-pgid, signal)
|
|
} catch {
|
|
// Group already exited.
|
|
}
|
|
})
|
|
signalGroup(snapshot.rootPgid, 'SIGKILL')
|
|
recordSelfInitiatedTreeKill({
|
|
pid: snapshot.rootPgid,
|
|
site: 'codex-app-server-teardown',
|
|
scope: 'posix-process-group'
|
|
})
|
|
}
|
|
if (!descendantsExited) {
|
|
child.kill('SIGCONT')
|
|
return false
|
|
}
|
|
child.kill('SIGKILL')
|
|
return true
|
|
}
|
|
|
|
/** Stops every process owned by one app-server launch before releasing its wrapper. */
|
|
async function terminateOnce(
|
|
child: TeardownChild,
|
|
spawnToken: string | undefined,
|
|
deps: CodexAppServerProcessTeardownDeps
|
|
): Promise<boolean> {
|
|
const rootPid = child.pid
|
|
if (!rootPid) {
|
|
child.kill('SIGKILL')
|
|
return false
|
|
}
|
|
if ((deps.platform ?? process.platform) === 'win32') {
|
|
const terminate = deps.terminateWindowsTree ?? terminateWindowsProcessTree
|
|
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
|
|
}
|
|
if (deps.dedicatedProcessGroup) {
|
|
return terminateDedicatedPosixGroup(rootPid, deps)
|
|
}
|
|
return terminatePosixTree(child, rootPid, spawnToken, deps)
|
|
}
|
|
|
|
export function terminateCodexAppServerProcessTree(
|
|
child: TeardownChild,
|
|
spawnToken?: string,
|
|
deps: CodexAppServerProcessTeardownDeps = {}
|
|
): Promise<boolean> {
|
|
const key = child as object
|
|
const active = activeTeardowns.get(key)
|
|
if (active) {
|
|
return active
|
|
}
|
|
const attempt = terminateOnce(child, spawnToken, deps).catch(() => false)
|
|
activeTeardowns.set(key, attempt)
|
|
void attempt.then(() => {
|
|
if (activeTeardowns.get(key) === attempt) {
|
|
activeTeardowns.delete(key)
|
|
}
|
|
})
|
|
return attempt
|
|
}
|