diff --git a/config/tsconfig.cli.json b/config/tsconfig.cli.json index 4fd076bd2d0..86738c4cb37 100644 --- a/config/tsconfig.cli.json +++ b/config/tsconfig.cli.json @@ -181,6 +181,8 @@ "../src/main/provider-process/provider-process-exit-deadline.ts", "../src/main/provider-process/managed-provider-process.ts", "../src/main/provider-process/provider-executable-missing.ts", + "../src/main/provider-process/provider-group-orphan-guard.ts", + "../src/main/provider-process/provider-process-row-read.ts", "../src/main/provider-process/provider-process-close.ts", "../src/main/provider-process/provider-process-launch.ts", "../src/main/provider-process/provider-process-supervisor.ts", diff --git a/src/main/acp/acp-agent-connection.test.ts b/src/main/acp/acp-agent-connection.test.ts index af865e8454c..bec2bc119c0 100644 --- a/src/main/acp/acp-agent-connection.test.ts +++ b/src/main/acp/acp-agent-connection.test.ts @@ -81,13 +81,17 @@ describe('ACP process-owning connection', () => { expect.objectContaining({ cwd: start.cwd, env: expect.objectContaining({ ACP_ACCOUNT_HOME: '/host/account' }), - stdio: ['pipe', 'pipe', 'pipe'] + stdio: ['pipe', 'pipe', 'pipe', ...(process.platform === 'win32' ? [] : ['ipc'])] }) ) expect(spawn.mock.calls[0][0].env).not.toHaveProperty('STRIPPED') - expect(await connection.start(start)).toMatchObject({ sessionId: 'session-1' }) + expect(await connection.start(start)).toMatchObject({ + sessionId: 'session-1' + }) agent.on('session/prompt', (frame) => agent.reply(frame, { stopReason: 'end_turn' })) - expect(await connection.prompt([...prompt])).toEqual({ stopReason: 'end_turn' }) + expect(await connection.prompt([...prompt])).toEqual({ + stopReason: 'end_turn' + }) connection.pauseReading() expect(child.stdout.isPaused()).toBe(true) connection.resumeReading() @@ -161,7 +165,9 @@ describe('ACP process-owning connection', () => { it('drops an overlong stderr line it could not classify instead of keeping its prefix', async () => { const { connection, child } = fixture({ - signIn: { read: (line) => (line.endsWith('authenticate') ? 'link' : undefined) } + signIn: { + read: (line) => (line.endsWith('authenticate') ? 'link' : undefined) + } }) await connection.start(start) const link = `https://accounts.example/auth?state=${'x'.repeat(33_000)}` @@ -282,7 +288,11 @@ describe('ACP process-owning connection', () => { connection.onExit(failing) connection.onExit(early) expect(() => child.emit('exit', 0, null)).not.toThrow() - expect(early).toHaveBeenCalledExactlyOnceWith({ code: 0, signal: null, processless: false }) + expect(early).toHaveBeenCalledExactlyOnceWith({ + code: 0, + signal: null, + processless: false + }) expect(() => connection.onExit(failing)).not.toThrow() const late = vi.fn() connection.onExit(late) diff --git a/src/main/codex/codex-app-server-client.test.ts b/src/main/codex/codex-app-server-client.test.ts index e02c5e71c10..cd7fe37ed22 100644 --- a/src/main/codex/codex-app-server-client.test.ts +++ b/src/main/codex/codex-app-server-client.test.ts @@ -18,6 +18,11 @@ import { } from '../provider-process/provider-process-supervisor' import { runCodexAppServerSession } from './codex-app-server-session' +// The orphan guard's process reads add timers of their own; the timer audit below is the session's. +vi.mock('../provider-process/provider-group-orphan-guard', () => ({ + guardSupervisedProviderGroup: () => false +})) + // Stub codex app-server speaking the same JSONL protocol: initialize → // initialized → hooks/list → config/batchWrite → hooks/list. Scenario-driven // via STUB_CONFIG so each test controls listings, errors, and hangs. diff --git a/src/main/codex/codex-app-server-session.ts b/src/main/codex/codex-app-server-session.ts index c63873c197f..fef826d22d1 100644 --- a/src/main/codex/codex-app-server-session.ts +++ b/src/main/codex/codex-app-server-session.ts @@ -122,10 +122,11 @@ export async function runCodexAppServerSession( site: CODEX_APP_SERVER_KILL_SITE, inheritedEnv: pairedEnv, spawnImpl: (spec) => - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: every stdio slot is 'pipe', so stdin, stdout and stderr exist. + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the managed spec's first three stdio slots are 'pipe', so stdin, stdout and stderr exist. spawnImpl(spec.program, [...(spec.args ?? [])], { env: spec.env, - stdio: ['pipe', 'pipe', 'pipe'], + // The managed spec's slots, so the supervisor's group-report channel survives. + stdio: spec.stdio ?? ['pipe', 'pipe', 'pipe'], windowsHide: true, ...(spec.detached ? { detached: true } : {}) }) as ChildProcessWithoutNullStreams diff --git a/src/main/codex/codex-state-db-backfill-recovery-process.ts b/src/main/codex/codex-state-db-backfill-recovery-process.ts index bc08343aeff..316c02a9693 100644 --- a/src/main/codex/codex-state-db-backfill-recovery-process.ts +++ b/src/main/codex/codex-state-db-backfill-recovery-process.ts @@ -10,6 +10,7 @@ import { } from '../provider-process/provider-process-supervisor' import type { CodexAppServerSpawn } from './codex-app-server-process-tree-kill' import { stopSupervisedChildProcess } from '../provider-process/supervised-child-process-stop' +import { guardSupervisedProviderGroup } from '../provider-process/provider-group-orphan-guard' import { resolveWslInteropSpawnCwd } from '../wsl-interop-spawn-directory' const BACKFILL_RECOVERY_KILL_SITE = 'codex-state-db-backfill-recovery' @@ -17,7 +18,10 @@ const BACKFILL_RECOVERY_KILL_SITE = 'codex-state-db-backfill-recovery' // for the spawn spec (a gone Orca) and the stop (Orca stopping it), so they cannot drift. const BACKFILL_RECOVERY_CLOSE_REQUEST: ProviderCloseRequest = 'stdin-end' -export type CodexBackfillRecoveryProcess = { child: ChildProcessHandle; supervised: boolean } +export type CodexBackfillRecoveryProcess = { + child: ChildProcessHandle + supervised: boolean +} /** Starts the read-only app-server whose presence lets Codex finish its own backfill. */ export function spawnCodexBackfillRecoveryProcess( @@ -47,17 +51,24 @@ export function spawnCodexBackfillRecoveryProcess( // Kept alive up to an hour; on POSIX the supervisor stops it with Orca, however Orca exits. const spawnSpec = createProviderSpawnSpec( { command, args: [...CODEX_READ_ONLY_APP_SERVER_ARGS], cwd: codexHomePath }, - withCliRuntimeOnPath(command, { ...process.env, CODEX_HOME: codexHomePath }), + withCliRuntimeOnPath(command, { + ...process.env, + CODEX_HOME: codexHomePath + }), process.platform, { lifetime: 'session', closeRequest: BACKFILL_RECOVERY_CLOSE_REQUEST } ) const child = spawnProcess(spawnSpec.program, spawnSpec.args, { cwd: codexHomePath, - stdio: ['pipe', 'ignore', 'ignore'], + stdio: ['pipe', 'ignore', 'ignore', ...spawnSpec.groupReportStdio], windowsHide: true, env: spawnSpec.env, ...(spawnSpec.supervised ? { detached: true } : {}) }) + guardSupervisedProviderGroup(child, { + site: BACKFILL_RECOVERY_KILL_SITE, + supervised: spawnSpec.supervised + }) return { child, supervised: spawnSpec.supervised } } diff --git a/src/main/jsonl-rpc/agent-connection.test.ts b/src/main/jsonl-rpc/agent-connection.test.ts index e9eb5537109..2a2dcc2a883 100644 --- a/src/main/jsonl-rpc/agent-connection.test.ts +++ b/src/main/jsonl-rpc/agent-connection.test.ts @@ -64,7 +64,7 @@ describe('JSON-lines RPC process ownership', () => { expect.objectContaining({ cwd: '/execution-host/folder', env: expect.objectContaining({ PI_CODING_AGENT_DIR: '/host/account' }), - stdio: ['pipe', 'pipe', 'pipe'] + stdio: ['pipe', 'pipe', 'pipe', ...(process.platform === 'win32' ? [] : ['ipc'])] }) ) expect(spawn.mock.calls[0][0].env).not.toHaveProperty('STRIPPED') @@ -173,7 +173,10 @@ describe('JSON-lines RPC process ownership', () => { await close child.emit('exit', null, 'SIGKILL') await connection.close() - expect(connection.lastCloseResult).toEqual({ root: 'live', tree: 'unverifiable' }) + expect(connection.lastCloseResult).toEqual({ + root: 'live', + tree: 'unverifiable' + }) expect(connection.rootVerdict).toBe('exited') }) diff --git a/src/main/opencode/opencode-launch-model-context.ts b/src/main/opencode/opencode-launch-model-context.ts index 76395efc45b..a462432167d 100644 --- a/src/main/opencode/opencode-launch-model-context.ts +++ b/src/main/opencode/opencode-launch-model-context.ts @@ -13,6 +13,7 @@ import { cancelUnreadResponseBody } from '../lib/unread-response-body' import type { ChildProcessHandle } from '@orca/process-host/process-spec' import { createProviderSpawnSpec } from '../provider-process/provider-process-supervisor' import { stopSupervisedChildProcess } from '../provider-process/supervised-child-process-stop' +import { guardSupervisedProviderGroup } from '../provider-process/provider-group-orphan-guard' const OPENCODE_PREFLIGHT_KILL_SITE = 'opencode-launch-model-preflight' // A supervisor's pipes are its own, so they close as it exits. @@ -29,7 +30,10 @@ const agent = z.object({ const location = z.object({ directory: z.string() }) const modelResponse = z.object({ location, data: availableModel.array() }) const agentResponse = z.object({ location, data: agent.array() }) -const defaultModelResponse = z.object({ location, data: availableModel.nullable() }) +const defaultModelResponse = z.object({ + location, + data: availableModel.nullable() +}) const configResponse = z.array( z.object({ type: z.enum(['directory', 'document']), @@ -107,7 +111,11 @@ export async function probeOpenCodeLaunchModelContext(options: { args: ['serve', '--hostname', '127.0.0.1', '--port', '0'], cwd: options.cwd }, - { ...options.env, OPENCODE_SERVER_USERNAME: 'opencode', OPENCODE_SERVER_PASSWORD: password }, + { + ...options.env, + OPENCODE_SERVER_USERNAME: 'opencode', + OPENCODE_SERVER_PASSWORD: password + }, process.platform, // `opencode serve` never exits on stdin end, so on POSIX only its supervisor stops it with Orca. { lifetime: 'one-shot' } @@ -117,7 +125,12 @@ export async function probeOpenCodeLaunchModelContext(options: { args: spawnSpec.args, cwd: options.cwd, env: spawnSpec.env, - detached: true + detached: true, + stdio: ['pipe', 'pipe', 'pipe', ...spawnSpec.groupReportStdio] + }) + guardSupervisedProviderGroup(child, { + site: OPENCODE_PREFLIGHT_KILL_SITE, + supervised: spawnSpec.supervised }) const output = createOutputSink(4096) let childClosed = false diff --git a/src/main/provider-process/managed-provider-process.ts b/src/main/provider-process/managed-provider-process.ts index 1d3006c73f7..18ca675b693 100644 --- a/src/main/provider-process/managed-provider-process.ts +++ b/src/main/provider-process/managed-provider-process.ts @@ -14,6 +14,7 @@ import { import type { DescendantTreeVerdict } from '../pty-descendant-exit-verification' import { isMissingProviderExecutable } from './provider-executable-missing' import { supervisedProviderSpawnFailure } from './provider-spawn-failure-report' +import { guardSupervisedProviderGroup } from './provider-group-orphan-guard' import { acceptProviderRootExit, closeProviderProcess, @@ -96,8 +97,9 @@ export function spawnManagedProviderProcess( cwd: spec.cwd, env: spec.env, detached: spec.detached, - stdio: ['pipe', 'pipe', 'pipe'] + stdio: ['pipe', 'pipe', 'pipe', ...spec.groupReportStdio] }) + guardSupervisedProviderGroup(child, { site: options.site, supervised: spec.supervised }) const listeners = new Set<(exit: ProviderProcessExit) => void>() let observed: ProviderProcessExit | null = null let spawnFailed = false @@ -199,7 +201,10 @@ 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 + } lastCloseResult ??= result return result } diff --git a/src/main/provider-process/provider-group-guard-launch-paths.test.ts b/src/main/provider-process/provider-group-guard-launch-paths.test.ts new file mode 100644 index 00000000000..98092888868 --- /dev/null +++ b/src/main/provider-process/provider-group-guard-launch-paths.test.ts @@ -0,0 +1,99 @@ +import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import type { ChildProcess } from 'node:child_process' +import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as OrphanGuard from './provider-group-orphan-guard' +import { spawnManagedProviderProcess } from './managed-provider-process' +import { runCodexAppServerSession } from '../codex/codex-app-server-session' +import { spawnCodexAppServerProcess } from '../codex/codex-app-server-process-tree-kill' +import { spawnCodexBackfillRecoveryProcess } from '../codex/codex-state-db-backfill-recovery-process' +import { probeOpenCodeLaunchModelContext } from '../opencode/opencode-launch-model-context' +import { spawnSourceControlAgent } from '../text-generation/source-control-agent-launch' + +// Every production launch path must hand the supervisor its report channel; an adapter that +// rewrites stdio drops it, and the guard then never watches the group. + +const { registrations } = vi.hoisted(() => { + const recorded: { site: string; armed: boolean }[] = [] + return { registrations: recorded } +}) + +vi.mock('./provider-group-orphan-guard', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + guardSupervisedProviderGroup: ( + ...args: Parameters + ) => { + const armed = actual.guardSupervisedProviderGroup(...args) + registrations.push({ site: args[1].site, armed }) + return armed + } + } +}) + +const dir = mkdtempSync(join(tmpdir(), 'orca-guard-launch-paths-')) +afterAll(() => rmSync(dir, { recursive: true, force: true })) +// Exits at once, so each supervisor relays its exit and stops on its own. +const QUICK_EXIT = ['-e', 'setTimeout(() => {}, 50)'] + +function exited(child: Pick): Promise { + return child.exitCode !== null || child.signalCode !== null + ? Promise.resolve() + : new Promise((resolve) => child.once('exit', () => resolve())) +} + +beforeEach(() => { + registrations.length = 0 +}) + +describe.runIf(process.platform !== 'win32')('provider group guard on every launch path', () => { + it('managed provider processes (Codex, Claude, ACP, JSON-lines, model catalog)', async () => { + const managed = spawnManagedProviderProcess( + { command: process.execPath, args: QUICK_EXIT }, + { site: 'managed' } + ) + expect(registrations).toEqual([{ site: 'managed', armed: true }]) + await managed.close() + }) + + it('Codex app-server sessions, whose adapter re-maps the spawn options', async () => { + await runCodexAppServerSession( + { command: process.execPath, cliPath: null, args: QUICK_EXIT, timeoutMs: 2_000 }, + async () => null + ).catch(() => null) + expect(registrations).toEqual([{ site: expect.any(String), armed: true }]) + }) + + it('the Codex backfill recovery app-server', async () => { + const { child } = spawnCodexBackfillRecoveryProcess( + dir, + spawnCodexAppServerProcess, + () => process.execPath + ) + expect(registrations).toEqual([{ site: 'codex-state-db-backfill-recovery', armed: true }]) + await exited(child) + }) + + it('the OpenCode launch-context probe', async () => { + const standIn = join(dir, 'opencode') + writeFileSync(standIn, '#!/bin/sh\nexit 0\n') + chmodSync(standIn, 0o755) + await probeOpenCodeLaunchModelContext({ executable: standIn, cwd: dir, env: process.env }) + expect(registrations).toEqual([{ site: 'opencode-launch-model-preflight', armed: true }]) + }) + + it('source-control text generation', async () => { + const child = spawnSourceControlAgent({ + binary: process.execPath, + args: QUICK_EXIT, + env: process.env, + stdinMode: 'pipe', + useCwdForNative: false + }) + expect(registrations).toEqual([{ site: 'source-control-text-generation', armed: true }]) + child.stdin?.end() + await exited(child) + }) +}) diff --git a/src/main/provider-process/provider-group-orphan-guard.integration.test.ts b/src/main/provider-process/provider-group-orphan-guard.integration.test.ts new file mode 100644 index 00000000000..2617f564a63 --- /dev/null +++ b/src/main/provider-process/provider-group-orphan-guard.integration.test.ts @@ -0,0 +1,136 @@ +import { afterEach, describe, expect, it } from 'vitest' +import { spawnProcess } from '@orca/process-host' +import type { ChildProcess } from 'node:child_process' +import { createProviderSpawnSpec } from './provider-process-supervisor' +import { + guardSupervisedProviderGroup, + type ProviderGroupSweepVerdict +} from './provider-group-orphan-guard' + +// Leads its own group with a SIGTERM-ignoring grandchild, as a server with helpers would. +const PROVIDER = String.raw` + const { spawn } = require('node:child_process') + process.stdout.on('error', () => {}) + const grandchild = spawn( + process.execPath, + ['-e', "process.on('SIGTERM', () => {}); process.stdout.write('armed'); setInterval(() => {}, 60000)"], + { stdio: ['ignore', 'pipe', 'ignore'] } + ) + grandchild.stdout.once('data', () => + process.stdout.write(JSON.stringify({ provider: process.pid, grandchild: grandchild.pid }) + '\n') + ) + setInterval(() => {}, 60000) +` + +const IDENTITY_TIMEOUT_MS = 15_000 +const recordedPids = 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: 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 launch(): { + supervisor: ChildProcess + proven: () => boolean + swept: Promise + pids: Promise<{ provider: number; grandchild: number }> +} { + const spec = createProviderSpawnSpec( + { command: process.execPath, args: ['-e', PROVIDER] }, + process.env, + process.platform, + { lifetime: 'session' } + ) + const supervisor = spawnProcess({ + program: spec.program, + args: spec.args, + env: spec.env, + cwd: spec.cwd, + detached: spec.detached, + stdio: ['pipe', 'pipe', 'ignore', ...spec.groupReportStdio] + }) + recordedPids.add(supervisor.pid!) + let identityProven = false + const swept = new Promise((resolve) => + guardSupervisedProviderGroup(supervisor, { + site: 'test', + supervised: spec.supervised, + onProven: () => { + identityProven = true + }, + onSwept: resolve + }) + ) + const pids = new Promise<{ provider: number; grandchild: number }>((resolve) => { + let buffered = '' + supervisor.stdout?.on('data', (chunk: Buffer) => { + buffered += chunk.toString() + const line = buffered.split('\n')[0] + if (buffered.includes('\n')) { + const parsed = JSON.parse(line) + recordedPids.add(parsed.provider) + recordedPids.add(parsed.grandchild) + resolve(parsed) + } + }) + }) + return { supervisor, proven: () => identityProven, swept, pids } +} + +afterEach(() => { + for (const pid of recordedPids) { + if (alive(pid)) { + process.kill(pid, 'SIGKILL') + } + } + recordedPids.clear() +}) + +describe.runIf(process.platform !== 'win32')('provider group orphan guard', () => { + it('kills the provider group a SIGKILLed supervisor leaves behind', async () => { + const { supervisor, proven, swept, pids } = launch() + const { provider, grandchild } = await pids + // A start in the read's own second is re-read after it, so this can take over a second. + expect(await waitFor(proven, IDENTITY_TIMEOUT_MS)).toBe(true) + + supervisor.kill('SIGKILL') + + await expect(swept).resolves.toBe('killed') + expect(await waitFor(() => !alive(provider) && !alive(grandchild), 10_000)).toBe(true) + }) + + it('leaves nothing to sweep when the supervisor stops its own group', async () => { + const { supervisor, proven, swept, pids } = launch() + await pids + expect(await waitFor(proven, IDENTITY_TIMEOUT_MS)).toBe(true) + + supervisor.kill('SIGTERM') + + await expect(swept).resolves.toBe('absent') + }) + + it('closes the report channel while the provider still runs', async () => { + const { supervisor, pids } = launch() + const disconnected = new Promise((resolve) => supervisor.once('disconnect', resolve)) + const { provider } = await pids + + await disconnected + expect(alive(provider)).toBe(true) + }) +}) diff --git a/src/main/provider-process/provider-group-orphan-guard.test.ts b/src/main/provider-process/provider-group-orphan-guard.test.ts new file mode 100644 index 00000000000..cc08881f33c --- /dev/null +++ b/src/main/provider-process/provider-group-orphan-guard.test.ts @@ -0,0 +1,270 @@ +import { EventEmitter } from 'node:events' +import { describe, expect, it, vi } from 'vitest' +import { + guardSupervisedProviderGroup, + sweepOrphanedProviderGroup, + type ProcessRowRead, + type ProviderGroupSweepVerdict +} from './provider-group-orphan-guard' + +const STARTED = 'Mon Oct 19 10:00:00 2026' +const identity = { pgid: 4242, startedAt: STARTED } + +function errno(code: string): Error { + return Object.assign(new Error(code), { code }) +} + +function row(overrides: Partial> = {}) { + return { + pid: 4242, + ppid: 1, + pgid: 4242, + startedAt: STARTED, + startResolution: 'second' as const, + ...overrides + } +} + +describe('sweepOrphanedProviderGroup', () => { + it('does nothing when the group is already gone', async () => { + const signalGroup = vi.fn(() => { + throw errno('ESRCH') + }) + const readProcessRow = vi.fn() + await expect( + sweepOrphanedProviderGroup(identity, { + site: 'test', + supervised: true, + signalGroup, + readProcessRow + }) + ).resolves.toBe('absent') + expect(readProcessRow).not.toHaveBeenCalled() + }) + + it('kills a surviving group whose leader is the recorded process', async () => { + const signalGroup = vi.fn() + await expect( + sweepOrphanedProviderGroup(identity, { + site: 'test', + supervised: true, + signalGroup, + readProcessRow: async () => row() + }) + ).resolves.toBe('killed') + expect(signalGroup).toHaveBeenLastCalledWith(4242, 'SIGKILL') + }) + + it('refuses a leaderless group, which nothing can prove is still ours', async () => { + const signalGroup = vi.fn() + await expect( + sweepOrphanedProviderGroup(identity, { + site: 'test', + supervised: true, + signalGroup, + readProcessRow: async () => null + }) + ).resolves.toBe('refused') + expect(signalGroup).not.toHaveBeenCalledWith(4242, 'SIGKILL') + }) + + it.each<[string, ProcessRowRead]>([ + ['a reused pid with another start time', row({ startedAt: 'Mon Oct 19 11:00:00 2026' })], + ['a reused pid leading another group', row({ pgid: 99 })], + ['a failed process read', 'unverifiable'] + ])('refuses %s', async (_label, leader) => { + const signalGroup = vi.fn() + await expect( + sweepOrphanedProviderGroup(identity, { + site: 'test', + supervised: true, + signalGroup, + readProcessRow: async () => leader + }) + ).resolves.toBe('refused') + expect(signalGroup).not.toHaveBeenCalledWith(4242, 'SIGKILL') + }) + + it('refuses a group this user cannot signal', async () => { + const signalGroup = vi.fn(() => { + throw errno('EPERM') + }) + await expect( + sweepOrphanedProviderGroup(identity, { site: 'test', supervised: true, signalGroup }) + ).resolves.toBe('refused') + }) +}) + +class FakeSupervisor extends EventEmitter { + pid = 777 + connected = true + report(message: unknown): void { + this.emit('message', message) + this.connected = false + this.emit('disconnect') + } +} + +function guard( + child: FakeSupervisor, + readProcessRow: (pid: number) => Promise, + signalGroup = vi.fn() +): Promise { + return new Promise((resolve) => + guardSupervisedProviderGroup(child, { + site: 'test', + supervised: true, + readProcessRow, + signalGroup, + onSwept: resolve + }) + ) +} + +describe('guardSupervisedProviderGroup', () => { + it('sweeps the reported group once the supervisor exits', async () => { + const child = new FakeSupervisor() + const signalGroup = vi.fn() + const swept = guard(child, async () => row({ ppid: 777 }), signalGroup) + child.report({ pgid: 4242 }) + child.emit('exit', null, 'SIGKILL') + await expect(swept).resolves.toBe('killed') + expect(signalGroup).toHaveBeenLastCalledWith(4242, 'SIGKILL') + }) + + it('never records a reported pid that its supervisor no longer parents', async () => { + const child = new FakeSupervisor() + const signalGroup = vi.fn() + const swept = guard(child, async () => row({ ppid: 1 }), signalGroup) + child.report({ pgid: 4242 }) + child.emit('exit', null, 'SIGKILL') + await expect(swept).resolves.toBe('unproven') + expect(signalGroup).not.toHaveBeenCalled() + }) + + it('re-reads a leader started in the read second, so a same-second reuse cannot match', async () => { + const child = new FakeSupervisor() + const startedAtMs = Date.parse(STARTED) + const clock = { now: startedAtMs + 300 } + const readProcessRow = vi.fn(async () => row({ ppid: 777 })) + const sleep = vi.fn(async (ms: number) => { + clock.now += ms + }) + const proven = new Promise((resolve) => + guardSupervisedProviderGroup(child, { + site: 'test', + supervised: true, + readProcessRow, + now: () => clock.now, + sleep, + onProven: resolve + }) + ) + child.report({ pgid: 4242 }) + await expect(proven).resolves.toEqual(identity) + expect(readProcessRow).toHaveBeenCalledTimes(2) + expect(clock.now).toBeGreaterThanOrEqual(startedAtMs + 1_000) + }) + + it('arms nothing when a same-second replacement holds the pid once the second ends', async () => { + const child = new FakeSupervisor() + const clock = { now: Date.parse(STARTED) + 300 } + // The first read is our leader; by the re-read its pid names a stranger with the same lstart. + const readProcessRow = vi + .fn<(pid: number) => Promise>() + .mockResolvedValueOnce(row({ ppid: 777 })) + .mockResolvedValueOnce(row({ ppid: 1 })) + const signalGroup = vi.fn() + const swept = new Promise((resolve) => + guardSupervisedProviderGroup(child, { + site: 'test', + supervised: true, + readProcessRow, + signalGroup, + now: () => clock.now, + sleep: async (ms) => { + clock.now += ms + }, + onSwept: resolve + }) + ) + child.report({ pgid: 4242 }) + await vi.waitFor(() => expect(readProcessRow).toHaveBeenCalledTimes(2)) + child.emit('exit', null, 'SIGKILL') + await expect(swept).resolves.toBe('unproven') + expect(signalGroup).not.toHaveBeenCalled() + }) + + it('trusts a Linux clock-tick start without waiting out the second', async () => { + const child = new FakeSupervisor() + const startedAt = 'clock-tick:123456' + const readProcessRow = vi.fn(async () => + row({ ppid: 777, startedAt, startResolution: 'clock-tick' }) + ) + const sleep = vi.fn(async () => {}) + const proven = new Promise((resolve) => + guardSupervisedProviderGroup(child, { + site: 'test', + supervised: true, + readProcessRow, + now: () => Date.now(), + sleep, + onProven: resolve + }) + ) + child.report({ pgid: 4242 }) + await expect(proven).resolves.toEqual({ pgid: 4242, startedAt }) + expect(sleep).not.toHaveBeenCalled() + }) + + it('records nothing when every read stays inside the start second', async () => { + const child = new FakeSupervisor() + const now = Date.parse(STARTED) + 100 + const signalGroup = vi.fn() + const swept = new Promise((resolve) => + guardSupervisedProviderGroup(child, { + site: 'test', + supervised: true, + readProcessRow: async () => row({ ppid: 777 }), + signalGroup, + now: () => now, + sleep: async () => {}, + onSwept: resolve + }) + ) + child.report({ pgid: 4242 }) + child.emit('exit', null, 'SIGKILL') + await expect(swept).resolves.toBe('unproven') + expect(signalGroup).not.toHaveBeenCalled() + }) + + it('warns once and watches nothing for a supervised spawn without the report channel', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const child = new FakeSupervisor() + child.connected = false + expect(guardSupervisedProviderGroup(child, { site: 'test', supervised: true })).toBe(false) + expect(guardSupervisedProviderGroup(child, { site: 'test', supervised: true })).toBe(false) + expect(child.listenerCount('exit')).toBe(0) + expect(warn).toHaveBeenCalledTimes(1) + warn.mockRestore() + }) + + it('stays quiet for an unsupervised spawn', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + expect( + guardSupervisedProviderGroup(new FakeSupervisor(), { site: 'test', supervised: false }) + ).toBe(false) + expect(warn).not.toHaveBeenCalled() + warn.mockRestore() + }) + + it('ignores a malformed report', async () => { + const child = new FakeSupervisor() + const readProcessRow = vi.fn(async () => row({ ppid: 777 })) + const swept = guard(child, readProcessRow) + child.report({ pgid: 1 }) + child.emit('exit', 0, null) + await expect(swept).resolves.toBe('unproven') + expect(readProcessRow).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/provider-process/provider-group-orphan-guard.ts b/src/main/provider-process/provider-group-orphan-guard.ts new file mode 100644 index 00000000000..259b34d3a0e --- /dev/null +++ b/src/main/provider-process/provider-group-orphan-guard.ts @@ -0,0 +1,224 @@ +import { hasUnambiguousStartTime } from '../pty-descendant-termination' +import { recordSelfInitiatedTreeKill } from '../crash-reporting/self-initiated-tree-kill-log' +import { readPosixProcessRow, type ProcessRowRead } from './provider-process-row-read' + +export type { ProcessRowRead } from './provider-process-row-read' + +// Covers the one stop the supervisor cannot make itself: its own SIGKILL leaves the provider's +// detached group running, so Orca, which outlives it, finishes the stop. POSIX only: Windows +// providers have no supervisor. + +// The supervisor disconnects right after reporting, so this bounds only a wedged channel. +const REPORT_END_WAIT_MS = 1_000 +const IDENTITY_READ_ATTEMPTS = 3 +const SECOND_BOUNDARY_MARGIN_MS = 50 + +/** A provider group proven ours while its supervisor still parented the leader. */ +export type ProviderGroupIdentity = { pgid: number; startedAt: string } + +export type ProviderGroupSweepVerdict = 'absent' | 'killed' | 'refused' + +export type ProviderGroupGuardDeps = { + site: string + /** Only a supervised POSIX spawn has a group to guard. */ + supervised: boolean + readProcessRow?: (pid: number) => Promise + signalGroup?: (pgid: number, signal: NodeJS.Signals | 0) => void + now?: () => number + sleep?: (ms: number) => Promise + onProven?: (identity: ProviderGroupIdentity) => void + /** `unproven`: no group was reported and proven ours, so nothing was swept. */ + onSwept?: (verdict: ProviderGroupSweepVerdict | 'unproven') => void +} + +let warnedMissingChannel = false + +type GuardedChild = { + readonly pid?: number + readonly connected: boolean + once(event: 'exit' | 'disconnect', listener: () => void): unknown + once(event: 'message', listener: (message: unknown) => void): unknown +} + +function defaultSignalGroup(pgid: number, signal: NodeJS.Signals | 0): void { + process.kill(-pgid, signal) +} + +/** SIGKILLs a surviving provider group, never one whose leader pid now names another process. */ +export async function sweepOrphanedProviderGroup( + identity: ProviderGroupIdentity, + deps: ProviderGroupGuardDeps +): Promise { + const signalGroup = deps.signalGroup ?? defaultSignalGroup + try { + signalGroup(identity.pgid, 0) + } catch (error) { + // EPERM: the group exists but is not this user's, so it is not ours. + return isErrnoCode(error, 'ESRCH') ? 'absent' : 'refused' + } + const leader = await (deps.readProcessRow ?? readPosixProcessRow)(identity.pgid) + // Only a leader that re-proves its recorded identity names our group; a leaderless group has + // nothing to prove it by, so its survivors are left alone. The recorded start predates its + // read's second, so a later reuse cannot share it. + if ( + !leader || + leader === 'unverifiable' || + leader.pgid !== identity.pgid || + leader.startedAt !== identity.startedAt + ) { + return 'refused' + } + try { + signalGroup(identity.pgid, 'SIGKILL') + } catch (error) { + return isErrnoCode(error, 'ESRCH') ? 'absent' : 'refused' + } + recordSelfInitiatedTreeKill({ + pid: identity.pgid, + site: deps.site, + scope: 'posix-process-group' + }) + return 'killed' +} + +/** + * Reads the group a supervisor reports on its IPC channel, proves it by the leader's parent and + * start time, and sweeps that group once the supervisor exits. True when it is watching. + */ +export function guardSupervisedProviderGroup( + child: GuardedChild, + deps: ProviderGroupGuardDeps +): boolean { + const supervisorPid = child.pid + if (!supervisorPid || !deps.supervised) { + return false + } + if (!hasReportChannel(child)) { + // A launch adapter that rewrites stdio drops the channel; say so once instead of silently. + if (!warnedMissingChannel) { + warnedMissingChannel = true + console.warn(`[provider-group-guard] ${deps.site}: supervised spawn has no report channel`) + } + return false + } + let identity: Promise = Promise.resolve(null) + // A gone supervisor parents nothing, so a re-read it would wait for can only fail; skip it. + const proof = createProofWait(deps.sleep) + let channelClosed = false + let onChannelClosed = (): void => {} + child.once('disconnect', () => { + channelClosed = true + onChannelClosed() + }) + child.once('message', (message: unknown) => { + const pgid = reportedGroup(message) + if (pgid !== null) { + identity = proveReportedGroup(pgid, supervisorPid, deps, proof) + void identity.then((proven) => proven && deps.onProven?.(proven)) + } + }) + const untilChannelClosed = (): Promise => + channelClosed + ? Promise.resolve() + : new Promise((resolve) => { + const timer = setTimeout(resolve, REPORT_END_WAIT_MS) + timer.unref?.() + onChannelClosed = () => { + clearTimeout(timer) + resolve() + } + }) + child.once('exit', () => { + proof.stop() + // Exit can be observed before the report is delivered. + void untilChannelClosed() + .then(() => identity) + .then((proven) => (proven ? sweepOrphanedProviderGroup(proven, deps) : ('unproven' as const))) + .then(deps.onSwept, () => deps.onSwept?.('refused')) + }) + return true +} + +/** + * The leader's identity, read while this supervisor still parents it (so its pid was not reused). + * lstart has one-second resolution, so a start in the read's own second is re-proven after that + * second ends; otherwise a pid reused within that second would carry the same start time. A + * supervisor gone by then no longer parents it, so nothing is armed. + */ +async function proveReportedGroup( + pgid: number, + supervisorPid: number, + deps: ProviderGroupGuardDeps, + proof: ProofWait +): Promise { + const readRow = deps.readProcessRow ?? readPosixProcessRow + const now = deps.now ?? Date.now + for (let attempt = 0; attempt < IDENTITY_READ_ATTEMPTS; attempt += 1) { + // The boundary before ps starts, as the descendant sweep takes it. + const capturedAtMs = now() + const row = await readRow(pgid) + if (!row || row === 'unverifiable' || row.ppid !== supervisorPid || row.pgid !== pgid) { + return null + } + if ( + row.startResolution === 'clock-tick' || + hasUnambiguousStartTime(row.startedAt, capturedAtMs) + ) { + return { pgid, startedAt: row.startedAt } + } + if (!(await proof.wait(1_000 - (now() % 1_000) + SECOND_BOUNDARY_MARGIN_MS))) { + return null + } + } + return null +} + +type ProofWait = { wait(ms: number): Promise; stop(): void } + +/** Waits that end early, false, once stopped; their timers never outlive the stop. */ +function createProofWait(sleep?: (ms: number) => Promise): ProofWait { + let stopped = false + let cancel = (): void => {} + return { + async wait(ms) { + if (stopped) { + return false + } + if (sleep) { + await sleep(ms) + return !stopped + } + return new Promise((resolve) => { + const timer = setTimeout(() => resolve(!stopped), ms) + timer.unref?.() + cancel = () => { + clearTimeout(timer) + resolve(false) + } + }) + }, + stop() { + stopped = true + cancel() + } + } +} + +function hasReportChannel(child: GuardedChild): boolean { + try { + return child.connected + } catch { + // A handle-less child can throw here; the guard must never fail the spawn it watches. + return false + } +} + +function reportedGroup(message: unknown): number | null { + const pgid = + typeof message === 'object' && message !== null && 'pgid' in message ? message.pgid : null + return typeof pgid === 'number' && Number.isSafeInteger(pgid) && pgid > 1 ? pgid : null +} + +function isErrnoCode(error: unknown, code: string): boolean { + return error instanceof Error && 'code' in error && error.code === code +} diff --git a/src/main/provider-process/provider-process-row-read.test.ts b/src/main/provider-process/provider-process-row-read.test.ts new file mode 100644 index 00000000000..17b97063ca7 --- /dev/null +++ b/src/main/provider-process/provider-process-row-read.test.ts @@ -0,0 +1,32 @@ +import { describe, expect, it } from 'vitest' +import { parseLinuxProcStat, readPosixProcessRow } from './provider-process-row-read' + +describe('parseLinuxProcStat', () => { + it('reads parent, group and start ticks past a comm with spaces and parentheses', () => { + const fields = ['S', '41', '4242', '4242', ...Array(15).fill('0'), '987654', '0'] + expect(parseLinuxProcStat(4242, `4242 (open (code) serve) ${fields.join(' ')}`)).toEqual({ + pid: 4242, + ppid: 41, + pgid: 4242, + startedAt: 'clock-tick:987654', + startResolution: 'clock-tick' + }) + }) + + it('rejects a truncated stat line', () => { + expect(parseLinuxProcStat(4242, '4242 (opencode) S 41')).toBeNull() + }) +}) + +describe.runIf(process.platform !== 'win32')('readPosixProcessRow', () => { + it('reads this process with the same start marker every time', async () => { + const first = await readPosixProcessRow(process.pid) + const second = await readPosixProcessRow(process.pid) + expect(first).toMatchObject({ pid: process.pid, ppid: process.ppid }) + expect(second).toEqual(first) + }) + + it('reports a pid with no process as absent, not unverifiable', async () => { + await expect(readPosixProcessRow(2 ** 22 + 12_345)).resolves.toBeNull() + }) +}) diff --git a/src/main/provider-process/provider-process-row-read.ts b/src/main/provider-process/provider-process-row-read.ts new file mode 100644 index 00000000000..0a483212b73 --- /dev/null +++ b/src/main/provider-process/provider-process-row-read.ts @@ -0,0 +1,66 @@ +import { readFile } from 'node:fs/promises' +import { runProcess } from '@orca/process-host' +import { parseProcessTable, type ProcessTableRow } from '../pty-process-table-parser' + +const PROCESS_ROW_TIMEOUT_MS = 2_000 + +/** + * One process's parent, group and start marker. `clock-tick` starts come from Linux /proc, where a + * pid reused within one tick would need the whole pid space to cycle; `second` starts are ps lstart. + */ +export type ProviderProcessRow = Pick & { + startResolution: 'clock-tick' | 'second' +} + +/** `null`: no such process. `'unverifiable'`: the read failed, which proves nothing. */ +export type ProcessRowRead = ProviderProcessRow | null | 'unverifiable' + +export function readPosixProcessRow( + pid: number, + platform: NodeJS.Platform = process.platform +): Promise { + return platform === 'linux' ? readLinuxProcStatRow(pid) : readPsProcessRow(pid) +} + +/** Parses /proc//stat; the comm field can hold spaces and parentheses, so cut past ") ". */ +export function parseLinuxProcStat(pid: number, stat: string): ProviderProcessRow | null { + const fields = stat.slice(stat.lastIndexOf(') ') + 2).split(' ') + const ppid = Number(fields[1]) + const pgid = Number(fields[2]) + const ticks = fields[19] + if (!Number.isSafeInteger(ppid) || !Number.isSafeInteger(pgid) || !/^\d+$/.test(ticks ?? '')) { + return null + } + return { pid, ppid, pgid, startedAt: `clock-tick:${ticks}`, startResolution: 'clock-tick' } +} + +async function readLinuxProcStatRow(pid: number): Promise { + try { + return parseLinuxProcStat(pid, await readFile(`/proc/${pid}/stat`, 'utf8')) ?? 'unverifiable' + } catch (error) { + return error instanceof Error && 'code' in error && error.code === 'ENOENT' + ? null + : 'unverifiable' + } +} + +async function readPsProcessRow(pid: number): Promise { + try { + const result = await runProcess({ + program: 'ps', + args: ['-o', 'pid=,ppid=,pgid=,lstart=', '-p', String(pid)], + // The same locale on every read, so two reads of one process compare equal. + env: { ...process.env, LANG: 'C', LC_ALL: 'C' }, + timeoutMs: PROCESS_ROW_TIMEOUT_MS, + maxOutputBytes: 4096 + }) + const row = parseProcessTable(result.stdout).find((entry) => entry.pid === pid) + if (row) { + return { ...row, startResolution: 'second' } + } + // ps exits 1 with no rows for a pid that does not exist; anything else is a failed read. + return !result.timedOut && result.code === 1 ? null : 'unverifiable' + } catch { + return 'unverifiable' + } +} diff --git a/src/main/provider-process/provider-process-supervisor.test.ts b/src/main/provider-process/provider-process-supervisor.test.ts index d83f6aed82e..4d536f04946 100644 --- a/src/main/provider-process/provider-process-supervisor.test.ts +++ b/src/main/provider-process/provider-process-supervisor.test.ts @@ -62,7 +62,11 @@ describe('structured provider supervision', () => { lifetime: 'one-shot' }) - expect(spec).toMatchObject({ program: process.execPath, detached: true, supervised: true }) + expect(spec).toMatchObject({ + program: process.execPath, + detached: true, + supervised: true + }) expect( JSON.parse(Buffer.from(spec.env.ORCA_PROVIDER_SUPERVISOR_SPEC!, 'base64').toString()) ).toMatchObject({ lifetime: 'one-shot' }) @@ -72,7 +76,11 @@ describe('structured provider supervision', () => { // The launch's override goes through the one env rule, then only the provider gets it. const spec = createProviderSpawnSpec( { ...command, env: { NODE_OPTIONS: '--require /missing.js' } }, - { PATH: '/bin', NODE_OPTIONS: '--inherited', NODE_REPL_EXTERNAL_MODULE: '/repl.js' }, + { + PATH: '/bin', + NODE_OPTIONS: '--inherited', + NODE_REPL_EXTERNAL_MODULE: '/repl.js' + }, 'linux' ) @@ -80,7 +88,10 @@ describe('structured provider supervision', () => { expect(spec.env).not.toHaveProperty('NODE_REPL_EXTERNAL_MODULE') expect( JSON.parse(Buffer.from(spec.env.ORCA_PROVIDER_SUPERVISOR_SPEC!, 'base64').toString()).nodeEnv - ).toEqual({ NODE_OPTIONS: '--require /missing.js', NODE_REPL_EXTERNAL_MODULE: '/repl.js' }) + ).toEqual({ + NODE_OPTIONS: '--require /missing.js', + NODE_REPL_EXTERNAL_MODULE: '/repl.js' + }) }) it('keeps every argv and env string of a 120 KiB argv prompt under the Linux 128 KiB cap', () => { @@ -128,22 +139,40 @@ describe('structured provider supervision', () => { env: { PATH: '/bin', CODEX_HOME: '/tmp/codex' }, cwd: '/work/repo', detached: false, - supervised: false + supervised: false, + groupReportStdio: [] }) }) it.each(['win32', 'darwin', 'linux'] as const)( 'applies launch environment overrides and deletions on %s', (platform) => { - const baseEnv = { PATH: '/bin', AGENT_HOME: '/inherited', PARENT_AGENT: 'parent' } - const overlay = { AGENT_HOME: '/pinned', LAUNCH_ONLY: 'added', PARENT_AGENT: 'overlay' } + const baseEnv = { + PATH: '/bin', + AGENT_HOME: '/inherited', + PARENT_AGENT: 'parent' + } + const overlay = { + AGENT_HOME: '/pinned', + LAUNCH_ONLY: 'added', + PARENT_AGENT: 'overlay' + } const spec = createProviderSpawnSpec( - { command: 'provider', args: [], env: overlay, envToDelete: ['PARENT_AGENT'] }, + { + command: 'provider', + args: [], + env: overlay, + envToDelete: ['PARENT_AGENT'] + }, baseEnv, platform ) - expect(spec.env).toMatchObject({ PATH: '/bin', AGENT_HOME: '/pinned', LAUNCH_ONLY: 'added' }) + expect(spec.env).toMatchObject({ + PATH: '/bin', + AGENT_HOME: '/pinned', + LAUNCH_ONLY: 'added' + }) expect(spec.env).not.toHaveProperty('PARENT_AGENT') expect(baseEnv.PARENT_AGENT).toBe('parent') expect(baseEnv.AGENT_HOME).toBe('/inherited') diff --git a/src/main/provider-process/provider-process-supervisor.ts b/src/main/provider-process/provider-process-supervisor.ts index 10fc1c5bc3a..d444ec6caa0 100644 --- a/src/main/provider-process/provider-process-supervisor.ts +++ b/src/main/provider-process/provider-process-supervisor.ts @@ -42,18 +42,31 @@ const exitWithSpawnFailure = (error, thrown) => { try { require('node:fs').writeSync(2, ${JSON.stringify(PROVIDER_SPAWN_FAILURE_MARKER)} + JSON.stringify(report) + '\\n') } catch {} process.exit(127) } +// Orca opens the report channel only when it will keep the group's identity. Linux children +// inherit its fd, so /dev/null takes that slot and only this pid holds the channel. +const reports = typeof process.send === 'function' +const reportShield = reports ? require('node:fs').openSync('/dev/null', 'r') : null let child try { child = spawn(providerCommand, providerArgs, { cwd: spec.cwd, env: childEnv, - stdio: ['pipe', 'pipe', 'pipe'], + stdio: ['pipe', 'pipe', 'pipe', ...(reportShield === null ? [] : [reportShield])], detached: true }) } catch (error) { // Node throws most spawn failures (ENOEXEC, ENOTDIR, ...) rather than emitting them. exitWithSpawnFailure(error, true) } +// With the group's identity in Orca, a SIGKILL of this pid cannot orphan the group. +if (reports) { + try { require('node:fs').closeSync(reportShield) } catch {} + const disconnect = () => { try { process.disconnect() } catch {} } + try { + if (child.pid) process.send({ pgid: child.pid }, disconnect) + else disconnect() + } catch { disconnect() } +} let timer let ownerShutdownTimer let settling = false @@ -269,6 +282,8 @@ export function createProviderSpawnSpec( detached: boolean /** The child is the supervisor, whose SIGTERM stops the provider and then itself. */ supervised: boolean + /** Appended after the three standard slots: the channel the supervisor reports its group on. */ + groupReportStdio: [] | ['ipc'] } { const childEnv = resolveProviderChildEnv(launch, baseEnv) const supervisor = @@ -285,6 +300,7 @@ export function createProviderSpawnSpec( env: supervisor?.env ?? childEnv, cwd: launch.cwd ?? process.cwd(), detached: platform !== 'win32', - supervised: supervisor !== null + supervised: supervisor !== null, + groupReportStdio: supervisor ? ['ipc'] : [] } } diff --git a/src/main/text-generation/commit-message-text-generation-model-discovery.test.ts b/src/main/text-generation/commit-message-text-generation-model-discovery.test.ts index e70cb2dafa1..968237b7b1b 100644 --- a/src/main/text-generation/commit-message-text-generation-model-discovery.test.ts +++ b/src/main/text-generation/commit-message-text-generation-model-discovery.test.ts @@ -40,12 +40,18 @@ const expectChildTerminated = createChildTerminationExpectation(terminateWindows const hostPlatform = process.platform afterEach(() => { - Object.defineProperty(process, 'platform', { configurable: true, value: hostPlatform }) + Object.defineProperty(process, 'platform', { + configurable: true, + value: hostPlatform + }) vi.unstubAllEnvs() }) beforeEach(() => { - Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + Object.defineProperty(process, 'platform', { + configurable: true, + value: 'win32' + }) // Windows resolves a bare agent name on PATH; the host's own installs must not answer it. vi.stubEnv('PATH', '') terminateWindowsProcessTreeMock.mockClear() @@ -67,13 +73,20 @@ describe('discoverCommitMessageModelsLocal on the Windows direct-child path', () it('discovers dynamic models through the agent CLI', async () => { // The host's own spawn shape: supervised on POSIX, direct on Windows. - Object.defineProperty(process, 'platform', { configurable: true, value: hostPlatform }) + Object.defineProperty(process, 'platform', { + configurable: true, + value: hostPlatform + }) const listeners = new Map void>() const child = { pid: 123, kill: vi.fn(), - stdout: { on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) }, - stderr: { on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) }, + stdout: { + on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) + }, + stderr: { + on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) + }, stdin: { end: vi.fn() }, on: vi.fn((event, callback) => listeners.set(event, callback)) } @@ -98,13 +111,20 @@ describe('discoverCommitMessageModelsLocal on the Windows direct-child path', () it('writes the Claude list_models request to stdin and parses the control response', async () => { // The host's own spawn shape: supervised on POSIX, direct on Windows. - Object.defineProperty(process, 'platform', { configurable: true, value: hostPlatform }) + Object.defineProperty(process, 'platform', { + configurable: true, + value: hostPlatform + }) const listeners = new Map void>() const child = { pid: 123, kill: vi.fn(), - stdout: { on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) }, - stderr: { on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) }, + stdout: { + on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) + }, + stderr: { + on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) + }, stdin: { on: vi.fn(), end: vi.fn() }, on: vi.fn((event, callback) => listeners.set(event, callback)) } @@ -159,7 +179,7 @@ describe('discoverCommitMessageModelsLocal on the Windows direct-child path', () ]) expect(spawnMock.mock.calls[0]![2]).toMatchObject({ windowsHide: true, - stdio: ['pipe', 'pipe', 'pipe'] + stdio: ['pipe', 'pipe', 'pipe', ...(process.platform === 'win32' ? [] : ['ipc'])] }) expect(child.stdin.end).toHaveBeenCalledWith(expect.stringContaining('"list_models"')) }) @@ -169,8 +189,12 @@ describe('discoverCommitMessageModelsLocal on the Windows direct-child path', () const child = { pid: 123, kill: vi.fn(), - stdout: { on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) }, - stderr: { on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) }, + stdout: { + on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) + }, + stderr: { + on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) + }, stdin: { on: vi.fn(), end: vi.fn() }, on: vi.fn((event, callback) => listeners.set(event, callback)) } @@ -196,13 +220,20 @@ describe('discoverCommitMessageModelsLocal on the Windows direct-child path', () it('discovers dynamic models through the configured agent command override', async () => { // The host's own spawn shape: supervised on POSIX, direct on Windows. - Object.defineProperty(process, 'platform', { configurable: true, value: hostPlatform }) + Object.defineProperty(process, 'platform', { + configurable: true, + value: hostPlatform + }) const listeners = new Map void>() const child = { pid: 123, kill: vi.fn(), - stdout: { on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) }, - stderr: { on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) }, + stdout: { + on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) + }, + stderr: { + on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) + }, stdin: { end: vi.fn() }, on: vi.fn((event, callback) => listeners.set(event, callback)) } @@ -239,8 +270,12 @@ describe('discoverCommitMessageModelsLocal on the Windows direct-child path', () const child = { pid: 123, kill: vi.fn(), - stdout: { on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) }, - stderr: { on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) }, + stdout: { + on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) + }, + stderr: { + on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) + }, stdin: { end: vi.fn() }, on: vi.fn((event, callback) => listeners.set(event, callback)) } @@ -283,8 +318,12 @@ describe('discoverCommitMessageModelsLocal on the Windows direct-child path', () const child = { pid: 123, kill: vi.fn(), - stdout: { on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) }, - stderr: { on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) }, + stdout: { + on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) + }, + stderr: { + on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) + }, stdin: { end: vi.fn() }, on: vi.fn((event, callback) => listeners.set(event, callback)) } @@ -307,8 +346,12 @@ describe('discoverCommitMessageModelsLocal on the Windows direct-child path', () const child = { pid: 123, kill: vi.fn(), - stdout: { on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) }, - stderr: { on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) }, + stdout: { + on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) + }, + stderr: { + on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) + }, stdin: { end: vi.fn() }, on: vi.fn((event, callback) => listeners.set(event, callback)) } @@ -331,8 +374,12 @@ describe('discoverCommitMessageModelsLocal on the Windows direct-child path', () const child = { pid: 123, kill: vi.fn(), - stdout: { on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) }, - stderr: { on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) }, + stdout: { + on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) + }, + stderr: { + on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) + }, stdin: { end: vi.fn() }, on: vi.fn((event, callback) => listeners.set(event, callback)) } @@ -462,10 +509,17 @@ describe('discoverCommitMessageModelsLocal on the Windows direct-child path', () expect(spawnMock).toHaveBeenCalledTimes(2) secondChild.stdout.emit( 'data', - Buffer.from(JSON.stringify({ models: [{ slug: 'gpt-5.5', display_name: 'GPT-5.5' }] })) + Buffer.from( + JSON.stringify({ + models: [{ slug: 'gpt-5.5', display_name: 'GPT-5.5' }] + }) + ) ) secondChild.emit('close', 0) - await expect(second).resolves.toMatchObject({ success: true, defaultModelId: 'gpt-5.5' }) + await expect(second).resolves.toMatchObject({ + success: true, + defaultModelId: 'gpt-5.5' + }) } finally { vi.useRealTimers() } @@ -494,10 +548,17 @@ describe('discoverCommitMessageModelsLocal on the Windows direct-child path', () secondChild.stdout.emit( 'data', - Buffer.from(JSON.stringify({ models: [{ slug: 'gpt-5.5', display_name: 'GPT-5.5' }] })) + Buffer.from( + JSON.stringify({ + models: [{ slug: 'gpt-5.5', display_name: 'GPT-5.5' }] + }) + ) ) secondChild.emit('close', 0) - await expect(second).resolves.toMatchObject({ success: true, defaultModelId: 'gpt-5.5' }) + await expect(second).resolves.toMatchObject({ + success: true, + defaultModelId: 'gpt-5.5' + }) } finally { vi.useRealTimers() } diff --git a/src/main/text-generation/source-control-agent-launch.test.ts b/src/main/text-generation/source-control-agent-launch.test.ts index 3d9f7d4fa67..103f90c5890 100644 --- a/src/main/text-generation/source-control-agent-launch.test.ts +++ b/src/main/text-generation/source-control-agent-launch.test.ts @@ -18,8 +18,12 @@ vi.mock('@orca/process-host', async (importOriginal) => { return { ...actual, spawnProcess: spawnProcessMock } }) -function fakeChild(): EventEmitter & { stdin: { on: () => void; end: () => void } } { - return Object.assign(new EventEmitter(), { stdin: { on: vi.fn(), end: vi.fn() } }) +function fakeChild(): EventEmitter & { + stdin: { on: () => void; end: () => void } +} { + return Object.assign(new EventEmitter(), { + stdin: { on: vi.fn(), end: vi.fn() } + }) } function lastSpawnSpec(): ProcessSpec { @@ -61,10 +65,13 @@ describe('spawnSourceControlAgent', () => { args: ['-e', POSIX_PROVIDER_SUPERVISOR_SCRIPT, '--', '/opt/agent/claude', '-p', '--verbose'], cwd, detached: true, - stdio: ['pipe', 'pipe', 'pipe'] + stdio: ['pipe', 'pipe', 'pipe', 'ipc'] }) expect(spec.env).toMatchObject({ PATH: '/usr/bin', AGENT_TOKEN: 'kept' }) - expect(decodedSupervisorSpec(spec)).toMatchObject({ cwd, lifetime: 'one-shot' }) + expect(decodedSupervisorSpec(spec)).toMatchObject({ + cwd, + lifetime: 'one-shot' + }) }) it('spawns a Windows agent directly, without a supervisor', () => { @@ -80,7 +87,10 @@ describe('spawnSourceControlAgent', () => { ) expect(child.supervised).toBeUndefined() - expect(lastSpawnSpec()).toMatchObject({ program: 'C:/tools/agent.exe', args: ['--print'] }) + expect(lastSpawnSpec()).toMatchObject({ + program: 'C:/tools/agent.exe', + args: ['--print'] + }) expect(lastSpawnSpec().env).not.toHaveProperty('ORCA_PROVIDER_SUPERVISOR_SPEC') }) @@ -113,7 +123,11 @@ describe.runIf(process.platform !== 'win32')('supervised agent processes', () => await expect( generateCommitMessageFromContext( - { branch: 'main', stagedSummary: 'M README.md', stagedPatch: '+test' }, + { + branch: 'main', + stagedSummary: 'M README.md', + stagedPatch: '+test' + }, { agentId: 'custom', model: '', customAgentCommand: `"${missing}"` }, { kind: 'local', cwd: folder } ) @@ -129,7 +143,11 @@ describe.runIf(process.platform !== 'win32')('supervised agent processes', () => await expect( generateCommitMessageFromContext( - { branch: 'main', stagedSummary: 'M README.md', stagedPatch: '+test' }, + { + branch: 'main', + stagedSummary: 'M README.md', + stagedPatch: '+test' + }, { agentId: 'custom', model: '', customAgentCommand: `"${agent}"` }, { kind: 'local', cwd: folder } ) @@ -152,8 +170,16 @@ describe.runIf(process.platform !== 'win32')('supervised agent processes', () => await expect( generateCommitMessageFromContext( - { branch: 'main', stagedSummary: 'M README.md', stagedPatch: '+test' }, - { agentId: 'custom', model: '', customAgentCommand: `"${command}"` }, + { + branch: 'main', + stagedSummary: 'M README.md', + stagedPatch: '+test' + }, + { + agentId: 'custom', + model: '', + customAgentCommand: `"${command}"` + }, { kind: 'local', cwd: folder } ) ).resolves.toEqual({ @@ -168,7 +194,11 @@ describe.runIf(process.platform !== 'win32')('supervised agent processes', () => await expect( generateCommitMessageFromContext( - { branch: 'main', stagedSummary: 'M README.md', stagedPatch: '+test' }, + { + branch: 'main', + stagedSummary: 'M README.md', + stagedPatch: '+test' + }, { agentId: 'custom', model: '', customAgentCommand: `"${missing}"` }, { kind: 'local', @@ -194,12 +224,22 @@ process.stdin.on('end', () => setTimeout(() => console.log(prompt.includes('READ await expect( generateCommitMessageFromContext( - { branch: 'main', stagedSummary: 'M README.md', stagedPatch: '+test' }, - { agentId: 'custom', model: '', customAgentCommand: `"${process.execPath}" "${agent}"` }, + { + branch: 'main', + stagedSummary: 'M README.md', + stagedPatch: '+test' + }, + { + agentId: 'custom', + model: '', + customAgentCommand: `"${process.execPath}" "${agent}"` + }, { kind: 'local', cwd: folder } ) ).resolves.toMatchObject({ success: true, message: 'Update README' }) expect(lastSpawnSpec().program).toBe(process.execPath) - expect(decodedSupervisorSpec(lastSpawnSpec())).toMatchObject({ lifetime: 'one-shot' }) + expect(decodedSupervisorSpec(lastSpawnSpec())).toMatchObject({ + lifetime: 'one-shot' + }) }) }) diff --git a/src/main/text-generation/source-control-agent-launch.ts b/src/main/text-generation/source-control-agent-launch.ts index 650cf574f69..7fae2264f2e 100644 --- a/src/main/text-generation/source-control-agent-launch.ts +++ b/src/main/text-generation/source-control-agent-launch.ts @@ -2,6 +2,8 @@ import { spawnProcess } from '@orca/process-host' import { withCliRuntimeOnPath } from '../../shared/node-cli-command-resolution' import { resolveCliCommand } from '../codex-cli/command' import { createProviderSpawnSpec } from '../provider-process/provider-process-supervisor' +import { guardSupervisedProviderGroup } from '../provider-process/provider-group-orphan-guard' +import { SOURCE_CONTROL_KILL_SITE } from './source-control-local-process' import { wslAwareSpawn } from '../git/runner' import type { SourceControlAgentSpawnInput, @@ -107,7 +109,11 @@ function spawnSupervisedAgent( env: spec.env, cwd: spec.cwd, detached: spec.detached, - stdio: ['pipe', 'pipe', 'pipe'] + stdio: ['pipe', 'pipe', 'pipe', ...spec.groupReportStdio] + }) + guardSupervisedProviderGroup(child, { + site: SOURCE_CONTROL_KILL_SITE, + supervised: spec.supervised }) return Object.assign(child, { supervised: spec.supervised }) } diff --git a/src/main/text-generation/source-control-local-process.ts b/src/main/text-generation/source-control-local-process.ts index 43874af6ac7..c0cbad83a20 100644 --- a/src/main/text-generation/source-control-local-process.ts +++ b/src/main/text-generation/source-control-local-process.ts @@ -24,7 +24,7 @@ import type { TextGenerationOperation } from './source-control-text-generation-types' -const SOURCE_CONTROL_KILL_SITE = 'source-control-text-generation' +export const SOURCE_CONTROL_KILL_SITE = 'source-control-text-generation' export async function killSourceControlAgentProcess( child: SpawnedSourceControlAgentProcess @@ -126,7 +126,10 @@ export function runLocalSourceControlPlan(input: { } catch (error) { markProcessClosed() if (error instanceof WindowsCmdLineBreakError) { - resolve({ success: false, error: userFacingUnsafeWindowsBatchArgs(plan.label) }) + resolve({ + success: false, + error: userFacingUnsafeWindowsBatchArgs(plan.label) + }) return } console.error('[commit-message] Failed to spawn local generator:', error) @@ -170,7 +173,11 @@ export function runLocalSourceControlPlan(input: { const cancel = (): void => { canceledByUser = true startTermination() - finalize({ success: false, error: 'Generation canceled.', canceled: true }) + finalize({ + success: false, + error: 'Generation canceled.', + canceled: true + }) } setLocalGenerationCancelToken(laneKey, cancel) timer = setTimeout(() => { @@ -219,7 +226,11 @@ export function runLocalSourceControlPlan(input: { const onClose = (code: number | null): void => { markClosedAfterTermination() if (canceledByUser) { - finalize({ success: false, error: 'Generation canceled.', canceled: true }) + finalize({ + success: false, + error: 'Generation canceled.', + canceled: true + }) return } if (outputLimitExceeded) {