From 6460dee71c6cdd2e680950f1266a36f0a847263f Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 30 May 2026 08:07:43 -0700 Subject: [PATCH] fix: settle relay agent exec timeouts --- src/relay/agent-exec-handler-test-harness.ts | 46 ++++++ src/relay/agent-exec-handler-windows.test.ts | 108 ++++++++++++ src/relay/agent-exec-handler.test.ts | 165 +++++-------------- src/relay/agent-exec-handler.ts | 38 +++-- 4 files changed, 222 insertions(+), 135 deletions(-) create mode 100644 src/relay/agent-exec-handler-test-harness.ts create mode 100644 src/relay/agent-exec-handler-windows.test.ts diff --git a/src/relay/agent-exec-handler-test-harness.ts b/src/relay/agent-exec-handler-test-harness.ts new file mode 100644 index 00000000000..7fe94d6c837 --- /dev/null +++ b/src/relay/agent-exec-handler-test-harness.ts @@ -0,0 +1,46 @@ +import { EventEmitter } from 'events' +import { vi } from 'vitest' +import type { MethodHandler, RequestContext } from './dispatcher' +import { AgentExecHandler } from './agent-exec-handler' + +export function withPlatform(platform: NodeJS.Platform, fn: () => T): T { + const original = process.platform + Object.defineProperty(process, 'platform', { configurable: true, value: platform }) + try { + return fn() + } finally { + Object.defineProperty(process, 'platform', { configurable: true, value: original }) + } +} + +export type FakeChild = EventEmitter & { + pid: number + kill: ReturnType + stdout: EventEmitter + stderr: EventEmitter + stdin: { end: ReturnType } +} + +export function createFakeChild(): FakeChild { + return Object.assign(new EventEmitter(), { + pid: 12345, + kill: vi.fn(), + stdout: new EventEmitter(), + stderr: new EventEmitter(), + stdin: { end: vi.fn() } + }) +} + +export function createHandlers(): Map { + const handlers = new Map() + new AgentExecHandler({ + onRequest: (method: string, handler: MethodHandler): void => { + handlers.set(method, handler) + } + } as never) + return handlers +} + +export function requestContext(clientId = 1): RequestContext { + return { clientId, isStale: () => false } +} diff --git a/src/relay/agent-exec-handler-windows.test.ts b/src/relay/agent-exec-handler-windows.test.ts new file mode 100644 index 00000000000..4ba0a97f9de --- /dev/null +++ b/src/relay/agent-exec-handler-windows.test.ts @@ -0,0 +1,108 @@ +import { exec, spawn } from 'child_process' +import { mkdtempSync, rmSync, writeFileSync } from 'fs' +import { tmpdir } from 'os' +import { join } from 'path' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as ChildProcess from 'child_process' +import { + createFakeChild, + createHandlers, + requestContext, + withPlatform +} from './agent-exec-handler-test-harness' + +vi.mock('child_process', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + exec: vi.fn(), + spawn: vi.fn() + } +}) + +const spawnMock = vi.mocked(spawn) +const execMock = vi.mocked(exec) + +describe('AgentExecHandler Windows command spawning', () => { + beforeEach(() => { + spawnMock.mockReset() + execMock.mockReset() + }) + + it('resolves bare Windows agent commands to batch shims before spawning', async () => { + const tempDir = mkdtempSync(join(tmpdir(), 'orca-agent-exec-')) + const originalComSpec = process.env.ComSpec + process.env.ComSpec = 'C:\\Windows\\System32\\cmd.exe' + try { + await withPlatform('win32', async () => { + const codexShim = join(tempDir, 'codex.cmd') + writeFileSync(codexShim, '@echo off\r\n') + const child = createFakeChild() + spawnMock.mockReturnValue(child as never) + const handlers = createHandlers() + + const pending = handlers.get('agent.execNonInteractive')!( + { + binary: 'codex', + args: ['exec', '-s', 'read-only'], + cwd: 'C:\\repo', + stdin: 'PROMPT', + timeoutMs: 5_000, + env: { PATH: tempDir } + }, + requestContext() + ) + + child.emit('close', 0) + + await expect(pending).resolves.toMatchObject({ + exitCode: 0, + timedOut: false + }) + expect(spawnMock).toHaveBeenCalledWith( + 'C:\\Windows\\System32\\cmd.exe', + ['/d', '/s', '/c', `"${codexShim}" "exec" "-s" "read-only"`], + { + cwd: 'C:\\repo', + env: expect.objectContaining({ PATH: tempDir }), + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true + } + ) + }) + } finally { + rmSync(tempDir, { recursive: true, force: true }) + if (originalComSpec === undefined) { + delete process.env.ComSpec + } else { + process.env.ComSpec = originalComSpec + } + } + }) + + it('rejects unsafe args when routing Windows batch shims through cmd.exe', async () => { + await withPlatform('win32', async () => { + const handlers = createHandlers() + + const result = await handlers.get('agent.execNonInteractive')!( + { + binary: 'C:\\tools\\agent.cmd', + args: ['hello & goodbye'], + cwd: 'C:\\repo', + stdin: null, + timeoutMs: 5_000 + }, + requestContext() + ) + + expect(result).toEqual({ + stdout: '', + stderr: '', + exitCode: null, + timedOut: false, + spawnError: 'UNSAFE_WINDOWS_BATCH_ARGUMENTS' + }) + expect(spawnMock).not.toHaveBeenCalled() + }) + }) +}) diff --git a/src/relay/agent-exec-handler.test.ts b/src/relay/agent-exec-handler.test.ts index edf9c57bcbf..9b157ef543b 100644 --- a/src/relay/agent-exec-handler.test.ts +++ b/src/relay/agent-exec-handler.test.ts @@ -1,12 +1,7 @@ -import { EventEmitter } from 'events' import { exec, spawn } from 'child_process' -import { mkdtempSync, rmSync, writeFileSync } from 'fs' -import { tmpdir } from 'os' -import { join } from 'path' import { beforeEach, describe, expect, it, vi } from 'vitest' import type * as ChildProcess from 'child_process' -import type { MethodHandler, RequestContext } from './dispatcher' -import { AgentExecHandler } from './agent-exec-handler' +import { createFakeChild, createHandlers, requestContext } from './agent-exec-handler-test-harness' vi.mock('child_process', async (importOriginal) => { const actual = await importOriginal() @@ -20,47 +15,7 @@ vi.mock('child_process', async (importOriginal) => { const spawnMock = vi.mocked(spawn) const execMock = vi.mocked(exec) -function withPlatform(platform: NodeJS.Platform, fn: () => T): T { - const original = process.platform - Object.defineProperty(process, 'platform', { configurable: true, value: platform }) - try { - return fn() - } finally { - Object.defineProperty(process, 'platform', { configurable: true, value: original }) - } -} - -type FakeChild = EventEmitter & { - pid: number - kill: ReturnType - stdout: EventEmitter - stderr: EventEmitter - stdin: { end: ReturnType } -} - -function createFakeChild(): FakeChild { - return Object.assign(new EventEmitter(), { - pid: 12345, - kill: vi.fn(), - stdout: new EventEmitter(), - stderr: new EventEmitter(), - stdin: { end: vi.fn() } - }) -} - -function createHandlers(): Map { - const handlers = new Map() - new AgentExecHandler({ - onRequest: (method: string, handler: MethodHandler): void => { - handlers.set(method, handler) - } - } as never) - return handlers -} - -function requestContext(clientId = 1): RequestContext { - return { clientId, isStale: () => false } -} +type AgentExecResult = { exitCode: number | null; timedOut: boolean } describe('AgentExecHandler', () => { beforeEach(() => { @@ -142,83 +97,6 @@ describe('AgentExecHandler', () => { }) }) - it('resolves bare Windows agent commands to batch shims before spawning', async () => { - const tempDir = mkdtempSync(join(tmpdir(), 'orca-agent-exec-')) - const originalComSpec = process.env.ComSpec - process.env.ComSpec = 'C:\\Windows\\System32\\cmd.exe' - try { - await withPlatform('win32', async () => { - const codexShim = join(tempDir, 'codex.cmd') - writeFileSync(codexShim, '@echo off\r\n') - const child = createFakeChild() - spawnMock.mockReturnValue(child as never) - const handlers = createHandlers() - - const pending = handlers.get('agent.execNonInteractive')!( - { - binary: 'codex', - args: ['exec', '-s', 'read-only'], - cwd: 'C:\\repo', - stdin: 'PROMPT', - timeoutMs: 5_000, - env: { PATH: tempDir } - }, - requestContext() - ) - - child.emit('close', 0) - - await expect(pending).resolves.toMatchObject({ - exitCode: 0, - timedOut: false - }) - expect(spawnMock).toHaveBeenCalledWith( - 'C:\\Windows\\System32\\cmd.exe', - ['/d', '/s', '/c', `"${codexShim}" "exec" "-s" "read-only"`], - { - cwd: 'C:\\repo', - env: expect.objectContaining({ PATH: tempDir }), - stdio: ['pipe', 'pipe', 'pipe'], - windowsHide: true - } - ) - }) - } finally { - rmSync(tempDir, { recursive: true, force: true }) - if (originalComSpec === undefined) { - delete process.env.ComSpec - } else { - process.env.ComSpec = originalComSpec - } - } - }) - - it('rejects unsafe args when routing Windows batch shims through cmd.exe', async () => { - await withPlatform('win32', async () => { - const handlers = createHandlers() - - const result = await handlers.get('agent.execNonInteractive')!( - { - binary: 'C:\\tools\\agent.cmd', - args: ['hello & goodbye'], - cwd: 'C:\\repo', - stdin: null, - timeoutMs: 5_000 - }, - requestContext() - ) - - expect(result).toEqual({ - stdout: '', - stderr: '', - exitCode: null, - timedOut: false, - spawnError: 'UNSAFE_WINDOWS_BATCH_ARGUMENTS' - }) - expect(spawnMock).not.toHaveBeenCalled() - }) - }) - it('cancels the in-flight command for the requested cwd', async () => { const child = createFakeChild() spawnMock.mockReturnValue(child as never) @@ -326,4 +204,43 @@ describe('AgentExecHandler', () => { handlers.get('agent.cancelExec')!({ cwd: '/repo' }, requestContext()) ).resolves.toEqual({ canceled: false }) }) + + it('settles timed-out commands even when the killed child does not close', async () => { + vi.useFakeTimers() + try { + const child = createFakeChild() + spawnMock.mockReturnValue(child as never) + const handlers = createHandlers() + + const pending = handlers.get('agent.execNonInteractive')!( + { + binary: 'agent', + args: [], + cwd: '/repo', + stdin: null, + timeoutMs: 5_000 + }, + requestContext() + ) as Promise + const outcomePromise = pending.then((result) => + result.timedOut ? `timed-out:${result.exitCode}` : 'not-timed-out' + ) + + await vi.advanceTimersByTimeAsync(5_000) + const outcome = await Promise.race([outcomePromise, Promise.resolve('pending')]) + + expect(outcome).toBe('timed-out:null') + if (process.platform === 'win32') { + expect(execMock).toHaveBeenCalledWith('taskkill /pid 12345 /T /F', expect.any(Function)) + } else { + expect(child.kill).toHaveBeenCalledWith('SIGKILL') + } + expect(child.stdout.listenerCount('data')).toBe(0) + expect(child.stderr.listenerCount('data')).toBe(0) + expect(child.listenerCount('error')).toBe(0) + expect(child.listenerCount('close')).toBe(0) + } finally { + vi.useRealTimers() + } + }) }) diff --git a/src/relay/agent-exec-handler.ts b/src/relay/agent-exec-handler.ts index 2a0ca4ae61a..65b3f393172 100644 --- a/src/relay/agent-exec-handler.ts +++ b/src/relay/agent-exec-handler.ts @@ -199,11 +199,18 @@ export class AgentExecHandler { let settled = false const laneKey = typeof cwd === 'string' ? this.laneKey(cwd, params.operation) : '' let entry: InFlightExec | null = null + let timer: ReturnType | null = null + let detachChildListeners = (): void => {} const finish = (result: ExecResult): void => { if (settled) { return } settled = true + if (timer) { + clearTimeout(timer) + timer = null + } + detachChildListeners() if (laneKey && entry && this.inFlightByLane.get(laneKey) === entry) { this.inFlightByLane.delete(laneKey) } @@ -219,32 +226,32 @@ export class AgentExecHandler { this.inFlightByLane.set(laneKey, entry) } - const timer = setTimeout(() => { + timer = setTimeout(() => { timedOut = true // Why: tree-kill because some CLIs trap SIGTERM and continue streaming; // also Windows wraps `.cmd` shims in cmd.exe, so the immediate child // is not the real node.exe process. killProcessTree(child) + finish({ stdout, stderr, exitCode: null, timedOut, canceled }) }, timeoutMs) - child.stdout?.on('data', (chunk: Buffer) => { + const onStdoutData = (chunk: Buffer): void => { stdoutBytes += chunk.byteLength if (stdoutBytes > MAX_OUTPUT_BYTES) { killProcessTree(child) return } stdout += chunk.toString('utf-8') - }) - child.stderr?.on('data', (chunk: Buffer) => { + } + const onStderrData = (chunk: Buffer): void => { stderrBytes += chunk.byteLength if (stderrBytes > MAX_OUTPUT_BYTES) { killProcessTree(child) return } stderr += chunk.toString('utf-8') - }) - child.on('error', (error) => { - clearTimeout(timer) + } + const onError = (error: Error): void => { finish({ stdout, stderr, @@ -252,11 +259,20 @@ export class AgentExecHandler { timedOut, spawnError: error.message }) - }) - child.on('close', (code) => { - clearTimeout(timer) + } + const onClose = (code: number | null): void => { finish({ stdout, stderr, exitCode: code, timedOut, canceled }) - }) + } + child.stdout?.on('data', onStdoutData) + child.stderr?.on('data', onStderrData) + child.on('error', onError) + child.on('close', onClose) + detachChildListeners = () => { + child.stdout?.off('data', onStdoutData) + child.stderr?.off('data', onStderrData) + child.off('error', onError) + child.off('close', onClose) + } if (stdinPayload !== null) { child.stdin?.end(stdinPayload)