diff --git a/config/scripts/test-runtime/sqlite-admission/vitest-node-runtime-files.mjs b/config/scripts/test-runtime/sqlite-admission/vitest-node-runtime-files.mjs index 4d1547a9893..fb31e0f843c 100644 --- a/config/scripts/test-runtime/sqlite-admission/vitest-node-runtime-files.mjs +++ b/config/scripts/test-runtime/sqlite-admission/vitest-node-runtime-files.mjs @@ -9,6 +9,8 @@ const NODE_RUNTIME_INCLUDE = [ 'src/main/ssh/legacy-relay/endpoints/ssh-relay-endpoint-incumbent-shell.integration.test.ts', 'src/main/ssh/remote-exec/ssh-remote-commands.test.ts', 'src/packages/process-host/src/run-process.test.ts', + // The host's synchronous no-start proof depends on Node's native spawn boundary. + 'src/main/provider-process/hosted-provider-child.integration.test.ts', 'tests/e2e/cursor-quota-transport.unit.test.ts', 'tests/e2e/helpers/electron-process-shutdown.unit.test.ts', 'src/main/ai-vault-search/storage/session-search-index-writer.test.ts', diff --git a/src/main/daemon/server/daemon-provider-processes.test.ts b/src/main/daemon/server/daemon-provider-processes.test.ts index a64c553c1e0..347e499f9ff 100644 --- a/src/main/daemon/server/daemon-provider-processes.test.ts +++ b/src/main/daemon/server/daemon-provider-processes.test.ts @@ -1,20 +1,22 @@ import { EventEmitter } from 'node:events' import { Socket } from 'node:net' import { afterEach, expect, it, vi } from 'vitest' -import { spawnProcess } from '@orca/process-host' +import { spawnOwnedProcess } from '@orca/process-host' import { DaemonProviderProcesses } from './daemon-provider-processes' -vi.mock('@orca/process-host', () => ({ spawnProcess: vi.fn() })) +vi.mock('@orca/process-host', () => ({ spawnOwnedProcess: vi.fn() })) afterEach(() => vi.restoreAllMocks()) it.each([10, 100])('drains %i live roots without rescanning retained outcomes', async (count) => { const children: EventEmitter[] = [] - vi.mocked(spawnProcess).mockImplementation(() => { + vi.mocked(spawnOwnedProcess).mockImplementation((_spec, ownership) => { const child = Object.assign(new EventEmitter(), { pid: 42, kill: vi.fn(() => true) }) children.push(child) - queueMicrotask(() => child.emit('spawn')) // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: This held-child fixture supplies the PID, events and kill method used by the provider registry. - return child as never + const held = child as never + ownership.onChild(held) + queueMicrotask(() => child.emit('spawn')) + return held }) const owner = new Socket() vi.spyOn(owner, 'write').mockReturnValue(true) @@ -60,10 +62,12 @@ it('retains known no-starts before publishing and never replaces a live nonce on ) expect(published).toEqual([{ pid: null, code: null, signal: null, processless: true }]) const child = Object.assign(new EventEmitter(), { pid: 43, kill: vi.fn(() => true) }) - vi.mocked(spawnProcess).mockImplementation(() => { - queueMicrotask(() => child.emit('spawn')) + vi.mocked(spawnOwnedProcess).mockImplementation((_spec, ownership) => { // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: Registry fixture supplies its held-child PID/events/kill. - return child as never + const held = child as never + ownership.onChild(held) + queueMicrotask(() => child.emit('spawn')) + return held }) const liveSpec = { ...spec, nonce: 'b'.repeat(64) } await providers.spawn(liveSpec, owner) diff --git a/src/main/daemon/server/daemon-provider-processes.ts b/src/main/daemon/server/daemon-provider-processes.ts index a36fb66e6ef..b7a02b98556 100644 --- a/src/main/daemon/server/daemon-provider-processes.ts +++ b/src/main/daemon/server/daemon-provider-processes.ts @@ -1,5 +1,5 @@ import type { ChildProcess } from 'node:child_process' -import { spawnProcess } from '@orca/process-host' +import { spawnOwnedProcess } from '@orca/process-host' import type { Socket } from 'node:net' import { encodeNdjson } from '../../../shared/main-process-ndjson-framer' import type { @@ -77,15 +77,44 @@ export class DaemonProviderProcesses { ) { refuse('Invalid provider launch') } - // The current GUI supplies its matching supervisor; the host supplies only its pinned runtime. - const child = spawnProcess({ - program: process.execPath, - args: ['-e', spec.source, '--', ...spec.args], - cwd: spec.cwd, - env: spec.env, - detached: true, - stdio: 'ignore' + let resolveReady!: (value: { pid: number }) => void + let rejectReady!: (error: Error) => void + const ready = new Promise<{ pid: number }>((resolve, reject) => { + resolveReady = resolve + rejectReady = reject }) + // A later observer can throw before this method returns its readiness promise. + void ready.catch(() => {}) + try { + // The GUI supplies its matching supervisor; the host supplies only its pinned runtime. + spawnOwnedProcess( + { + program: process.execPath, + args: ['-e', spec.source, '--', ...spec.args], + cwd: spec.cwd, + env: spec.env, + detached: true + }, + { + onChild: (child) => this.ownChild(spec.nonce, child, owner, resolveReady, rejectReady), + onNoStart: () => refuse('Provider supervisor could not start') + } + ) + return await ready + } catch { + const failure = new Error('Provider supervisor could not start') + rejectReady(failure) + throw failure + } + } + + private ownChild( + nonce: string, + child: ChildProcess, + owner: Socket, + resolveReady: (value: { pid: number }) => void, + rejectReady: (error: Error) => void + ): void { let resolveClosed = (): void => {} const entry: ProviderEntry = { child, @@ -94,41 +123,34 @@ export class DaemonProviderProcesses { resolveClosed = resolve }) } - this.live.set(spec.nonce, entry) - this.onLiveChanged() + this.live.set(nonce, entry) const finish = (outcome: HostedProviderOutcome): void => { - if (this.live.get(spec.nonce) !== entry) { + if (this.live.get(nonce) !== entry) { return } - this.live.delete(spec.nonce) - this.retainOutcome(spec.nonce, entry.owner, outcome) + this.live.delete(nonce) + this.retainOutcome(nonce, entry.owner, outcome) this.onLiveChanged() } child.once('exit', (code, signal) => - finish({ - pid: child.pid ?? null, - code, - signal, - processless: false - }) + finish({ pid: child.pid ?? null, code, signal, processless: false }) ) child.once('close', () => resolveClosed()) - return new Promise((resolve, reject) => { - child.once('spawn', () => resolve({ pid: child.pid! })) - child.once('error', (error: NodeJS.ErrnoException) => { - // Do not relay errors containing launch payloads, environment or private source. - if (child.pid === undefined) { - finish({ - pid: null, - code: null, - signal: null, - processless: true, - ...(typeof error.code === 'string' ? { spawnErrorCode: error.code } : {}) - }) - } - reject(new Error('Provider supervisor could not start')) - }) + child.once('spawn', () => resolveReady({ pid: child.pid! })) + child.once('error', (error: NodeJS.ErrnoException) => { + // Do not relay errors containing launch payloads, environment or private source. + if (child.pid === undefined) { + finish({ + pid: null, + code: null, + signal: null, + processless: true, + ...(typeof error.code === 'string' ? { spawnErrorCode: error.code } : {}) + }) + } + rejectReady(new Error('Provider supervisor could not start')) }) + this.onLiveChanged() } query(nonce: string, owner: Socket, acknowledge = false): HostedProviderState { diff --git a/src/main/provider-process/hosted-provider-child.integration.test.ts b/src/main/provider-process/hosted-provider-child.integration.test.ts index ebd680685ea..8024c6252b4 100644 --- a/src/main/provider-process/hosted-provider-child.integration.test.ts +++ b/src/main/provider-process/hosted-provider-child.integration.test.ts @@ -1,3 +1,5 @@ +import { createServer, type Socket } from 'node:net' +import { setSpawnObserver } from '@orca/process-host/spawn-observer' import { runLocalSourceControlPlan } from '../text-generation/execution/source-control-local-process' import { runCodexProcessWithHomeLock } from '../text-generation/execution/source-control-local-generation' import { stopSupervisedChildProcess } from './supervised-child-process-stop' @@ -12,7 +14,10 @@ import { DaemonClient } from '../daemon/transport/client/daemon-client' import { HostedProviderChild } from './hosted-provider-child' import { createProviderSpawnSpec } from './provider-process-supervisor' import { guardSupervisedProviderGroup } from './provider-group-orphan-guard' -import type { HostedProviderState } from '../daemon/protocol/daemon-provider-process-protocol' +import type { + HostedProviderOutcome, + HostedProviderState +} from '../daemon/protocol/daemon-provider-process-protocol' import { ProviderOwnerListener } from './provider-owner-channels' import { createSupervisorOutputGate } from './provider-supervisor-output-gate.test-support' @@ -54,6 +59,7 @@ describe.skipIf(process.platform === 'win32')('hosted provider production transp await server.shutdown() client.disconnect() children.clear() + setSpawnObserver(null) vi.restoreAllMocks() await rm(directory, { recursive: true, force: true }) }) @@ -62,7 +68,8 @@ describe.skipIf(process.platform === 'win32')('hosted provider production transp source: string, lifetime: 'session' | 'one-shot' = 'one-shot', cwd?: string, - instrument?: (source: string) => string + instrument?: (source: string) => string, + environment: NodeJS.ProcessEnv = {} ) { const spec = createProviderSpawnSpec( { @@ -70,7 +77,7 @@ describe.skipIf(process.platform === 'win32')('hosted provider production transp args: ['-e', source], cwd: cwd ?? directory }, - { PATH: process.env.PATH, PROVIDER_ENV: 'from-gui' }, + { PATH: process.env.PATH, PROVIDER_ENV: 'from-gui', ...environment }, process.platform, { lifetime } ) @@ -90,6 +97,180 @@ describe.skipIf(process.platform === 'win32')('hosted provider production transp return child } + function generate( + source: string, + environment: NodeJS.ProcessEnv = {}, + instrument?: (source: string) => string + ) { + const created = Promise.withResolvers() + const result = runCodexProcessWithHomeLock(directory, () => + runLocalSourceControlPlan({ + plan: { binary: 'codex', args: [], label: 'Codex', stdinPayload: null }, + cwd: directory, + env: undefined, + operation: 'commit-message', + spawnAgent: () => { + const child = launch(source, 'one-shot', undefined, instrument, environment) + created.resolve(child) + return Object.assign(child, { supervised: true }) + } + }) + ) + return { created: created.promise, result } + } + + it('settles native no-start before refusal and releases the Codex home for a hosted reply', async () => { + const timeline: string[] = [] + const outcomes: HostedProviderOutcome[] = [] + const retained: HostedProviderState[] = [] + let nonce = '' + const observe = client.onProviderExit.bind(client) + vi.spyOn(client, 'onProviderExit').mockImplementation((id, listener, reconnected) => { + nonce = id + return observe( + id, + (outcome) => { + timeline.push('outcome') + outcomes.push(outcome) + listener(outcome) + }, + reconnected + ) + }) + const request = client.request.bind(client) + vi.spyOn(client, 'request').mockImplementation(async function ( + ...args: Parameters + ): Promise { + if (args[0] === 'queryProvider') { + const state = await request('queryProvider', { + nonce, + acknowledge: false + }) + retained.push(state) + } + try { + return await request(...args) + } catch (error) { + if (args[0] === 'spawnProvider') { + timeline.push('refusal') + } + throw error + } + }) + const first = generate('throw new Error("must never launch")', { + NONSECRET_LIMIT: 'x'.repeat(8 * 1024 * 1024) + }) + const child = await first.created + const closes: unknown[][] = [] + const exits: unknown[][] = [] + child.on('close', (...args) => closes.push(args)) + child.on('exit', (...args) => exits.push(args)) + await expect(child.ready).rejects.toThrow() + expect(child.spawnAbsenceKnown).toBe(true) + expect(await first.result).toMatchObject({ success: false }) + await request('ping', {}) + expect(timeline).toEqual(['outcome', 'refusal']) + expect(outcomes).toEqual([{ pid: null, code: null, signal: null, processless: true }]) + expect(retained).toContainEqual({ state: 'exited', outcome: outcomes[0] }) + expect(closes).toEqual([[null, null]]) + expect(exits).toEqual([]) + expect(child.rootAbsent).toBe(true) + expect(await generate('process.stdout.write("next hosted reply")').result).toMatchObject({ + success: true, + rawOutput: 'next hosted reply' + }) + }) + + it('retains an actual writer after a throwing observer until held termination releases its home', async () => { + const request = client.request.bind(client) + const gatePath = join(directory, 'startup-gate.sock') + const gateConnected = Promise.withResolvers() + const gateSockets = new Set() + const gate = createServer((socket) => { + gateSockets.add(socket) + gateConnected.resolve() + }) + await new Promise((resolve, reject) => { + gate.once('error', reject) + gate.listen(gatePath, resolve) + }) + const releaseSignals = Promise.withResolvers() + const signalPending = Promise.withResolvers() + let nonce = '' + const outcomes: HostedProviderOutcome[] = [] + const observe = client.onProviderExit.bind(client) + vi.spyOn(client, 'onProviderExit').mockImplementation((id, listener, reconnected) => { + nonce = id + return observe( + id, + (outcome) => { + outcomes.push(outcome) + listener(outcome) + }, + reconnected + ) + }) + vi.spyOn(client, 'request').mockImplementation(async function ( + ...args: Parameters + ): Promise { + if (args[0] === 'signalProvider') { + signalPending.resolve() + await releaseSignals.promise + } + return request(...args) + }) + setSpawnObserver(() => { + setSpawnObserver(null) + throw Object.assign(new Error('Nonsecret post-spawn observer failure'), { code: 'E2BIG' }) + }) + const first = generate( + 'process.stdout.write("writer")', + {}, + (source) => + `require('node:net').connect(${JSON.stringify(gatePath)}).once('data', () => {${source}}).on('error', () => process.exit(1))` + ) + try { + const child = await first.created + const closes: unknown[][] = [] + child.on('close', (...args) => closes.push(args)) + await expect(child.ready).rejects.toThrow('Provider supervisor could not start') + await signalPending.promise + await gateConnected.promise + const state = await request('queryProvider', { nonce }) + expect(state.state).toBe('live') + if (state.state !== 'live') { + throw new Error('Actual writer was lost') + } + expect(() => process.kill(state.pid, 0)).not.toThrow() + expect(child.spawnAbsenceKnown).toBe(false) + expect(child.rootAbsent).toBe(false) + expect(outcomes).toEqual([]) + expect(closes).toEqual([]) + let nextStarted = false + const next = generate('process.stdout.write("reply after real exit")') + void next.created.then(() => { + nextStarted = true + }) + await request('ping', {}) + expect(nextStarted).toBe(false) + releaseSignals.resolve() + expect(await first.result).toMatchObject({ success: false }) + expect(await next.result).toMatchObject({ success: true, rawOutput: 'reply after real exit' }) + expect(outcomes[0]).toMatchObject({ pid: state.pid, processless: false }) + expect(child.rootAbsent).toBe(true) + expect(child.spawnAbsenceKnown).toBe(false) + expect(closes).toHaveLength(1) + expect(() => process.kill(state.pid, 0)).toThrow() + } finally { + setSpawnObserver(null) + releaseSignals.resolve() + for (const socket of gateSockets) { + socket.destroy() + } + await new Promise((resolve) => gate.close(() => resolve())) + } + }) + it.each(['shutdown', 'endpoint'] as const)( 'settles a %s no-start before its refusal and releases the Codex home', async (cause) => { diff --git a/src/packages/process-host/src/run-process.ts b/src/packages/process-host/src/run-process.ts index 7ddfb634a24..11a8cb1fad4 100644 --- a/src/packages/process-host/src/run-process.ts +++ b/src/packages/process-host/src/run-process.ts @@ -1,9 +1,4 @@ -import { - spawn as nodeSpawn, - spawnSync as nodeSpawnSync, - type ChildProcess, - type ChildProcessWithoutNullStreams -} from 'node:child_process' +import { spawnSync as nodeSpawnSync, type ChildProcess } from 'node:child_process' import { resolveSpawn, type ResolvedSpawn } from './spawn-resolution' import { forceTerminateProcessTree, signalProcessTree } from './process-tree-termination' import { inFlightWindowsTreeKill } from './windows-tree-kill' @@ -12,27 +7,15 @@ import { hasSpawnObserver, notifySpawnObserver } from './spawn-observer' import { createOutputSink } from './bounded-output-sink' import { createChildTerminationReporter } from './child-termination-reporter' -import type { PipedProcessSpec, ProcessSpec, ProcessResult } from './process-spec' +import type { ProcessSpec, ProcessResult } from './process-spec' +import { spawnProcess } from './spawn-process' +export { spawnProcess, spawnOwnedProcess } from './spawn-process' import { DEFAULT_PROCESS_TIMEOUT_MS, DEFAULT_MAX_OUTPUT_BYTES } from './process-spec' // Bound waits for children that ignore termination and never emit close. const PROCESS_EXIT_GRACE_MS = 2_000 // An unverifiable, silent tree must not leave barrier callers pending forever. const BARRIER_UNVERIFIED_EXIT_GRACE_MS = 10_000 -/** Callers own stream error handling; unhandled stream errors can crash the host process. */ -export function spawnProcess(spec: PipedProcessSpec): ChildProcessWithoutNullStreams -export function spawnProcess(spec: ProcessSpec): ChildProcess -export function spawnProcess(spec: ProcessSpec): ChildProcess { - const resolved = resolveSpawn(spec, process.platform) - // uv_spawn blocks the calling thread; measure that block for diagnostics. - const spawnStartedAt = hasSpawnObserver() ? performance.now() : null - const child = nodeSpawn(resolved.file, [...resolved.args], resolved.options) - if (spawnStartedAt !== null) { - notifySpawnObserver(resolved.file, resolved.args, performance.now() - spawnStartedAt) - } - return child -} - /** Nonzero exits resolve; spawn errors reject. Tail capture preserves final diagnostics. */ export function runProcess( spec: ProcessSpec, diff --git a/src/packages/process-host/src/spawn-process.ts b/src/packages/process-host/src/spawn-process.ts new file mode 100644 index 00000000000..621331e9a9b --- /dev/null +++ b/src/packages/process-host/src/spawn-process.ts @@ -0,0 +1,61 @@ +import { + spawn as nodeSpawn, + type ChildProcess, + type ChildProcessWithoutNullStreams +} from 'node:child_process' +import { resolveSpawn, type ResolvedSpawn } from './spawn-resolution' +import { hasSpawnObserver, notifySpawnObserver } from './spawn-observer' +import type { PipedProcessSpec, ProcessSpec } from './process-spec' + +type SpawnOwnership = { + onChild: (child: ChildProcess) => void + onNoStart: () => void +} + +type OwnedProcessSpec = Pick + +/** Ignore stdio excludes post-spawn pipe/IPC setup from the native no-start proof. */ +export function spawnOwnedProcess(spec: OwnedProcessSpec, ownership: SpawnOwnership): ChildProcess { + const resolved = resolveSpawn( + { + program: spec.program, + args: spec.args, + cwd: spec.cwd, + env: spec.env, + detached: spec.detached, + stdio: 'ignore' + }, + process.platform + ) + return spawnResolvedProcess(resolved, ownership) +} + +/** Callers own stream error handling; unhandled stream errors can crash the host process. */ +export function spawnProcess(spec: PipedProcessSpec): ChildProcessWithoutNullStreams +export function spawnProcess(spec: ProcessSpec): ChildProcess +export function spawnProcess(spec: ProcessSpec): ChildProcess { + return spawnResolvedProcess(resolveSpawn(spec, process.platform)) +} + +function spawnResolvedProcess(resolved: ResolvedSpawn, ownership?: SpawnOwnership): ChildProcess { + const { file, args, options } = resolved + const argv = [...args] + // uv_spawn blocks the calling thread; measure that block for diagnostics. + const spawnStartedAt = hasSpawnObserver() ? performance.now() : null + let result: { child: ChildProcess } | { error: unknown } + try { + result = { child: nodeSpawn(file, argv, options) } + } catch (error) { + result = { error } + } + if ('error' in result) { + ownership?.onNoStart() + throw result.error + } + const { child } = result + ownership?.onChild(child) + if (spawnStartedAt !== null) { + notifySpawnObserver(file, args, performance.now() - spawnStartedAt) + } + return child +}