3 Commits

Include renames

Author	SHA1	Message	Date
Neil	0a821e5bc8	fix(crash-reporting): make the own-Chromium gate a real choke point, and stop a refusal leaking the root (#18459) ... * 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 <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	2026-09-04 16:42:42 -07:00
Neil	7b530f1eb5	fix(crash-reporting): record Orca-initiated tree kills so a killed renderer is decidable (#18367) ... * 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.	2026-09-03 02:40:58 -07:00
Neil	1cf562deea	refactor: split source control AI modules (#16179)	2026-08-25 00:31:06 -07:00