From d7e2c58549bb3d1ce232626323ac815760b134f2 Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Thu, 3 Sep 2026 17:10:43 -0700 Subject: [PATCH] fix(claude): let a re-walked descendant become eligible for the forced sweep A descendant first observed by a capture inside its own birth second could never be SIGKILLed: `ps lstart` is second-resolution, so that capture cannot rule out a pid recycled later in the same second, and the merge pinned each retained row to the boundary of the walk that first saw it. SIGTERM-resistant children forked in that window were signalled and then never escalated -- they survived close, quit and restart, reparented to init, and had to be killed by hand. Advancing that boundary on any later capture would be unsound: a later capture matching pid, pgid and start-second is exactly what an impostor would also show. But a capture is not a match -- it is a fresh ppid walk from a root Node pins through its own handle, so a row it re-derives is proved ours at that instant without appealing to its start time. Chain the fence from there instead, and take that walk at the close boundary while the root certainly still lives: the root may leave inside the grace window, and the post-timeout refresh never runs. A row absent from the later walk still keeps its earlier boundary, and a row no walk has ever re-derived in a later second is still never escalated. --- ...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 | 161 ++++++++++++++++++ 6 files changed, 196 insertions(+), 24 deletions(-) create mode 100644 src/main/claude/claude-descendant-escalation-boundary.test.ts 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 04d58067353..3804e1041d2 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,19 +24,17 @@ function windowsSnapshot(): WindowsDescendantSnapshot { } describe('Claude child root identity', () => { - it('keeps a retained row boundary when a refresh observes no new descendants', () => { + it('advances a re-walked row boundary to the refresh that re-derived it', () => { 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, capturedAtMsByPid: { '200': previous.capturedAtMs } } - }) + ).toEqual({ platform: 'posix', tree: next }) }) 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 3f15f8e8fca..be75c6427ed 100644 --- a/src/main/claude/claude-agent-sdk-exit-proof.test.ts +++ b/src/main/claude/claude-agent-sdk-exit-proof.test.ts @@ -513,17 +513,20 @@ describe('claude child tree reaper', () => { expect(child.kill).toHaveBeenCalledWith('SIGKILL') }) - it('keeps the original capture boundary for retained POSIX rows', async () => { + it('advances a re-walked POSIX row boundary but not one the refresh missed', 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 = { - ...snapshotOf(4243), - capturedAtMs: 1_700_000_000_900 + ...base, + capturedAtMs: 1_700_000_000_900, + descendants: [...base.descendants, unseen] } const refreshed = { - ...first, + ...base, capturedAtMs: 1_700_000_002_100, descendants: [ - ...first.descendants, + ...base.descendants, { pid: 4244, ppid: 424242, @@ -546,11 +549,13 @@ describe('claude child tree reaper', () => { expect(terminateDescendants).toHaveBeenCalledWith({ ...refreshed, - // The retained 4243 row was first observed in the earlier displayed - // second. Its per-row boundary must not advance with the refresh. + descendants: [...base.descendants, unseen, refreshed.descendants[1]], capturedAtMsByPid: { - '4243': first.capturedAtMs, - '4244': refreshed.capturedAtMs + // 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 } }) }) 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 9f843527e30..c6bcfc60180 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,17 +167,16 @@ describe('Claude root kill fallback', () => { expect(child.kill).not.toHaveBeenCalled() }) - it('chains per-pid Windows boundaries across a second merge', async () => { - const first = windowsSnapshot(1_000) + it('chains the boundary of a row dropped from later walks', () => { + const first: WindowsDescendantSnapshot = { ...windowsSnapshot(1_000) } const second: WindowsDescendantSnapshot = { ...windowsSnapshot(2_000), - descendants: [ - { pid: 4243, creationTimeMs: 1_700_000_000_000 }, - { pid: 4244, creationTimeMs: 1_700_000_000_002 } - ] + descendants: [{ 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 } @@ -185,6 +184,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': 2_000 }) + expect(rechained?.tree.capturedAtMsByPid).toEqual({ '4243': 1_000, '4244': 3_000 }) }) }) diff --git a/src/main/claude/claude-child-exit-proof-ladder.ts b/src/main/claude/claude-child-exit-proof-ladder.ts index 85ed629f1b9..4c685adb494 100644 --- a/src/main/claude/claude-child-exit-proof-ladder.ts +++ b/src/main/claude/claude-child-exit-proof-ladder.ts @@ -19,6 +19,10 @@ 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 e0955648b02..1bd9f57cda3 100644 --- a/src/main/claude/claude-child-tree-snapshot.ts +++ b/src/main/claude/claude-child-tree-snapshot.ts @@ -10,6 +10,9 @@ 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[], @@ -34,9 +37,11 @@ function mergeRowsByPid( if (prior && !sameIdentity(prior, row)) { return null } - if (!prior) { - capturedAtMsByPid[String(row.pid)] = nextBoundary(row) - } + // 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) 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 new file mode 100644 index 00000000000..5ba1f9fedb0 --- /dev/null +++ b/src/main/claude/claude-descendant-escalation-boundary.test.ts @@ -0,0 +1,161 @@ +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' + +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] + +function tableRows(descendantStartedAt: string): 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 + })) + ] +} + +/** 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 killedPids(calls: [number, NodeJS.Signals][]): number[] { + return calls.flatMap(([pid, signal]) => (signal === 'SIGKILL' ? [pid] : [])) +} + +function signalledPids(calls: [number, NodeJS.Signals][]): number[] { + return calls.flatMap(([pid, signal]) => (signal === 'SIGTERM' ? [pid] : [])) +} + +async function sweep(captures: DescendantSnapshot[]): Promise<[number, NodeJS.Signals][]> { + const calls: [number, NodeJS.Signals][] = [] + const captureDescendants = vi.fn() + for (const capture of captures) { + captureDescendants.mockResolvedValueOnce(capture) + } + const tree = createClaudeChildTreeReaper( + { pid: ROOT_PID, kill: vi.fn(() => true) }, + { + platform: 'linux', + exited: () => false, + captureDescendants, + terminateDescendants: sigtermResistantVerifier((pid, signal) => calls.push([pid, signal])) + } + ) + // The close ladder's shape: arm, then re-walk the live root at the boundary. + await tree.capture() + await tree.refresh?.() + await tree.reap() + 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)]) + + expect(signalledPids(calls)).toEqual(DESCENDANT_PIDS) + expect(killedPids(calls)).toEqual(DESCENDANT_PIDS) + }) + + 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() }) + }) + } + ) + + await tree.capture() + await tree.reap() + + expect(killedPids(calls)).toEqual(DESCENDANT_PIDS) + }) + + 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)]) + + expect(signalledPids(calls)).toEqual(DESCENDANT_PIDS) + expect(killedPids(calls)).toEqual([]) + }) +}) + +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']) + }) +})