From bafc314bee5dfab9a3329ff027cff9e682a0bab9 Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Thu, 3 Sep 2026 18:36:21 -0700 Subject: [PATCH] fix(claude): fence the forced sweep on re-derivation, not on lstart's second A descendant forked in the same wall-clock second as every walk that sees it was signalled with SIGTERM and then never escalated, so a SIGTERM-resistant child survived tab close, app quit and a full relaunch. Two children of one parent 96ms apart across a second boundary took opposite paths. The leak predates this branch: it reproduces with the change reverted. `ps lstart` has one-second resolution, so a walk landing inside a row's birth second can never rule out a pid recycled later in that same second. But a walk is not a match: a ppid walk only reaches what the root actually parents, and the root is pinned by Node's own handle, so a row the walk re-derived is ours whatever second it was born in -- a stranger would have to have been forked into our tree, and then it is not a stranger. Fence the escalation on that. Rows a merge retained from an earlier walk are not re-derived and still answer to the start-time fence, which remains correct for them. Scoped to callers that revalidate identity before signalling, which is the Claude close path. Codex teardown reaches this same verifier and is unchanged; the argument holds there too, but widening it is its own deliberate change. Also reverts two changes from the previous attempt at this leak. Advancing the capture boundary on a later walk is inert once the sweep fences on re-derivation -- both key on the same set of rows, so the new term short-circuits for exactly the rows whose boundary it advanced. The extra ladder refresh was a duplicate full process-table read: close() already awaits tree.refresh() immediately before proveClaudeChildExit, on the only path that reaches it. Known property: the kill lands roughly a grace window after the walk that proved membership, so a pid recycled inside that gap could in principle be signalled. It is bounded -- matchingSnapshotRows already requires the live row to carry the same start-second and pgid, so an impostor must be born in the remainder of that one second, land on that exact pid, and sit in the same process group, and it has already received the unfenced SIGTERM from the same loop. --- ...aude-agent-sdk-exit-proof-identity.test.ts | 8 +- .../claude-agent-sdk-exit-proof.test.ts | 23 +-- ...laude-agent-sdk-root-kill-fallback.test.ts | 13 +- .../claude/claude-child-exit-proof-ladder.ts | 4 - src/main/claude/claude-child-tree-snapshot.ts | 11 +- ...ude-descendant-escalation-boundary.test.ts | 175 +++++++----------- src/main/pty-descendant-exit-verification.ts | 8 + src/main/pty-descendant-termination.test.ts | 8 +- src/main/pty-descendant-termination.ts | 11 +- 9 files changed, 117 insertions(+), 144 deletions(-) diff --git a/src/main/claude/claude-agent-sdk-exit-proof-identity.test.ts b/src/main/claude/claude-agent-sdk-exit-proof-identity.test.ts index 3804e1041d2..04d58067353 100644 --- a/src/main/claude/claude-agent-sdk-exit-proof-identity.test.ts +++ b/src/main/claude/claude-agent-sdk-exit-proof-identity.test.ts @@ -24,17 +24,19 @@ function windowsSnapshot(): WindowsDescendantSnapshot { } describe('Claude child root identity', () => { - it('advances a re-walked row boundary to the refresh that re-derived it', () => { + it('keeps a retained row boundary when a refresh observes no new descendants', () => { const previous = posixSnapshot(1_700_000_000_900) const next = posixSnapshot(1_700_000_002_100) - // Every row carries the refresh boundary, so no per-pid map is needed. expect( mergeClaudeCapturedTrees( { platform: 'posix', tree: previous }, { platform: 'posix', tree: next } ) - ).toEqual({ platform: 'posix', tree: next }) + ).toEqual({ + platform: 'posix', + tree: { ...next, capturedAtMsByPid: { '200': previous.capturedAtMs } } + }) }) it('keeps the descendant verdict when a POSIX root probe is unavailable', async () => { diff --git a/src/main/claude/claude-agent-sdk-exit-proof.test.ts b/src/main/claude/claude-agent-sdk-exit-proof.test.ts index be75c6427ed..3f15f8e8fca 100644 --- a/src/main/claude/claude-agent-sdk-exit-proof.test.ts +++ b/src/main/claude/claude-agent-sdk-exit-proof.test.ts @@ -513,20 +513,17 @@ describe('claude child tree reaper', () => { expect(child.kill).toHaveBeenCalledWith('SIGKILL') }) - it('advances a re-walked POSIX row boundary but not one the refresh missed', async () => { + it('keeps the original capture boundary for retained POSIX rows', async () => { const child = mockChild() - const unseen = { pid: 4245, ppid: 424242, pgid: 1, startedAt: 'Mon Jan 1 00:00:00 2026' } - const base = snapshotOf(4243) const first = { - ...base, - capturedAtMs: 1_700_000_000_900, - descendants: [...base.descendants, unseen] + ...snapshotOf(4243), + capturedAtMs: 1_700_000_000_900 } const refreshed = { - ...base, + ...first, capturedAtMs: 1_700_000_002_100, descendants: [ - ...base.descendants, + ...first.descendants, { pid: 4244, ppid: 424242, @@ -549,13 +546,11 @@ describe('claude child tree reaper', () => { expect(terminateDescendants).toHaveBeenCalledWith({ ...refreshed, - descendants: [...base.descendants, unseen, refreshed.descendants[1]], + // The retained 4243 row was first observed in the earlier displayed + // second. Its per-row boundary must not advance with the refresh. capturedAtMsByPid: { - // Re-derived from the live root by this walk, so proved ours again at it. - '4243': refreshed.capturedAtMs, - '4244': refreshed.capturedAtMs, - // Absent from the refresh: nothing re-proved it, so it keeps its own. - '4245': first.capturedAtMs + '4243': first.capturedAtMs, + '4244': refreshed.capturedAtMs } }) }) diff --git a/src/main/claude/claude-agent-sdk-root-kill-fallback.test.ts b/src/main/claude/claude-agent-sdk-root-kill-fallback.test.ts index c6bcfc60180..9f843527e30 100644 --- a/src/main/claude/claude-agent-sdk-root-kill-fallback.test.ts +++ b/src/main/claude/claude-agent-sdk-root-kill-fallback.test.ts @@ -167,16 +167,17 @@ describe('Claude root kill fallback', () => { expect(child.kill).not.toHaveBeenCalled() }) - it('chains the boundary of a row dropped from later walks', () => { - const first: WindowsDescendantSnapshot = { ...windowsSnapshot(1_000) } + it('chains per-pid Windows boundaries across a second merge', async () => { + const first = windowsSnapshot(1_000) const second: WindowsDescendantSnapshot = { ...windowsSnapshot(2_000), - descendants: [{ pid: 4244, creationTimeMs: 1_700_000_000_002 }] + descendants: [ + { pid: 4243, creationTimeMs: 1_700_000_000_000 }, + { pid: 4244, creationTimeMs: 1_700_000_000_002 } + ] } const third: WindowsDescendantSnapshot = { ...second, capturedAtMs: 3_000 } - // 4243 is absent from both refreshes, so it keeps the boundary of the walk - // that did see it rather than inheriting the latest scan's scalar. const merged = mergeClaudeCapturedTrees( { platform: 'win32', tree: first }, { platform: 'win32', tree: second } @@ -184,6 +185,6 @@ describe('Claude root kill fallback', () => { expect(merged?.tree.capturedAtMsByPid).toEqual({ '4243': 1_000, '4244': 2_000 }) const rechained = mergeClaudeCapturedTrees(merged!, { platform: 'win32', tree: third }) - expect(rechained?.tree.capturedAtMsByPid).toEqual({ '4243': 1_000, '4244': 3_000 }) + expect(rechained?.tree.capturedAtMsByPid).toEqual({ '4243': 1_000, '4244': 2_000 }) }) }) diff --git a/src/main/claude/claude-child-exit-proof-ladder.ts b/src/main/claude/claude-child-exit-proof-ladder.ts index 4c685adb494..85ed629f1b9 100644 --- a/src/main/claude/claude-child-exit-proof-ladder.ts +++ b/src/main/claude/claude-child-exit-proof-ladder.ts @@ -19,10 +19,6 @@ export async function proveClaudeChildExitWithReaper( const tree = input.tree ?? createTree() // Arm before stdin closes: only a live root can identify its descendants. await tree.capture() - // And re-walk it here, while the root certainly still lives: a descendant the - // arm first saw inside its own birth second is otherwise never eligible for a - // forced sweep, and the root may exit before any later walk gets the chance. - await tree.refresh?.() try { input.child.stdin?.end() } catch { diff --git a/src/main/claude/claude-child-tree-snapshot.ts b/src/main/claude/claude-child-tree-snapshot.ts index 1bd9f57cda3..e0955648b02 100644 --- a/src/main/claude/claude-child-tree-snapshot.ts +++ b/src/main/claude/claude-child-tree-snapshot.ts @@ -10,9 +10,6 @@ export type ClaudeCapturedTree = * Process-table reads are not atomic: a refresh can omit a still-live row, but * it can also observe a new process after the old row exited. Retain rows absent * from the refresh, but reject a PID whose identity changed between reads. - * - * Each row also carries the instant its membership of the tree was last proved, - * which is what the forced sweep fences on. */ function mergeRowsByPid( previous: readonly Row[], @@ -37,11 +34,9 @@ function mergeRowsByPid( if (prior && !sameIdentity(prior, row)) { return null } - // Why a retained row may take the later boundary: this walk re-derived it - // from the live root by ppid, which proves the process holding that pid is - // ours at this instant without appealing to its start time. A row only the - // earlier walk saw keeps the earlier boundary -- nothing re-proved it. - capturedAtMsByPid[String(row.pid)] = nextBoundary(row) + if (!prior) { + capturedAtMsByPid[String(row.pid)] = nextBoundary(row) + } merged.set(row.pid, row) } const boundaries = Object.values(capturedAtMsByPid) diff --git a/src/main/claude/claude-descendant-escalation-boundary.test.ts b/src/main/claude/claude-descendant-escalation-boundary.test.ts index 5ba1f9fedb0..55459df0c7f 100644 --- a/src/main/claude/claude-descendant-escalation-boundary.test.ts +++ b/src/main/claude/claude-descendant-escalation-boundary.test.ts @@ -1,62 +1,67 @@ import { describe, expect, it, vi } from 'vitest' -import type { DescendantTreeVerdict } from '../pty-descendant-exit-verification' import { terminateDescendantSnapshotWithVerdict } from '../pty-descendant-exit-verification' import { collectDescendantRows, type DescendantSnapshot, type ProcessTableRow } from '../pty-descendant-termination' -import { createClaudeChildTreeReaper, proveClaudeChildExit } from './claude-agent-sdk-exit-proof' +import { createClaudeChildTreeReaper } from './claude-agent-sdk-exit-proof' const ROOT_PID = 500 const ORCA_PGID = 400 -const ROOT_STARTED_AT = 'Thu Sep 3 16:37:20 2026' -const BIRTH_SECOND = 'Thu Sep 3 16:37:40 2026' -const BIRTH_MS = Date.parse(BIRTH_SECOND) -/** The measured shape: ten MCP-like children forked inside second :40. */ -const DESCENDANT_PIDS = [600, 601, 602, 603, 604, 605, 606, 607, 608, 609] +const ROOT_STARTED_AT = 'Thu Sep 3 18:04:50 2026' +/** The second both close-time walks land in. */ +const WALK_SECOND = 'Thu Sep 3 18:05:04 2026' +const WALK_MS = Date.parse(WALK_SECOND) +const EARLIER_SECOND = 'Thu Sep 3 18:05:03 2026' -function tableRows(descendantStartedAt: string): ProcessTableRow[] { +/** The measured split: `s20` at :03.946 died, `s21` at :04.042 leaked. */ +const EARLIER_BORN = [700, 701, 702] +const WALK_SECOND_BORN = [721, 722, 723, 724] + +type Cohort = { pids: number[]; startedAt: string } + +const LIVE_TREE: Cohort[] = [ + { pids: EARLIER_BORN, startedAt: EARLIER_SECOND }, + { pids: WALK_SECOND_BORN, startedAt: WALK_SECOND } +] + +function rowsFor(cohorts: Cohort[]): ProcessTableRow[] { return [ { pid: ROOT_PID, ppid: 1, pgid: ORCA_PGID, startedAt: ROOT_STARTED_AT }, - ...DESCENDANT_PIDS.map((pid) => ({ - pid, - ppid: ROOT_PID, - pgid: ORCA_PGID, - startedAt: descendantStartedAt - })) + ...cohorts.flatMap((cohort) => + cohort.pids.map((pid) => ({ + pid, + ppid: ROOT_PID, + pgid: ORCA_PGID, + startedAt: cohort.startedAt + })) + ) ] } /** A real ppid walk from the root, exactly as production captures one. */ -function walk(capturedAtMs: number, descendantStartedAt = BIRTH_SECOND): DescendantSnapshot { - return collectDescendantRows(ROOT_PID, tableRows(descendantStartedAt), capturedAtMs) -} - -/** - * Drives the real verifier against a process table where every descendant traps - * SIGTERM, so only a forced sweep can end them. - */ -function sigtermResistantVerifier(sendSignal: (pid: number, signal: NodeJS.Signals) => void) { - return (snapshot: DescendantSnapshot) => - terminateDescendantSnapshotWithVerdict(snapshot, { - requireIdentityBeforeSignal: true, - graceMs: 0, - verifyMs: 120, - sendSignal, - readTable: async () => ({ rows: tableRows(BIRTH_SECOND), capturedAtMs: Date.now() }) - }) +function walk(capturedAtMs: number, cohorts: Cohort[] = LIVE_TREE): DescendantSnapshot { + return collectDescendantRows(ROOT_PID, rowsFor(cohorts), capturedAtMs) } function killedPids(calls: [number, NodeJS.Signals][]): number[] { - return calls.flatMap(([pid, signal]) => (signal === 'SIGKILL' ? [pid] : [])) + return calls.flatMap(([pid, signal]) => (signal === 'SIGKILL' ? [pid] : [])).sort((a, b) => a - b) } function signalledPids(calls: [number, NodeJS.Signals][]): number[] { - return calls.flatMap(([pid, signal]) => (signal === 'SIGTERM' ? [pid] : [])) + return calls.flatMap(([pid, signal]) => (signal === 'SIGTERM' ? [pid] : [])).sort((a, b) => a - b) } -async function sweep(captures: DescendantSnapshot[]): Promise<[number, NodeJS.Signals][]> { +/** + * Drives the real reaper and the real verifier against a process table where + * every descendant traps SIGTERM, so only a forced sweep can end them. The root + * is alive for both walks and gone by the sweep, which is the measured teardown. + */ +async function sweep( + captures: DescendantSnapshot[], + liveTree: Cohort[] = LIVE_TREE +): Promise<[number, NodeJS.Signals][]> { const calls: [number, NodeJS.Signals][] = [] const captureDescendants = vi.fn() for (const capture of captures) { @@ -68,7 +73,14 @@ async function sweep(captures: DescendantSnapshot[]): Promise<[number, NodeJS.Si platform: 'linux', exited: () => false, captureDescendants, - terminateDescendants: sigtermResistantVerifier((pid, signal) => calls.push([pid, signal])) + terminateDescendants: (snapshot) => + terminateDescendantSnapshotWithVerdict(snapshot, { + requireIdentityBeforeSignal: true, + graceMs: 0, + verifyMs: 120, + sendSignal: (pid, signal) => calls.push([pid, signal]), + readTable: async () => ({ rows: rowsFor(liveTree), capturedAtMs: Date.now() }) + }) } ) // The close ladder's shape: arm, then re-walk the live root at the boundary. @@ -78,84 +90,35 @@ async function sweep(captures: DescendantSnapshot[]): Promise<[number, NodeJS.Si return calls } -describe('Claude descendant forced-sweep boundary', () => { - it('escalates descendants first seen in their own birth second', async () => { - // Arm lands inside second :40, the same second the children were forked in; - // the close-boundary walk re-derives them from the live root in second :41. - const calls = await sweep([walk(BIRTH_MS + 231), walk(BIRTH_MS + 1_200)]) +describe('Claude descendant forced-sweep fence', () => { + it('escalates a descendant forked in the same second as both close walks', async () => { + // Both walks land inside second :04, one ps duration apart, and the root is + // gone before a third could run. A descendant born at :04.042 is no less + // ours than its sibling born 96ms earlier at :03.946. + const calls = await sweep([walk(WALK_MS + 42), walk(WALK_MS + 140)]) - expect(signalledPids(calls)).toEqual(DESCENDANT_PIDS) - expect(killedPids(calls)).toEqual(DESCENDANT_PIDS) + expect(signalledPids(calls)).toEqual([...EARLIER_BORN, ...WALK_SECOND_BORN]) + expect(killedPids(calls)).toEqual([...EARLIER_BORN, ...WALK_SECOND_BORN]) }) - it('still escalates descendants born before the capture that first saw them', async () => { - const earlier = 'Thu Sep 3 16:37:17 2026' - const calls: [number, NodeJS.Signals][] = [] - const tree = createClaudeChildTreeReaper( - { pid: ROOT_PID, kill: vi.fn(() => true) }, - { - platform: 'linux', - exited: () => false, - captureDescendants: vi.fn(async () => - collectDescendantRows(ROOT_PID, tableRows(earlier), BIRTH_MS + 231) - ), - terminateDescendants: (snapshot) => - terminateDescendantSnapshotWithVerdict(snapshot, { - requireIdentityBeforeSignal: true, - graceMs: 0, - verifyMs: 120, - sendSignal: (pid, signal) => calls.push([pid, signal]), - readTable: async () => ({ rows: tableRows(earlier), capturedAtMs: Date.now() }) - }) - } - ) + it('still escalates descendants born before the walk that first saw them', async () => { + const onlyEarlier = [{ pids: EARLIER_BORN, startedAt: EARLIER_SECOND }] + const calls = await sweep([walk(WALK_MS + 42, onlyEarlier)], onlyEarlier) - await tree.capture() - await tree.reap() - - expect(killedPids(calls)).toEqual(DESCENDANT_PIDS) + expect(killedPids(calls)).toEqual(EARLIER_BORN) }) - it('withholds the sweep while no walk has re-proved the rows in a later second', async () => { - // Both walks land inside the birth second: nothing has disambiguated the - // rows, so the forced sweep must still stand down. - const calls = await sweep([walk(BIRTH_MS + 231), walk(BIRTH_MS + 640)]) + it('withholds the sweep from a row no walk re-derived, on its start second alone', async () => { + // 900 was seen once, in its own birth second, and the refresh did not find + // it. The merge retains the row, but nothing re-proved it belongs to us, so + // the second-resolution fence is all there is and it still says no. + const retained = { pids: [900], startedAt: WALK_SECOND } + const firstWalk = walk(WALK_MS + 42, [...LIVE_TREE, retained]) + const refresh = walk(WALK_MS + 140) - expect(signalledPids(calls)).toEqual(DESCENDANT_PIDS) - expect(killedPids(calls)).toEqual([]) - }) -}) + const calls = await sweep([firstWalk, refresh], [...LIVE_TREE, retained]) -describe('Claude close-boundary re-walk', () => { - it('re-walks the live root before stdin closes, not only after the grace window', async () => { - // The measured teardown: the root leaves on its own inside the grace window, - // so the post-timeout refresh never runs and this is the last walk that can - // happen while the root is still there to be walked from. - const order: string[] = [] - const tree = { - capture: vi.fn(async () => { - order.push('capture') - }), - refresh: vi.fn(async () => { - order.push('refresh') - }), - reap: vi.fn(async (): Promise => { - order.push('reap') - return 'exited' - }), - get treeVerdict(): DescendantTreeVerdict { - return 'unverifiable' - } - } - const stdin = { end: vi.fn(() => order.push('stdin-end')) } - - await proveClaudeChildExit({ - child: { pid: ROOT_PID, kill: vi.fn(() => true), stdin } as never, - exitPromise: Promise.resolve(), - exited: () => true, - tree - }) - - expect(order).toEqual(['capture', 'refresh', 'stdin-end', 'reap']) + expect(signalledPids(calls)).toEqual([...EARLIER_BORN, ...WALK_SECOND_BORN, 900]) + expect(killedPids(calls)).toEqual([...EARLIER_BORN, ...WALK_SECOND_BORN]) }) }) diff --git a/src/main/pty-descendant-exit-verification.ts b/src/main/pty-descendant-exit-verification.ts index 8d8903a3988..4c8471fa955 100644 --- a/src/main/pty-descendant-exit-verification.ts +++ b/src/main/pty-descendant-exit-verification.ts @@ -142,7 +142,15 @@ export async function terminateDescendantSnapshotWithVerdict( if (!forced && Date.now() >= deadline - verifyMs + graceMs) { forced = true for (const row of live) { + // A row a walk re-derived from a live root is ours whatever second it + // was born in, which start time alone can never establish for one born + // in its own capture second. Rows no walk re-derived still answer to + // the second-resolution fence, which is all the evidence they have. + // Scoped to the identity-revalidating callers; the same argument holds + // for the rest, but widening it is a deliberate change of its own. if ( + (deps.requireIdentityBeforeSignal === true && + snapshot.reDerivedPids?.has(row.pid) === true) || hasUnambiguousStartIdentity( row, snapshot.capturedAtMsByPid?.[String(row.pid)] ?? snapshot.capturedAtMs diff --git a/src/main/pty-descendant-termination.test.ts b/src/main/pty-descendant-termination.test.ts index fc4a41b8bc6..0c4bea81306 100644 --- a/src/main/pty-descendant-termination.test.ts +++ b/src/main/pty-descendant-termination.test.ts @@ -60,7 +60,9 @@ function snapshot( ...(rootPgid === null ? {} : { root: { pid: 10, startedAt: 'Mon Jul 13 12:54:47 2026' } }), rootPgid, descendants, - capturedAtMs + capturedAtMs, + // Everything a walk returns was re-derived by it. + ...(rootPgid === null ? {} : { reDerivedPids: new Set(descendants.map((row) => row.pid)) }) } } @@ -454,7 +456,9 @@ describe('terminateDescendantSnapshotAndWait', () => { const pending = terminateDescendantSnapshotWithVerdict( { ...snapshot([retained, fresh], 10, refreshBoundary), - capturedAtMsByPid: { '20': oldBoundary, '30': refreshBoundary } + capturedAtMsByPid: { '20': oldBoundary, '30': refreshBoundary }, + // What a merge produces: only the refresh re-derived 30; 20 is retained. + reDerivedPids: new Set([30]) }, { sendSignal, diff --git a/src/main/pty-descendant-termination.ts b/src/main/pty-descendant-termination.ts index 755c6b31da2..4f56c3e977b 100644 --- a/src/main/pty-descendant-termination.ts +++ b/src/main/pty-descendant-termination.ts @@ -32,6 +32,14 @@ export type DescendantSnapshot = { capturedAtMs: number /** Per-PID identity boundaries for merged captures. */ capturedAtMsByPid?: Readonly> + /** + * PIDs this walk re-derived from a live root. A ppid walk only reaches what + * the root actually parents, so membership is proof of ownership that owes + * nothing to `lstart`'s one-second resolution: a stranger would have to have + * been forked into our own tree, and then it is not a stranger. Rows a merge + * retained from an earlier walk are absent, and still answer to start time. + */ + reDerivedPids?: ReadonlySet } export type ProcessTableCapture = { @@ -204,7 +212,8 @@ export function collectDescendantRows( root: { pid: rootRow.pid, startedAt: rootRow.startedAt }, rootPgid: rootRow.pgid, descendants, - capturedAtMs + capturedAtMs, + reDerivedPids: new Set(descendants.map((row) => row.pid)) } }