fix: settle relay agent exec timeouts

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<T>(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<typeof vi.fn>
+  stdout: EventEmitter
+  stderr: EventEmitter
+  stdin: { end: ReturnType<typeof vi.fn> }
+}
+
+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<string, MethodHandler> {
+  const handlers = new Map<string, MethodHandler>()
+  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 }
+}

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<typeof ChildProcess>()
+  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()
+    })
+  })
+})

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<typeof ChildProcess>()
@@ -20,47 +15,7 @@ vi.mock('child_process', async (importOriginal) => {
 const spawnMock = vi.mocked(spawn)
 const execMock = vi.mocked(exec)
 
-function withPlatform<T>(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<typeof vi.fn>
-  stdout: EventEmitter
-  stderr: EventEmitter
-  stdin: { end: ReturnType<typeof vi.fn> }
-}
-
-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<string, MethodHandler> {
-  const handlers = new Map<string, MethodHandler>()
-  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<AgentExecResult>
+      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()
+    }
+  })
 })

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<typeof setTimeout> | 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)