From 451c2929ed3b94804da804817823aafc6f8aafcd Mon Sep 17 00:00:00 2001 From: pku-xht Date: Tue, 4 Aug 2026 20:26:45 +0800 Subject: [PATCH] fix(subagent): close terminal teardown races --- packages/subagent/subagent-acp/src/run.ts | 44 +++++++++--- .../subagent-acp/tests/subagent-acp.spec.ts | 68 ++++++++++++++++++- packages/subagent/subagent-codex/src/wire.ts | 2 +- .../tests/subagent-codex.spec.ts | 14 ++-- .../subprocess/subprocess-local/src/spawn.ts | 21 +++--- .../subprocess-local/tests/spawn.spec.ts | 16 ++++- 6 files changed, 135 insertions(+), 30 deletions(-) diff --git a/packages/subagent/subagent-acp/src/run.ts b/packages/subagent/subagent-acp/src/run.ts index fba0403739..f264261b0e 100644 --- a/packages/subagent/subagent-acp/src/run.ts +++ b/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. */ -async function treeExitsWithin(child: SubprocessHandle, ms: number): Promise { - const controller = new AbortController() - const timer = setTimeout(() => { controller.abort() }, ms) - try { - return await child.waitForExit(controller.signal) - } finally { - clearTimeout(timer) +/** Largest delay Node schedules without collapsing it to one millisecond. */ +const MAX_TIMER_DELAY_MS = 2_147_483_647n + +function scaledFiniteMilliseconds(ms: number, scale: number): bigint { + const whole = Math.floor(ms) + return BigInt(whole) * BigInt(scale) + + BigInt(Math.ceil((ms - whole) * scale)) +} + +/** + * 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 { + 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') } } diff --git a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts b/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts index f2cbeda27b..1a8bc577c1 100644 --- a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts +++ b/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((resolve) => { + signal?.addEventListener('abort', () => { resolve(false) }, { once: true }) + if (waitCount === 2) reportExited = resolve + }) + }) + const child: Parameters[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((resolve) => { + signal?.addEventListener('abort', () => { resolve(false) }, { once: true }) + })) + const child: Parameters[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'], diff --git a/packages/subagent/subagent-codex/src/wire.ts b/packages/subagent/subagent-codex/src/wire.ts index 304c5eadb4..f933c1a04b 100644 --- a/packages/subagent/subagent-codex/src/wire.ts +++ b/packages/subagent/subagent-codex/src/wire.ts @@ -256,7 +256,7 @@ export class CodexAppServerWire { } private async guarded(pending: Promise, signal: AbortSignal): Promise { - const withFatal = Promise.race([pending, this.fatal.promise]) + const withFatal = Promise.race([this.fatal.promise, pending]) return raceAbort(withFatal, signal) } diff --git a/packages/subagent/subagent-codex/tests/subagent-codex.spec.ts b/packages/subagent/subagent-codex/tests/subagent-codex.spec.ts index 18e28cc7cc..8e6c7ebd51 100644 --- a/packages/subagent/subagent-codex/tests/subagent-codex.spec.ts +++ b/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'), - agentMessage('late answer', 'final_answer'), + { id: turnStart.id, result: { turn: { id: 'turn-1' } } }, + { 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() }) diff --git a/packages/subprocess/subprocess-local/src/spawn.ts b/packages/subprocess/subprocess-local/src/spawn.ts index d3cbb0cf55..37e36bd213 100644 --- a/packages/subprocess/subprocess-local/src/spawn.ts +++ b/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 | 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 = 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 - }) + graceTimer = scheduleFiniteTimeout(spec.graceMs, () => { kill('SIGKILL') }) } // 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 } diff --git a/packages/subprocess/subprocess-local/tests/spawn.spec.ts b/packages/subprocess/subprocess-local/tests/spawn.spec.ts index 87c81116ff..f08e18c2ed 100644 --- a/packages/subprocess/subprocess-local/tests/spawn.spec.ts +++ b/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 () => {