fix(subagent): close terminal teardown races

packages/subagent/subagent-acp/src/run.ts

@@ -90,15 +90,41 @@ export const DEFAULT_DISPOSE_EOF_GRACE_MS = 6_000
 /** Default POSIX grace between SIGTERM and SIGKILL on dispose (the `disposeGraceMs` config). */
 export const DEFAULT_DISPOSE_GRACE_MS = 3_000
 
-/** Bounded whole-tree exit wait: polls the handle's tree liveness until it exits or `ms` elapses. */
+/** Largest delay Node schedules without collapsing it to one millisecond. */
-async function treeExitsWithin(child: SubprocessHandle, ms: number): Promise<boolean> {
+const MAX_TIMER_DELAY_MS = 2_147_483_647n
-  const controller = new AbortController()
+
-  const timer = setTimeout(() => { controller.abort() }, ms)
+function scaledFiniteMilliseconds(ms: number, scale: number): bigint {
-  try {
+  const whole = Math.floor(ms)
-    return await child.waitForExit(controller.signal)
+  return BigInt(whole) * BigInt(scale)
-  } finally {
+    + BigInt(Math.ceil((ms - whole) * scale))
-    clearTimeout(timer)
+}
+
+/**
+ * Bounded whole-tree exit wait across Node-safe timer segments.
+ * @param child - process tree whose liveness is authoritative.
+ * @param ms - positive finite base window in milliseconds.
+ * @param scale - integer multiplier applied without Number overflow.
+ */
+async function treeExitsWithin(
+  child: SubprocessHandle,
+  ms: number,
+  scale = 1,
+): Promise<boolean> {
+  let remaining = scaledFiniteMilliseconds(ms, scale)
+  while (remaining > 0n) {
+    const chunk = remaining > MAX_TIMER_DELAY_MS
+      ? MAX_TIMER_DELAY_MS
+      : remaining
+    remaining -= chunk
+    const controller = new AbortController()
+    const timer = setTimeout(() => { controller.abort() }, Number(chunk))
+    try {
+      if (await child.waitForExit(controller.signal)) return true
+    } finally {
+      clearTimeout(timer)
+    }
   }
+  return false
 }
 
 /**
@@ -125,7 +151,7 @@ export async function disposeAcpChild(child: SubprocessHandle, eofGraceMs: numbe
   // (this plugin passes disposeGraceMs there), so the bound covers both the
   // escalation window and an equal confirmation window after the SIGKILL.
   child.terminate()
-  if (!(await treeExitsWithin(child, graceMs * 2))) {
+  if (!(await treeExitsWithin(child, graceMs, 2))) {
     throw new Error('ACP child process tree did not exit within its dispose windows')
   }
 }

packages/subagent/subagent-acp/tests/subagent-acp.spec.ts

@@ -1,4 +1,4 @@
-import { describe, expect, it } from 'vitest'
+import { describe, expect, it, vi } from 'vitest'
 import { Context } from 'cordis'
 import Loader from '@cordisjs/plugin-loader'
 import { chmodSync, existsSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from 'node:fs'
@@ -190,6 +190,72 @@ describe('disposeAcpChild (the backend-owned teardown ladder over seam verbs)',
     await expect(disposeAcpChild(never, 20, 20)).rejects.toThrow(/did not exit within its dispose windows/)
   })
 
+  it('keeps an oversized finite escalation window instead of collapsing it to one millisecond', async () => {
+    vi.useFakeTimers()
+    try {
+      let waitCount = 0
+      let reportExited!: (exited: boolean) => void
+      const terminate = vi.fn()
+      const waitForExit = vi.fn((signal?: AbortSignal) => {
+        waitCount += 1
+        return new Promise<boolean>((resolve) => {
+          signal?.addEventListener('abort', () => { resolve(false) }, { once: true })
+          if (waitCount === 2) reportExited = resolve
+        })
+      })
+      const child: Parameters<typeof disposeAcpChild>[0] = {
+        pid: 1,
+        stdin: undefined,
+        stdout: undefined,
+        stderr: undefined,
+        collected: {},
+        done: new Promise(() => {}),
+        terminate,
+        waitForExit,
+      }
+      const disposal = disposeAcpChild(child, 0.25, Number.MAX_VALUE)
+      await vi.advanceTimersByTimeAsync(1)
+      expect(terminate).toHaveBeenCalledOnce()
+      expect(waitForExit).toHaveBeenCalledTimes(2)
+      const escalationSignal = waitForExit.mock.calls[1]?.[0]
+      await vi.advanceTimersByTimeAsync(1)
+      expect(escalationSignal?.aborted).toBe(false)
+      reportExited(true)
+      await expect(disposal).resolves.toBeUndefined()
+      expect(vi.getTimerCount()).toBe(0)
+    } finally {
+      vi.useRealTimers()
+    }
+  })
+
+  it('chains a doubled grace beyond one Node timer segment', async () => {
+    vi.useFakeTimers()
+    try {
+      const waitForExit = vi.fn((signal?: AbortSignal) => new Promise<boolean>((resolve) => {
+        signal?.addEventListener('abort', () => { resolve(false) }, { once: true })
+      }))
+      const child: Parameters<typeof disposeAcpChild>[0] = {
+        pid: 1,
+        stdin: undefined,
+        stdout: undefined,
+        stderr: undefined,
+        collected: {},
+        done: new Promise(() => {}),
+        terminate: vi.fn(),
+        waitForExit,
+      }
+      const disposal = disposeAcpChild(child, 0.25, 1_073_741_823.75)
+      const rejected = expect(disposal).rejects.toThrow(/did not exit within its dispose windows/)
+      await vi.advanceTimersByTimeAsync(1)
+      await vi.advanceTimersByTimeAsync(2_147_483_647)
+      expect(waitForExit).toHaveBeenCalledTimes(3)
+      await vi.advanceTimersByTimeAsync(1)
+      await rejected
+    } finally {
+      vi.useRealTimers()
+    }
+  })
+
   it('observes a spawn-level rejection and returns without a process to reap', async () => {
     const child = spawnSubprocess({
       argv: ['bash', '-c', 'true'],

packages/subagent/subagent-codex/src/wire.ts

@@ -256,7 +256,7 @@ export class CodexAppServerWire {
   }
 
   private async guarded<T>(pending: Promise<T>, signal: AbortSignal): Promise<T> {
-    const withFatal = Promise.race([pending, this.fatal.promise])
+    const withFatal = Promise.race([this.fatal.promise, pending])
     return raceAbort(withFatal, signal)
   }
 

packages/subagent/subagent-codex/tests/subagent-codex.spec.ts

@@ -369,8 +369,9 @@ describe('CodexAppServerWire', () => {
         { type: 'text', text: 'second', text_elements: [] },
       ],
     })
+    child.peer.respond(turnStart, { turn: { id: 'turn-1' } })
+    await nextTask()
     child.peer.send(
-      { id: turnStart.id, result: { turn: { id: 'turn-1' } } },
       {
         method: 'turn/started',
         params: { threadId: 'thread-1', turn: { id: 'turn-1' } },
@@ -516,18 +517,17 @@ describe('CodexAppServerWire', () => {
     }
   })
 
-  it('keeps an earlier fatal frame authoritative over later completion in the same chunk', async () => {
+  it('keeps an unsupported request authoritative over an early terminal in the same chunk', async () => {
     const { child, wire } = await initializeWire()
     const result = wire.runTurn(['task'], new AbortController().signal, () => false)
     const turnStart = await child.peer.nextMethod('turn/start')
-    child.peer.respond(turnStart, { turn: { id: 'turn-1' } })
-    await nextTask()
     child.peer.send(
-      agentMessage('invalid', 'future_phase'),
+      { id: turnStart.id, result: { turn: { id: 'turn-1' } } },
-      agentMessage('late answer', 'final_answer'),
+      { id: 'future-request', method: 'future/request', params: {} },
+      agentMessage('early answer', 'final_answer'),
       turnCompleted('completed'),
     )
-    await expect(result).rejects.toThrow('unknown agent message phase')
+    await expect(result).rejects.toThrow('unsupported app-server request')
     wire.close()
   })
 

packages/subprocess/subprocess-local/src/spawn.ts

@@ -383,6 +383,7 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter
   const stderrCollector = collectStream(errMode, child.stderr, 'stderr')
 
   let graceTimer: ReturnType<typeof scheduleFiniteTimeout> | undefined
+  let terminationStarted = false
   let settled = false
 
   // Failed spawns use pid -1 so signalling remains a no-op.
@@ -418,14 +419,15 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter
   // child and must stay signalable, while a fully-dead tree (possible pid
   // reuse) must not be re-signalled by a later tier.
   const kill = (sig: NodeJS.Signals): void => {
-    /* v8 ignore next -- the exit monitor cancels the ordinary dead-tree timer;
+    /* v8 ignore next -- a successful consumer wait cancels the ordinary dead-tree timer;
        this remains the timer/death race guard and cannot be staged deterministically. */
     if (!treeAlive()) return
     signalTree(platform, pid, sig, child, taskkill)
   }
 
   const terminate = (): void => {
-    if (graceTimer !== undefined) return // escalation already in flight
+    if (terminationStarted) return
+    terminationStarted = true
     if (!treeAlive()) return
     kill('SIGTERM')
     // The escalation must survive direct-child settlement — the leader dying
@@ -433,15 +435,7 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter
     // kill() re-probes tree liveness before force-killing. It stays ref'd:
     // the pending SIGKILL is a commitment, and a parent exiting before it
     // fires would orphan a trapped survivor. Self-bounds at graceMs.
-    const timer = scheduleFiniteTimeout(spec.graceMs, () => { kill('SIGKILL') })
+    graceTimer = scheduleFiniteTimeout(spec.graceMs, () => { kill('SIGKILL') })
-    graceTimer = timer
-    // A very large configured grace must not pin the parent after TERM already
-    // removed the whole tree. Keep the escalation armed only while its target
-    // remains alive; direct-child settlement alone is not sufficient.
-    void waitForExit().then(() => {
-      timer.cancel()
-      graceTimer = undefined
-    })
   }
 
   // The caller owns timeout classification; this layer only reacts to abort.
@@ -497,6 +491,11 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter
       if (signal?.aborted) return false
       await sleepTick()
     }
+    // Successful observation is the permanent no-more-signals boundary. It
+    // also cancels an escalation whose TERM tier already removed the tree.
+    terminationStarted = true
+    graceTimer?.cancel()
+    graceTimer = undefined
     return true
   }
 

packages/subprocess/subprocess-local/tests/spawn.spec.ts

@@ -668,7 +668,6 @@ describe('coverage seams', () => {
   it('terminate() after the tree died delivers no termination signal', async () => {
     const running = spawnSubprocess(spec('true'))
     await running.done
-    await running.waitForExit()
     const spy = vi.spyOn(process, 'kill')
     try {
       running.terminate()
@@ -677,6 +676,21 @@ describe('coverage seams', () => {
     } finally {
       spy.mockRestore()
     }
+    await running.waitForExit()
+  })
+
+  it('repeated terminate after exit never probes or signals a reused process group', async () => {
+    const running = spawnSubprocess(spec('sleep 60'))
+    running.terminate()
+    await running.done
+    await running.waitForExit()
+    const spy = vi.spyOn(process, 'kill').mockImplementation(() => true)
+    try {
+      running.terminate()
+      expect(spy).not.toHaveBeenCalled()
+    } finally {
+      spy.mockRestore()
+    }
   })
 
   it('waitForExit on a failed spawn reports exited immediately', async () => {