diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9f833c400ca9..941b26456390 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -431,6 +431,9 @@ importers: cmdk: specifier: ^1.1.1 version: 1.1.1(@types/react-dom@19.2.7(@types/react@19.2.18))(@types/react@19.2.18)(react-dom@19.2.8(react@19.2.8))(react@19.2.8) + devalue: + specifier: 6.0.2 + version: 6.0.2 diff: specifier: 8.0.4 version: 8.0.4 @@ -4881,6 +4884,10 @@ packages: detect-node@2.1.0: resolution: {integrity: sha512-T0NIuQpnTvFDATNuHN5roPwSBG83rFsuO+MXXH9/3N1eFbn4wcPjttvjMLEPWJ0RGUYgQE7cGgS3tNxbqCGM7g==} + devalue@6.0.2: + resolution: {integrity: sha512-6V1HzU4to74skg//ScZR2mp9bdz+rESC9jQzVIIG/ha0HYgymHcXIQQCjH0+ivRaSuY82fa9fZ7Ro6QZT4s4uw==} + engines: {node: '>=22.17'} + devlop@1.1.0: resolution: {integrity: sha512-RWmIqhcFf1lRYBvNmr7qTNuyCt/7/ns2jbpp1+PalgE/rDQcBT0fioSMUpJ93irlUhC5hrg4cYqe6U+0ImW0rA==} @@ -11836,6 +11843,8 @@ snapshots: detect-node@2.1.0: optional: true + devalue@6.0.2: {} + devlop@1.1.0: dependencies: dequal: 2.0.3 diff --git a/src/main/acp/acp-agent-connection.test.ts b/src/main/acp/acp-agent-connection.test.ts index 46fad1e969f2..7c7f0015caea 100644 --- a/src/main/acp/acp-agent-connection.test.ts +++ b/src/main/acp/acp-agent-connection.test.ts @@ -2,7 +2,7 @@ import { EventEmitter } from 'node:events' import { PassThrough } from 'node:stream' import { afterEach, describe, expect, it, vi } from 'vitest' import type { spawnProcess } from '../../shared/child-process/run-process' -import { PROVIDER_SUPERVISOR_MAX_STOP_MS } from '../provider-process/provider-process-supervisor' +import { SUPERVISED_PROVIDER_GRACEFUL_EXIT_MS } from '../provider-process/provider-process-supervisor' import { ROOT_ONLY_GRACEFUL_EXIT_MS } from '../provider-process/provider-process-close' import type { terminateProviderProcessTree } from '../provider-process/provider-process-teardown' import { @@ -12,8 +12,9 @@ import { } from './acp-agent-connection' import { AcpScriptedAgent, deferred, tick } from './acp-scripted-agent.test-support' +const UNOBSERVED = { tree: 'unverifiable', providerKilled: false } as const const teardown = vi.hoisted(() => - vi.fn(async () => 'unverifiable') + vi.fn(async () => UNOBSERVED) ) vi.mock('../provider-process/provider-process-teardown', () => ({ terminateProviderProcessTree: teardown @@ -21,7 +22,7 @@ vi.mock('../provider-process/provider-process-teardown', () => ({ const opened: { connection: AcpAgentConnection; exit: () => void }[] = [] const grace = - process.platform === 'win32' ? ROOT_ONLY_GRACEFUL_EXIT_MS : PROVIDER_SUPERVISOR_MAX_STOP_MS + process.platform === 'win32' ? ROOT_ONLY_GRACEFUL_EXIT_MS : SUPERVISED_PROVIDER_GRACEFUL_EXIT_MS const start = { cwd: '/execution-host/folder', mcpServers: [] } const prompt = [{ type: 'text', text: 'hello' }] as const @@ -66,7 +67,7 @@ afterEach(async () => { exit() await connection.close() } - teardown.mockReset().mockResolvedValue('unverifiable') + teardown.mockReset().mockResolvedValue(UNOBSERVED) vi.useRealTimers() }) @@ -93,7 +94,11 @@ describe('ACP process-owning connection', () => { connection.resumeReading() expect(child.stdout.isPaused()).toBe(false) expect(await connection.close()).toBe(true) - expect(connection.lastCloseResult).toEqual({ root: 'exited', tree: null }) + expect(connection.lastCloseResult).toEqual({ + root: 'exited', + tree: null, + providerKilled: false + }) }) it('settles prompts and permission signals on proven exit with stdout still open', async () => { @@ -187,7 +192,7 @@ describe('ACP process-owning connection', () => { expect(teardown).toHaveBeenCalledOnce() teardown.mockImplementationOnce(async () => { child.emit('exit', null, 'SIGKILL') - return 'unverifiable' + return UNOBSERVED }) const retry = connection.close() await vi.advanceTimersByTimeAsync(grace + 1_000) @@ -203,7 +208,7 @@ describe('ACP process-owning connection', () => { async (tree) => { vi.useFakeTimers() const { connection, child } = fixture({}, { exitOnEnd: false }) - teardown.mockResolvedValueOnce(tree) + teardown.mockResolvedValueOnce({ tree, providerKilled: false }) const close = connection.close() await vi.advanceTimersByTimeAsync(grace + 1_000) expect(await close).toBe(false) @@ -211,7 +216,7 @@ describe('ACP process-owning connection', () => { expect(connection.processTreeUnproven).toBe(true) expect(await connection.close()).toBe(true) expect(connection.processTreeUnproven).toBe(true) - expect(connection.lastCloseResult).toEqual({ root: 'live', tree }) + expect(connection.lastCloseResult).toEqual({ root: 'live', tree, providerKilled: false }) expect(teardown).toHaveBeenCalledOnce() } ) diff --git a/src/main/claude/claude-agent-sdk-exit-proof-supervised.test.ts b/src/main/claude/claude-agent-sdk-exit-proof-supervised.test.ts new file mode 100644 index 000000000000..9523af868e60 --- /dev/null +++ b/src/main/claude/claude-agent-sdk-exit-proof-supervised.test.ts @@ -0,0 +1,128 @@ +// The Claude reaper's forced step, per platform: on POSIX the root is the provider supervisor and +// Claude leads a group of its own; on Windows the root is Claude. + +import type { ChildProcess } from 'node:child_process' +import { describe, expect, it, vi } from 'vitest' +import type { DescendantTreeVerdict } from '../pty-descendant-exit-verification' +import type { DescendantSnapshot } from '../pty-descendant-termination' +import type { WindowsDescendantSnapshot } from '../windows-descendant-exit-verification' +import { createClaudeChildTreeReaper } from './claude-agent-sdk-exit-proof' + +const SUPERVISOR = 424_242 +const CLAUDE = 424_243 + +/** The supervisor's walk: Claude leads its own group, with one tool in it. */ +function supervisorWalk(): DescendantSnapshot { + const started = 'Mon Jan 1 00:00:00 2026' + return { + root: { pid: SUPERVISOR, startedAt: started }, + rootPgid: SUPERVISOR, + descendants: [ + { pid: CLAUDE, ppid: SUPERVISOR, pgid: CLAUDE, startedAt: started }, + { pid: CLAUDE + 1, ppid: CLAUDE, pgid: CLAUDE, startedAt: started } + ], + capturedAtMs: 1 + } +} + +function root(kills: string[], delivered = true) { + return { + pid: SUPERVISOR, + kill: vi.fn((signal) => { + kills.push(`root ${String(signal)}`) + return delivered + }) + } +} + +describe('claude reaper forced step on a supervised POSIX root', () => { + it("kills Claude's own group before the supervisor, then verifies the rest of the tree", async () => { + const kills: string[] = [] + const verify = Promise.withResolvers() + const terminateDescendants = vi.fn(() => { + kills.push('verify') + return verify.promise + }) + const tree = createClaudeChildTreeReaper(root(kills), { + platform: 'darwin', + supervised: true, + captureDescendants: async () => supervisorWalk(), + terminateDescendants, + signalProcessGroup: (pgid, signal) => kills.push(`group ${pgid} ${signal}`) + }) + + const reaped = tree.reap() + await vi.waitFor(() => expect(terminateDescendants).toHaveBeenCalled()) + expect(tree.providerKilled).toBe(true) + verify.resolve('exited') + + await expect(reaped).resolves.toBe('exited') + expect(kills).toEqual(['root SIGSTOP', `group ${CLAUDE} SIGKILL`, 'root SIGKILL', 'verify']) + }) + + // Before: no snapshot meant a SIGKILL to the supervisor alone, orphaning Claude in its own group + // with nobody left to stop it, and the root's exit then read as Claude gone. + it('never kills the supervisor alone when the process table cannot be read', async () => { + const kills: string[] = [] + const signalProcessGroup = vi.fn() + const tree = createClaudeChildTreeReaper(root(kills), { + platform: 'linux', + supervised: true, + captureDescendants: async () => null, + signalProcessGroup + }) + + await expect(tree.reap()).resolves.toBe('unverifiable') + expect(kills).toEqual(['root SIGSTOP', 'root SIGCONT']) + expect(signalProcessGroup).not.toHaveBeenCalled() + expect(tree.providerKilled).toBe(false) + }) + + it('signals nothing once the supervised root has exited, and only verifies the tree', async () => { + const kills: string[] = [] + let exited = false + const terminateDescendants = vi.fn(async () => 'exited' as const) + const tree = createClaudeChildTreeReaper(root(kills), { + platform: 'darwin', + supervised: true, + exited: () => exited, + captureDescendants: async () => supervisorWalk(), + terminateDescendants + }) + await tree.capture() + exited = true + + await expect(tree.reap()).resolves.toBe('exited') + expect(kills).toEqual([]) + expect(tree.providerKilled).toBe(false) + }) +}) + +describe('claude reaper forced step on Windows, where the root is Claude', () => { + function windowsWalk(): WindowsDescendantSnapshot { + return { + root: { pid: SUPERVISOR, creationTimeMs: 1_700_000_000_001 }, + descendants: [], + unidentifiedCount: 0, + capturedAtMs: 1 + } + } + + it.each([ + [true, 'delivered'], + [false, 'not delivered'] + ])('reads providerKilled=%s from a TerminateProcess %s', async (delivered) => { + const kills: string[] = [] + const tree = createClaudeChildTreeReaper(root(kills, delivered), { + platform: 'win32', + verifyRootIdentity: async () => true, + captureWindowsDescendants: async () => windowsWalk(), + terminateWindowsTree: async () => undefined, + terminateWindowsDescendants: async () => 'exited' + }) + + await tree.reap() + expect(kills).toEqual(['root SIGKILL']) + expect(tree.providerKilled).toBe(delivered) + }) +}) 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 e27e74046d60..9497a2105c45 100644 --- a/src/main/claude/claude-agent-sdk-exit-proof.test.ts +++ b/src/main/claude/claude-agent-sdk-exit-proof.test.ts @@ -144,7 +144,8 @@ function mockTree(verdicts: DescendantTreeVerdict[]): ClaudeChildTreeReaper & { }), get treeVerdict() { return treeVerdict - } + }, + providerKilled: false } } diff --git a/src/main/claude/claude-agent-sdk-exit-proof.ts b/src/main/claude/claude-agent-sdk-exit-proof.ts index 82db647cb5a7..6db95f51d9e5 100644 --- a/src/main/claude/claude-agent-sdk-exit-proof.ts +++ b/src/main/claude/claude-agent-sdk-exit-proof.ts @@ -18,6 +18,7 @@ import { } from '../windows-descendant-exit-verification' import { mergeClaudeCapturedTrees, type ClaudeCapturedTree } from './claude-child-tree-snapshot' import { terminateClaudeRoot, terminateClaudeWindowsRoot } from './claude-child-root-termination' +import { killSupervisedProviderGroup } from '../provider-process/provider-supervised-group-kill' import { proveClaudeChildExitWithReaper, type ClaudeChildExitProofInput @@ -61,6 +62,9 @@ export type ClaudeChildTreeReaperDeps = { platform?: NodeJS.Platform /** Whether the root's exit has been observed; only a live root can be walked. */ exited?: () => boolean + /** The root is the POSIX provider supervisor, whose provider runs in a group of its own. */ + supervised?: boolean + signalProcessGroup?: (pgid: number, signal: NodeJS.Signals) => void captureDescendants?: (rootPid: number) => Promise terminateDescendants?: (snapshot: DescendantSnapshot) => Promise terminateWindowsTree?: (root: WindowsProcessIdentity) => Promise @@ -93,6 +97,8 @@ export type ClaudeChildTreeReaper = { * which no later caller may collapse into "unknown". */ readonly treeVerdict: DescendantTreeVerdict + /** Latched once a reap's kill reached Claude itself; never an observed exit. */ + readonly providerKilled: boolean } /** @@ -119,6 +125,7 @@ export function createClaudeChildTreeReaper( let queuedRefresh: Promise | null = null let inFlight: Promise | null = null let treeVerdict: DescendantTreeVerdict = 'unverifiable' + let providerKilled = false // Consulted only on win32: POSIX signals descendants by revalidated identity // and reaches the root solely through Node's handle, so neither needs a probe. @@ -262,7 +269,10 @@ export function createClaudeChildTreeReaper( /** The only source of a tree verdict: every `exited` here is an observation. */ async function judgeTree(): Promise { - const killRoot = (): boolean => terminateClaudeRoot({ child, exited }) + // Only an unsupervised root is Claude itself; a supervisor is killed by the forced step below. + const killRoot = (): void => { + providerKilled ||= terminateClaudeRoot({ child, exited }) + } const rootPid = child.pid if (!rootPid) { // Never spawned, so the OS never created a tree to orphan. @@ -272,7 +282,7 @@ export function createClaudeChildTreeReaper( if (platform === 'win32') { // Why taskkill's own outcome is never the verdict: it resolves identically // on a timeout, an access denial, a recycled root and a real kill. - const { rootVerified } = await terminateClaudeWindowsRoot({ + const { rootVerified, rootKilled } = await terminateClaudeWindowsRoot({ snapshot: snapshot?.platform === 'win32' ? snapshot.tree : null, exited, verifyRoot: (root) => verifyRoot(root), @@ -282,8 +292,10 @@ export function createClaudeChildTreeReaper( : terminateIdentifiedWindowsProcessTree(root, { ownsRoot: () => !exited() }).then(() => undefined), - killRoot + // Windows runs Claude without a supervisor: the root is Claude. + killRoot: () => terminateClaudeRoot({ child, exited }) }) + providerKilled ||= rootKilled if (!rootVerified && !exited()) { return 'unverifiable' } @@ -293,30 +305,44 @@ export function createClaudeChildTreeReaper( ) : 'unverifiable' } + if (deps.supervised && !exited()) { + // Killing the supervisor alone would orphan Claude in its own group; this kills that group + // first, and leaves the supervisor running its own stop when the group cannot be named. + const forced = await killSupervisedProviderGroup(child, rootPid, { + site: 'claude-provider-teardown', + ...(deps.captureDescendants ? { captureDescendants: deps.captureDescendants } : {}), + ...(deps.signalProcessGroup ? { signalProcessGroup: deps.signalProcessGroup } : {}) + }) + providerKilled ||= forced.provider !== 'unknown' + } if (snapshot?.platform !== 'posix') { - killRoot() + if (!deps.supervised) { + killRoot() + } return 'unverifiable' } if (snapshot.tree.descendants.length === 0) { // Read while the root was alive and childless: a later table read has no // row it could match, so it would add nothing to this observation. - killRoot() + if (!deps.supervised) { + killRoot() + } return 'exited' } - // Why the root is killed while verification is already running, and never - // SIGSTOPped first the way the Codex non-group path does: measured on macOS, a - // killed child of a stopped parent stays a zombie row in ps with its lstart - // and pgid intact, so verification cannot pass until the root is dead. The - // descendants are signalled by the verifier as soon as it revalidates their - // identities; the root's death then reparents any zombies to init, which - // reaps them. After a root exit the kill is a no-op: Node drops the handle - // on exit and never signals a possibly recycled pid. + // Why an unsupervised root is killed while verification is already running: measured on + // macOS, a killed child of a stopped or living parent that has not reaped it stays a zombie row + // in ps with its lstart and pgid intact, so verification cannot pass until the root is dead. + // A supervised root is already dead here when the forced step killed Claude's group. Either + // way the root's death reparents zombies to init, which reaps them. After a root exit the + // kill is a no-op: Node drops the handle on exit and never signals a possibly recycled pid. const verdictPromise = deps.terminateDescendants ? deps.terminateDescendants(snapshot.tree) : terminateDescendantSnapshotWithVerdict(snapshot.tree, { requireIdentityBeforeSignal: true }) - killRoot() + if (!deps.supervised) { + killRoot() + } // What the verification observed is the verdict: a kill that reports no // signal means the handle was already gone, never that the tree survived. return verdictPromise @@ -346,6 +372,9 @@ export function createClaudeChildTreeReaper( }, get treeVerdict() { return treeVerdict + }, + get providerKilled() { + return providerKilled } } } @@ -362,7 +391,8 @@ export function createClaudeChildTreeReaper( export function proveClaudeChildExit(input: ClaudeChildExitProofInput): Promise { return proveClaudeChildExitWithReaper(input, () => createClaudeChildTreeReaper(input.managed.child, { - exited: () => input.managed.rootVerdict === 'exited' + exited: () => input.managed.rootVerdict === 'exited', + supervised: input.managed.supervised }) ) } diff --git a/src/main/claude/claude-agent-sdk-process-spawn.test.ts b/src/main/claude/claude-agent-sdk-process-spawn.test.ts index cadfb056a30d..79d55ca8e593 100644 --- a/src/main/claude/claude-agent-sdk-process-spawn.test.ts +++ b/src/main/claude/claude-agent-sdk-process-spawn.test.ts @@ -132,7 +132,8 @@ describe('claude agent SDK process spawn', () => { const tree = { capture: vi.fn(async () => {}), reap: vi.fn(async () => 'exited' as const), - treeVerdict: 'exited' as const + treeVerdict: 'exited' as const, + providerKilled: false } await expect(proveClaudeChildExitWithReaper({ managed, tree }, () => tree)).resolves.toBe( true diff --git a/src/main/claude/claude-child-exit-proof-ladder.test.ts b/src/main/claude/claude-child-exit-proof-ladder.test.ts index e327d361d58d..961b7a570ae8 100644 --- a/src/main/claude/claude-child-exit-proof-ladder.test.ts +++ b/src/main/claude/claude-child-exit-proof-ladder.test.ts @@ -12,7 +12,8 @@ function fakeTree(): ClaudeChildTreeReaper & { reap: ReturnType } capture: vi.fn(async () => {}), refresh: vi.fn(async () => {}), reap: vi.fn(async () => 'exited' as const), - treeVerdict: 'exited' + treeVerdict: 'exited', + providerKilled: false } } @@ -59,7 +60,11 @@ describe('Claude child exit proof ladder', () => { await expect(proof).resolves.toBe(true) expect(root.child.stdin.writableEnded).toBe(true) expect(root.child.kill).toHaveBeenCalledExactlyOnceWith('SIGTERM') - expect(root.managed.lastCloseResult).toEqual({ root: 'exited', tree: 'exited' }) + expect(root.managed.lastCloseResult).toEqual({ + root: 'exited', + tree: 'exited', + providerKilled: false + }) expect(tree.reap).not.toHaveBeenCalled() expect(vi.getTimerCount()).toBe(0) }) @@ -74,7 +79,11 @@ describe('Claude child exit proof ladder', () => { expect(root.child.stdin.writableEnded).toBe(true) expect(root.child.kill).not.toHaveBeenCalledWith('SIGTERM') expect(tree.reap).toHaveBeenCalledOnce() - expect(root.managed.lastCloseResult).toEqual({ root: 'live', tree: 'exited' }) + expect(root.managed.lastCloseResult).toEqual({ + root: 'live', + tree: 'exited', + providerKilled: false + }) expect(vi.getTimerCount()).toBe(0) }) }) diff --git a/src/main/claude/claude-child-exit-proof-ladder.ts b/src/main/claude/claude-child-exit-proof-ladder.ts index a3f1de842364..2b8b9759c6a6 100644 --- a/src/main/claude/claude-child-exit-proof-ladder.ts +++ b/src/main/claude/claude-child-exit-proof-ladder.ts @@ -1,11 +1,11 @@ import type { ManagedProviderProcess } from '../provider-process/managed-provider-process' -import { PROVIDER_SUPERVISOR_MAX_STOP_MS } from '../provider-process/provider-process-supervisor' +import { SUPERVISED_PROVIDER_GRACEFUL_EXIT_MS } from '../provider-process/provider-process-supervisor' import type { ProviderProcessClosePolicy } from '../provider-process/provider-process-close' import type { ClaudeChildTreeReaper } from './claude-agent-sdk-exit-proof' export const GRACEFUL_EXIT_MS = 1_500 // A signalled supervisor escalates on its own; forcing it sooner kills it and orphans Claude. -export const SUPERVISED_GRACEFUL_EXIT_MS = PROVIDER_SUPERVISOR_MAX_STOP_MS + 500 +export const SUPERVISED_GRACEFUL_EXIT_MS = SUPERVISED_PROVIDER_GRACEFUL_EXIT_MS const FORCED_EXIT_MS = 1_000 export function claudeChildClosePolicy(supervised: boolean): ProviderProcessClosePolicy { diff --git a/src/main/claude/claude-child-root-termination.ts b/src/main/claude/claude-child-root-termination.ts index bba299195789..d88bc7826745 100644 --- a/src/main/claude/claude-child-root-termination.ts +++ b/src/main/claude/claude-child-root-termination.ts @@ -19,8 +19,8 @@ type RootTerminationInput = { * nothing. A probe here could only let an unreadable process table cost the tree * the one fallback that still works once every table read has failed. * - * On POSIX the root is the provider supervisor, killed only after its own stop had - * its whole bound; Claude, in its own group, is reached by the descendant kill. + * Used only for a root that is Claude itself. A POSIX supervisor is killed by the reaper's + * forced step, after its own stop had its whole bound and only after Claude's own group. * * False means no signal was sent, because the root had already left. */ @@ -43,7 +43,7 @@ type WindowsRootTerminationInput = { */ export async function terminateClaudeWindowsRoot( input: WindowsRootTerminationInput -): Promise<{ rootVerified: boolean }> { +): Promise<{ rootVerified: boolean; rootKilled: boolean }> { const { snapshot, exited, verifyRoot, terminateTree, killRoot } = input let rootVerified = false if (!exited() && snapshot) { @@ -52,6 +52,5 @@ export async function terminateClaudeWindowsRoot( await terminateTree(snapshot.root).catch(() => {}) } } - killRoot() - return { rootVerified } + return { rootVerified, rootKilled: killRoot() } } diff --git a/src/main/claude/claude-stream-json-connection.ts b/src/main/claude/claude-stream-json-connection.ts index c9f5ee2689d1..da7d0a45710f 100644 --- a/src/main/claude/claude-stream-json-connection.ts +++ b/src/main/claude/claude-stream-json-connection.ts @@ -86,6 +86,8 @@ export type ClaudeChildExitVerdict = { root: DescendantTreeVerdict tree: DescendantTreeVerdict processless?: boolean + /** A close's kill reached Claude itself, so it cannot write again even before its exit is seen. */ + providerKilled?: true } export type ClaudeStreamJsonConnection = ClaudeControlSurface & { @@ -185,7 +187,10 @@ export async function openClaudeStreamJsonConnection( const waitUntilReadable = (): Promise => readingBarrier ?? Promise.resolve() // One reaper per child: every close attempt and error-path reap shares its proof. const rootSettled = (): boolean => managed.rootVerdict === 'exited' - const tree = createClaudeChildTreeReaper(child, { exited: rootSettled }) + const tree = createClaudeChildTreeReaper(child, { + exited: rootSettled, + supervised: managed.supervised + }) // Arm lazily on actual child output instead of issuing a process-table scan for // every session at startup. A natural SDK exit can race a later close, while @@ -349,7 +354,8 @@ export async function openClaudeStreamJsonConnection( return { root: managed.rootVerdict, tree: tree.treeVerdict, - ...(managed.processless ? { processless: true } : {}) + ...(managed.processless ? { processless: true } : {}), + ...(tree.providerKilled ? { providerKilled: true as const } : {}) } as const }, pauseReading, diff --git a/src/main/claude/claude-structured-acquisition-launch.ts b/src/main/claude/claude-structured-acquisition-launch.ts index 4de526c7c4db..02d58652b642 100644 --- a/src/main/claude/claude-structured-acquisition-launch.ts +++ b/src/main/claude/claude-structured-acquisition-launch.ts @@ -45,7 +45,7 @@ export async function resolveClaudeAcquisitionLaunch(args: { const closed = await stopAgentSessionProviderRoot(() => closeClaudePublishedSessionForDeps(sessions, sessionId, deps) ) - if (!closed) { + if (closed === 'unproven') { throw new AgentSessionAcquisitionExitUnprovenError( new Error(`claude session ${sessionId} could not be stopped`) ) diff --git a/src/main/claude/claude-structured-session-close.ts b/src/main/claude/claude-structured-session-close.ts index 3093d2ead0da..b52b4d7666d8 100644 --- a/src/main/claude/claude-structured-session-close.ts +++ b/src/main/claude/claude-structured-session-close.ts @@ -10,7 +10,8 @@ import { AgentSessionAcquisitionExitProvenError, AgentSessionAcquisitionExitUnprovenError, AgentSessionAcquisitionRootExitObservedError, - AgentSessionPreSpawnError + AgentSessionPreSpawnError, + AgentSessionProviderKilledError } from '../native-chat/agent-session-wire/structured-agent-session-adapter' import type { ClaudeStreamJsonConnection } from './claude-stream-json-connection' import type { ClaudeJournalTranslator } from './claude-journal-translator-contract' @@ -38,8 +39,12 @@ export function claudeAcquisitionCleanupError( if (verdict?.processless === true) { return new AgentSessionPreSpawnError(cause) } - return claudeRootExitObserved(connection) - ? new AgentSessionAcquisitionRootExitObservedError(cause) + if (claudeRootExitObserved(connection)) { + return new AgentSessionAcquisitionRootExitObservedError(cause) + } + // The kill reached Claude itself: it cannot write again, though its exit was not seen yet. + return verdict?.providerKilled === true + ? new AgentSessionProviderKilledError(cause) : new AgentSessionAcquisitionExitUnprovenError(cause) } @@ -110,9 +115,10 @@ async function finalizeClaudePublishedSession( session.connection, new Error('provider close unproven') ) - // Only a genuinely unknown exit stays indexed for a retry. A proven root exit or processless - // close is final — the owner releases the lease on it — so the session finalizes like a proven - // close and still reports the verdict; kept indexed, it refused every later start of the chat. + // Only a genuinely unknown exit stays indexed for a retry. A proven root exit, a processless + // close or a kill that reached Claude is final — the owner releases the lease on it — so the + // session finalizes like a proven close and still reports the verdict; kept indexed, it refused + // every later start of the chat. if (cleanupError instanceof AgentSessionAcquisitionExitUnprovenError) { return false } @@ -126,8 +132,9 @@ async function finalizeClaudePublishedSession( } session.childWork.clear() session.backgroundTasks.clear() - // The exit is proven, so the session ends now. Saving its resume point is bookkeeping that - // follows, reported on failure; it never holds the close or reads as an unproven exit. + // The exit is proven, or Claude killed, so the session ends now. Saving its resume point is + // bookkeeping that follows, reported on failure; it never holds the close or reads as an + // unproven exit. session.closeFinalized = true input.sessions.delete(input.sessionId) let callbackError: unknown @@ -150,7 +157,10 @@ async function finalizeClaudePublishedSession( cause: 'requested-close', fence: session.fence, acquisitionGeneration: session.acquisitionGeneration, - observedAt: Date.now() + observedAt: Date.now(), + ...(rootExitVerdict instanceof AgentSessionProviderKilledError + ? { rootExitUnobserved: true as const } + : {}) } as const try { try { diff --git a/src/main/claude/claude-supervised-stop.integration.test.ts b/src/main/claude/claude-supervised-stop.integration.test.ts index c3dcb0087496..c7e500e5e39a 100644 --- a/src/main/claude/claude-supervised-stop.integration.test.ts +++ b/src/main/claude/claude-supervised-stop.integration.test.ts @@ -171,7 +171,11 @@ describe.runIf(process.platform !== 'win32')('Claude under the POSIX provider su const startedAt = Date.now() await expect(close()).resolves.toBe(true) - expect(managed.lastCloseResult).toEqual({ root: 'exited', tree: 'exited' }) + expect(managed.lastCloseResult).toEqual({ + root: 'exited', + tree: 'exited', + providerKilled: false + }) await expect(close()).resolves.toBe(true) // Claude's own SIGTERM reap ran at once, not after the supervisor's stdin-end grace. @@ -189,7 +193,11 @@ describe.runIf(process.platform !== 'win32')('Claude under the POSIX provider su const startedAt = Date.now() await expect(close()).resolves.toBe(true) - expect(managed.lastCloseResult).toEqual({ root: 'exited', tree: 'exited' }) + expect(managed.lastCloseResult).toEqual({ + root: 'exited', + tree: 'exited', + providerKilled: false + }) await expect(close()).resolves.toBe(true) const elapsed = Date.now() - startedAt diff --git a/src/main/codex/codex-app-server-connection-tree-unproven.test.ts b/src/main/codex/codex-app-server-connection-tree-unproven.test.ts index 0e0cca283e87..023d1b8d173d 100644 --- a/src/main/codex/codex-app-server-connection-tree-unproven.test.ts +++ b/src/main/codex/codex-app-server-connection-tree-unproven.test.ts @@ -7,7 +7,7 @@ import { openCodexAppServerConnection } from './codex-app-server-connection' const teardown = vi.hoisted(() => { const state: { verdict: ProviderProcessTeardownVerdict; rootExits: () => void } = { - verdict: null, + verdict: { tree: null, providerKilled: false }, rootExits: () => {} } return state @@ -48,7 +48,7 @@ describe('Codex process-tree diagnostic after a forced close', () => { [null, false] ] as const)('teardown observing %s reads unproven=%s', async (verdict, unproven) => { vi.useFakeTimers() - teardown.verdict = verdict + teardown.verdict = { tree: verdict, providerKilled: false } const { child, spawnImpl } = stubChild() teardown.rootExits = () => child.emit('exit', null, 'SIGKILL') const connection = await openCodexAppServerConnection( @@ -64,4 +64,25 @@ describe('Codex process-tree diagnostic after a forced close', () => { await expect(connection.close()).resolves.toBe(true) expect(connection.processTreeUnproven).toBe(unproven) }) + + it('reports a kill that reached the app-server until its exit is seen', async () => { + vi.useFakeTimers() + teardown.verdict = { tree: null, providerKilled: true } + const { child, spawnImpl } = stubChild() + teardown.rootExits = () => {} + const connection = await openCodexAppServerConnection( + { command: 'codex', args: ['app-server'] }, + {}, + spawnImpl + ) + const closing = connection.close() + await vi.advanceTimersByTimeAsync(10_000) + await expect(closing).resolves.toBe(false) + expect(connection.providerKilled).toBe(true) + + child.emit('exit', null, 'SIGKILL') + // Seen at last, the exit is the answer; the kill no longer stands in for it. + expect(connection.providerKilled).toBe(false) + await expect(connection.close()).resolves.toBe(true) + }) }) diff --git a/src/main/codex/codex-app-server-connection-types.ts b/src/main/codex/codex-app-server-connection-types.ts index bf33bc5d0b80..3c8d7b6b580e 100644 --- a/src/main/codex/codex-app-server-connection-types.ts +++ b/src/main/codex/codex-app-server-connection-types.ts @@ -34,4 +34,7 @@ export type CodexAppServerConnection = { close: () => Promise /** The root exited, but the forced kill of its process tree could not prove the tree gone. */ readonly processTreeUnproven?: boolean + /** The last close's kill reached the app-server itself and its exit has not been seen: it + * cannot write again, so its thread may be resumed elsewhere. */ + readonly providerKilled?: boolean } diff --git a/src/main/codex/codex-app-server-connection.test.ts b/src/main/codex/codex-app-server-connection.test.ts index 3e41a821dc65..ab9225c5c761 100644 --- a/src/main/codex/codex-app-server-connection.test.ts +++ b/src/main/codex/codex-app-server-connection.test.ts @@ -11,11 +11,34 @@ import { type CodexAppServerConnection, type CodexAppServerConnectionHandlers } from './codex-app-server-connection' -import { PROVIDER_SUPERVISOR_MAX_STOP_MS } from '../provider-process/provider-process-supervisor' +import { SUPERVISED_PROVIDER_GRACEFUL_EXIT_MS } from '../provider-process/provider-process-supervisor' import { isCodexAppServerUnsupportedError } from './codex-app-server-session' +import type * as PtyDescendantTermination from '../pty-descendant-termination' +import type * as PtyDescendantExitVerification from '../pty-descendant-exit-verification' // close() waits out the supervisor's own stop before forcing the tree. -const GRACEFUL_EXIT_MS = process.platform === 'win32' ? 1_500 : PROVIDER_SUPERVISOR_MAX_STOP_MS +const GRACEFUL_EXIT_MS = process.platform === 'win32' ? 1_500 : SUPERVISED_PROVIDER_GRACEFUL_EXIT_MS + +// A stub root is in no process table, so the forced step reads it as the paused supervisor of a +// provider whose group is already empty (an unreachable pgid answers the real group signal ESRCH). +const processTable = vi.hoisted(() => ({ + walk: (rootPid: number) => ({ + root: { pid: rootPid, startedAt: 'Mon Oct 5 09:59:00 2026' }, + rootPgid: rootPid, + descendants: [ + { pid: 2_147_483_647, ppid: rootPid, pgid: 2_147_483_647, startedAt: 'Mon Oct 5' } + ], + capturedAtMs: 1 + }) +})) +vi.mock('../pty-descendant-termination', async (importOriginal) => ({ + ...(await importOriginal()), + captureDescendantSnapshot: async (rootPid: number) => processTable.walk(rootPid) +})) +vi.mock('../pty-descendant-exit-verification', async (importOriginal) => ({ + ...(await importOriginal()), + terminateDescendantSnapshotWithVerdict: async () => 'exited' +})) const originalCodexHome = process.env.CODEX_HOME @@ -98,7 +121,7 @@ function stubChild(options: { exitOnStdinEnd?: boolean } = {}): { // Keep the synthetic pid outside any real process table so teardown never // mistakes an unrelated process for this stub. child.pid = 9_999_999 - child.kill = vi.fn() + child.kill = vi.fn(() => true) const written: Record[] = [] child.stdin.on('data', (chunk: Buffer) => { for (const line of chunk.toString('utf8').split('\n')) { @@ -490,6 +513,7 @@ describe('openCodexAppServerConnection', () => { if (signal === 'SIGKILL') { resolve() } + return true }) }) const closing = connection.close() diff --git a/src/main/codex/codex-app-server-connection.ts b/src/main/codex/codex-app-server-connection.ts index 0739ca65af69..4ea3e147ed7f 100644 --- a/src/main/codex/codex-app-server-connection.ts +++ b/src/main/codex/codex-app-server-connection.ts @@ -209,6 +209,9 @@ export async function openCodexAppServerConnection( get closed() { return closing || managed.rootVerdict === 'exited' || terminalError !== null }, + get providerKilled() { + return managed.rootVerdict !== 'exited' && managed.lastCloseResult?.providerKilled === true + }, get processTreeUnproven() { const tree = managed.lastCloseResult?.tree return ( diff --git a/src/main/codex/codex-structured-acquisition-lifecycle.ts b/src/main/codex/codex-structured-acquisition-lifecycle.ts index 740f95bce2d1..a4ebd24df8de 100644 --- a/src/main/codex/codex-structured-acquisition-lifecycle.ts +++ b/src/main/codex/codex-structured-acquisition-lifecycle.ts @@ -1,4 +1,7 @@ -import { AgentSessionAcquisitionExitUnprovenError } from '../native-chat/agent-session-wire/structured-agent-session-adapter' +import { + AgentSessionAcquisitionExitUnprovenError, + AgentSessionProviderKilledError +} from '../native-chat/agent-session-wire/structured-agent-session-adapter' import { isCodexAppServerHandshakeExitUnprovenError } from './codex-app-server-handshake-exit-proof' import { cancelCodexAcquisitionAttempt, @@ -38,11 +41,18 @@ export async function closeFailedCodexAcquisition(input: { } input.dispose() try { - if (!(await input.registry.closeFailedAttempt(input.sessionId, input.attempt))) { + const closed = await input.registry.closeFailedAttempt(input.sessionId, input.attempt) + if (closed === 'unproven') { throw new AgentSessionAcquisitionExitUnprovenError(input.cause) } + if (closed === 'killed') { + throw new AgentSessionProviderKilledError(input.cause) + } } catch (cleanupError) { - if (cleanupError instanceof AgentSessionAcquisitionExitUnprovenError) { + if ( + cleanupError instanceof AgentSessionAcquisitionExitUnprovenError || + cleanupError instanceof AgentSessionProviderKilledError + ) { throw cleanupError } throw new AgentSessionAcquisitionExitUnprovenError( diff --git a/src/main/codex/codex-structured-session-close.test.ts b/src/main/codex/codex-structured-session-close.test.ts index 6d0d9ad74195..bcd07a346e8f 100644 --- a/src/main/codex/codex-structured-session-close.test.ts +++ b/src/main/codex/codex-structured-session-close.test.ts @@ -15,7 +15,10 @@ import { handleCodexSessionExit } from './codex-structured-session-close' import { CodexBackgroundTaskTracker } from './codex-background-task-tracker' import { CodexPromptRegistry } from './codex-structured-prompt-replies' import type { CodexSession } from './codex-structured-session-state' -import type { StructuredAgentSessionAdapter } from '../native-chat/agent-session-wire/structured-agent-session-adapter' +import { + AgentSessionProviderKilledError, + type StructuredAgentSessionAdapter +} from '../native-chat/agent-session-wire/structured-agent-session-adapter' import { claudeAndCodexRouter } from '../native-chat/agent-session-wire/structured-agent-session-adapter-router-test-support' import { codexProviderHandle } from '../../shared/agent-session-provider-handle-encoding' @@ -231,4 +234,32 @@ describe('Codex structured session close lifecycle', () => { { cause: 'unexpected-exit', reason: 'journal sink failed', fence: 7 } ]) }) + + it("ends a session whose close killed the app-server, though its exit wasn't seen", async () => { + const { adapter, connections, events } = adapterFixture() + await adapter.acquire({ identity: identity('session-1'), fence: 7, spawnToken: 'spawn-1' }) + const current = connections[0]! + Object.assign(current.connection, { providerKilled: true, close: async () => false }) + + await expect(adapter.closeSession('session-1')).rejects.toBeInstanceOf( + AgentSessionProviderKilledError + ) + expect(events.filter((event) => event.type === 'ended')).toMatchObject([ + { cause: 'requested-close', fence: 7, rootExitUnobserved: true } + ]) + // The late exit of that process ends nothing: the session already left. + current.handlers.onExit?.(new Error('codex exited'), { expected: true }) + expect(events.filter((event) => event.type === 'ended')).toHaveLength(1) + // A later close has nothing left to stop. + await expect(adapter.closeSession('session-1')).resolves.toBe(true) + }) + + it('keeps a session whose close neither saw the exit nor reached the app-server', async () => { + const { adapter, connections, events } = adapterFixture() + await adapter.acquire({ identity: identity('session-1'), fence: 7, spawnToken: 'spawn-1' }) + Object.assign(connections[0]!.connection, { close: async () => false }) + + await expect(adapter.closeSession('session-1')).resolves.toBe(false) + expect(events.filter((event) => event.type === 'ended')).toEqual([]) + }) }) diff --git a/src/main/codex/codex-structured-session-close.ts b/src/main/codex/codex-structured-session-close.ts index 465d7801fec9..9035834f9a20 100644 --- a/src/main/codex/codex-structured-session-close.ts +++ b/src/main/codex/codex-structured-session-close.ts @@ -17,6 +17,8 @@ export function handleCodexSessionExit(input: { error: Error /** Set for the end of a close Orca began: by that close, or by the root's own exit report. */ closedByOrca?: true + /** The close's kill reached the app-server, but its exit was not seen. */ + rootExitUnobserved?: true prompts?: CodexSession['prompts'] onEvent?: (event: CodexStructuredSessionEvent) => void logger?: StructuredAgentSessionLogger @@ -41,7 +43,8 @@ export function handleCodexSessionExit(input: { cause: session.orcaClose?.requested ? 'requested-close' : 'unexpected-exit', fence: session.fence, acquisitionGeneration: session.acquisitionGeneration, - observedAt: session.exitObservedAt + observedAt: session.exitObservedAt, + ...(input.rootExitUnobserved ? { rootExitUnobserved: true as const } : {}) } as const // A synchronous sink rejection (usually backpressure) leaves the terminal rows to the host's // exit settlement, which writes its own bounded fallback. The exit itself is observed, so the @@ -97,10 +100,11 @@ export async function closeCodexPublishedSession( requested: options?.requestedClose ?? true, reason: options?.unexpectedReason ?? new Error('codex session closed') } - // Keep the session indexed until the child exit is observed. A timeout or - // failed kill must leave the live connection available for a safe retry. + // Keep the session indexed until the child exit is observed or the kill reached the app-server + // itself. A failed kill must leave the live connection available for a safe retry. const exited = await session.connection.close() - if (exited !== true) { + const killed = exited !== true && session.connection.providerKilled === true + if (exited !== true && !killed) { return false } if (!session.ended) { @@ -110,6 +114,7 @@ export async function closeCodexPublishedSession( connection: session.connection, error: session.orcaClose.reason, closedByOrca: true, + ...(killed ? { rootExitUnobserved: true as const } : {}), prompts: session.prompts, ...(onEvent ? { onEvent } : {}), ...(options?.logger ? { logger: options.logger } : {}) diff --git a/src/main/codex/codex-structured-session-state.ts b/src/main/codex/codex-structured-session-state.ts index 5773b659f5e1..eb39483c2744 100644 --- a/src/main/codex/codex-structured-session-state.ts +++ b/src/main/codex/codex-structured-session-state.ts @@ -253,13 +253,21 @@ export class CodexAcquisitionRegistry { } } - async closeFailedAttempt(sessionId: string, attempt: CodexAcquisitionAttempt): Promise { - const stopped = (await attempt.window.connection?.close()) ?? true + /** `killed`: the close's kill reached the app-server but its exit was not seen. */ + async closeFailedAttempt( + sessionId: string, + attempt: CodexAcquisitionAttempt + ): Promise<'exited' | 'killed' | 'unproven'> { + const connection = attempt.window.connection + const stopped = (await connection?.close()) ?? true if (stopped) { attempt.exitProven = true + } + const verdict = stopped ? 'exited' : connection?.providerKilled ? 'killed' : 'unproven' + if (verdict !== 'unproven') { this.deleteIfCurrent(sessionId, attempt) } - return stopped + return verdict } sessionIds(): IterableIterator { diff --git a/src/main/codex/codex-structured-session-teardown.ts b/src/main/codex/codex-structured-session-teardown.ts index e7395fb3c845..2ba98f4b58fd 100644 --- a/src/main/codex/codex-structured-session-teardown.ts +++ b/src/main/codex/codex-structured-session-teardown.ts @@ -2,9 +2,12 @@ // // Every path funnels through `settled` so the ephemeral surfaces a closed // session owns are cleared exactly once, and only when the child was actually -// proven stopped — a refused close leaves the session indexed for a retry. +// proven stopped or killed — a refused close leaves the session indexed for a retry. -import { AgentSessionAcquisitionRootExitObservedError } from '../native-chat/agent-session-wire/structured-agent-session-adapter' +import { + AgentSessionAcquisitionRootExitObservedError, + AgentSessionProviderKilledError +} from '../native-chat/agent-session-wire/structured-agent-session-adapter' import type { StructuredAgentSessionLogger } from '../native-chat/agent-session-wire/structured-agent-session-logger' import { closeAllCodexSessions, @@ -40,6 +43,12 @@ export class CodexStructuredSessionTeardown { this.deps.logger ) ) + if (closed && connection?.providerKilled) { + // Ended without its exit seen; the host releases the lease with no exit evidence. + throw new AgentSessionProviderKilledError( + new Error("codex app-server was killed, but its exit wasn't seen yet") + ) + } if (closed && connection?.processTreeUnproven) { // The root exited, so the close is proven; its owner reports the children left unconfirmed. throw new AgentSessionAcquisitionRootExitObservedError( diff --git a/src/main/codex/codex-structured-thread-open-active-writer.test.ts b/src/main/codex/codex-structured-thread-open-active-writer.test.ts new file mode 100644 index 000000000000..b9fbb8c1c7c5 --- /dev/null +++ b/src/main/codex/codex-structured-thread-open-active-writer.test.ts @@ -0,0 +1,117 @@ +// A resume that meets Codex's lock on a thread another process still writes: right after Orca +// killed the previous app-server, the OS has not yet dropped that process's lock. + +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + CodexAppServerRequestError, + openCodexAppServerConnection +} from './codex-app-server-connection' +import { + CODEX_ACTIVE_WRITER_RETRY_DELAYS_MS, + openCodexThread +} from './codex-structured-thread-open' + +const THREAD = 'thread-1' + +function activeWriter(threadId = THREAD, code = -32600): CodexAppServerRequestError { + return new CodexAppServerRequestError( + 'thread/resume', + code, + `codex app-server thread/resume failed: thread ${threadId} already has an active writer` + ) +} + +/** Codex refusing the resume `refusals` times, then opening the thread. */ +function codexHeldFor(refusals: number, error: Error = activeWriter()) { + let left = refusals + return vi.fn(async (method: string, _params?: Record) => { + if (method === 'thread/resume' && left > 0) { + left -= 1 + throw error + } + return { thread: { id: THREAD } } + }) +} + +function resume(request: ReturnType) { + return openCodexThread({ request }, { cwd: '/workspace', resumeThreadId: THREAD }, 2_000) +} + +afterEach(() => vi.useRealTimers()) + +describe('openCodexThread after the previous writer', () => { + it('resumes once the previous process lets go of the thread, waiting 100 then 400 ms', async () => { + vi.useFakeTimers() + const request = codexHeldFor(2) + + const opened = resume(request) + await vi.advanceTimersByTimeAsync(99) + expect(request).toHaveBeenCalledTimes(1) + await vi.advanceTimersByTimeAsync(1) + expect(request).toHaveBeenCalledTimes(2) + await vi.advanceTimersByTimeAsync(400) + + await expect(opened).resolves.toMatchObject({ threadId: THREAD }) + expect(request).toHaveBeenCalledTimes(3) + }) + + it('fails that one resume with the refusal once every retry met the lock', async () => { + vi.useFakeTimers() + const request = codexHeldFor(Number.POSITIVE_INFINITY) + + const opened = resume(request) + const settled = expect(opened).rejects.toThrow('already has an active writer') + await vi.advanceTimersByTimeAsync( + CODEX_ACTIVE_WRITER_RETRY_DELAYS_MS.reduce((sum, ms) => sum + ms, 0) + ) + + await settled + expect(request).toHaveBeenCalledTimes(CODEX_ACTIVE_WRITER_RETRY_DELAYS_MS.length + 1) + }) + + it.each([ + ['another thread', activeWriter('thread-other')], + ['another code', activeWriter(THREAD, -32603)], + ['any other refusal', new CodexAppServerRequestError('thread/resume', -32600, 'bad cwd')] + ])('does not wait on %s', async (_case, error) => { + const request = codexHeldFor(1, error) + + await expect(resume(request)).rejects.toBe(error) + expect(request).toHaveBeenCalledTimes(1) + }) + + // Every other test builds the error itself; this one sends Codex's raw JSON-RPC frame through + // the real connection, so a change to Orca's own error wording cannot hide the refusal. + it("recognizes Codex's raw refusal frame through the real connection", async () => { + const fakeAppServer = String.raw` + const readline = require('node:readline') + const send = (payload) => process.stdout.write(JSON.stringify(payload) + '\n') + let refusals = 1 + readline.createInterface({ input: process.stdin }).on('line', (line) => { + const message = JSON.parse(line) + if (message.method === 'initialize') return send({ id: message.id, result: {} }) + if (message.method === 'thread/resume' && refusals-- > 0) { + const threadId = message.params.threadId + return send({ + id: message.id, + error: { code: -32600, message: 'thread ' + threadId + ' already has an active writer' } + }) + } + if (message.method === 'thread/resume') { + return send({ id: message.id, result: { thread: { id: message.params.threadId } } }) + } + }) + ` + const connection = await openCodexAppServerConnection({ + command: process.execPath, + args: ['-e', fakeAppServer] + }) + try { + await expect( + openCodexThread(connection, { cwd: '/workspace', resumeThreadId: THREAD }, 5_000) + ).resolves.toMatchObject({ threadId: THREAD }) + } finally { + await connection.close() + } + }) +}) diff --git a/src/main/codex/codex-structured-thread-open.ts b/src/main/codex/codex-structured-thread-open.ts index 19a3980cb208..ce178654988f 100644 --- a/src/main/codex/codex-structured-thread-open.ts +++ b/src/main/codex/codex-structured-thread-open.ts @@ -63,6 +63,43 @@ async function resumeCodexThread( } } +/** Waits between resumes Codex refused because another process still holds the thread. */ +export const CODEX_ACTIVE_WRITER_RETRY_DELAYS_MS = [100, 400, 1_000] as const + +/** + * Codex's own refusal to open a thread another app-server still writes. It locks each thread's + * rollout per process, and the OS drops that lock the moment the holder dies, so a refusal right + * after Orca killed the previous process clears within moments. Codex releases without the lock + * never answer this. + */ +function isCodexActiveWriterError(error: unknown, threadId: string): boolean { + return ( + isCodexAppServerRequestError(error) && + error.method === 'thread/resume' && + error.code === -32600 && + error.message.endsWith(`thread ${threadId} already has an active writer`) + ) +} + +/** A resume that meets the previous process's lock tries again a few times, then fails. */ +async function resumeAfterPreviousWriter( + connection: Pick, + params: Record & { threadId: string }, + timeoutMs: number | undefined +): Promise { + for (const retryDelayMs of CODEX_ACTIVE_WRITER_RETRY_DELAYS_MS) { + try { + return await resumeCodexThread(connection, params, timeoutMs) + } catch (error) { + if (!isCodexActiveWriterError(error, params.threadId)) { + throw error + } + await new Promise((resolve) => setTimeout(resolve, retryDelayMs)) + } + } + return resumeCodexThread(connection, params, timeoutMs) +} + /** * Codex's own answer that it holds no rollout for this exact thread: the thread was started but * never given input, so there is no conversation to lose. Codex matches the same exact text @@ -114,7 +151,7 @@ export async function openCodexThread( ...(launch.resumePath ? { path: launch.resumePath } : {}) } try { - opened = await resumeCodexThread(connection, resumeParams, timeoutMs) + opened = await resumeAfterPreviousWriter(connection, resumeParams, timeoutMs) } catch (error) { if (!launch.supersedeIfUnsaved || !isCodexNoRolloutError(error, resumeThreadId)) { throw error diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-adapter.test.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-adapter.test.ts index daa7d2028428..4fb2dd2a07db 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-adapter.test.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-adapter.test.ts @@ -4,9 +4,15 @@ import { AgentSessionAcquisitionExitProvenError, AgentSessionAcquisitionExitUnprovenError, AgentSessionAcquisitionRefusal, - AgentSessionAcquisitionRootExitObservedError + AgentSessionAcquisitionRootExitObservedError, + AgentSessionPreSpawnError, + AgentSessionProviderKilledError } from './structured-agent-session-adapter' -import { rethrowAfterAgentSessionAcquisitionCleanup } from './structured-agent-session-provider-exit-proof' +import { + rethrowAfterAgentSessionAcquisitionCleanup, + stopAgentSessionProviderRoot +} from './structured-agent-session-provider-exit-proof' +import { failedAcquisitionSettlement } from './structured-agent-session-failed-create-refusal' describe('failed agent-session acquisition cleanup', () => { it('names a failure exit-proven after proven cleanup, keeping its diagnostic and cause', async () => { @@ -81,3 +87,67 @@ describe('failed agent-session acquisition cleanup', () => { expect(error).toMatchObject({ cause: expect.any(AggregateError) }) }) }) + +describe('a kill that reached the provider, its exit unseen', () => { + it('keeps the killed verdict through acquisition cleanup, with the provider diagnostic', async () => { + const cause = new Error('handshake failed') + const killed = new AgentSessionProviderKilledError(new Error('codex stopped responding')) + const error = await rethrowAfterAgentSessionAcquisitionCleanup( + { + releaseAcquisition: vi.fn(async () => { + throw killed + }) + }, + 'session-1', + cause + ).catch((thrown: unknown) => thrown) + + expect(error).toBeInstanceOf(AgentSessionProviderKilledError) + expect(error).toMatchObject({ + message: 'codex stopped responding', + cause: { errors: [cause, killed] } + }) + }) + + it('settles the failed start as a start failure with nothing proven exited', () => { + const settled = failedAcquisitionSettlement( + new AgentSessionProviderKilledError(new Error('codex stopped responding')), + { record: null, newSession: false } + ) + + expect(settled.exitProof).toBe('killed') + // A failed start, worded as one: never "the previous agent may still be running". + expect(settled.outcome).toMatchObject({ + status: 'failed', + code: 'agent_session_operation_invalid' + }) + }) + + it.each([ + ['a proven close', async () => true, 'exited'], + ['an unproven close', async () => false, 'unproven'], + [ + 'a delivered kill', + async () => { + throw new AgentSessionProviderKilledError(new Error('killed')) + }, + 'killed' + ], + [ + 'a root exit', + async () => { + throw new AgentSessionAcquisitionRootExitObservedError(new Error('exited')) + }, + 'exited' + ], + [ + 'a processless child', + async () => { + throw new AgentSessionPreSpawnError(new Error('ENOENT')) + }, + 'exited' + ] + ] as const)('reads %s as %s', async (_label, stop, verdict) => { + await expect(stopAgentSessionProviderRoot(stop)).resolves.toBe(verdict) + }) +}) diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts index f94a3b134201..5f017a340ca4 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts @@ -106,6 +106,19 @@ export class AgentSessionAcquisitionExitProvenError extends Error { } } +/** + * Orca's kill reached the provider, the process that writes the conversation, but its root's exit + * was not seen in time. A killed process never runs its own code again, so it cannot write another + * turn: the session is over and a new process may start. Nothing claims the exit, so no exit + * evidence is recorded. Like a root exit, the provider's own diagnostic is the message. + */ +export class AgentSessionProviderKilledError extends Error { + constructor(cause: unknown) { + super(cause instanceof Error ? cause.message : String(cause), { cause }) + this.name = 'AgentSessionProviderKilledError' + } +} + export class AgentSessionAcquisitionExitUnprovenError extends Error { constructor(cause: unknown) { super('agent_session_acquisition_exit_unproven', { cause }) @@ -201,6 +214,9 @@ export type StructuredAgentSessionEndedEvent = { observedAt?: number /** The provider ended before it finished starting, so resuming it would repeat the failure. */ startupUnproven?: true + /** Orca's kill reached the provider but its root's exit was not seen: the session ended without + * exit evidence (`AgentSessionProviderKilledError`). */ + rootExitUnobserved?: true } /** The child a publish-first acquire handed over has now proven its start: startup facts applied @@ -257,7 +273,8 @@ export type StructuredAgentSessionAdapter = StructuredAgentSessionAdapterStop & /** Reaps an acquired provider when the host cannot commit or prove its lease. * Returns true only after provider child exit is proven. Throws * `AgentSessionAcquisitionRootExitObservedError` when the provider root's own - * exit was observed first-hand but its descendants were not proven gone. */ + * exit was observed first-hand but its descendants were not proven gone, and + * `AgentSessionProviderKilledError` when its kill reached the provider unseen. */ releaseAcquisition?(input: { sessionId: string }): Promise dispatch(input: { sessionId: string @@ -375,8 +392,9 @@ export type StructuredAgentSessionAdapter = StructuredAgentSessionAdapterStop & accountHome: AgentSessionAccountHome }): Promise /** Gracefully stops the structured owner after its event stream is drained. */ - /** Returns true only after the provider child exit is proven. A root-exit or processless verdict - * is thrown only once the session is finalized; read it through `stopAgentSessionProviderRoot`. */ + /** Returns true only after the provider child exit is proven. A root-exit, killed or processless + * verdict is thrown only once the session is finalized; read it through + * `stopAgentSessionProviderRoot`. */ closeSession?(sessionId: string): Promise /** Stops a provider after a sink failure; the resulting exit is recovered as unexpected. */ forceCloseSession?(sessionId: string): Promise diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-agent-start.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-agent-start.ts index b5a742b788bc..477a33dd05eb 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-agent-start.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-agent-start.ts @@ -64,7 +64,7 @@ export async function ensureStructuredAgentSessionAgent( startedFor?: string ): Promise { // A child a stop began closing takes no input, and none may start beside it: the start waits on - // that close, and is refused while its exit stays unverifiable. + // that close, and is refused while its provider is neither proven gone nor killed. const closed = await joinClosingStructuredAgentSessionChild(context, sessionId) if (!closed.ok) { return closed diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-attach-failure.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-attach-failure.ts index 361c8f73aac8..35891a8a3b8a 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-attach-failure.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-attach-failure.ts @@ -2,7 +2,8 @@ import type { AgentSessionRecord } from '../../../shared/agent-session-record' import type { AttachFlowInput } from './structured-agent-session-attach-flow' import { AgentSessionAcquisitionExitUnprovenError, - AgentSessionAcquisitionRootExitObservedError + AgentSessionAcquisitionRootExitObservedError, + AgentSessionProviderKilledError } from './structured-agent-session-adapter' import { rethrowAfterAgentSessionAcquisitionCleanup } from './structured-agent-session-provider-exit-proof' @@ -12,7 +13,7 @@ export async function settlePostAcquisitionAttachFailure( cause: unknown ): Promise { let cleanupError: unknown = cause - let exitProof: 'exit-proven' | 'root-exit-observed' | 'unproven' = 'unproven' + let exitProof: 'exit-proven' | 'root-exit-observed' | 'killed' | 'unproven' = 'unproven' try { await rethrowAfterAgentSessionAcquisitionCleanup(input.adapter, record.sessionId, cause) } catch (error) { @@ -22,9 +23,16 @@ export async function settlePostAcquisitionAttachFailure( ? 'unproven' : error instanceof AgentSessionAcquisitionRootExitObservedError ? 'root-exit-observed' - : 'exit-proven' + : error instanceof AgentSessionProviderKilledError + ? 'killed' + : 'exit-proven' } - input.onAcquisitionReleased?.(cause, { rootGone: exitProof !== 'unproven' }) + input.onAcquisitionReleased?.( + cause, + exitProof === 'killed' + ? { rootGone: false, providerKilled: true } + : { rootGone: exitProof !== 'unproven' } + ) try { await input.store.settleFailedPostAcquisitionAttachment({ sessionId: record.sessionId, diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.ts index 453a40075aed..e6104ed856da 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.ts @@ -49,6 +49,7 @@ import { import type { ProviderHistoryWindow } from '../agent-session-journal/journal-submission-reconciler' import type { AgentSessionJournal } from '../agent-session-journal/journal-store' import type { StructuredAgentSessionLogger } from './structured-agent-session-logger' +import type { StructuredAgentSessionStopVerdict } from './structured-agent-session-host-types' export type AttachFlowInput = { store: AgentSessionRecordStore @@ -79,8 +80,8 @@ export type AttachFlowInput = { /** The conversation's own open journal, which the attach adopts: it never opens one itself. */ openConversation: (record: AgentSessionRecord) => Promise /** A failure after acquisition released the session's acquisition; `cause` is that failure and - * `rootGone` whether the release saw the provider root go. */ - onAcquisitionReleased?: (cause: unknown, verdict: { rootGone: boolean }) => void + * `verdict` whether the release saw the provider root go, or killed the provider. */ + onAcquisitionReleased?: (cause: unknown, verdict: StructuredAgentSessionStopVerdict) => void /** The error an acquisition failed with, for a host-side reader of the provider's words; the * refusal never carries them. */ onAcquisitionFailed?: (error: unknown) => void diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-chat-stop.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-chat-stop.ts index 834d4f569fa8..236d1be6834e 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-chat-stop.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-chat-stop.ts @@ -148,7 +148,8 @@ export function mutateWithChatStop( sessionId, error }), - // The host drops its child only once the exit is proven, and nothing else runs meanwhile. + // The host drops its child only once the exit is proven or the provider killed, and + // nothing else runs meanwhile. childReleased: () => context.sessions.get(sessionId)?.child !== child, endSession: (owed) => { windDown = owed diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-child-close.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-child-close.ts index 0022df7c1565..077c6dfd5635 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-child-close.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-child-close.ts @@ -4,33 +4,43 @@ // it is about. Joining it is asking the adapter to close: the adapter runs one close per child at // a time and bounds it by its own kill escalation, so every caller waits on the same attempt, and // a caller that comes after an attempt ended unproven runs it again. The verdict is the root's -// exit alone. Proven, the one exit handler ends the record; a proof that lands with no caller -// waiting reaches that handler as the adapter's report of the exit. +// exit, or a kill that reached the provider: a killed process never writes again, so a new one may +// start beside nothing. Either way the one exit handler ends the record, a kill with no exit +// evidence; a proof that lands with no caller waiting reaches that handler as the adapter's report +// of the exit, and a late exit after a kill finds the record already ended. import { refuse } from '../../../shared/agent-session-wire-refusals' import type { AgentSessionWireRefusal } from '../../../shared/agent-session-wire' import type { StructuredAgentSessionLifetimeContext } from './structured-agent-session-host-lifetime' import type { StructuredAgentSessionProviderChild } from './structured-agent-session-host-types' -import { stopAgentSessionProviderRoot } from './structured-agent-session-provider-exit-proof' +import { + stopAgentSessionProviderRoot, + type AgentSessionProviderStop +} from './structured-agent-session-provider-exit-proof' import { releaseStoredStructuredAgentSessionOwnerAfterExit } from './structured-agent-session-lease-release' import { isSurfaceReleasableAgentSessionRecord } from '../../runtime/agent-session-surface-release-transition' -/** What a caller learned about the child's exit: proven, or not. A root still there after the - * close's kill reads `unverifiable`, never `exited`. */ -export type StructuredAgentSessionChildCloseVerdict = 'exited' | 'unverifiable' +/** What a caller learned about the child: its exit proven, its provider `killed` without the exit + * seen, or neither. A provider the close could not kill reads `unverifiable`. */ +export type StructuredAgentSessionChildCloseVerdict = 'exited' | 'killed' | 'unverifiable' -/** Joins the child's close and, once its root's exit is proven, ends the record. */ +/** Joins the child's close and, once its exit is proven or its provider killed, ends the record. */ export async function joinStructuredAgentSessionChildClose( context: StructuredAgentSessionLifetimeContext, sessionId: string, child: StructuredAgentSessionProviderChild ): Promise { - if (!(await closeProviderRoot(context, sessionId))) { + const stopped = await closeProviderRoot(context, sessionId) + if (stopped === 'unproven') { return 'unverifiable' } - await context.endExitedChild(sessionId, child, { expected: true, reason: 'closed by Orca' }) + await context.endExitedChild(sessionId, child, { + expected: true, + reason: 'closed by Orca', + ...(stopped === 'killed' ? { rootExitUnobserved: true as const } : {}) + }) context.restartWitness?.stopped(sessionId) - return 'exited' + return stopped } /** The refusal of an operation that met a child whose close is still unproven. */ @@ -43,8 +53,8 @@ export function previousExitUnverifiableRefusal(): AgentSessionWireRefusal { } /** For an operation that reaches the provider: a child a stop began closing takes no input and - * none may start beside it, so the operation joins that close and is refused while it is still - * unproven. */ + * none may start beside it, so the operation joins that close and is refused while its provider + * is neither proven gone nor killed. */ export async function joinClosingStructuredAgentSessionChild( context: StructuredAgentSessionLifetimeContext, sessionId: string @@ -53,11 +63,11 @@ export async function joinClosingStructuredAgentSessionChild( if (!child?.close) { return { ok: true } } - if ((await joinStructuredAgentSessionChildClose(context, sessionId, child)) === 'exited') { + if ((await joinStructuredAgentSessionChildClose(context, sessionId, child)) !== 'unverifiable') { return { ok: true } } // A stop reports this through its own failure; here the refusal is the only trace. - context.deps.logger.warn("the agent's process did not exit after Orca stopped and killed it", { + context.deps.logger.warn("Orca's kill did not reach the agent's process, and it did not exit", { scope: 'provider-close-unproven', sessionId }) @@ -67,12 +77,12 @@ export async function joinClosingStructuredAgentSessionChild( function closeProviderRoot( context: StructuredAgentSessionLifetimeContext, sessionId: string -): Promise { +): Promise { const { adapter, logger } = context.deps - // An adapter with no close has nothing to stop; anything else must PROVE the exit. + // An adapter with no close has nothing to stop; anything else must prove the exit or the kill. const stop = adapter.disposeSession ?? adapter.closeSession if (!stop) { - return Promise.resolve(true) + return Promise.resolve('exited') } return stopAgentSessionProviderRoot( () => stop.call(adapter, sessionId), @@ -89,12 +99,13 @@ function closeProviderRoot( sessionId, error }) - return false + return 'unproven' as const }) } -/** Whether the lease still names the child this host last proved gone, unreleased: only that - * in-memory proof lets this host release it without a probe, so the handle carrying it stays. */ +/** Whether the lease still names the child this host last proved gone or killed, unreleased: only + * that in-memory proof lets this host release it without a probe, so the handle carrying it + * stays. */ export function structuredAgentSessionEndedChildHoldsLease( context: Pick, sessionId: string @@ -102,7 +113,7 @@ export function structuredAgentSessionEndedChildHoldsLease( const ended = context.sessions.get(sessionId)?.lastEndedChild const record = context.deps.store.getRecord(sessionId) return ( - ended?.rootGone === true && + (ended?.rootGone === true || ended?.providerKilled === true) && record !== null && isSurfaceReleasableAgentSessionRecord(record) && record.lease.runtimeFence === ended.fence @@ -125,7 +136,8 @@ export async function releaseLeaseOfEndedStructuredAgentSessionChild( sessionId, expectedFence: ended.fence, now: context.now(), - ...(ended.reason ? { exitReason: ended.reason } : {}) + ...(ended.reason ? { exitReason: ended.reason } : {}), + ...(ended.rootGone ? {} : { rootExitUnobserved: true as const }) }).catch((error: unknown) => context.deps.logger.warn("releasing an exited agent's lease failed", { scope: 'ended-child-lease-release', diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-child-exit.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-child-exit.ts index c606a73e310f..3743b6f53ef9 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-child-exit.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-child-exit.ts @@ -1,5 +1,6 @@ // The one handler that ends a provider child's record, whether its exit was expected (a close this -// host asked for) or not (the child died on its own). The exit was seen or proven first-hand, so +// host asked for) or not (the child died on its own). The exit was seen or proven first-hand, or +// Orca's kill reached the provider so nothing of it can write again, so // every step after it is bookkeeping: each is attempted and reported, none keeps the child on // record, and the record ends in `finally`. `expected` changes only what the chat is told. @@ -37,6 +38,9 @@ export type StructuredAgentSessionChildExit = { /** Host receipt of the exit: the end time of a turn it interrupted. */ observedAt?: number startupUnproven?: true + /** Orca's kill reached the provider but its root's exit was not seen: the record ends, and the + * lease is released without exit evidence. */ + rootExitUnobserved?: true } export type StructuredAgentSessionChildExitSession = Pick< @@ -81,7 +85,8 @@ export function settleStructuredAgentSessionChildExit< reason: event.reason, ...(event.failure ? { failure: event.failure } : {}), ...(event.observedAt === undefined ? {} : { observedAt: event.observedAt }), - ...(event.startupUnproven ? { startupUnproven: event.startupUnproven } : {}) + ...(event.startupUnproven ? { startupUnproven: event.startupUnproven } : {}), + ...(event.rootExitUnobserved ? { rootExitUnobserved: event.rootExitUnobserved } : {}) }) }) } @@ -115,8 +120,9 @@ export async function endExitedStructuredAgentSessionChildUnderSerialize< reason: expected ? (close?.reason ?? null) : exit.reason, ...(!expected && exit.failure ? { failure: exit.failure } : {}), duringStartup: exitedDuringStartup, - // The adapter publishes an exit only once it saw the root go, first-hand or proven. - rootGone: true, + // The adapter publishes an exit only once it saw the root go, or once its kill reached the + // provider (`rootExitUnobserved`). + ...(exit.rootExitUnobserved ? { rootGone: false, providerKilled: true } : { rootGone: true }), ...(expected && close ? { endedAt: close.requestedAt } : {}) }) context.publishStatus?.(sessionId) @@ -183,7 +189,8 @@ export async function endExitedStructuredAgentSessionChildUnderSerialize< expectedFence: child.fence, now: context.now(), exitObservedAt: observedAt, - exitReason: exit.reason + exitReason: exit.reason, + ...(exit.rootExitUnobserved ? { rootExitUnobserved: true as const } : {}) }) released = true } catch (error) { diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-claude-stop-exit-ends-record.test.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-claude-stop-exit-ends-record.test.ts index afb4ac1c1c11..3d260f2ff00f 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-claude-stop-exit-ends-record.test.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-claude-stop-exit-ends-record.test.ts @@ -617,3 +617,52 @@ it('runs the stop again for every ask after a close that came back unproven', as await resumedWith(connection, 'Third.') expect(connection.closeCount).toBe(4) }) + +/** Stops a running turn; the close's kill reaches Claude itself, but its exit is never seen. */ +async function stopWithKilledClose(): Promise { + const connection = claude.connections[0]! + await openTurn(connection) + connection.exitVerdict = { root: 'live', tree: 'unverifiable', providerKilled: true } + connection.close = async () => { + connection.closeCount += 1 + connection.closed = true + return false + } + refuseWritesOnceClosed(connection) + await expect(stop()).resolves.toMatchObject({ ok: true, value: { cancelled: true } }) + frame(connection, INTERRUPTED_RESULT) + await laneDrained() + return connection +} + +it("sends the next message to a fresh Claude once the Stop's kill reached the old one, its exit unseen", async () => { + const connection = await stopWithKilledClose() + // Ended at the Stop: nothing is left for a send to join, and no exit is claimed. + expect(child()).toBeNull() + expect(lease()).toMatchObject({ claimStatus: 'released', deathEvidence: null }) + expect(host['sessions'].get(SESSION)?.lastEndedChild).toMatchObject({ + cause: 'user-stop', + rootGone: false, + providerKilled: true + }) + + const next = await send('Carry on.') + const resumed = await resumedWith(connection, 'Carry on.') + expect(connection.closeCount).toBe(1) + expect((await submission(next))?.rejection).toBeUndefined() + expect(scopes()).not.toContain('provider-close-unproven') + + // The old process's exit, seen at last, ends nothing: its record already ended. + connection.handlers.onExit?.(new Error('claude exited'), { expected: true }) + await laneDrained() + expect(child()).not.toBeNull() + expect(claude.connections.at(-1)).toBe(resumed) +}) + +it("takes an option change at rest once the Stop's kill reached Claude", async () => { + const connection = await stopWithKilledClose() + + await expect(setModel('claude-opus-5')).resolves.toMatchObject({ ok: true }) + expect(connection.calls.some((call) => call.subtype === 'set_model')).toBe(false) + expect(store.getRecord(SESSION)?.options).toMatchObject({ model: 'claude-opus-5' }) +}) diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-codex-stop-row.test.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-codex-stop-row.test.ts index 7ee9d070f4c9..4a5cbab379a4 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-codex-stop-row.test.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-codex-stop-row.test.ts @@ -535,4 +535,53 @@ describe('a message after a Codex Stop whose exit was unproven', () => { expect(log.entries.map((entry) => entry.fields.scope)).toContain('provider-close-after-exit') expect(host['sessions'].get(SESSION)?.lastEndedChild).toMatchObject({ rootGone: true }) }) + + it("goes to a fresh Codex once the Stop's kill reached the old one, waiting out its thread lock", async () => { + await runningTurn() + codex.routes['turn/interrupt'] = () => { + throw interruptFailure('internal error') + } + const old = codex.connections.at(-1)! + const request = old.request + // The close's kill reached the app-server; its exit is never seen. + old.close = async () => { + old.closed = true + return false + } + Object.defineProperty(old, 'providerKilled', { get: () => old.closed }) + old.request = (method, params) => + old.closed + ? Promise.reject(new Error('codex app-server is closing')) + : request(method, params) + await stop() + await host.flushStreamedEvents(SESSION) + expect(host['sessions'].get(SESSION)?.child).toBeNull() + expect(host.deps.store.getRecord(SESSION)?.lease).toMatchObject({ + claimStatus: 'released', + deathEvidence: null + }) + // Codex still holds the old writer's lock on the thread for a moment after the kill. + launch.resumeThreadId = THREAD + const resume = codex.routes['thread/resume']! + let locked = 1 + codex.routes['thread/resume'] = (params) => { + if (locked-- > 0) { + throw new CodexAppServerRequestError( + 'thread/resume', + -32600, + `codex app-server thread/resume failed: thread ${THREAD} already has an active writer` + ) + } + return resume(params) + } + + expect(await send('carry on')).toMatchObject({ ok: true }) + await vi.waitFor(() => expect(codex.connections).toHaveLength(2)) + await vi.waitFor(() => expect(startedWith(codex.connections[1]!, 'carry on')).toBe(true)) + expect( + codex.connections[1]!.calls.filter((call) => call.method === 'thread/resume') + ).toHaveLength(2) + expect(startedWith(old, 'carry on')).toBe(false) + expect(log.entries.map((entry) => entry.fields.scope)).not.toContain('provider-close-unproven') + }) }) diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-event-recovery.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-event-recovery.ts index 398d04873d41..8e1e1db7eec2 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-event-recovery.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-event-recovery.ts @@ -64,7 +64,9 @@ export class StructuredAgentSessionEventRecovery { const child = this.context.sessions.get(sessionId)?.child const stop = this.context.deps.adapter.forceCloseSession ?? this.context.deps.adapter.closeSession - if (!child || !stop || !(await stopAgentSessionProviderRoot(() => stop(sessionId)))) { + const stopped = + child && stop ? await stopAgentSessionProviderRoot(() => stop(sessionId)) : 'unproven' + if (!child || stopped === 'unproven') { return } // Ended in the same step as the stop, so no report of that close can end it first as a @@ -72,7 +74,8 @@ export class StructuredAgentSessionEventRecovery { await this.endExitedChildUnderSerialize(sessionId, child, { expected: false, reason: `journal sink failure: ${error instanceof Error ? error.message : String(error)}`, - failure: agentSessionFailureFact('hostFault') + failure: agentSessionFailureFact('hostFault'), + ...(stopped === 'killed' ? { rootExitUnobserved: true as const } : {}) }) }) .catch((error: unknown) => diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-failed-create-refusal.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-failed-create-refusal.ts index 90f52db3d095..04ca731dd1fb 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-failed-create-refusal.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-failed-create-refusal.ts @@ -21,6 +21,7 @@ import { AgentSessionAcquisitionRefusal, AgentSessionAcquisitionRootExitObservedError, AgentSessionPreSpawnError, + AgentSessionProviderKilledError, isAgentSessionPreSpawnError } from './structured-agent-session-adapter' import { @@ -70,7 +71,9 @@ export function failedAcquisitionSettlement( ? 'processless' : error instanceof AgentSessionAcquisitionRootExitObservedError ? 'root-exit-observed' - : 'exit-proven' + : error instanceof AgentSessionProviderKilledError + ? 'killed' + : 'exit-proven' const raw = error instanceof Error ? error.message : String(error) const details = failedAcquisitionDetails(error) const message = @@ -90,11 +93,12 @@ export function failedAcquisitionSettlement( } } -/** Cleanup proved the child gone after the start failed, whatever failed it. */ +/** Cleanup proved the child gone, or killed it, after the start failed, whatever failed it. */ function isExitProvenAcquisitionFailure(error: unknown): error is Error { return ( error instanceof AgentSessionAcquisitionRootExitObservedError || - error instanceof AgentSessionAcquisitionExitProvenError + error instanceof AgentSessionAcquisitionExitProvenError || + error instanceof AgentSessionProviderKilledError ) } diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.ts index 5e8d7e2097f9..9764c82a970b 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.ts @@ -91,7 +91,8 @@ export async function abandonQueuedStructuredAgentSessionMessages( /** * The agent goes to rest; the conversation stays. Begins the child's close, or joins the one - * already begun, and waits for the exit's proof as long as a caller may. Still unproven, it throws + * already begun, and waits for the exit's proof, or the provider's kill, as long as a caller may. + * Neither, it throws * and the close keeps running: a later proof, or the next asker's attempt, ends the record. * `ending` is how the child's end is told: a user's Stop, the host stopping it for a cause (with * its text), or an eviction the conversation's close follows. The first stop's ending decides. @@ -139,11 +140,13 @@ export async function stopStructuredAgentSessionAgentUnderSerialize( () => context.restartWitness?.beforeStop(sessionId) ) } - if ((await joinStructuredAgentSessionChildClose(context, sessionId, child)) !== 'exited') { + if ( + (await joinStructuredAgentSessionChildClose(context, sessionId, child)) === 'unverifiable' + ) { throw new StructuredAgentSessionEvictionError( 'stop-provider-child', sessionId, - new Error('provider child exit was not proven') + new Error('provider child was neither proven exited nor killed') ) } } finally { diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-host-types.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-host-types.ts index 243e2d8bea2f..c1c87e8c2d4d 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-host-types.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-host-types.ts @@ -73,8 +73,9 @@ export type StructuredAgentSessionProviderChild = StructuredAgentSessionProvider } /** What ending a child established about its provider root. A stop's comes only from - * `stopAgentSessionProviderRoot`; an observed exit's root is gone by definition. */ -export type StructuredAgentSessionStopVerdict = { rootGone: boolean } + * `stopAgentSessionProviderRoot`; an observed exit's root is gone by definition. `providerKilled`: + * the root's exit was not seen, but Orca's kill reached the process that writes the conversation. */ +export type StructuredAgentSessionStopVerdict = { rootGone: boolean; providerKilled?: true } export type { StructuredAgentSessionChildEndCause } diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-lease-release.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-lease-release.ts index 63e3add288e4..a3b5dfd3eb75 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-lease-release.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-lease-release.ts @@ -17,7 +17,8 @@ export type StructuredAgentSessionLeaseStore = Pick< 'getRecord' | 'transitionHandoff' > -/** Releases the lease of the child whose exit this host observed, with that exit's evidence. +/** Releases the lease of the child whose exit this host observed, with that exit's evidence, or + * whose provider it killed, with none. * Throws `agent_session_checkpoint_stale` when the record no longer names that child. */ export async function releaseStoredStructuredAgentSessionOwnerAfterExit(input: { store: StructuredAgentSessionLeaseStore @@ -26,6 +27,8 @@ export async function releaseStoredStructuredAgentSessionOwnerAfterExit(input: { now: number exitObservedAt?: number exitReason?: string + /** Orca's kill reached the provider but no exit was seen: released with no exit evidence. */ + rootExitUnobserved?: true }): Promise { const record = input.store.getRecord(input.sessionId) if ( @@ -40,6 +43,7 @@ export async function releaseStoredStructuredAgentSessionOwnerAfterExit(input: { expectedFence: input.expectedFence, now: input.now, ...(input.exitObservedAt === undefined ? {} : { exitObservedAt: input.exitObservedAt }), - ...(input.exitReason ? { exitReason: input.exitReason } : {}) + ...(input.exitReason ? { exitReason: input.exitReason } : {}), + ...(input.rootExitUnobserved ? { rootExitUnobserved: input.rootExitUnobserved } : {}) }) } diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-provider-exit-proof.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-provider-exit-proof.ts index 1545a8fa618d..5d75887640dc 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-provider-exit-proof.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-provider-exit-proof.ts @@ -8,6 +8,7 @@ import { AgentSessionAcquisitionExitUnprovenError, AgentSessionAcquisitionRefusal, AgentSessionAcquisitionRootExitObservedError, + AgentSessionProviderKilledError, isAgentSessionPreSpawnError, type StructuredAgentSessionAdapter } from './structured-agent-session-adapter' @@ -21,15 +22,21 @@ export async function rethrowAfterAgentSessionAcquisitionCleanup( try { released = (await adapter.releaseAcquisition?.({ sessionId })) === true } catch (cleanupError) { - // A root exit the cleanup observed first-hand keeps its classification and its - // provider diagnostic; the failure that triggered cleanup rides along as cause. - throw cleanupError instanceof AgentSessionAcquisitionRootExitObservedError - ? new AgentSessionAcquisitionRootExitObservedError( - new AggregateError([cause, cleanupError], cleanupError.message) - ) - : new AgentSessionAcquisitionExitUnprovenError( - new AggregateError([cause, cleanupError], 'agent session acquisition cleanup failed') - ) + // A root exit the cleanup observed first-hand, or a kill it delivered, keeps its + // classification and its provider diagnostic; the failure that triggered cleanup rides along. + if (cleanupError instanceof AgentSessionAcquisitionRootExitObservedError) { + throw new AgentSessionAcquisitionRootExitObservedError( + new AggregateError([cause, cleanupError], cleanupError.message) + ) + } + if (cleanupError instanceof AgentSessionProviderKilledError) { + throw new AgentSessionProviderKilledError( + new AggregateError([cause, cleanupError], cleanupError.message) + ) + } + throw new AgentSessionAcquisitionExitUnprovenError( + new AggregateError([cause, cleanupError], 'agent session acquisition cleanup failed') + ) } if (released) { throw provenExitAcquisitionFailure(cause) @@ -43,32 +50,40 @@ function provenExitAcquisitionFailure(cause: unknown): unknown { const classified = cause instanceof AgentSessionAcquisitionRefusal || cause instanceof AgentSessionAcquisitionRootExitObservedError || + cause instanceof AgentSessionProviderKilledError || cause instanceof AgentSessionAcquisitionExitUnprovenError || isAgentSessionPreSpawnError(cause) || (cause instanceof Error && isAgentSessionWireRefusalCode(cause.message)) return classified ? cause : new AgentSessionAcquisitionExitProvenError(cause) } -/** Whether a stop left the provider root gone. The lease follows the root, so a first-hand root - * exit or a processless child ends the session whatever its descendants did. A descendant left - * unconfirmed, or bookkeeping that failed after the proven exit, is only `report`ed. Any other - * failure throws. */ +/** What a stop established about the provider: its root seen gone; `killed`, Orca's kill reached + * the process that writes the conversation but no exit was seen; or `unproven`. */ +export type AgentSessionProviderStop = 'exited' | 'killed' | 'unproven' + +/** What a stop left of the provider. The lease follows the root, so a first-hand root exit or a + * processless child ends the session whatever its descendants did; a delivered kill ends it too, + * since nothing it reached can write again. A descendant left unconfirmed, or bookkeeping that + * failed after the proven exit, is only `report`ed. Any other failure throws. */ export async function stopAgentSessionProviderRoot( stop: () => Promise, report?: (error: Error) => void -): Promise { +): Promise { try { - return (await stop()) === true + return (await stop()) === true ? 'exited' : 'unproven' } catch (error) { if ( error instanceof AgentSessionAcquisitionRootExitObservedError || error instanceof AgentSessionAcquisitionExitProvenError ) { report?.(error) - return true + return 'exited' + } + if (error instanceof AgentSessionProviderKilledError) { + return 'killed' } if (isAgentSessionPreSpawnError(error)) { - return true + return 'exited' } throw error } diff --git a/src/main/provider-process/managed-provider-process-fallback-tree.test.ts b/src/main/provider-process/managed-provider-process-fallback-tree.test.ts index 4ec32aa9a8bc..9d4de30f497c 100644 --- a/src/main/provider-process/managed-provider-process-fallback-tree.test.ts +++ b/src/main/provider-process/managed-provider-process-fallback-tree.test.ts @@ -4,6 +4,11 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import type { spawnProcess } from '../../shared/child-process/run-process' import type { DescendantSnapshot } from '../pty-descendant-termination' import { spawnManagedProviderProcess } from './managed-provider-process' +import { SUPERVISED_PROVIDER_GRACEFUL_EXIT_MS } from './provider-process-supervisor' + +/** Above pid_max on every supported POSIX host, so the real group signal is ESRCH. */ +const UNREACHABLE_PGID = 2_147_483_647 +const SUPERVISED_LADDER_MS = SUPERVISED_PROVIDER_GRACEFUL_EXIT_MS + 1_000 // The real fallback teardown; only the primitives that touch the OS are faked. const os = vi.hoisted(() => ({ @@ -52,25 +57,32 @@ describe('fallback teardown never claims descendants it did not observe', () => vi.useFakeTimers() const closing = rootOnly('win32').close() await vi.advanceTimersByTimeAsync(1_500) - await expect(closing).resolves.toEqual({ root: 'exited', tree: null }) + await expect(closing).resolves.toEqual({ root: 'exited', tree: null, providerKilled: true }) expect(os.taskkill).toHaveBeenCalledOnce() }) - it('reports no observation when the POSIX process table cannot be read', async () => { + it('reports no observation, and kills nothing, when the POSIX process table cannot be read', async () => { vi.useFakeTimers() const closing = rootOnly('darwin').close() - await vi.advanceTimersByTimeAsync(5_500) - await expect(closing).resolves.toEqual({ root: 'exited', tree: null }) + await vi.advanceTimersByTimeAsync(SUPERVISED_LADDER_MS) + // The supervisor is resumed to finish its own stop; killing it alone would orphan the provider. + await expect(closing).resolves.toEqual({ root: 'live', tree: null, providerKilled: false }) expect(os.capture).toHaveBeenCalledOnce() expect(os.verifySnapshot).not.toHaveBeenCalled() }) it('reports exited only when the captured descendants were verified gone', async () => { vi.useFakeTimers() - os.capture.mockResolvedValueOnce({ rootPgid: 1, descendants: [], capturedAtMs: 1 }) + os.capture.mockResolvedValueOnce({ + rootPgid: 4242, + descendants: [ + { pid: UNREACHABLE_PGID, ppid: 4242, pgid: UNREACHABLE_PGID, startedAt: 'Mon Oct 5' } + ], + capturedAtMs: 1 + }) const closing = rootOnly('darwin').close() - await vi.advanceTimersByTimeAsync(5_500) - await expect(closing).resolves.toEqual({ root: 'exited', tree: 'exited' }) + await vi.advanceTimersByTimeAsync(SUPERVISED_LADDER_MS) + await expect(closing).resolves.toEqual({ root: 'exited', tree: 'exited', providerKilled: true }) expect(os.verifySnapshot).toHaveBeenCalledOnce() }) }) diff --git a/src/main/provider-process/managed-provider-process-root-only.test.ts b/src/main/provider-process/managed-provider-process-root-only.test.ts index bb3f407c7dc0..7dba9715e2a7 100644 --- a/src/main/provider-process/managed-provider-process-root-only.test.ts +++ b/src/main/provider-process/managed-provider-process-root-only.test.ts @@ -5,9 +5,13 @@ import type { spawnProcess } from '../../shared/child-process/run-process' import { spawnManagedProviderProcess } from './managed-provider-process' import { ROOT_ONLY_GRACEFUL_EXIT_MS } from './provider-process-close' import type { ProviderProcessTeardownVerdict } from './provider-process-teardown' +import { PROVIDER_SUPERVISOR_MAX_STOP_MS } from './provider-process-supervisor' const teardown = vi.hoisted(() => ({ - terminate: vi.fn(async (): Promise => 'exited') + terminate: vi.fn(async (): Promise => ({ + tree: 'exited', + providerKilled: true + })) })) vi.mock('./provider-process-teardown', () => ({ terminateProviderProcessTree: teardown.terminate @@ -46,7 +50,11 @@ describe('root-only managed provider close', () => { const fixture = fakeChild() fixture.child.stdin.once('finish', () => fixture.child.emit('exit', 0, null)) const managed = rootOnly(fixture) - await expect(managed.close()).resolves.toEqual({ root: 'exited', tree: null }) + await expect(managed.close()).resolves.toEqual({ + root: 'exited', + tree: null, + providerKilled: false + }) expect(teardown.terminate).not.toHaveBeenCalled() }) @@ -56,20 +64,46 @@ describe('root-only managed provider close', () => { vi.useFakeTimers() teardown.terminate.mockImplementationOnce(async () => { fixture.child.emit('exit', null, 'SIGKILL') - return tree + return { tree, providerKilled: true } }) const fixture = fakeChild() const managed = rootOnly(fixture) const closing = managed.close() await vi.advanceTimersByTimeAsync(ROOT_ONLY_GRACEFUL_EXIT_MS) - await expect(closing).resolves.toEqual({ root: 'exited', tree }) + await expect(closing).resolves.toEqual({ root: 'exited', tree, providerKilled: true }) // The root is gone, so the close is done: a repeat answers from the memo, not a second teardown. - await expect(managed.close()).resolves.toEqual({ root: 'exited', tree }) + await expect(managed.close()).resolves.toEqual({ root: 'exited', tree, providerKilled: true }) expect(teardown.terminate).toHaveBeenCalledOnce() - expect(managed.lastCloseResult).toEqual({ root: 'exited', tree }) + expect(managed.lastCloseResult).toEqual({ root: 'exited', tree, providerKilled: true }) } ) + it('reports a kill that reached the provider while the root is still not seen to exit', async () => { + vi.useFakeTimers() + teardown.terminate.mockResolvedValueOnce({ tree: null, providerKilled: true }) + const fixture = fakeChild() + const managed = rootOnly(fixture) + const closing = managed.close() + await vi.advanceTimersByTimeAsync(ROOT_ONLY_GRACEFUL_EXIT_MS + 1_000) + await expect(closing).resolves.toEqual({ root: 'live', tree: null, providerKilled: true }) + }) + + // Forcing a supervisor at its own worst case races its exit on a loaded host, and the forced + // step then pauses a supervisor that was about to report its provider gone. + it("gives a supervised root the supervisor's whole stop plus half a second before forcing", async () => { + vi.useFakeTimers() + const fixture = fakeChild() + const managed = spawnManagedProviderProcess( + { command: 'fixture-provider', args: [] }, + { spawnImpl: fixture.spawn, platform: 'linux', site: 'fixture-provider-teardown' } + ) + void managed.close() + await vi.advanceTimersByTimeAsync(PROVIDER_SUPERVISOR_MAX_STOP_MS + 499) + expect(teardown.terminate).not.toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(1) + expect(teardown.terminate).toHaveBeenCalledOnce() + }) + it('waits the default root-only grace before forcing', async () => { vi.useFakeTimers() const fixture = fakeChild() @@ -85,8 +119,9 @@ describe('root-only managed provider close', () => { const fixture = fakeChild() const managed = rootOnly(fixture) fixture.child.emit('exit', 0, null) - await expect(managed.close()).resolves.toEqual({ root: 'exited', tree: null }) - expect(managed.lastCloseResult).toEqual({ root: 'exited', tree: null }) + const answer = { root: 'exited', tree: null, providerKilled: false } + await expect(managed.close()).resolves.toEqual(answer) + expect(managed.lastCloseResult).toEqual(answer) expect(fixture.child.stdin.writableEnded).toBe(false) expect(fixture.child.kill).not.toHaveBeenCalled() }) diff --git a/src/main/provider-process/managed-provider-process.test.ts b/src/main/provider-process/managed-provider-process.test.ts index 5fdc8eea9604..992e9e365284 100644 --- a/src/main/provider-process/managed-provider-process.test.ts +++ b/src/main/provider-process/managed-provider-process.test.ts @@ -55,7 +55,8 @@ function fakeTree(initial: DescendantTreeVerdict = 'unverifiable') { reap: vi.fn(async () => verdict), get treeVerdict() { return verdict - } + }, + providerKilled: false } return { tree, @@ -220,13 +221,26 @@ describe('managed provider process', () => { fixture.child.emit('exit', 0, null) await expect(managed.close(proof.tree)).resolves.toEqual({ root: 'exited', - tree: 'unverifiable' + tree: 'unverifiable', + providerKilled: false }) proof.setVerdict('live') - await expect(managed.close(proof.tree)).resolves.toEqual({ root: 'exited', tree: 'live' }) + await expect(managed.close(proof.tree)).resolves.toEqual({ + root: 'exited', + tree: 'live', + providerKilled: false + }) proof.setVerdict('exited') - await expect(managed.close(proof.tree)).resolves.toEqual({ root: 'exited', tree: 'exited' }) - await expect(managed.close(proof.tree)).resolves.toEqual({ root: 'exited', tree: 'exited' }) + await expect(managed.close(proof.tree)).resolves.toEqual({ + root: 'exited', + tree: 'exited', + providerKilled: false + }) + await expect(managed.close(proof.tree)).resolves.toEqual({ + root: 'exited', + tree: 'exited', + providerKilled: false + }) expect(proof.tree.capture).toHaveBeenCalledTimes(3) }) @@ -241,7 +255,11 @@ describe('managed provider process', () => { const close = managed.close() await vi.advanceTimersByTimeAsync(150) await expect(close).resolves.toMatchObject({ root: 'exited' }) - expect(managed.lastCloseResult).toEqual({ root: 'exited', tree: 'unverifiable' }) + expect(managed.lastCloseResult).toEqual({ + root: 'exited', + tree: 'unverifiable', + providerKilled: false + }) }) it('keeps the cleanup diagnostic tied to the close that finished before a late root exit', async () => { @@ -254,7 +272,11 @@ describe('managed provider process', () => { await expect(close).resolves.toMatchObject({ root: 'live' }) fixture.child.emit('exit', null, 'SIGKILL') await expect(managed.close()).resolves.toMatchObject({ root: 'exited' }) - expect(managed.lastCloseResult).toEqual({ root: 'live', tree: 'unverifiable' }) + expect(managed.lastCloseResult).toEqual({ + root: 'live', + tree: 'unverifiable', + providerKilled: false + }) }) it('lets a supervised caller signal immediately and waits its configured grace before forcing', async () => { diff --git a/src/main/provider-process/managed-provider-process.ts b/src/main/provider-process/managed-provider-process.ts index 55b8f59e29f2..b4c99a8c3d31 100644 --- a/src/main/provider-process/managed-provider-process.ts +++ b/src/main/provider-process/managed-provider-process.ts @@ -147,7 +147,11 @@ export function spawnManagedProviderProcess( return exitProof.run(async () => { // The one already-exited guard. It observed nothing, so an earlier close's findings stay. if (observed && !tree) { - const result: ProviderProcessCloseResult = { root: 'exited', tree: null } + const result: ProviderProcessCloseResult = { + root: 'exited', + tree: null, + providerKilled: false + } lastCloseResult ??= result return result } diff --git a/src/main/provider-process/provider-process-close.ts b/src/main/provider-process/provider-process-close.ts index e67645f2cf23..f201be66ede0 100644 --- a/src/main/provider-process/provider-process-close.ts +++ b/src/main/provider-process/provider-process-close.ts @@ -1,7 +1,7 @@ import type { SpawnedProcess } from '../../shared/child-process/run-process' import type { DescendantTreeVerdict } from '../pty-descendant-exit-verification' import { waitForProcessExitUntil } from './provider-process-exit-deadline' -import { PROVIDER_SUPERVISOR_MAX_STOP_MS } from './provider-process-supervisor' +import { SUPERVISED_PROVIDER_GRACEFUL_EXIT_MS } from './provider-process-supervisor' import type { ProviderProcessTeardownVerdict } from './provider-process-teardown' export type ProviderProcessTree = { @@ -9,6 +9,8 @@ export type ProviderProcessTree = { refresh?: () => Promise reap(): Promise readonly treeVerdict: DescendantTreeVerdict + /** Whether a reap's kill reached the provider; see `ProviderProcessTeardownVerdict`. */ + readonly providerKilled: boolean } export type ProviderProcessClosePolicy = { @@ -31,6 +33,8 @@ export type ProviderProcessCloseResult = { root: DescendantTreeVerdict /** Null when this close made no observation of the descendants. */ tree: DescendantTreeVerdict | null + /** This close's kill reached the provider, whether or not the root's exit was seen after it. */ + providerKilled: boolean } export const ROOT_ONLY_GRACEFUL_EXIT_MS = 1_500 @@ -39,7 +43,7 @@ const ROOT_ONLY_FORCED_EXIT_MS = 1_000 /** Default for providers without a descendant reaper: end stdin, wait, then the fallback teardown. */ export function rootOnlyProviderClosePolicy(supervised: boolean): ProviderProcessClosePolicy { return { - gracefulExitMs: supervised ? PROVIDER_SUPERVISOR_MAX_STOP_MS : ROOT_ONLY_GRACEFUL_EXIT_MS, + gracefulExitMs: supervised ? SUPERVISED_PROVIDER_GRACEFUL_EXIT_MS : ROOT_ONLY_GRACEFUL_EXIT_MS, forcedExitMs: ROOT_ONLY_FORCED_EXIT_MS } } @@ -66,7 +70,7 @@ export async function closeProviderProcess( child.kill('SIGTERM') } let reaped = false - let fallbackTree: DescendantTreeVerdict | null = null + let fallback: ProviderProcessTeardownVerdict | null = null if (input.rootVerdict() !== 'exited') { await waitForProcessExitUntil(input.exitPromise, policy.gracefulExitMs) if (input.rootVerdict() !== 'exited') { @@ -75,7 +79,7 @@ export async function closeProviderProcess( if (tree) { await tree.reap() } else { - fallbackTree = await input.terminateTree() + fallback = await input.terminateTree() } await waitForProcessExitUntil(input.exitPromise, policy.forcedExitMs) } @@ -83,5 +87,9 @@ export async function closeProviderProcess( if (!reaped && input.rootVerdict() === 'exited' && tree && tree.treeVerdict !== 'exited') { await tree.reap() } - return { root: input.rootVerdict(), tree: tree ? tree.treeVerdict : fallbackTree } + return { + root: input.rootVerdict(), + tree: tree ? tree.treeVerdict : (fallback?.tree ?? null), + providerKilled: tree ? tree.providerKilled : fallback?.providerKilled === true + } } diff --git a/src/main/provider-process/provider-process-supervisor.ts b/src/main/provider-process/provider-process-supervisor.ts index ac47a4a51120..2869a3a21d8e 100644 --- a/src/main/provider-process/provider-process-supervisor.ts +++ b/src/main/provider-process/provider-process-supervisor.ts @@ -16,6 +16,9 @@ export const PROVIDER_GROUP_REAP_TIMEOUT_MS = 1_500 */ export const PROVIDER_SUPERVISOR_MAX_STOP_MS = PROVIDER_STDIN_END_GRACE_MS + PROVIDER_SIGTERM_GRACE_MS + PROVIDER_GROUP_REAP_TIMEOUT_MS +/** How long a close waits for a supervisor before forcing it: its own worst case, plus room for a + * loaded host to schedule its exit. */ +export const SUPERVISED_PROVIDER_GRACEFUL_EXIT_MS = PROVIDER_SUPERVISOR_MAX_STOP_MS + 500 /** Inline supervisor source kept dependency-free for the spawned Node child. */ export const POSIX_PROVIDER_SUPERVISOR_SCRIPT = ` diff --git a/src/main/provider-process/provider-process-teardown.test.ts b/src/main/provider-process/provider-process-teardown.test.ts index 75e181aaa46d..6e0ed15764c1 100644 --- a/src/main/provider-process/provider-process-teardown.test.ts +++ b/src/main/provider-process/provider-process-teardown.test.ts @@ -5,6 +5,7 @@ import { resetSelfInitiatedTreeKillLogForTest } from '../crash-reporting/self-initiated-tree-kill-log' import type { DescendantTreeVerdict } from '../pty-descendant-exit-verification' +import type { DescendantSnapshot, ProcessTableRow } from '../pty-descendant-termination' import { terminateProviderProcessTree } from './provider-process-teardown' /** Above pid_max on every supported POSIX host, so the group signal is a real ESRCH. */ @@ -17,12 +18,30 @@ function child() { } } +function row(pid: number, ppid: number, pgid: number): ProcessTableRow { + return { pid, ppid, pgid, startedAt: 'Mon Oct 5 10:00:00 2026' } +} + +/** A paused supervisor's walk: its provider (pid 1235) leads a group of its own. */ +function supervisorWalk(provider = 1235): DescendantSnapshot { + return { + root: { pid: 1234, startedAt: 'Mon Oct 5 09:59:00 2026' }, + rootPgid: 1234, + descendants: [row(provider, 1234, provider)], + capturedAtMs: 1 + } +} + +function signals(target: ReturnType): unknown[] { + return target.kill.mock.calls.map(([signal]) => signal) +} + describe('terminateProviderProcessTree', () => { beforeEach(() => { resetSelfInitiatedTreeKillLogForTest() }) - it('waits for the Windows tree kill before releasing the wrapper, and claims no observation', async () => { + it('waits for the Windows tree kill, then reports the root kill as the provider kill', async () => { const target = child() const release = Promise.withResolvers() const terminateWindowsTree = vi.fn(() => release.promise) @@ -34,13 +53,25 @@ describe('terminateProviderProcessTree', () => { }) expect(target.kill).not.toHaveBeenCalled() release.resolve() - // taskkill resolves alike on success, failure and timeout. - await expect(teardown).resolves.toBeNull() + // taskkill resolves alike on success, failure and timeout; TerminateProcess of the root is + // the provider's own kill, since Windows runs it without a supervisor. + await expect(teardown).resolves.toEqual({ tree: null, providerKilled: true }) expect(terminateWindowsTree).toHaveBeenCalledWith(1234, { site: 'codex-app-server-teardown' }) expect(target.kill).toHaveBeenCalledWith('SIGKILL') }) + it('claims no Windows provider kill when TerminateProcess was not delivered', async () => { + const target = { pid: 1234, kill: vi.fn(() => false) } + await expect( + terminateProviderProcessTree(target, { + site: 'codex-app-server-teardown', + platform: 'win32', + terminateWindowsTree: async () => undefined + }) + ).resolves.toEqual({ tree: null, providerKilled: false }) + }) + it('passes a non-Codex diagnostic label to Windows teardown', async () => { const terminateWindowsTree = vi.fn(async () => undefined) @@ -53,63 +84,78 @@ describe('terminateProviderProcessTree', () => { expect(terminateWindowsTree).toHaveBeenCalledWith(1234, { site: 'provider-test-teardown' }) }) - it('waits for an owned POSIX snapshot before killing the wrapper', async () => { + it("kills the provider's group, then the supervisor, before judging the rest of the tree", async () => { const target = child() - const snapshot = { rootPgid: 1234, descendants: [], capturedAtMs: 1 } + const order: string[] = [] + target.kill.mockImplementation((signal) => { + order.push(`supervisor ${String(signal)}`) + return true + }) + const walk = supervisorWalk() const release = Promise.withResolvers() + const terminateDescendants = vi.fn((snapshot: DescendantSnapshot) => { + order.push(`verify ${snapshot.descendants.length}`) + return release.promise + }) const teardown = terminateProviderProcessTree(target, { site: 'codex-app-server-teardown', platform: 'darwin', - captureDescendants: async () => snapshot, - terminateDescendants: () => release.promise + captureDescendants: async () => walk, + terminateDescendants, + signalProcessGroup: (pgid, signal) => order.push(`group ${pgid} ${signal}`) }) - await vi.waitFor(() => expect(target.kill).toHaveBeenCalledWith('SIGSTOP')) - expect(target.kill).not.toHaveBeenCalledWith('SIGKILL') + await vi.waitFor(() => expect(terminateDescendants).toHaveBeenCalledWith(walk)) release.resolve('exited') - await expect(teardown).resolves.toBe('exited') - - expect(target.kill).toHaveBeenLastCalledWith('SIGKILL') - }) + await expect(teardown).resolves.toEqual({ tree: 'exited', providerKilled: true }) - it('signals a proven dedicated POSIX process group without scanning descendants', async () => { - const target = child() - const captureDescendants = vi.fn() - const signalProcessGroup = vi.fn() - - await expect( - terminateProviderProcessTree(target, { - site: 'provider-test-teardown', - platform: 'darwin', - dedicatedProcessGroup: true, - captureDescendants, - signalProcessGroup - }) - ).resolves.toBeNull() - - expect(signalProcessGroup).toHaveBeenCalledWith(1234, 'SIGKILL') - expect(captureDescendants).not.toHaveBeenCalled() - expect(target.kill).not.toHaveBeenCalled() + expect(order).toEqual([ + 'supervisor SIGSTOP', + 'group 1235 SIGKILL', + 'supervisor SIGKILL', + 'verify 1' + ]) expect(findSelfInitiatedTreeKills(Date.now())).toEqual([ - expect.objectContaining({ site: 'provider-test-teardown', scope: 'posix-process-group' }) + expect.objectContaining({ + pid: 1235, + site: 'codex-app-server-teardown', + scope: 'posix-process-group' + }) ]) }) - it('keeps the dedicated-group wrapper reachable when signalling is unproven', async () => { - const target = child() + it.each(['live', 'unverifiable'] as const)( + 'reports a %s descendant left outside the killed group as-is, and still counts the provider killed', + async (observed) => { + const target = child() + await expect( + terminateProviderProcessTree(target, { + site: 'codex-app-server-teardown', + platform: 'darwin', + captureDescendants: async () => supervisorWalk(), + terminateDescendants: async () => observed, + signalProcessGroup: () => {} + }) + ).resolves.toEqual({ tree: observed, providerKilled: true }) + expect(signals(target)).toEqual(['SIGSTOP', 'SIGKILL']) + } + ) + // Before: an unreadable table SIGKILLed the paused supervisor, orphaning the provider in its own + // group with nobody left to stop it, and the root's exit then read as the provider gone. + it('never kills the supervisor without the group kill when the process table cannot be read', async () => { + const target = child() + const signalProcessGroup = vi.fn() await expect( terminateProviderProcessTree(target, { site: 'codex-app-server-teardown', - platform: 'linux', - dedicatedProcessGroup: true, - signalProcessGroup: () => { - throw Object.assign(new Error('denied'), { code: 'EPERM' }) - } + platform: 'darwin', + captureDescendants: async () => null, + signalProcessGroup }) - ).resolves.toBe('unverifiable') - - expect(target.kill).not.toHaveBeenCalled() + ).resolves.toEqual({ tree: null, providerKilled: false }) + expect(signals(target)).toEqual(['SIGSTOP', 'SIGCONT']) + expect(signalProcessGroup).not.toHaveBeenCalled() }) /** @@ -119,79 +165,60 @@ describe('terminateProviderProcessTree', () => { * through the real `process.kill(-pgid)` because the swallow being tested * lives in the production default, not in an injectable seam. */ - it('does not claim a snapshot group that was already gone', async () => { - const target = { pid: UNREACHABLE_PGID, kill: vi.fn(() => true) } + it('does not claim a provider group that was already gone, yet knows nothing in it runs', async () => { + const target = child() await expect( terminateProviderProcessTree(target, { site: 'codex-app-server-teardown', platform: 'darwin', - captureDescendants: async () => ({ - rootPgid: UNREACHABLE_PGID, - descendants: [], - capturedAtMs: 1 - }), + captureDescendants: async () => supervisorWalk(UNREACHABLE_PGID), terminateDescendants: async () => 'exited' }) - ).resolves.toBe('exited') + ).resolves.toEqual({ tree: 'exited', providerKilled: true }) expect(target.kill).toHaveBeenLastCalledWith('SIGKILL') expect(findSelfInitiatedTreeKills(Date.now())).toEqual([]) }) - it('claims a snapshot group the signal actually reached', async () => { + it('signals a proven dedicated POSIX process group without scanning descendants', async () => { const target = child() + const captureDescendants = vi.fn() const signalProcessGroup = vi.fn() await expect( terminateProviderProcessTree(target, { - site: 'codex-app-server-teardown', + site: 'provider-test-teardown', platform: 'darwin', - captureDescendants: async () => ({ rootPgid: 1234, descendants: [], capturedAtMs: 1 }), - terminateDescendants: async () => 'exited', + dedicatedProcessGroup: true, + captureDescendants, signalProcessGroup }) - ).resolves.toBe('exited') + ).resolves.toEqual({ tree: null, providerKilled: true }) expect(signalProcessGroup).toHaveBeenCalledWith(1234, 'SIGKILL') + expect(captureDescendants).not.toHaveBeenCalled() + expect(target.kill).not.toHaveBeenCalled() expect(findSelfInitiatedTreeKills(Date.now())).toEqual([ - expect.objectContaining({ - pid: 1234, - site: 'codex-app-server-teardown', - scope: 'posix-process-group' - }) + expect.objectContaining({ site: 'provider-test-teardown', scope: 'posix-process-group' }) ]) }) - it.each([ - ['live', 'live'], - ['unverifiable', 'unverifiable'] - ] as const)( - 'reports a %s descendant snapshot as-is and leaves the stopped root resumable', - async (observed, verdict) => { - const target = child() - await expect( - terminateProviderProcessTree(target, { - site: 'codex-app-server-teardown', - platform: 'darwin', - captureDescendants: async () => ({ rootPgid: 1234, descendants: [], capturedAtMs: 1 }), - terminateDescendants: async () => observed - }) - ).resolves.toBe(verdict) - expect(target.kill).toHaveBeenLastCalledWith('SIGCONT') - } - ) - - it('claims no observation when the POSIX process table cannot be read', async () => { + it('keeps the dedicated-group wrapper reachable when signalling is unproven', async () => { const target = child() + await expect( terminateProviderProcessTree(target, { site: 'codex-app-server-teardown', - platform: 'darwin', - captureDescendants: async () => null + platform: 'linux', + dedicatedProcessGroup: true, + signalProcessGroup: () => { + throw Object.assign(new Error('denied'), { code: 'EPERM' }) + } }) - ).resolves.toBeNull() - expect(target.kill).toHaveBeenLastCalledWith('SIGKILL') + ).resolves.toEqual({ tree: 'unverifiable', providerKilled: false }) + + expect(target.kill).not.toHaveBeenCalled() }) it('claims no observation from an ESRCH dedicated group and reports a spawnless child as unverifiable', async () => { @@ -204,11 +231,11 @@ describe('terminateProviderProcessTree', () => { throw Object.assign(new Error('gone'), { code: 'ESRCH' }) } }) - ).resolves.toBeNull() + ).resolves.toEqual({ tree: null, providerKilled: false }) const spawnless = { pid: undefined, kill: vi.fn(() => true) } await expect( terminateProviderProcessTree(spawnless, { site: 'provider-test-teardown', platform: 'linux' }) - ).resolves.toBe('unverifiable') + ).resolves.toEqual({ tree: 'unverifiable', providerKilled: false }) }) it('tears down 40 dedicated groups without process-table scans or cross-group fanout', async () => { @@ -232,7 +259,9 @@ describe('terminateProviderProcessTree', () => { ) ) - expect(results).toEqual(Array.from({ length: targets.length }, () => null)) + expect(results).toEqual( + Array.from({ length: targets.length }, () => ({ tree: null, providerKilled: true })) + ) expect(signalProcessGroup.mock.calls).toEqual(targets.map((target) => [target.pid, 'SIGKILL'])) expect(captureDescendants).not.toHaveBeenCalled() expect(killMocks.every((kill) => kill.mock.calls.length === 0)).toBe(true) diff --git a/src/main/provider-process/provider-process-teardown.ts b/src/main/provider-process/provider-process-teardown.ts index 7d30586777b0..0f8ae37b2e76 100644 --- a/src/main/provider-process/provider-process-teardown.ts +++ b/src/main/provider-process/provider-process-teardown.ts @@ -1,14 +1,21 @@ import type { ChildProcessHandle } from '../../shared/child-process/run-process' -import { captureDescendantSnapshot, type DescendantSnapshot } from '../pty-descendant-termination' +import type { DescendantSnapshot } from '../pty-descendant-termination' import { terminateDescendantSnapshotWithVerdict, type DescendantTreeVerdict } from '../pty-descendant-exit-verification' import { terminateWindowsProcessTree } from '../windows-process-tree-kill' import { recordSelfInitiatedTreeKill } from '../crash-reporting/self-initiated-tree-kill-log' +import { killSupervisedProviderGroup } from './provider-supervised-group-kill' -/** What the teardown observed of the descendants; null when it signalled but observed nothing. */ -export type ProviderProcessTeardownVerdict = DescendantTreeVerdict | null +/** What a forced teardown established. */ +export type ProviderProcessTeardownVerdict = { + /** What it observed of the descendants; null when it made no observation. */ + tree: DescendantTreeVerdict | null + /** The kill reached the provider, the process that writes the conversation: SIGKILL to its own + * group on POSIX, TerminateProcess of the root on Windows. Never an observed exit. */ + providerKilled: boolean +} const activeTeardowns = new WeakMap>() @@ -36,7 +43,8 @@ function terminateDedicatedPosixGroup( } catch (error) { // ESRCH says only that the group is empty; a descendant that left it, or a root that never led it, may live. // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: Node process.kill errors expose an optional errno code; only that field is read. - return (error as NodeJS.ErrnoException).code === 'ESRCH' ? null : 'unverifiable' + const tree = (error as NodeJS.ErrnoException).code === 'ESRCH' ? null : 'unverifiable' + return { tree, providerKilled: false } } // Outside the try: that catch is the ESRCH contract, not a breadcrumb handler. recordSelfInitiatedTreeKill({ @@ -45,7 +53,7 @@ function terminateDedicatedPosixGroup( scope: 'posix-process-group' }) // A delivered signal is not an observed exit. - return null + return { tree: null, providerKilled: true } } async function terminatePosixTree( @@ -53,53 +61,21 @@ async function terminatePosixTree( rootPid: number, deps: ProviderProcessTeardownDeps ): Promise { - child.kill('SIGSTOP') - const capture = deps.captureDescendants ?? captureDescendantSnapshot - const snapshot = await capture(rootPid).catch(() => null) - if (!snapshot) { - // No observation rather than the reaper's `unverifiable`: Codex's diagnostic treated this as accepted. - // The reaper-move follow-up maps it to `unverifiable` and takes that Codex change deliberately. - child.kill('SIGKILL') - return null + const forced = await killSupervisedProviderGroup(child, rootPid, { + site: deps.site, + ...(deps.captureDescendants ? { captureDescendants: deps.captureDescendants } : {}), + ...(deps.signalProcessGroup ? { signalProcessGroup: deps.signalProcessGroup } : {}) + }) + if (forced.provider === 'unknown' || !forced.snapshot) { + // Nothing was killed; the resumed supervisor's own stop is still under way. + return { tree: null, providerKilled: false } } + // Descendants that left the provider's group are judged by identity from the paused read. const terminate = deps.terminateDescendants ?? - ((captured: DescendantSnapshot) => terminateDescendantSnapshotWithVerdict(captured)) - const verdict = await terminate(snapshot) - const descendantsExited = verdict === 'exited' - // 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) => process.kill(-pgid, signal)) - let groupSignalled = false - try { - signalGroup(snapshot.rootPgid, 'SIGKILL') - groupSignalled = true - } catch { - // Already-gone is still the desired outcome, but nothing here killed it, - // and a crumb for a kill we never landed is a false render-process-gone suspect. - } - if (groupSignalled) { - // Outside the try, as in terminateDedicatedPosixGroup: that catch is the - // already-gone contract, not a breadcrumb handler. - recordSelfInitiatedTreeKill({ - pid: snapshot.rootPgid, - site: deps.site, - scope: 'posix-process-group' - }) - } - } - if (!descendantsExited) { - child.kill('SIGCONT') - return verdict - } - child.kill('SIGKILL') - return 'exited' + ((captured: DescendantSnapshot) => + terminateDescendantSnapshotWithVerdict(captured, { requireIdentityBeforeSignal: true })) + return { tree: await terminate(forced.snapshot), providerKilled: true } } /** Stops every process owned by one provider launch before releasing its wrapper. */ @@ -110,15 +86,16 @@ async function terminateOnce( const rootPid = child.pid if (!rootPid) { child.kill('SIGKILL') - return 'unverifiable' + return { tree: 'unverifiable', providerKilled: false } } if ((deps.platform ?? process.platform) === 'win32') { const terminate = deps.terminateWindowsTree ?? terminateWindowsProcessTree await terminate(rootPid, { site: deps.site }) - // taskkill owns the tree; this preserves the prior direct-child fallback when it fails. - child.kill('SIGKILL') + // taskkill owns the tree; TerminateProcess of the root is the provider's own kill, since + // Windows runs it without a supervisor. + const providerKilled = child.kill('SIGKILL') // taskkill resolves alike on success, failure and timeout, so nothing was observed. - return null + return { tree: null, providerKilled } } if (deps.dedicatedProcessGroup) { return terminateDedicatedPosixGroup(rootPid, deps) @@ -135,9 +112,10 @@ export function terminateProviderProcessTree( if (active) { return active } - const attempt = terminateOnce(child, deps).catch( - (): ProviderProcessTeardownVerdict => 'unverifiable' - ) + const attempt = terminateOnce(child, deps).catch((): ProviderProcessTeardownVerdict => ({ + tree: 'unverifiable', + providerKilled: false + })) activeTeardowns.set(key, attempt) void attempt.then(() => { if (activeTeardowns.get(key) === attempt) { diff --git a/src/main/provider-process/provider-supervised-group-kill.integration.test.ts b/src/main/provider-process/provider-supervised-group-kill.integration.test.ts new file mode 100644 index 000000000000..ebf65781349d --- /dev/null +++ b/src/main/provider-process/provider-supervised-group-kill.integration.test.ts @@ -0,0 +1,105 @@ +import { afterEach, describe, expect, it } from 'vitest' +import { spawnProcess } from '../../shared/child-process/run-process' +import { captureDescendantSnapshot } from '../pty-descendant-termination' +import { supervisedPosixLaunch } from './provider-process-supervisor' +import { killSupervisedProviderGroup } from './provider-supervised-group-kill' + +// A provider that ignores stdin end and SIGTERM, with a tool in its own group. +const PROVIDER = String.raw` + const { spawn } = require('node:child_process') + process.on('SIGTERM', () => {}) + const tool = spawn(process.execPath, ['-e', 'setInterval(() => {}, 60000)'], { stdio: 'ignore' }) + process.stdout.write(JSON.stringify({ provider: process.pid, tool: tool.pid }) + '\n') + setInterval(() => {}, 60000) +` + +const spawned = new Set() + +function alive(pid: number): boolean { + try { + process.kill(pid, 0) + return true + } catch (error) { + return !(error instanceof Error && 'code' in error && error.code === 'ESRCH') + } +} + +async function waitFor(predicate: () => boolean, timeoutMs = 5_000): Promise { + const deadline = Date.now() + timeoutMs + while (!predicate()) { + if (Date.now() >= deadline) { + return false + } + await new Promise((resolve) => setTimeout(resolve, 25)) + } + return true +} + +async function launch() { + const spec = supervisedPosixLaunch( + { command: process.execPath, args: ['-e', PROVIDER] }, + { ...process.env } + ) + const supervisor = spawnProcess({ + program: spec.command, + args: spec.args, + env: spec.env, + detached: true, + stdio: ['pipe', 'pipe', 'ignore'] + }) + spawned.add(supervisor.pid!) + const exited = new Promise((resolve) => + supervisor.once('exit', (_code, signal) => resolve(signal)) + ) + const pids = await new Promise<{ provider: number; tool: number }>((resolve, reject) => { + const timer = setTimeout(() => reject(new Error('the provider never reported')), 10_000) + supervisor.stdout!.once('data', (chunk: Buffer) => { + clearTimeout(timer) + resolve(JSON.parse(chunk.toString())) + }) + }) + spawned.add(pids.provider) + spawned.add(pids.tool) + return { supervisor, exited, ...pids } +} + +afterEach(() => { + for (const pid of spawned) { + if (alive(pid)) { + process.kill(pid, 'SIGKILL') + } + } + spawned.clear() +}) + +describe.runIf(process.platform !== 'win32')('the supervised provider forced kill', () => { + it("kills the provider and its group, which a SIGKILL to the supervisor alone can't reach", async () => { + const { supervisor, exited, provider, tool } = await launch() + + const result = await killSupervisedProviderGroup(supervisor, supervisor.pid!, { + site: 'integration-teardown' + }) + + expect(result.provider).toBe('killed') + expect(await exited).toBe('SIGKILL') + expect(await waitFor(() => !alive(provider) && !alive(tool))).toBe(true) + }) + + it('reads a provider that died while the supervisor was paused as gone, not unknown', async () => { + const { supervisor, exited, provider } = await launch() + + const result = await killSupervisedProviderGroup(supervisor, supervisor.pid!, { + site: 'integration-teardown', + captureDescendants: async (rootPid) => { + const snapshot = await captureDescendantSnapshot(rootPid) + // The paused supervisor cannot reap it: the group holds a zombie leader and a live tool. + process.kill(provider, 'SIGKILL') + return snapshot + } + }) + + // The live tool still takes the group signal; a zombie-only group answers EPERM on macOS. + expect(['killed', 'gone']).toContain(result.provider) + expect(await exited).toBe('SIGKILL') + }) +}) diff --git a/src/main/provider-process/provider-supervised-group-kill.test.ts b/src/main/provider-process/provider-supervised-group-kill.test.ts new file mode 100644 index 000000000000..9323ad7740fd --- /dev/null +++ b/src/main/provider-process/provider-supervised-group-kill.test.ts @@ -0,0 +1,180 @@ +import type { ChildProcess } from 'node:child_process' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { + findSelfInitiatedTreeKills, + resetSelfInitiatedTreeKillLogForTest +} from '../crash-reporting/self-initiated-tree-kill-log' +import type { DescendantSnapshot, ProcessTableRow } from '../pty-descendant-termination' +import { killSupervisedProviderGroup } from './provider-supervised-group-kill' + +const SUPERVISOR = 1_000 +const PROVIDER = 1_001 + +function supervisor(alive = true) { + return { kill: vi.fn(() => alive) } +} + +function row(pid: number, ppid: number, pgid: number): ProcessTableRow { + return { pid, ppid, pgid, startedAt: 'Mon Oct 5 10:00:00 2026' } +} + +/** The supervisor leads its own group; the provider leads another, with a tool in it. */ +function walk(descendants: ProcessTableRow[] = providerTree()): DescendantSnapshot { + return { + root: { pid: SUPERVISOR, startedAt: 'Mon Oct 5 09:59:00 2026' }, + rootPgid: SUPERVISOR, + descendants, + capturedAtMs: 1 + } +} + +function providerTree(): ProcessTableRow[] { + return [row(PROVIDER, SUPERVISOR, PROVIDER), row(1_002, PROVIDER, PROVIDER)] +} + +function errno(code: string): NodeJS.ErrnoException { + return Object.assign(new Error(code), { code }) +} + +function signals(target: ReturnType): unknown[] { + return target.kill.mock.calls.map(([signal]) => signal) +} + +describe('killSupervisedProviderGroup', () => { + beforeEach(() => { + resetSelfInitiatedTreeKillLogForTest() + }) + + it("kills the provider's own group while the supervisor is paused, then the supervisor", async () => { + const target = supervisor() + const order: string[] = [] + target.kill.mockImplementation((signal) => { + order.push(`supervisor ${String(signal)}`) + return true + }) + const signalProcessGroup = vi.fn((pgid: number, signal: NodeJS.Signals) => { + order.push(`group ${pgid} ${signal}`) + }) + + const result = await killSupervisedProviderGroup(target, SUPERVISOR, { + site: 'test-teardown', + captureDescendants: async () => walk(), + signalProcessGroup + }) + + expect(result.provider).toBe('killed') + expect(order).toEqual(['supervisor SIGSTOP', `group ${PROVIDER} SIGKILL`, 'supervisor SIGKILL']) + expect(findSelfInitiatedTreeKills(Date.now())).toEqual([ + expect.objectContaining({ + pid: PROVIDER, + site: 'test-teardown', + scope: 'posix-process-group' + }) + ]) + }) + + it.each([ + ['ESRCH', 'an empty group'], + ['EPERM', 'a group of zombies on macOS'] + ])('reads %s (%s) as nothing left to run, and still kills the supervisor', async (code) => { + const target = supervisor() + + const result = await killSupervisedProviderGroup(target, SUPERVISOR, { + site: 'test-teardown', + captureDescendants: async () => walk(), + signalProcessGroup: () => { + throw errno(code) + } + }) + + expect(result.provider).toBe('gone') + expect(signals(target)).toEqual(['SIGSTOP', 'SIGKILL']) + // Nothing here killed it, so there is no kill to report as Orca's. + expect(findSelfInitiatedTreeKills(Date.now())).toEqual([]) + }) + + it('resumes the supervisor, killing nothing, when the group signal fails for another reason', async () => { + const target = supervisor() + + const result = await killSupervisedProviderGroup(target, SUPERVISOR, { + site: 'test-teardown', + captureDescendants: async () => walk(), + signalProcessGroup: () => { + throw errno('EINVAL') + } + }) + + expect(result.provider).toBe('unknown') + expect(signals(target)).toEqual(['SIGSTOP', 'SIGCONT']) + }) + + it.each([ + ['the process table cannot be read', async () => null], + [ + 'the read fails', + async () => { + throw new Error('ps timed out') + } + ], + [ + 'the walk did not see the supervisor', + async (): Promise => ({ + rootPgid: null, + descendants: [], + capturedAtMs: 1 + }) + ], + [ + 'two children claim to lead their own group', + async () => walk([...providerTree(), row(1_003, SUPERVISOR, 1_003)]) + ] + ])( + 'never kills the supervisor without the group kill when %s', + async (_situation, captureDescendants) => { + const target = supervisor() + const signalProcessGroup = vi.fn() + + const result = await killSupervisedProviderGroup(target, SUPERVISOR, { + site: 'test-teardown', + captureDescendants, + signalProcessGroup + }) + + expect(result.provider).toBe('unknown') + expect(signalProcessGroup).not.toHaveBeenCalled() + // Resumed, the supervisor's own stop still reaches the provider's group. + expect(signals(target)).toEqual(['SIGSTOP', 'SIGCONT']) + } + ) + + it('reads a paused supervisor with no provider child as the provider already reaped', async () => { + const target = supervisor() + const signalProcessGroup = vi.fn() + + const result = await killSupervisedProviderGroup(target, SUPERVISOR, { + site: 'test-teardown', + // A grandchild leading its own group is not the supervisor's provider. + captureDescendants: async () => walk([row(1_005, 1_004, 1_005)]), + signalProcessGroup + }) + + expect(result.provider).toBe('gone') + expect(signalProcessGroup).not.toHaveBeenCalled() + // It is clearing the rest of its group; killing it now could orphan what is left there. + expect(signals(target)).toEqual(['SIGSTOP', 'SIGCONT']) + }) + + it('does nothing more once the supervisor has exited', async () => { + const target = supervisor(false) + const captureDescendants = vi.fn() + + const result = await killSupervisedProviderGroup(target, SUPERVISOR, { + site: 'test-teardown', + captureDescendants + }) + + expect(result).toEqual({ provider: 'unknown', snapshot: null }) + expect(captureDescendants).not.toHaveBeenCalled() + expect(signals(target)).toEqual(['SIGSTOP']) + }) +}) diff --git a/src/main/provider-process/provider-supervised-group-kill.ts b/src/main/provider-process/provider-supervised-group-kill.ts new file mode 100644 index 000000000000..616960d5d221 --- /dev/null +++ b/src/main/provider-process/provider-supervised-group-kill.ts @@ -0,0 +1,104 @@ +// The forced step of a POSIX provider close: kill the provider's own process group, then its +// supervisor. +// +// The supervisor starts the provider (the process that writes the conversation) in a process group +// of its own, so a SIGKILL to the supervisor alone leaves the provider running with nobody to stop +// it. Paused, the supervisor cannot reap its child, so the child's row, pid and group id stay +// exactly the provider's until the group kill lands; that is what makes the kill's delivery a fact +// about the provider rather than a guess about a pid. + +import type { ChildProcessHandle } from '../../shared/child-process/run-process' +import { captureDescendantSnapshot, type DescendantSnapshot } from '../pty-descendant-termination' +import { recordSelfInitiatedTreeKill } from '../crash-reporting/self-initiated-tree-kill-log' + +/** + * - `killed`: SIGKILL reached the provider's group; nothing in it runs its own code again. + * - `gone`: the paused supervisor holds no provider, or its group has nothing left to signal. + * - `unknown`: no exact answer, so nothing was killed and the supervisor runs its own stop. + */ +export type SupervisedProviderKill = 'killed' | 'gone' | 'unknown' + +export type SupervisedProviderGroupKillDeps = { + site: string + captureDescendants?: (rootPid: number) => Promise + signalProcessGroup?: (pgid: number, signal: NodeJS.Signals) => void +} + +type SupervisorHandle = Pick + +function signalProcessGroup(pgid: number, signal: NodeJS.Signals): void { + process.kill(-pgid, signal) +} + +/** Errors that mean no process in the group can run: ESRCH, an empty group; EPERM, on macOS a + * group whose members are all zombies (the provider runs as this user, so a live one is + * signalable). */ +function groupHasNothingToRun(error: unknown): boolean { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: Node process.kill errors expose an optional errno code; only that field is read. + const code = (error as NodeJS.ErrnoException | null)?.code + return code === 'ESRCH' || code === 'EPERM' +} + +/** The supervisor's own child that leads its own group: the provider. Null when the walk did not + * see the supervisor itself, or saw more than one candidate. */ +function providerGroupLeader( + snapshot: DescendantSnapshot, + supervisorPid: number +): { pid: number } | 'none' | null { + if (snapshot.rootPgid === null) { + return null + } + const leaders = snapshot.descendants.filter( + (row) => row.ppid === supervisorPid && row.pgid === row.pid + ) + if (leaders.length === 0) { + return 'none' + } + return leaders.length === 1 ? leaders[0] : null +} + +/** + * Pauses the supervisor, kills the provider's group, then kills the supervisor. The supervisor is + * never killed without the group kill: without an exact answer it is resumed so its own stop + * still reaches the group. Resolves the walk taken while the supervisor was paused, so callers can + * judge the rest of the tree from the same read. + */ +export async function killSupervisedProviderGroup( + supervisor: SupervisorHandle, + supervisorPid: number, + deps: SupervisedProviderGroupKillDeps +): Promise<{ provider: SupervisedProviderKill; snapshot: DescendantSnapshot | null }> { + if (!supervisor.kill('SIGSTOP')) { + // The handle is gone, so the supervisor already exited; its exit is the answer. + return { provider: 'unknown', snapshot: null } + } + const capture = deps.captureDescendants ?? captureDescendantSnapshot + const snapshot = await capture(supervisorPid).catch(() => null) + const leader = snapshot ? providerGroupLeader(snapshot, supervisorPid) : null + if (leader === null) { + supervisor.kill('SIGCONT') + return { provider: 'unknown', snapshot } + } + if (leader === 'none') { + // Already reaped by the supervisor, which is mid-way through clearing the rest of its group. + supervisor.kill('SIGCONT') + return { provider: 'gone', snapshot } + } + const signal = deps.signalProcessGroup ?? signalProcessGroup + let provider: SupervisedProviderKill + try { + signal(leader.pid, 'SIGKILL') + provider = 'killed' + } catch (error) { + if (!groupHasNothingToRun(error)) { + supervisor.kill('SIGCONT') + return { provider: 'unknown', snapshot } + } + provider = 'gone' + } + if (provider === 'killed') { + recordSelfInitiatedTreeKill({ pid: leader.pid, site: deps.site, scope: 'posix-process-group' }) + } + supervisor.kill('SIGKILL') + return { provider, snapshot } +} diff --git a/src/main/runtime/agent-session-acquisition-failure-settlement.ts b/src/main/runtime/agent-session-acquisition-failure-settlement.ts index f6efaa890c37..8f21a152096b 100644 --- a/src/main/runtime/agent-session-acquisition-failure-settlement.ts +++ b/src/main/runtime/agent-session-acquisition-failure-settlement.ts @@ -18,6 +18,8 @@ import type { AgentSessionStoreState } from './agent-session-record-store-file' * identity this lease is keyed on is dead, but its descendants were not proven * gone. Releases the lease and says exactly that, claiming nothing more. * - `processless`: the attempt failed before a process existed. + * - `killed`: cleanup's kill reached the provider but its exit was not seen. Nothing it reached can + * write again, so the lease is released, with no evidence since no exit was observed. * - `unproven`: nothing about the process was observed. A recorded owner goes to recovery, which * concludes about it; a reservation that recorded none is released, since the adapter already * closed the stdio of anything it spawned. @@ -26,6 +28,7 @@ export type AgentSessionAcquisitionExitProof = | 'exit-proven' | 'root-exit-observed' | 'processless' + | 'killed' | 'unproven' export type AgentSessionFailedAcquisitionSettlement = { @@ -103,15 +106,18 @@ export function settleFailedAgentSessionPostAcquisitionAttachment( claimStatus: 'released', lastRenewedAt: args.now, handoffOperationId: null, - deathEvidence: { - kind: 'exit-observed', - detail: - args.exitProof === 'root-exit-observed' - ? 'the provider process exited; its descendants were not proven gone' - : 'post-acquisition cleanup proved no provider child remains', - observedAt: args.now, - ownerFence: record.lease.runtimeFence - } + deathEvidence: + args.exitProof === 'killed' + ? null + : { + kind: 'exit-observed', + detail: + args.exitProof === 'root-exit-observed' + ? 'the provider process exited; its descendants were not proven gone' + : 'post-acquisition cleanup proved no provider child remains', + observedAt: args.now, + ownerFence: record.lease.runtimeFence + } }) state.records.set(args.sessionId, next) state.operations = settleAgentSessionOperation(state.operations, args) @@ -164,7 +170,7 @@ function acquisitionDeathEvidence( observedAt: number, ownerFence: number ): AgentSessionDeathEvidence | null { - if (exitProof === 'unproven') { + if (exitProof === 'unproven' || exitProof === 'killed') { return null } const proof = { observedAt, ownerFence } diff --git a/src/main/runtime/agent-session-death-evidence-persistence.test.ts b/src/main/runtime/agent-session-death-evidence-persistence.test.ts index 73d01b5b88e8..6d1f74004ffa 100644 --- a/src/main/runtime/agent-session-death-evidence-persistence.test.ts +++ b/src/main/runtime/agent-session-death-evidence-persistence.test.ts @@ -219,4 +219,40 @@ describe('a failed acquisition', () => { expect(settled.lease.deathEvidence).toMatchObject({ ownerFence: fence }) } ) + + it.each([ + ['before the owner proved its handle', false], + ['after the owner proved its handle', true] + ] as const)( + 'releases a failed start whose cleanup killed the provider, with no exit evidence, %s', + async (_when, proved) => { + const store = await open() + const fence = await spawnedOwner(store) + const killed = { ...unproven(fence, NOW + 60_000), exitProof: 'killed' as const } + if (proved) { + await store.proveOwner({ + sessionId: SESSION, + fence, + link: { + linkId: 'link-1', + handle: claudeProviderHandle('provider-session-1', null), + origin: 'created', + mintedAtFence: fence, + observedAt: NOW + }, + now: NOW + }) + await store.settleFailedPostAcquisitionAttachment(killed) + } else { + await store.settleFailedAcquisition(killed) + } + // Released, never parked in recovery: nothing it reached can write again. + expect(store.getRecord(SESSION)?.lease).toMatchObject({ + claimStatus: 'released', + handoffStage: null, + ownerProcess: null, + deathEvidence: null + }) + } + ) }) diff --git a/src/main/runtime/agent-session-surface-release-transition.ts b/src/main/runtime/agent-session-surface-release-transition.ts index 1099e8d6c045..1bf94276f30e 100644 --- a/src/main/runtime/agent-session-surface-release-transition.ts +++ b/src/main/runtime/agent-session-surface-release-transition.ts @@ -3,7 +3,8 @@ // Every other release in the wire needs a probe, because every other release is about a process // somebody else started and nobody watched die. This one is different: the host stopped its own // lease-owning provider root through the adapter. Its observed exit is sufficient because the -// lease follows that root, even when descendants remain `unverifiable`. +// lease follows that root, even when descendants remain `unverifiable`; so is a kill the host +// delivered to the provider itself, which never writes again, released with no exit evidence. // // The fence still moves, so the next owner is a new generation: an attach or settlement still // holding the stopped owner's fence is refused as stale rather than acting on its successor. @@ -37,6 +38,9 @@ export function releaseAgentSessionOwnerAfterSurfaceClose(args: { exitObservedAt?: number /** Why the provider exited, when the host saw it die on its own. */ exitReason?: string + /** The host's kill reached the provider but no exit was seen: nothing to record as evidence, + * the same release recovery writes for an owner it could not prove gone. */ + rootExitUnobserved?: true }): AgentSessionRecord { const { record } = args assertFence(record.lease, args.expectedFence) @@ -51,15 +55,17 @@ export function releaseAgentSessionOwnerAfterSurfaceClose(args: { claimStatus: 'released', handoffStage: null, lastRenewedAt: args.now, - deathEvidence: { - kind: 'exit-observed', - // A provider's exit reason can carry kilobytes of stderr; a longer detail fails the write. - detail: args.exitReason - ? args.exitReason.slice(0, MAX_AGENT_SESSION_DEATH_DETAIL_CHARS) - : 'the last surface holding this session released it', - observedAt: args.exitObservedAt ?? args.now, - ownerFence: record.lease.runtimeFence - } + deathEvidence: args.rootExitUnobserved + ? null + : { + kind: 'exit-observed', + // A provider's exit reason can carry kilobytes of stderr; a longer detail fails the write. + detail: args.exitReason + ? args.exitReason.slice(0, MAX_AGENT_SESSION_DEATH_DETAIL_CHARS) + : 'the last surface holding this session released it', + observedAt: args.exitObservedAt ?? args.now, + ownerFence: record.lease.runtimeFence + } }) } @@ -72,6 +78,7 @@ export function releaseStoredAgentSessionOwnerAfterSurfaceClose( now: number exitObservedAt?: number exitReason?: string + rootExitUnobserved?: true } ): Promise { return store.transitionHandoff(args.sessionId, (record) =>