From b4c19f12c47887d1ab5c459823a2ed21f79ccdb6 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 28 Sep 2026 20:49:32 -0700 Subject: [PATCH] fix(claude): run structured Claude under the POSIX provider supervisor (#23476) * fix(codex): the provider supervisor outlives its provider group when stopped A signalled supervisor forwards the signal to the provider group, escalates to SIGKILL after the grace, and exits only once the group is gone, so recovery's proof that the recorded pid is dead also proves the provider is. It refuses to spawn when its parent is already not the owner named in its spec, and watches that owner rather than whichever parent it first saw. The grace is a spec field. Recovery's SIGTERM stage now outlasts the supervisor's own stop, since a SIGKILL that lands first cannot be handled and leaves the group running. * fix(codex): a closed owner pipe no longer ends the supervisor before its provider group When Orca dies, the supervisor's stdout pipe has no reader. Provider output in the window before the parent-death watch fired raised an unhandled EPIPE that exited the supervisor with the provider group still running. * fix(codex): bound the supervisor grace so recovery's SIGTERM stage always covers it Recovery sized its SIGTERM stage from the default grace, so a launch with a longer grace would be SIGKILLed mid-stop and orphan its group with no test noticing. The spec now refuses any grace above one exported maximum, and recovery derives its SIGTERM stage from that maximum. * fix(codex): every supervisor stop asks the provider with SIGTERM first Owner death, stdin end after the grace, and a signal to the supervisor now all take one path: SIGTERM the provider group, SIGKILL it after the grace, and exit only once it is gone. The signal handlers are registered before the provider is spawned, so a stop that lands in the spawn window still reaps it. The longest stop grows to two graces plus the reap wait, and both recovery's SIGTERM stage and the connection's graceful close now wait that long before forcing, since forcing the supervisor sooner can orphan its group. * fix(claude): run the structured Claude child under the POSIX provider supervisor A close now stops Claude with a SIGTERM through the supervisor instead of letting stdin end finish the turn, and Orca's death stops it through the supervisor. * test(claude): pin the supervised stop against a real Claude CLI, opt-in * test(claude): a requested stop reads interrupted through the frames the supervised SIGTERM makes Claude emit * test(claude): show what the real CLI did when it never ran the tool * test(claude): Orca's death now reaps Claude's own tool through its SIGTERM * refactor(claude): take supervision from the spawn spec so the close ladder cannot disagree with the spawn createProviderSpawnSpec now reports whether it wrapped the provider in the supervisor, and the Claude spawner reads that instead of repeating the platform check. The close ladder's SIGTERM follows the process actually spawned. * fix(native-chat): derive quit's chat-eviction bound from the longest supervised provider close Quit's child-eviction phase was a hand-picked 8 s. It is now the sink drain plus the longest supervised close over Claude and Codex plus a named 1 s margin, so a provider close that grows widens it instead of silently outrunning it. A close's tree-kill fallback stays outside the bound: once main exits, the supervisor stops its provider group on owner death, which a new test now proves for a clean owner exit, and next launch's recovery settles the lease. --- .../claude-agent-sdk-process-spawn.test.ts | 104 +++++++- .../claude/claude-agent-sdk-process-spawn.ts | 38 ++- .../claude-child-exit-proof-ladder.test.ts | 72 ++++++ .../claude/claude-child-exit-proof-ladder.ts | 19 +- .../claude/claude-child-root-termination.ts | 3 + .../claude-stream-json-connection.test.ts | 101 +++++--- .../claude/claude-stream-json-connection.ts | 7 +- .../claude-structured-requested-stop.test.ts | 90 +++++++ .../claude-supervised-stop-real-cli.test.ts | 244 ++++++++++++++++++ ...claude-supervised-stop.integration.test.ts | 225 ++++++++++++++++ src/main/codex/codex-app-server-connection.ts | 2 +- ...erver-posix-supervisor.integration.test.ts | 32 ++- .../codex-app-server-posix-supervisor.test.ts | 3 +- .../codex-app-server-posix-supervisor.ts | 26 +- ...ctured-agent-session-host-teardown.test.ts | 68 ++++- .../structured-agent-session-host-teardown.ts | 21 +- ...tured-agent-session-recovery-resolution.ts | 4 +- 17 files changed, 979 insertions(+), 80 deletions(-) create mode 100644 src/main/claude/claude-child-exit-proof-ladder.test.ts create mode 100644 src/main/claude/claude-structured-requested-stop.test.ts create mode 100644 src/main/claude/claude-supervised-stop-real-cli.test.ts create mode 100644 src/main/claude/claude-supervised-stop.integration.test.ts 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 cd3520cf6d5..1b6c192d433 100644 --- a/src/main/claude/claude-agent-sdk-process-spawn.test.ts +++ b/src/main/claude/claude-agent-sdk-process-spawn.test.ts @@ -4,14 +4,22 @@ import { describe, expect, it, vi } from 'vitest' import type { SpawnOptions as SdkSpawnOptions } from '@anthropic-ai/claude-agent-sdk' import { resolveSpawn, type spawnProcess } from '../../shared/child-process/run-process' import type { ProcessSpec } from '../../shared/child-process/process-spec' +import type * as ProviderSupervisor from '../codex/codex-app-server-posix-supervisor' +import { createProviderSpawnSpec } from '../codex/codex-app-server-posix-supervisor' import { createClaudeCodeProcessSpawn } from './claude-agent-sdk-process-spawn' +import { proveClaudeChildExitWithReaper } from './claude-child-exit-proof-ladder' + +vi.mock('../codex/codex-app-server-posix-supervisor', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, createProviderSpawnSpec: vi.fn(actual.createProviderSpawnSpec) } +}) type FakeChild = EventEmitter & { pid: number stdin: PassThrough stdout: PassThrough stderr: PassThrough - kill: ReturnType + kill: ReturnType boolean>> } function fakeSpawn() { @@ -20,7 +28,7 @@ function fakeSpawn() { child.stdin = new PassThrough() child.stdout = new PassThrough() child.stderr = new PassThrough() - child.kill = vi.fn(() => true) + child.kill = vi.fn((_signal?: NodeJS.Signals | number) => true) const specs: ProcessSpec[] = [] const spawnImpl = ((spec: ProcessSpec) => { specs.push(spec) @@ -43,7 +51,7 @@ function sdkOptions(overrides: Partial = {}): SdkSpawnOptions { describe('claude agent SDK process spawn', () => { it('routes the SDK spawn through Orca and retains the pid the lease adjudicates on', () => { const process = fakeSpawn() - const spawn = createClaudeCodeProcessSpawn(process.spawnImpl) + const spawn = createClaudeCodeProcessSpawn(process.spawnImpl, 'win32') expect(spawn.pid).toBeUndefined() expect(spawn.child).toBeNull() @@ -52,15 +60,103 @@ describe('claude agent SDK process spawn', () => { expect(child).toBe(process.child) expect(spawn.child).toBe(process.child) expect(spawn.pid).toBe(4321) + // Windows has no supervisor: Claude itself is the child. + expect(spawn.supervised).toBe(false) expect(process.specs[0]).toEqual({ program: '/usr/local/bin/claude', args: ['--output-format', 'stream-json'], cwd: '/work/repo', env: { PATH: '/usr/bin', CLAUDE_CONFIG_DIR: '/accounts/one' }, + detached: false, stdio: ['pipe', 'pipe', 'pipe'] }) }) + it.each(['darwin', 'linux'] as const)( + 'starts Claude under the provider supervisor on %s, which is then the pid the lease records', + (platform) => { + const process = fakeSpawn() + const spawn = createClaudeCodeProcessSpawn(process.spawnImpl, platform) + spawn.spawn(sdkOptions()) + + const [spec] = process.specs + if (!spec) { + throw new Error('the spawner never built a spec') + } + expect(spawn.supervised).toBe(true) + expect(spawn.pid).toBe(4321) + expect(spec.program).toBe(globalThis.process.execPath) + expect(spec.args?.[0]).toBe('-e') + expect(spec.detached).toBe(true) + expect(spec.cwd).toBe('/work/repo') + const supervisorSpec = JSON.parse( + Buffer.from(String(spec.env?.ORCA_PROVIDER_SUPERVISOR_SPEC), 'base64').toString() + ) + expect(supervisorSpec).toMatchObject({ + command: '/usr/local/bin/claude', + args: ['--output-format', 'stream-json'], + cwd: '/work/repo', + ownerPid: globalThis.process.pid + }) + // The supervisor passes its env to Claude minus its own two keys. + expect(spec.env).toMatchObject({ PATH: '/usr/bin', CLAUDE_CONFIG_DIR: '/accounts/one' }) + } + ) + + it.each([ + { platform: 'darwin', specSupervised: false }, + { platform: 'win32', specSupervised: true } + ] as const)( + 'stops Claude by the spawn spec\u2019s supervision on $platform, never the platform', + async ({ platform, specSupervised }) => { + const actual = await vi.importActual( + '../codex/codex-app-server-posix-supervisor' + ) + vi.mocked(createProviderSpawnSpec).mockImplementationOnce((...args) => ({ + ...actual.createProviderSpawnSpec(...args), + supervised: specSupervised + })) + const process = fakeSpawn() + const spawn = createClaudeCodeProcessSpawn(process.spawnImpl, platform) + spawn.spawn(sdkOptions()) + expect(spawn.supervised).toBe(specSupervised) + + let exited = false + let settle = (): void => {} + const exitPromise = new Promise((resolve) => { + settle = resolve + }) + // Claude leaves shortly after stdin ends, so the ladder never needs its forced rung. + process.child.stdin.on('finish', () => + setTimeout(() => { + exited = true + settle() + }, 10) + ) + const tree = { + capture: vi.fn(async () => {}), + reap: vi.fn(async () => 'exited' as const), + treeVerdict: 'exited' as const + } + await proveClaudeChildExitWithReaper( + { + child: process.child, + exitPromise, + exited: () => exited, + tree, + supervised: spawn.supervised + }, + () => tree + ) + // SIGTERM to an unsupervised Claude on Windows is TerminateProcess; a skipped one leaves it running. + if (specSupervised) { + expect(process.child.kill).toHaveBeenCalledWith('SIGTERM') + } else { + expect(process.child.kill).not.toHaveBeenCalled() + } + } + ) + it('keeps the child out of the SDK abort path so exit proof stays Orca-owned', () => { const process = fakeSpawn() const controller = new AbortController() @@ -86,7 +182,7 @@ describe('claude agent SDK process spawn', () => { it('hands a Windows .cmd shim to Orca\u2019s argument encoder', () => { const process = fakeSpawn() - createClaudeCodeProcessSpawn(process.spawnImpl).spawn( + createClaudeCodeProcessSpawn(process.spawnImpl, 'win32').spawn( sdkOptions({ command: 'C:\\Users\\dev\\AppData\\npm\\claude.cmd', args: ['--setting-sources=user,project,local', '--session-id', 'a b&c'] diff --git a/src/main/claude/claude-agent-sdk-process-spawn.ts b/src/main/claude/claude-agent-sdk-process-spawn.ts index a2b1ad7f158..bbb4b295aad 100644 --- a/src/main/claude/claude-agent-sdk-process-spawn.ts +++ b/src/main/claude/claude-agent-sdk-process-spawn.ts @@ -1,5 +1,6 @@ import type { SpawnOptions as ClaudeAgentSdkSpawnOptions } from '@anthropic-ai/claude-agent-sdk' import { spawnProcess } from '../../shared/child-process/run-process' +import { createProviderSpawnSpec } from '../codex/codex-app-server-posix-supervisor' /** Derived rather than imported: only src/shared/child-process may name node:child_process. */ type ClaudeCodeChild = ReturnType @@ -11,8 +12,13 @@ export type ClaudeCodeProcessSpawn = { spawn: (options: ClaudeAgentSdkSpawnOptions) => ClaudeCodeChild /** The retained child, so Orca keeps its own tree-kill and exit-proof ladder. Null until the SDK spawns. */ readonly child: ClaudeCodeChild | null - /** Ownership proof: the durable lease adjudicates on this pid plus start time plus the spawn token. */ + /** + * Ownership proof: the durable lease adjudicates on this pid plus start time plus the spawn + * token. On POSIX it is the provider supervisor's, which outlives Claude by construction. + */ readonly pid: number | undefined + /** The spawn spec's verdict, so the close ladder never re-decides it. False until the SDK spawns. */ + readonly supervised: boolean readonly stderrTail: string } @@ -31,24 +37,41 @@ function definedEnv(env: Record): Record { + const spec = createProviderSpawnSpec( + { + command: options.command, + args: [...options.args], + ...(options.cwd === undefined ? {} : { cwd: options.cwd }) + }, + definedEnv(options.env), + platform + ) // Why `options.signal` is dropped: it would let the SDK kill the child outside // Orca's ladder, and close() may never report an exit it did not observe. const spawned = spawnImpl({ - program: options.command, - args: [...options.args], - ...(options.cwd === undefined ? {} : { cwd: options.cwd }), - env: definedEnv(options.env), + program: spec.program, + args: spec.args, + cwd: spec.cwd, + env: spec.env, + detached: spec.detached, stdio: ['pipe', 'pipe', 'pipe'] }) child = spawned + supervised = spec.supervised // The SDK drains stderr only for its own local spawn, so a custom spawner must: // otherwise the child blocks on a full pipe and exit errors lose their tail. spawned.stderr.setEncoding('utf8').on('data', (chunk: string) => { @@ -62,6 +85,9 @@ export function createClaudeCodeProcessSpawn( get pid() { return child?.pid }, + get supervised() { + return supervised + }, get stderrTail() { return stderrTail } diff --git a/src/main/claude/claude-child-exit-proof-ladder.test.ts b/src/main/claude/claude-child-exit-proof-ladder.test.ts new file mode 100644 index 00000000000..2d8da7f629d --- /dev/null +++ b/src/main/claude/claude-child-exit-proof-ladder.test.ts @@ -0,0 +1,72 @@ +import { describe, expect, it, vi } from 'vitest' +import { PROVIDER_SUPERVISOR_MAX_STOP_MS } from '../codex/codex-app-server-posix-supervisor' +import type { ClaudeChildTreeReaper } from './claude-agent-sdk-exit-proof' +import { proveClaudeChildExitWithReaper } from './claude-child-exit-proof-ladder' + +function fakeTree(): ClaudeChildTreeReaper & { reap: ReturnType } { + return { + capture: vi.fn(async () => {}), + refresh: vi.fn(async () => {}), + reap: vi.fn(async () => 'exited' as const), + treeVerdict: 'exited' + } +} + +/** A root that leaves only once a SIGTERM has had `stopMs` to act, the way a supervisor does. */ +function rootStoppedBySigterm(stopMs: number) { + let exited = false + let settle = (): void => {} + const exitPromise = new Promise((resolve) => { + settle = resolve + }) + const kill = vi.fn((signal?: NodeJS.Signals | number) => { + if (signal === 'SIGTERM') { + setTimeout(() => { + exited = true + settle() + }, stopMs) + } + return true + }) + const stdin = { end: vi.fn() } + return { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: The ladder reads only pid, kill and stdin.end from its child. + child: { pid: 4321, kill, stdin } as unknown as Parameters< + typeof proveClaudeChildExitWithReaper + >[0]['child'], + kill, + stdin, + exitPromise, + exited: () => exited + } +} + +describe('Claude child exit proof ladder', () => { + it('stops a supervised child with SIGTERM and waits out the supervisor stop before forcing', async () => { + // Slower than the unsupervised 1.5 s grace, still inside the supervisor's own bound. + const root = rootStoppedBySigterm(PROVIDER_SUPERVISOR_MAX_STOP_MS - 500) + const tree = fakeTree() + + await expect( + proveClaudeChildExitWithReaper({ ...root, supervised: true, tree }, () => tree) + ).resolves.toBe(true) + + expect(root.stdin.end).toHaveBeenCalled() + expect(root.kill).toHaveBeenCalledWith('SIGTERM') + // Forcing here would SIGKILL the supervisor mid-stop and orphan Claude in its own group. + expect(root.kill).not.toHaveBeenCalledWith('SIGKILL') + expect(tree.reap).not.toHaveBeenCalled() + }, 10_000) + + it('never signals an unsupervised child for the graceful stop', async () => { + const root = rootStoppedBySigterm(0) + const tree = fakeTree() + + await proveClaudeChildExitWithReaper({ ...root, tree }, () => tree) + + // On Windows a direct SIGTERM is TerminateProcess: stdin end stays the only graceful rung. + expect(root.stdin.end).toHaveBeenCalled() + expect(root.kill).not.toHaveBeenCalledWith('SIGTERM') + expect(tree.reap).toHaveBeenCalled() + }, 10_000) +}) diff --git a/src/main/claude/claude-child-exit-proof-ladder.ts b/src/main/claude/claude-child-exit-proof-ladder.ts index 85ed629f1b9..37d4e47508d 100644 --- a/src/main/claude/claude-child-exit-proof-ladder.ts +++ b/src/main/claude/claude-child-exit-proof-ladder.ts @@ -1,8 +1,11 @@ import type { SpawnedProcess } from '../../shared/child-process/run-process' import { waitForProcessExitUntil } from '../codex/codex-process-exit-deadline' +import { PROVIDER_SUPERVISOR_MAX_STOP_MS } from '../codex/codex-app-server-posix-supervisor' import type { ClaudeChildTreeReaper } from './claude-agent-sdk-exit-proof' -const GRACEFUL_EXIT_MS = 1_500 +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 const FORCED_EXIT_MS = 1_000 export type ClaudeChildExitProofInput = { @@ -10,6 +13,8 @@ export type ClaudeChildExitProofInput = { exitPromise: Promise exited: () => boolean tree?: ClaudeChildTreeReaper + /** The child is the POSIX provider supervisor: SIGTERM stops Claude, which reaps its tools. */ + supervised?: boolean } export async function proveClaudeChildExitWithReaper( @@ -17,16 +22,24 @@ export async function proveClaudeChildExitWithReaper( createTree: () => ClaudeChildTreeReaper ): Promise { const tree = input.tree ?? createTree() - // Arm before stdin closes: only a live root can identify its descendants. + // Arm before the stop: only a live root can identify its descendants. await tree.capture() try { input.child.stdin?.end() } catch { // The reap below still owns the process. } + // Stdin end alone lets Claude finish its turn, tools and edits included; a close is a stop. + // Windows has no supervisor, and a direct SIGTERM there is TerminateProcess. + if (input.supervised && !input.exited()) { + input.child.kill('SIGTERM') + } let reaped = false if (!input.exited()) { - await waitForProcessExitUntil(input.exitPromise, GRACEFUL_EXIT_MS) + await waitForProcessExitUntil( + input.exitPromise, + input.supervised ? SUPERVISED_GRACEFUL_EXIT_MS : GRACEFUL_EXIT_MS + ) if (!input.exited()) { reaped = true await tree.refresh?.() diff --git a/src/main/claude/claude-child-root-termination.ts b/src/main/claude/claude-child-root-termination.ts index bed422532e1..bba29919578 100644 --- a/src/main/claude/claude-child-root-termination.ts +++ b/src/main/claude/claude-child-root-termination.ts @@ -19,6 +19,9 @@ 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. + * * False means no signal was sent, because the root had already left. */ export function terminateClaudeRoot(input: RootTerminationInput): boolean { diff --git a/src/main/claude/claude-stream-json-connection.test.ts b/src/main/claude/claude-stream-json-connection.test.ts index 6d3f0d89f30..e3682f03306 100644 --- a/src/main/claude/claude-stream-json-connection.test.ts +++ b/src/main/claude/claude-stream-json-connection.test.ts @@ -107,6 +107,18 @@ async function open( return connection } +function launchedArgv(spec: ProcessSpec | undefined): string[] { + const supervised = spec?.env?.ORCA_PROVIDER_SUPERVISOR_SPEC + if (!supervised) { + return [spec?.program ?? '', ...(spec?.args ?? [])] + } + const launch: unknown = JSON.parse(Buffer.from(supervised, 'base64').toString()) + if (!launch || typeof launch !== 'object' || !('command' in launch) || !('args' in launch)) { + return [] + } + return [String(launch.command), ...(Array.isArray(launch.args) ? launch.args.map(String) : [])] +} + function childEnv(): Record { return (spawned.at(-1)?.env ?? {}) as Record } @@ -201,8 +213,9 @@ describe('Claude stream-json connection', () => { const report = await until(() => readReportSafely(scenario), 'the scripted CLI report') expect(report.argv[0]).toBe(FAKE_CLI) // The .mjs fixture makes the SDK run it under node; a real CLI path is the program - // itself. Either way the resolved path is what Orca's spawner is asked to execute. - expect([spawned.at(-1)?.program, ...(spawned.at(-1)?.args ?? [])]).toContain(FAKE_CLI) + // itself. Either way the resolved path is what Orca's spawner is asked to execute, + // through the POSIX supervisor's spec where there is one. + expect(launchedArgv(spawned.at(-1))).toContain(FAKE_CLI) expect(report.argv).toContain('--replay-user-messages') expect(report.argv).toContain(`--session-id=${SESSION_ID}`) }) @@ -647,37 +660,63 @@ describe('Claude stream-json connection', () => { 20_000 ) - it('settles a spawn error followed by close as processless and closes idempotently', async () => { - const scenario = scriptScenario([HOLD_OPEN]) - const missingCli = join(scenario.cwd, 'claude-that-does-not-exist') - let fault: Error | null = null - let exit: Error | null = null - const connection = await open( - { ...launchFor(scenario), pathToClaudeCodeExecutable: missingCli }, - { - onFault: (error) => { - fault = error - }, - onExit: (error) => { - exit = error + it.skipIf(process.platform === 'win32')( + 'reports a missing CLI under the supervisor as a root exit that names the spawn error', + async () => { + const scenario = scriptScenario([HOLD_OPEN]) + const missingCli = join(scenario.cwd, 'claude-that-does-not-exist') + let exit: Error | null = null + const connection = await open( + { ...launchFor(scenario), pathToClaudeCodeExecutable: missingCli }, + { + onExit: (error) => { + exit = error + } } - } - ) + ) - await until( - () => (connection.exitVerdict.root === 'processless' ? connection.exitVerdict : null), - 'the processless spawn settlement' - ) - expect(connection.pid).toBeUndefined() - expect(fault).toBeInstanceOf(Error) - expect(exit).toBeNull() - await expect(Promise.all([connection.close(), connection.close()])).resolves.toEqual([ - true, - true - ]) - await expect(connection.close()).resolves.toBe(true) - expect(connection.exitVerdict).toEqual({ root: 'processless', tree: 'exited' }) - }) + const reported = await until(() => exit, 'the supervised spawn failure') + // The supervisor spawned, so this is its exit; only its stderr can say why. + expect(reported.message).toMatch(/\(code 127\).*ENOENT/s) + // A first-hand root exit, which releases the lease like a processless start did. + expect(connection.exitVerdict.root).toBe('exited') + } + ) + + it.skipIf(process.platform !== 'win32')( + 'settles a spawn error followed by close as processless and closes idempotently', + async () => { + const scenario = scriptScenario([HOLD_OPEN]) + const missingCli = join(scenario.cwd, 'claude-that-does-not-exist') + let fault: Error | null = null + let exit: Error | null = null + const connection = await open( + { ...launchFor(scenario), pathToClaudeCodeExecutable: missingCli }, + { + onFault: (error) => { + fault = error + }, + onExit: (error) => { + exit = error + } + } + ) + + await until( + () => (connection.exitVerdict.root === 'processless' ? connection.exitVerdict : null), + 'the processless spawn settlement' + ) + expect(connection.pid).toBeUndefined() + expect(fault).toBeInstanceOf(Error) + expect(exit).toBeNull() + await expect(Promise.all([connection.close(), connection.close()])).resolves.toEqual([ + true, + true + ]) + await expect(connection.close()).resolves.toBe(true) + expect(connection.exitVerdict).toEqual({ root: 'processless', tree: 'exited' }) + } + ) it('does not treat a child error event as first-hand root exit proof', async () => { const scenario = scriptScenario([HOLD_OPEN]) diff --git a/src/main/claude/claude-stream-json-connection.ts b/src/main/claude/claude-stream-json-connection.ts index 486407c0137..6f67e04b36e 100644 --- a/src/main/claude/claude-stream-json-connection.ts +++ b/src/main/claude/claude-stream-json-connection.ts @@ -309,15 +309,16 @@ export async function openClaudeStreamJsonConnection( closePromise ??= (async () => { closing = true resumeReading() - // Arm the descendant proof before ending stdin. The SDK may exit the root - // immediately; a post-exit walk cannot recover descendants that reparented. + // Arm the descendant proof before the stop. The root may exit immediately; + // a post-exit walk cannot recover descendants that reparented. await (tree.refresh?.() ?? tree.capture()) inbox.end() const proven = await proveClaudeChildExit({ child, exitPromise, exited: rootSettled, - tree + tree, + supervised: spawner.supervised }) inbox.fail(new Error('claude stream-json connection closed')) if (!proven) { diff --git a/src/main/claude/claude-structured-requested-stop.test.ts b/src/main/claude/claude-structured-requested-stop.test.ts new file mode 100644 index 00000000000..993109f4a37 --- /dev/null +++ b/src/main/claude/claude-structured-requested-stop.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, it } from 'vitest' +import type { + AgentJournalItemBody, + AgentJournalItemIdentity +} from '../../shared/agent-session-journal-types' +import { readAgentJournalTurn } from '../../shared/agent-session-turn-record' +import type { StructuredAgentSessionEventSink } from '../native-chat/agent-session-wire/structured-agent-session-event-sink' +import type { ClaudeStructuredSessionEvent } from './claude-structured-session-state' +import { + PROVIDER_SESSION_ID, + USER_MESSAGE, + adapterFor, + fakeClaude, + identityFor +} from './claude-structured-session-test-support' + +function journalSink() { + const items: { identity: AgentJournalItemIdentity; body: AgentJournalItemBody }[] = [] + const sink: StructuredAgentSessionEventSink = { + appendItem: (identity, body) => items.push({ identity, body }), + appendTombstone: () => {}, + publish: () => {} + } + return { sink, items } +} + +function frame(type: 'assistant' | 'user', uuid: string, content: unknown[]) { + return { + type, + uuid, + session_id: PROVIDER_SESSION_ID, + parent_tool_use_id: null, + message: { role: type, content } + } +} + +describe('a requested stop of a structured Claude chat', () => { + it('reads interrupted, not failed or crashed, through the frames the stop makes Claude emit', async () => { + const claude = fakeClaude({ replayUuid: 'turn-1' }) + const events: ClaudeStructuredSessionEvent[] = [] + const adapter = adapterFor(claude, {}, events) + const journal = journalSink() + await adapter.acquire({ + identity: identityFor(), + fence: 7, + spawnToken: 'spawn-9', + events: journal.sink + }) + await adapter.dispatch({ + sessionId: 'session-1', + clientMessageId: 'client-1', + body: USER_MESSAGE, + fence: 7 + }) + const connection = claude.connections[0]! + connection.handlers.onMessage?.( + frame('assistant', 'assistant-1', [ + { type: 'tool_use', id: 'tool-1', name: 'Bash', input: { command: 'sleep 120' } } + ]) + ) + // What Claude 2.1.283 emitted within 35 ms of the supervised SIGTERM mid-tool, recorded + // against the real CLI: the killed tool's result, a status frame, and no `result` frame. + connection.close = async () => { + connection.handlers.onMessage?.( + frame('user', 'user-2', [ + { type: 'tool_result', tool_use_id: 'tool-1', is_error: true, content: 'Exit code 137' } + ]) + ) + connection.handlers.onMessage?.({ + type: 'system', + subtype: 'status', + uuid: 'status-1', + session_id: PROVIDER_SESSION_ID + }) + connection.closed = true + return true + } + + await expect(adapter.closeSession('session-1')).resolves.toBe(true) + + const turns = journal.items.flatMap((item) => { + const turn = readAgentJournalTurn(item.body) + return turn ? [turn] : [] + }) + expect(turns.at(-1)).toMatchObject({ turnId: 'turn-1', state: 'interrupted' }) + const ended = events.filter((event) => event.type === 'ended') + expect(ended).toEqual([expect.objectContaining({ reason: 'claude session closed' })]) + expect(ended[0]).not.toHaveProperty('cause') + }) +}) diff --git a/src/main/claude/claude-supervised-stop-real-cli.test.ts b/src/main/claude/claude-supervised-stop-real-cli.test.ts new file mode 100644 index 00000000000..82432e3fd6e --- /dev/null +++ b/src/main/claude/claude-supervised-stop-real-cli.test.ts @@ -0,0 +1,244 @@ +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { homedir, tmpdir } from 'node:os' +import { join } from 'node:path' +import { randomUUID } from 'node:crypto' +import { afterEach, describe, expect, it } from 'vitest' +import { runProcess } from '../../shared/child-process/run-process' +import { resolveSessionFilePath } from '../native-chat/session-file-resolver' +import { SUPERVISED_GRACEFUL_EXIT_MS } from './claude-child-exit-proof-ladder' +import { + openClaudeStreamJsonConnection, + type ClaudeStreamJsonConnection +} from './claude-stream-json-connection' +import { + CLAUDE_STRUCTURED_BASE_OPTIONS, + claudeStructuredPermissionOptions +} from './claude-structured-launch-resolution' + +// Opt-in only: spends a real (haiku) turn per case. Run it in an isolated HOME with +// ORCA_REAL_CLAUDE_BIN set, and ORCA_REAL_CLAUDE_SETTINGS when auth lives in a settings file. +const CLAUDE_BIN = process.env.ORCA_REAL_CLAUDE_BIN ?? '' +const enabled = + process.env.ORCA_REAL_CLAUDE_SUPERVISED_STOP === '1' && + process.platform !== 'win32' && + CLAUDE_BIN.length > 0 +const FRAMES_OUT = process.env.ORCA_REAL_CLAUDE_FRAMES_OUT +const TOOL_MARKER = 'orca_supervised_stop_probe' +const TOOL_PROMPT = `Use the Bash tool to run exactly this command, with no timeout argument, and nothing else: python3 -c "import time; time.sleep(120) # ${TOOL_MARKER}"` + +type Frame = { at: number; message: Record } +type Row = { pid: number; ppid: number; command: string } + +const recordedPids = new Set() +const tempDirs: string[] = [] +const connections: ClaudeStreamJsonConnection[] = [] + +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 processTable(): Promise { + const result = await runProcess({ program: 'ps', args: ['-axo', 'pid=,ppid=,command='] }) + return result.stdout.split('\n').flatMap((line) => { + const match = /^\s*(\d+)\s+(\d+)\s+(.*)$/.exec(line) + return match ? [{ pid: Number(match[1]), ppid: Number(match[2]), command: match[3] }] : [] + }) +} + +async function descendantsOf(rootPid: number): Promise { + const rows = await processTable() + const found: Row[] = [] + const frontier = [rootPid] + while (frontier.length > 0) { + const parent = frontier.pop() + for (const row of rows.filter((candidate) => candidate.ppid === parent)) { + found.push(row) + frontier.push(row.pid) + } + } + return found +} + +async function until(read: () => Promise | T | null, what: string, ms = 90_000) { + const deadline = Date.now() + ms + for (;;) { + const value = await read() + if (value !== null) { + return value + } + if (Date.now() >= deadline) { + throw new Error(`timed out waiting for ${what}`) + } + await new Promise((resolve) => setTimeout(resolve, 250)) + } +} + +async function open(sessionId: string, cwd: string, resume: boolean, frames: Frame[]) { + const settings = process.env.ORCA_REAL_CLAUDE_SETTINGS + const permission = claudeStructuredPermissionOptions('bypassPermissions') + const connection = await openClaudeStreamJsonConnection( + { + pathToClaudeCodeExecutable: CLAUDE_BIN, + options: { + ...CLAUDE_STRUCTURED_BASE_OPTIONS, + model: 'haiku', + extraArgs: { + ...CLAUDE_STRUCTURED_BASE_OPTIONS.extraArgs, + ...permission.extraArgs, + ...(settings ? { settings } : {}) + }, + ...(resume ? { resume: sessionId } : { sessionId }) + }, + cwd + }, + { onMessage: (message) => frames.push({ at: Date.now(), message }) } + ) + connections.push(connection) + recordedPids.add(connection.pid!) + return connection +} + +function userMessage(text: string): Record { + return { + type: 'user', + message: { role: 'user', content: [{ type: 'text', text }] }, + parent_tool_use_id: null, + session_id: '' + } +} + +async function transcriptLines(sessionId: string): Promise { + const configDir = process.env.CLAUDE_CONFIG_DIR?.trim() || join(homedir(), '.claude') + const path = await until( + () => + resolveSessionFilePath('claude', sessionId, { + claudeProjectsDir: join(configDir, 'projects') + }), + 'the transcript', + 15_000 + ) + return readFileSync(path, 'utf8') +} + +function expectLineAtomic(contents: string): void { + expect(contents.length).toBeGreaterThan(0) + expect(contents.endsWith('\n')).toBe(true) + for (const line of contents.split('\n').filter((entry) => entry.trim())) { + expect(() => JSON.parse(line)).not.toThrow() + } +} + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null +} + +function summarize(frames: Frame[], since: number): string[] { + return frames + .filter((frame) => frame.at >= since) + .map(({ at, message }) => { + const inner = isRecord(message.message) ? message.message.content : undefined + const blocks = (Array.isArray(inner) ? inner : []) + .filter(isRecord) + .map((block) => + block.type === 'tool_result' + ? `tool_result(is_error=${String(block.is_error)}:${JSON.stringify(block.content).slice(0, 80)})` + : String(block.type) + ) + return `+${at - since}ms ${String(message.type)}/${String(message.subtype ?? '')}${ + message.type === 'result' ? `(is_error=${String(message.is_error)})` : '' + } ${blocks.join(',')}`.trim() + }) +} + +async function resumeAndAsk(sessionId: string, cwd: string): Promise { + const frames: Frame[] = [] + const resumed = await open(sessionId, cwd, true, frames) + await resumed.send(userMessage('In one short line: what command did I ask you to run?')) + const result = await until( + () => frames.find((frame) => frame.message.type === 'result')?.message ?? null, + 'the resumed turn result' + ) + expect(result.subtype).toBe('success') + await expect(resumed.close()).resolves.toBe(true) +} + +type StopCase = 'close mid-tool' | 'SIGTERM to the supervisor mid-tool' | 'close while idle' + +afterEach(async () => { + for (const pid of recordedPids) { + if (alive(pid)) { + process.kill(pid, 'SIGKILL') + } + } + recordedPids.clear() + connections.splice(0) + for (const dir of tempDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }) + } +}) + +describe.runIf(enabled)('real Claude stopped through the POSIX supervisor', () => { + it.each(['close mid-tool', 'SIGTERM to the supervisor mid-tool', 'close while idle'])( + '%s: stops Claude and its tool, keeps the transcript line-atomic, and resumes', + async (stopCase) => { + const cwd = mkdtempSync(join(tmpdir(), 'orca-real-claude-stop-')) + tempDirs.push(cwd) + const sessionId = randomUUID() + const frames: Frame[] = [] + const connection = await open(sessionId, cwd, false, frames) + const supervisor = connection.pid! + const midTool = stopCase !== 'close while idle' + await connection.send(userMessage(midTool ? TOOL_PROMPT : 'Reply with the single word: ok')) + const descendants = midTool + ? await until(async () => { + const rows = await descendantsOf(supervisor) + return rows.some((row) => row.command.includes(TOOL_MARKER)) ? rows : null + }, 'the Bash tool').catch((error: unknown) => { + console.log('[no tool] frames so far:', summarize(frames, 0)) + throw error + }) + : await until( + async () => + frames.some((frame) => frame.message.type === 'result') + ? await descendantsOf(supervisor) + : null, + 'the idle turn' + ) + for (const row of descendants) { + recordedPids.add(row.pid) + } + + const signalledAt = Date.now() + if (stopCase === 'SIGTERM to the supervisor mid-tool') { + process.kill(supervisor, 'SIGTERM') + } else { + await expect(connection.close()).resolves.toBe(true) + } + const gone = await until( + () => ([supervisor, ...descendants.map((row) => row.pid)].some(alive) ? null : true), + 'the supervisor, Claude and its tools to exit', + SUPERVISED_GRACEFUL_EXIT_MS + 2_000 + ) + const stoppedMs = Date.now() - signalledAt + const after = summarize(frames, signalledAt) + console.log(`[${stopCase}] stopped in ${stoppedMs} ms; frames after the stop:`, after) + if (FRAMES_OUT) { + writeFileSync( + `${FRAMES_OUT}.${stopCase.replaceAll(' ', '-')}.json`, + JSON.stringify({ stoppedMs, after }, null, 2) + ) + } + expect(gone).toBe(true) + + expectLineAtomic(await transcriptLines(sessionId)) + await resumeAndAsk(sessionId, cwd) + expectLineAtomic(await transcriptLines(sessionId)) + }, + 180_000 + ) +}) diff --git a/src/main/claude/claude-supervised-stop.integration.test.ts b/src/main/claude/claude-supervised-stop.integration.test.ts new file mode 100644 index 00000000000..d0a5f9ea0cc --- /dev/null +++ b/src/main/claude/claude-supervised-stop.integration.test.ts @@ -0,0 +1,225 @@ +import { spawn, type ChildProcess } from 'node:child_process' +import { existsSync, mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import type { SpawnOptions as SdkSpawnOptions } from '@anthropic-ai/claude-agent-sdk' +import { spawnProcess } from '../../shared/child-process/run-process' +import type { ProcessSpec } from '../../shared/child-process/process-spec' +import { + PROVIDER_SIGTERM_GRACE_MS, + PROVIDER_STDIN_END_GRACE_MS, + PROVIDER_SUPERVISOR_MAX_STOP_MS +} from '../codex/codex-app-server-posix-supervisor' +import { proveClaudeChildExit } from './claude-agent-sdk-exit-proof' +import { createClaudeCodeProcessSpawn } from './claude-agent-sdk-process-spawn' + +// Stands in for Claude mid-turn: stdin end does not stop it, the way EOF lets the real CLI finish +// its turn. Its tool leads its own group, as the CLI's Bash tool shells do, and only Claude's own +// SIGTERM handler reaps it. The daemon double-forks out of the tree before anything looks. +const FAKE_CLAUDE = String.raw` + const { spawn } = require('node:child_process') + const { writeFileSync } = require('node:fs') + process.stdin.resume() + process.stdout.on('error', () => {}) + const tool = spawn(process.execPath, ['-e', 'setInterval(() => {}, 60000)'], { + detached: true, + stdio: 'ignore' + }) + const forker = spawn( + process.execPath, + ['-e', "const d = require('node:child_process').spawn(process.execPath, ['-e', 'setInterval(() => {}, 60000)'], { detached: true, stdio: 'ignore' }); d.unref(); process.stdout.write(String(d.pid))"], + { stdio: ['ignore', 'pipe', 'ignore'] } + ) + let daemon = '' + forker.stdout.on('data', (chunk) => { daemon += chunk }) + forker.once('exit', () => { + process.stdout.write(JSON.stringify({ claude: process.pid, tool: tool.pid, daemon: Number(daemon) }) + '\n') + }) + process.on('SIGTERM', () => { + if (process.env.ORCA_TEST_CLAUDE_IGNORES_SIGTERM) return + try { process.kill(-tool.pid, 'SIGKILL') } catch {} + writeFileSync(process.env.ORCA_TEST_SIGTERM_MARKER, 'reaped') + process.exit(143) + }) + setInterval(() => {}, 60000) +` + +// Stands in for Orca's main: starts exactly the spec Claude's spawner built, then can be SIGKILLed. +const OWNER = String.raw` + const { spawn } = require('node:child_process') + const spec = JSON.parse(process.env.ORCA_TEST_SPAWN_SPEC) + const env = { ...spec.env } + if (env.ORCA_PROVIDER_SUPERVISOR_SPEC) { + const supervisor = JSON.parse(Buffer.from(env.ORCA_PROVIDER_SUPERVISOR_SPEC, 'base64').toString()) + supervisor.ownerPid = process.pid + env.ORCA_PROVIDER_SUPERVISOR_SPEC = Buffer.from(JSON.stringify(supervisor)).toString('base64') + } + const child = spawn(spec.program, spec.args, { + cwd: spec.cwd, + env, + detached: spec.detached, + stdio: ['pipe', 'pipe', 'ignore'] + }) + process.stdout.write(JSON.stringify({ root: child.pid }) + '\n') + child.stdout.pipe(process.stdout) + setInterval(() => {}, 60000) +` + +const recordedPids = new Set() +const tempDirs: string[] = [] + +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: number): Promise { + const deadline = Date.now() + timeoutMs + while (!predicate()) { + if (Date.now() >= deadline) { + return false + } + await new Promise((resolve) => setTimeout(resolve, 25)) + } + return true +} + +function readPids(child: ChildProcess, keys: readonly string[]): Promise> { + return new Promise((resolve, reject) => { + const pids: Record = {} + let buffered = '' + const timeout = setTimeout(() => reject(new Error(`no ${keys.join('/')} pids`)), 10_000) + const onData = (chunk: Buffer): void => { + buffered += chunk.toString() + const lines = buffered.split('\n') + buffered = lines.pop() ?? '' + for (const line of lines) { + const parsed: unknown = JSON.parse(line) + for (const [key, pid] of Object.entries(parsed ?? {})) { + if (typeof pid === 'number' && pid > 0) { + pids[key] = pid + recordedPids.add(pid) + } + } + } + if (keys.every((key) => key in pids)) { + clearTimeout(timeout) + child.stdout!.off('data', onData) + child.stdout!.resume() + resolve(pids) + } + } + child.stdout!.on('data', onData) + }) +} + +function sdkOptions(env: Record): SdkSpawnOptions { + const dir = mkdtempSync(join(tmpdir(), 'orca-claude-supervised-')) + tempDirs.push(dir) + return { + command: process.execPath, + args: ['-e', FAKE_CLAUDE], + cwd: dir, + env: { + ...process.env, + ORCA_TEST_SIGTERM_MARKER: join(dir, 'sigterm-reap'), + ...env + }, + signal: new AbortController().signal + } +} + +/** Claude spawned through Orca's own spawner and stopped by Orca's own close ladder. */ +async function spawnClaude(env: Record = {}) { + const spawner = createClaudeCodeProcessSpawn(spawnProcess) + const options = sdkOptions(env) + const child = spawner.spawn(options) + recordedPids.add(child.pid!) + let exited = false + const exit = new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve) => + child.once('exit', (code, signal) => { + exited = true + resolve({ code, signal }) + }) + ) + const pids = await readPids(child, ['claude', 'tool', 'daemon']) + const close = (): Promise => + proveClaudeChildExit({ + child, + exitPromise: exit.then(() => undefined), + exited: () => exited, + supervised: spawner.supervised + }) + return { child, exit, pids, close, marker: String(options.env.ORCA_TEST_SIGTERM_MARKER) } +} + +afterEach(() => { + for (const pid of recordedPids) { + if (alive(pid)) { + process.kill(pid, 'SIGKILL') + } + } + recordedPids.clear() + for (const dir of tempDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }) + } +}) + +describe.runIf(process.platform !== 'win32')('Claude under the POSIX provider supervisor', () => { + it('stops a mid-turn Claude on close instead of letting stdin end finish its turn', async () => { + const { child, exit, pids, close, marker } = await spawnClaude() + expect(child.pid).not.toBe(pids.claude) + + const startedAt = Date.now() + await expect(close()).resolves.toBe(true) + + // Claude's own SIGTERM reap ran at once, not after the supervisor's stdin-end grace. + expect(Date.now() - startedAt).toBeLessThan(PROVIDER_STDIN_END_GRACE_MS) + expect(existsSync(marker)).toBe(true) + await expect(exit).resolves.toEqual({ code: null, signal: 'SIGTERM' }) + expect(alive(pids.claude)).toBe(false) + expect(alive(pids.tool)).toBe(false) + }) + + it('lets the supervisor escalate a Claude that ignores SIGTERM, and exits only after it', async () => { + const { exit, pids, close } = await spawnClaude({ ORCA_TEST_CLAUDE_IGNORES_SIGTERM: '1' }) + + const startedAt = Date.now() + await expect(close()).resolves.toBe(true) + + const elapsed = Date.now() - startedAt + expect(elapsed).toBeGreaterThanOrEqual(PROVIDER_SIGTERM_GRACE_MS) + expect(elapsed).toBeLessThan(PROVIDER_SUPERVISOR_MAX_STOP_MS + 1_000) + // The supervisor's own SIGTERM stop finished the job; nothing forced the supervisor itself. + await expect(exit).resolves.toEqual({ code: null, signal: 'SIGTERM' }) + expect(alive(pids.claude)).toBe(false) + }) + + it("stops Claude and its tool when Orca's main dies, and leaves a daemon that left its tree alone", async () => { + const specs: ProcessSpec[] = [] + createClaudeCodeProcessSpawn((spec) => { + specs.push(spec) + return spawnProcess({ program: 'true' }) + }).spawn(sdkOptions({})) + const owner = spawn(process.execPath, ['-e', OWNER], { + env: { ...process.env, ORCA_TEST_SPAWN_SPEC: JSON.stringify(specs[0]) }, + stdio: ['ignore', 'pipe', 'ignore'] + }) + recordedPids.add(owner.pid!) + const pids = await readPids(owner, ['root', 'claude', 'tool', 'daemon']) + + owner.kill('SIGKILL') + + expect(await waitFor(() => !alive(pids.claude), PROVIDER_SUPERVISOR_MAX_STOP_MS)).toBe(true) + expect(await waitFor(() => !alive(pids.root), PROVIDER_SUPERVISOR_MAX_STOP_MS)).toBe(true) + // In its own group, so only Claude's SIGTERM handler could have reaped it. + expect(alive(pids.tool)).toBe(false) + // Not the conversation's writer: nothing proves it orphaned, so the stop never reaches it. + expect(alive(pids.daemon)).toBe(true) + }) +}) diff --git a/src/main/codex/codex-app-server-connection.ts b/src/main/codex/codex-app-server-connection.ts index ef566bca336..5bed3b594d7 100644 --- a/src/main/codex/codex-app-server-connection.ts +++ b/src/main/codex/codex-app-server-connection.ts @@ -46,7 +46,7 @@ export type CodexAppServerLaunch = { } const DEFAULT_REQUEST_TIMEOUT_MS = 30_000 -const GRACEFUL_EXIT_MS = 1_500 +export const GRACEFUL_EXIT_MS = 1_500 const FORCED_EXIT_MS = 1_000 const STDERR_TAIL_MAX_BYTES = 8192 diff --git a/src/main/codex/codex-app-server-posix-supervisor.integration.test.ts b/src/main/codex/codex-app-server-posix-supervisor.integration.test.ts index e23cdbc88c3..846afaa973f 100644 --- a/src/main/codex/codex-app-server-posix-supervisor.integration.test.ts +++ b/src/main/codex/codex-app-server-posix-supervisor.integration.test.ts @@ -52,8 +52,11 @@ const RECORDS_SIGTERM_PROVIDER = String.raw` // Stands in for Orca: launches the supervisor as its own child, then can be killed outright. A // second child holds the supervisor's stdin open, so only the parent-death watch can notice. +// A clean-quit owner has no holder and exits normally on SIGUSR2, the way Orca quits. const OWNER = String.raw` const { spawn } = require('node:child_process') + const quitsCleanly = Boolean(process.env.ORCA_TEST_OWNER_QUITS_CLEANLY) + if (quitsCleanly) process.on('SIGUSR2', () => process.exit(0)) const spec = JSON.parse(Buffer.from(process.env.ORCA_PROVIDER_SUPERVISOR_SPEC, 'base64').toString()) spec.ownerPid = process.pid const supervisor = spawn(process.execPath, ['-e', process.env.ORCA_TEST_SUPERVISOR_SCRIPT], { @@ -61,10 +64,12 @@ const OWNER = String.raw` stdio: ['pipe', 'pipe', 'ignore'], detached: true }) - const holder = spawn(process.execPath, ['-e', 'setInterval(() => {}, 60000)'], { - stdio: ['ignore', supervisor.stdin, 'ignore'] - }) - process.stdout.write(JSON.stringify({ supervisor: supervisor.pid, holder: holder.pid }) + '\n') + const holder = quitsCleanly + ? null + : spawn(process.execPath, ['-e', 'setInterval(() => {}, 60000)'], { + stdio: ['ignore', supervisor.stdin, 'ignore'] + }) + process.stdout.write(JSON.stringify({ supervisor: supervisor.pid, ...(holder && { holder: holder.pid }) }) + '\n') supervisor.stdout.pipe(process.stdout) setInterval(() => {}, 60000) ` @@ -175,7 +180,7 @@ async function launchUnderOwner( stdio: ['ignore', 'pipe', 'ignore'] }) recordedPids.add(owner.pid!) - const pids = await readPids(owner, ['supervisor', 'holder', 'provider', 'grandchild']) + const pids = await readPids(owner, ['supervisor', 'provider', 'grandchild']) return { owner, pids } } @@ -318,6 +323,23 @@ describe.runIf(process.platform !== 'win32')('POSIX provider supervisor processe expect(alive(provider)).toBe(false) }) + it('reaps the provider group and exits when its owner quits cleanly', async () => { + const { owner, pids } = await launchUnderOwner( + { sigtermGraceMs: 300 }, + { ORCA_TEST_OWNER_QUITS_CLEANLY: '1' } + ) + const ownerExit = new Promise((resolve) => + owner.once('exit', (code, signal) => resolve({ code, signal })) + ) + + owner.kill('SIGUSR2') + + await expect(ownerExit).resolves.toEqual({ code: 0, signal: null }) + expect(await waitFor(() => !alive(-pids.provider), 3_000)).toBe(true) + expect(await waitFor(() => !alive(pids.supervisor), 3_000)).toBe(true) + expect(alive(pids.grandchild)).toBe(false) + }) + it('asks the provider to stop with SIGTERM when its owner dies', async () => { const signalFile = join(tempDir(), 'provider-signal') const { owner, pids } = await launchUnderOwner( diff --git a/src/main/codex/codex-app-server-posix-supervisor.test.ts b/src/main/codex/codex-app-server-posix-supervisor.test.ts index 28a088716f6..6dbe03e5b2a 100644 --- a/src/main/codex/codex-app-server-posix-supervisor.test.ts +++ b/src/main/codex/codex-app-server-posix-supervisor.test.ts @@ -72,7 +72,8 @@ describe('structured provider supervision', () => { args: ['app-server', '--flag'], env: { PATH: '/bin' }, cwd: '/work/repo', - detached: false + detached: false, + supervised: false }) }) }) diff --git a/src/main/codex/codex-app-server-posix-supervisor.ts b/src/main/codex/codex-app-server-posix-supervisor.ts index 30714ee7911..21dd56aeb5b 100644 --- a/src/main/codex/codex-app-server-posix-supervisor.ts +++ b/src/main/codex/codex-app-server-posix-supervisor.ts @@ -113,9 +113,10 @@ timer = setInterval(() => { if (ownerGone()) stopProviderGroup(null) }, 100) timer.unref() -child.once('error', () => { +child.once('error', (error) => { clearInterval(timer) - process.exit(127) + // The owner sees only this pid's exit; stderr is where a missing provider binary can say so. + process.stderr.write(String(error && error.message) + '\\n', () => process.exit(127)) }) child.once('exit', (code, signal) => { void reapProviderExit(code, signal) @@ -176,13 +177,22 @@ export function createProviderSpawnSpec( launch: CodexAppServerLaunch, childEnv: NodeJS.ProcessEnv, platform: NodeJS.Platform -): { program: string; args: string[]; env: NodeJS.ProcessEnv; cwd: string; detached: boolean } { - const supervised = platform === 'win32' ? null : supervisedPosixLaunch(launch, childEnv) +): { + program: string + args: string[] + env: NodeJS.ProcessEnv + cwd: string + detached: boolean + /** The child is the supervisor, whose SIGTERM stops the provider and then itself. */ + supervised: boolean +} { + const supervisor = platform === 'win32' ? null : supervisedPosixLaunch(launch, childEnv) return { - program: supervised?.command ?? launch.command, - args: supervised?.args ?? launch.args, - env: supervised?.env ?? childEnv, + program: supervisor?.command ?? launch.command, + args: supervisor?.args ?? launch.args, + env: supervisor?.env ?? childEnv, cwd: launch.cwd ?? process.cwd(), - detached: platform !== 'win32' + detached: platform !== 'win32', + supervised: supervisor !== null } } diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.test.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.test.ts index 760b2603d99..1baaafb6f6b 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.test.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.test.ts @@ -1,17 +1,23 @@ import { describe, expect, it, vi } from 'vitest' +import { WILL_QUIT_TEARDOWN_DEADLINE_MS } from '../../../shared/quit-teardown-deadline' +import { + GRACEFUL_EXIT_MS as CLAUDE_WINDOWS_GRACEFUL_EXIT_MS, + SUPERVISED_GRACEFUL_EXIT_MS +} from '../../claude/claude-child-exit-proof-ladder' +import { GRACEFUL_EXIT_MS as CODEX_WINDOWS_GRACEFUL_EXIT_MS } from '../../codex/codex-app-server-connection' import { PROVIDER_SUPERVISOR_MAX_STOP_MS } from '../../codex/codex-app-server-posix-supervisor' import { SNAPSHOT_DRAIN_TIMEOUT_MS } from './structured-agent-session-eviction' import { CHILD_EVICTION_TIMEOUT_MS, + EVICTION_MARGIN_MS, + RESUME_MARKER_RECORD_TIMEOUT_MS, structuredAgentSessionHostTeardownPhases } from './structured-agent-session-host-teardown' -// Codex close observes the supervisor's exit after its timers fire, which run late on a loaded host. -const SUPERVISOR_EXIT_OBSERVATION_HEADROOM_MS = 1_000 +const noop = async (): Promise => undefined describe('structured agent-session host teardown', () => { it('names every phase, so the quit-path order is pinned rather than incidental', () => { - const noop = async (): Promise => undefined const phases = structuredAgentSessionHostTeardownPhases({ idleSweep: { dispose: noop }, runtimeState: { stopLeaseRenewal: () => undefined, flushAllEventSinks: noop }, @@ -31,14 +37,54 @@ describe('structured agent-session host teardown', () => { ]) }) - it("fits the Codex supervisor's longest stop inside quit's child-eviction bound", () => { - // A quit that times out first leaves the provider running and its lease unreleased. Eviction - // drains the sink for the resume offer before it stops the child, inside the same bound. - expect( - SNAPSHOT_DRAIN_TIMEOUT_MS + - PROVIDER_SUPERVISOR_MAX_STOP_MS + - SUPERVISOR_EXIT_OBSERVATION_HEADROOM_MS - ).toBeLessThan(CHILD_EVICTION_TIMEOUT_MS) + it("derives quit's child-eviction bound from every provider's supervised close", () => { + // Eviction drains the sink for the resume offer before it stops the child, inside one bound. + // Each close's tree-kill fallback is deliberately outside it: once main exits the supervisor + // stops its group itself, and next launch's recovery settles the lease. + const closes = { + claude: SUPERVISED_GRACEFUL_EXIT_MS, + codex: PROVIDER_SUPERVISOR_MAX_STOP_MS, + // No supervisor on Windows, so its closes wait less than any supervised one. + claudeWindows: CLAUDE_WINDOWS_GRACEFUL_EXIT_MS, + codexWindows: CODEX_WINDOWS_GRACEFUL_EXIT_MS + } + expect(CHILD_EVICTION_TIMEOUT_MS).toBe( + SNAPSHOT_DRAIN_TIMEOUT_MS + Math.max(closes.claude, closes.codex) + EVICTION_MARGIN_MS + ) + for (const closeMs of Object.values(closes)) { + expect(SNAPSHOT_DRAIN_TIMEOUT_MS + closeMs + EVICTION_MARGIN_MS).toBeLessThanOrEqual( + CHILD_EVICTION_TIMEOUT_MS + ) + } + // The resume markers recorded after eviction still fit under quit's global deadline. + expect(CHILD_EVICTION_TIMEOUT_MS + RESUME_MARKER_RECORD_TIMEOUT_MS).toBeLessThan( + WILL_QUIT_TEARDOWN_DEADLINE_MS + ) + }) + + it('ends child eviction as soon as every chat has closed, not at its bound', async () => { + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + const evict = structuredAgentSessionHostTeardownPhases({ + idleSweep: { dispose: noop }, + runtimeState: { stopLeaseRenewal: () => undefined, flushAllEventSinks: noop }, + tasks: { drainAttaches: noop }, + evictOwnedSessions: noop, + beginResumeMarkers: () => {}, + recordResumeMarkers: noop + }).find((phase) => phase.name === 'evict-owned-sessions') + try { + let finished = false + const run = Promise.resolve(evict?.run()).then(() => { + finished = true + }) + // No timer advances: the phase settles with the eviction, not at CHILD_EVICTION_TIMEOUT_MS. + await vi.advanceTimersByTimeAsync(0) + expect(finished).toBe(true) + await run + expect(vi.getTimerCount()).toBe(0) + } finally { + vi.useRealTimers() + } }) it('bounds stalled recovery publication without preventing later cleanup', async () => { diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.ts index eade95a7dd2..bc9a48c5dac 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.ts @@ -7,6 +7,9 @@ // nothing can ever close them. import type { AgentSessionResumeTrigger } from '../../../shared/agent-session-resume-marker' +import { SUPERVISED_GRACEFUL_EXIT_MS } from '../../claude/claude-child-exit-proof-ladder' +import { PROVIDER_SUPERVISOR_MAX_STOP_MS } from '../../codex/codex-app-server-posix-supervisor' +import { SNAPSHOT_DRAIN_TIMEOUT_MS } from './structured-agent-session-eviction' import type { StructuredAgentSessionRestartResume } from './structured-agent-session-restart-resume-host' import { abandonQueuedStructuredAgentSessionMessages, @@ -23,12 +26,20 @@ export type StructuredAgentSessionTeardownPhase = { } /** Advisory persistence must not hold shutdown open. */ -const RESUME_MARKER_RECORD_TIMEOUT_MS = 2_000 +export const RESUME_MARKER_RECORD_TIMEOUT_MS = 2_000 -/** Eight steps at ten seconds each would outlast the global quit deadline, and a quit that dies - * mid-eviction leaves the lease unreleased — the exact state restart has to clean up. Bounded - * well below that deadline so the phases after this one still get to run. */ -export const CHILD_EVICTION_TIMEOUT_MS = 8_000 +/** Covers a provider's stop observed late on a loaded host. */ +export const EVICTION_MARGIN_MS = 1_000 + +/** A quit that dies mid-eviction leaves the lease unreleased — the exact state restart has to + * clean up — so this covers the sink drain plus the longest supervised provider close, well below + * the global quit deadline so later phases still run. A close's tree-kill fallback is outside it: + * once main exits the supervisor stops its group itself, and next launch's recovery settles the + * lease. Windows closes have no supervisor and wait less. */ +export const CHILD_EVICTION_TIMEOUT_MS = + SNAPSHOT_DRAIN_TIMEOUT_MS + + Math.max(SUPERVISED_GRACEFUL_EXIT_MS, PROVIDER_SUPERVISOR_MAX_STOP_MS) + + EVICTION_MARGIN_MS /** Bounds a phase without swallowing its failure, which `withTimeout` alone would. */ async function withPhaseTimeout(run: () => Promise, timeoutMs: number): Promise { diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-recovery-resolution.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-recovery-resolution.ts index 36d950a09a4..385d438f7a0 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-recovery-resolution.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-recovery-resolution.ts @@ -30,8 +30,8 @@ export type StructuredSessionRecoveryResolutionDeps = { } const STOP_PROBE_INTERVAL_MS = 250 -// A POSIX Codex owner is its provider supervisor, which exits only after its provider group. A -// SIGKILL that lands first leaves the group running, so SIGTERM outlasts the supervisor's stop. +// A POSIX structured owner is its provider supervisor, which exits only after its provider +// group. A SIGKILL that lands first leaves the group running, so SIGTERM outlasts its stop. const STOP_PROBES: Record = { SIGTERM: Math.ceil(PROVIDER_SUPERVISOR_MAX_STOP_MS / STOP_PROBE_INTERVAL_MS) + 1, SIGKILL: 4