From 462519596860baad18e6f2c2a61d60de82eaee37 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sun, 19 Jul 2026 12:19:33 +0800 Subject: [PATCH] fix(subagent): bound forced child termination Observe signal errors and bound the final forced-exit edge with disposeGraceMs so a refused or ineffective SIGKILL cannot leave disposal pending forever. Apply the confirmation bound on POSIX and Windows, remove listeners and timers on every outcome, and update the ACP consumer contract plus the generated config catalog. Cover emitted signal errors, synchronous kill exceptions, refused termination, and accepted termination that never reports exit. --- docs/config-catalog.md | 2 +- packages/subagent/subagent-acp/README.md | 4 +- packages/subagent/subagent-acp/src/index.ts | 2 +- packages/subagent/subagent-acp/src/run.ts | 6 +- .../subagent/subagent-subprocess/README.md | 4 +- .../subagent/subagent-subprocess/src/index.ts | 58 +++++++++++++---- .../tests/subagent-subprocess.spec.ts | 65 +++++++++++++++++++ 7 files changed, 118 insertions(+), 23 deletions(-) diff --git a/docs/config-catalog.md b/docs/config-catalog.md index 7921b98f55..dd652c7d45 100644 --- a/docs/config-catalog.md +++ b/docs/config-catalog.md @@ -866,7 +866,7 @@ export interface Config { * before the parent escalates to a signal. */ disposeEofGraceMs?: number - /** POSIX grace period (ms) between `SIGTERM` and `SIGKILL`; unused on Windows. */ + /** Termination confirmation window (ms), including forced exit on every platform. */ disposeGraceMs?: number } diff --git a/packages/subagent/subagent-acp/README.md b/packages/subagent/subagent-acp/README.md index 830335bbb5..240b79c2d7 100644 --- a/packages/subagent/subagent-acp/README.md +++ b/packages/subagent/subagent-acp/README.md @@ -10,7 +10,7 @@ The returned run id is minted in the parent namespace. The child server's sessio After publication, the provider sends the prompt and collects streamed `agent_message_chunk` text into `SubagentResult.output`. A prompt/transport failure resolves with `stopReason: 'error'`, or `aborted` when the required request signal or disposal requested cancellation. -`dispose()` is idempotent. It removes the signal listener, requests ACP cancellation when possible, closes stdin, and waits `disposeEofGraceMs`. POSIX then escalates through SIGTERM and `disposeGraceMs` before SIGKILL; Windows force-terminates directly because Node maps both signals to `TerminateProcess`. Disposal resolves only after child exit. Every run uses a fresh process; process pooling is not implemented. +`dispose()` is idempotent. It removes the signal listener, requests ACP cancellation when possible, closes stdin, and waits `disposeEofGraceMs`. POSIX then escalates through SIGTERM and `disposeGraceMs` before SIGKILL; Windows force-terminates directly because Node maps both signals to `TerminateProcess`. After forced termination, every platform waits at most `disposeGraceMs` for exit and rejects on a signal error or missing exit. Every run uses a fresh process; process pooling is not implemented. ## Capabilities and context @@ -27,7 +27,7 @@ ACP advertises no start-time capabilities because this process cannot enforce th | `permission` | `reject` | Auto-answer permission requests by rejecting or choosing the first allow-shaped option. | | `env` | `{}` | Explicit child environment layered over a credential-scrubbed parent environment. | | `disposeEofGraceMs` | `6000` | Grace after stdin EOF before platform termination. | -| `disposeGraceMs` | `3000` | POSIX grace after SIGTERM before SIGKILL; unused on Windows. | +| `disposeGraceMs` | `3000` | Exit-confirmation grace after termination; POSIX also waits this long after SIGTERM before SIGKILL. | ```yaml - id: subagent-acp diff --git a/packages/subagent/subagent-acp/src/index.ts b/packages/subagent/subagent-acp/src/index.ts index 697be5b98a..0a761831ea 100644 --- a/packages/subagent/subagent-acp/src/index.ts +++ b/packages/subagent/subagent-acp/src/index.ts @@ -46,7 +46,7 @@ export interface Config { * before the parent escalates to a signal. */ disposeEofGraceMs?: number - /** POSIX grace period (ms) between `SIGTERM` and `SIGKILL`; unused on Windows. */ + /** Termination confirmation window (ms), including forced exit on every platform. */ disposeGraceMs?: number } diff --git a/packages/subagent/subagent-acp/src/run.ts b/packages/subagent/subagent-acp/src/run.ts index 9f037a170c..09a6ed81e9 100644 --- a/packages/subagent/subagent-acp/src/run.ts +++ b/packages/subagent/subagent-acp/src/run.ts @@ -56,9 +56,9 @@ export interface AcpRunSpec { */ disposeEofGraceMs: number /** - * POSIX grace period (ms) between `SIGTERM` and `SIGKILL` in - * {@link SubagentRun.dispose}; unused on Windows. The plugin fills this from - * its `disposeGraceMs` config. + * Termination confirmation window (ms) in {@link SubagentRun.dispose}; POSIX applies it after + * `SIGTERM` and `SIGKILL`, while Windows applies it after direct forced termination. The plugin + * fills this from its `disposeGraceMs` config. */ disposeGraceMs: number /** diff --git a/packages/subagent/subagent-subprocess/README.md b/packages/subagent/subagent-subprocess/README.md index 4a72f6279b..80be43125e 100644 --- a/packages/subagent/subagent-subprocess/README.md +++ b/packages/subagent/subagent-subprocess/README.md @@ -20,9 +20,9 @@ The platform-aware dispose ladder resolves only once the child has ACTUALLY exit 1. stdin EOF (when stdin is piped), then wait `graces.disposeEofGraceMs` — a cooperative child quiesces on its own, its flushes and nested-subprocess teardown intact; 2. on POSIX, `SIGTERM`, then wait `graces.disposeGraceMs`; -3. force termination and await exit — `SIGKILL` on POSIX and Node's `TerminateProcess` mapping on Windows. +3. force termination — `SIGKILL` on POSIX and Node's `TerminateProcess` mapping on Windows — then wait at most `graces.disposeGraceMs` for exit; a signal error or missing exit rejects disposal. -The two graces (`DisposeLadderGraces`) come from the consuming plugin's `disposeEofGraceMs`/`disposeGraceMs` Config fields; `disposeGraceMs` is unused on Windows because Node maps `SIGTERM` and `SIGKILL` to the same forced termination. The EOF window is deliberately separate and usually wider, since cooperative teardown may await a signal-trapping grandchild plus a final flush. +The two graces (`DisposeLadderGraces`) come from the consuming plugin's `disposeEofGraceMs`/`disposeGraceMs` Config fields. POSIX uses `disposeGraceMs` after both the graceful and forced signals; Windows skips the redundant graceful signal but uses it to bound forced-exit confirmation. The EOF window is deliberately separate and usually wider, since cooperative teardown may await a signal-trapping grandchild plus a final flush. The exit waits are internal to this ladder. They clean up their timer and listener on either outcome, so escalation never accumulates listeners on the child. diff --git a/packages/subagent/subagent-subprocess/src/index.ts b/packages/subagent/subagent-subprocess/src/index.ts index de4702bac9..47a97bafb6 100644 --- a/packages/subagent/subagent-subprocess/src/index.ts +++ b/packages/subagent/subagent-subprocess/src/index.ts @@ -51,16 +51,6 @@ export function spawnFailure(child: ChildProcess): Promise { }) } -/** - * Resolve once the child process exits (any code/signal); immediate if it is - * already gone. - * @param child - the child process to await. - */ -function waitForExit(child: ChildProcess): Promise { - if (child.exitCode !== null || child.signalCode !== null) return Promise.resolve() - return new Promise(resolve => child.once('exit', () => { resolve() })) -} - /** * Race the child's exit against a timer. Neither outcome leaves anything * behind on the child: the exit listener is removed on timeout and the timer @@ -104,10 +94,49 @@ export interface DisposeLadderGraces { * headroom. */ disposeEofGraceMs: number - /** POSIX tier-2 window (ms): between `SIGTERM` and the `SIGKILL` escalation. */ + /** + * Termination confirmation window (ms): POSIX applies it after `SIGTERM` and again after + * `SIGKILL`; Windows applies it after the direct forced termination. + */ disposeGraceMs: number } +/** Force-terminate a child and reject if no exit edge arrives within the configured grace. */ +function forceTerminateWithin(child: ChildProcess, ms: number): Promise { + if (child.exitCode !== null || child.signalCode !== null) return Promise.resolve() + return new Promise((resolve, reject) => { + let accepted = false + let settled = false + const cleanup = (): void => { + clearTimeout(timer) + child.off('exit', onExit) + child.off('error', onError) + } + const settle = (complete: () => void): void => { + if (settled) return + settled = true + cleanup() + complete() + } + const onExit = (): void => { settle(resolve) } + const onError = (error: Error): void => { settle(() => { reject(error) }) } + child.once('exit', onExit) + child.once('error', onError) + const timer = setTimeout(() => { + const disposition = accepted ? 'accepted' : 'refused' + settle(() => { + reject(new Error(`child process did not exit within ${ms}ms after SIGKILL was ${disposition}`)) + }) + }, ms).unref() + try { + accepted = child.kill('SIGKILL') + if (child.exitCode !== null || child.signalCode !== null) settle(resolve) + } catch (error: unknown) { + settle(() => { reject(new Error('SIGKILL failed', { cause: error })) }) + } + }) +} + /** * Tear a child process down to quiescence, resolving only after exit: close stdin and allow * cooperative flush, then use the host's graceful and forced termination semantics. POSIX @@ -117,6 +146,8 @@ export interface DisposeLadderGraces { * @param child - the child process to tear down. * @param graces - the two grace periods, from the consuming plugin's Config. * @param platform - the host platform, injectable for unit coverage. + * @throws When forced termination errors or the child does not report exit within + * `disposeGraceMs`. */ export async function disposeChildProcess( child: ChildProcess, @@ -133,9 +164,8 @@ export async function disposeChildProcess( child.kill('SIGTERM') if (await exitsWithin(child, graces.disposeGraceMs)) return } - // 3. Force-kill and await the (now-certain) exit. - child.kill('SIGKILL') - await waitForExit(child) + // 3. Force-kill and await a bounded exit edge. + await forceTerminateWithin(child, graces.disposeGraceMs) } /** diff --git a/packages/subagent/subagent-subprocess/tests/subagent-subprocess.spec.ts b/packages/subagent/subagent-subprocess/tests/subagent-subprocess.spec.ts index 901989e074..7c17333c03 100644 --- a/packages/subagent/subagent-subprocess/tests/subagent-subprocess.spec.ts +++ b/packages/subagent/subagent-subprocess/tests/subagent-subprocess.spec.ts @@ -234,6 +234,71 @@ describe('disposeChildProcess', () => { expect(fake.kills).toEqual(['SIGKILL']) expect(fake.signalCode).toBe('SIGKILL') }) + + it('propagates a forced-termination error without waiting for the grace', async () => { + const fake = new FakeChild() + const failure = Object.assign(new Error('kill EPERM'), { code: 'EPERM' }) + vi.spyOn(fake, 'kill').mockImplementation((signal) => { + fake.kills.push(signal) + fake.emit('error', failure) + return false + }) + + await expect(disposeChildProcess( + asChild(fake), + { disposeEofGraceMs: 1, disposeGraceMs: 1000 }, + 'win32', + )).rejects.toBe(failure) + expect(fake.kills).toEqual(['SIGKILL']) + expect(fake.listenerCount('error')).toBe(0) + expect(fake.listenerCount('exit')).toBe(0) + }) + + it('wraps a synchronous forced-termination exception and removes its listeners', async () => { + const fake = new FakeChild() + const failure = new Error('invalid signal state') + vi.spyOn(fake, 'kill').mockImplementation(() => { throw failure }) + + await expect(disposeChildProcess( + asChild(fake), + { disposeEofGraceMs: 1, disposeGraceMs: 1000 }, + 'win32', + )).rejects.toMatchObject({ message: 'SIGKILL failed', cause: failure }) + expect(fake.listenerCount('error')).toBe(0) + expect(fake.listenerCount('exit')).toBe(0) + }) + + it('bounds a refused forced termination that produces no error or exit', async () => { + const fake = new FakeChild() + vi.spyOn(fake, 'kill').mockImplementation((signal) => { + fake.kills.push(signal) + return false + }) + + await expect(disposeChildProcess( + asChild(fake), + { disposeEofGraceMs: 1, disposeGraceMs: 10 }, + 'win32', + )).rejects.toThrow('child process did not exit within 10ms after SIGKILL was refused') + expect(fake.listenerCount('error')).toBe(0) + expect(fake.listenerCount('exit')).toBe(0) + }) + + it('bounds an accepted forced termination that never reports exit', async () => { + const fake = new FakeChild() + vi.spyOn(fake, 'kill').mockImplementation((signal) => { + fake.kills.push(signal) + return true + }) + + await expect(disposeChildProcess( + asChild(fake), + { disposeEofGraceMs: 1, disposeGraceMs: 10 }, + 'win32', + )).rejects.toThrow('child process did not exit within 10ms after SIGKILL was accepted') + expect(fake.listenerCount('error')).toBe(0) + expect(fake.listenerCount('exit')).toBe(0) + }) }) describe('createIsolatedConfigDir', () => {