From 6801130f8cc79c5001c458e52150e622024ac840 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Mon, 22 Jun 2026 11:39:30 +0800 Subject: [PATCH] Bound ACP dispose with SIGKILL escalation; skip spawn when pre-aborted (Codex review round 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two lifecycle findings from the review: - A (blocker): dispose() could hang forever. It only sent SIGTERM and awaited exit, with no escalation — a child that traps SIGTERM (or our acp-agent if it doesn't quiesce on stdin EOF) would wedge dispose, stranding tool-subagent's finally cleanup and orphaning child-owned work (e.g. bash subprocesses). dispose now: ends stdin (graceful ACP close so the child can flush + exit), SIGTERM, then escalates to SIGKILL if it doesn't exit within a grace period (DEFAULT_DISPOSE_GRACE_MS, injectable via spec.disposeGraceMs), awaiting the certain exit. Mirrors the bash executor's bounded teardown. Regression test drives a SIGTERM-trapping mock subprocess and asserts dispose returns promptly — proven to hang (red) without the escalation. - B: an already-aborted request still spawned the configured binary. startAcpRun now returns an inert already-aborted run BEFORE spawning, so a pre-cancelled request launches nothing. Test points the command at `touch ` and asserts the sentinel never appears. The dispose regression test exposed (via systematic-debugging) that the child must signal trap-armed readiness before the test cancels — a bare timeout raced the trap install and the default SIGTERM handler killed the child, making the guard a no-op. The mock now touches its ready file once the trap is in place and the test waits on that condition. The `cancelled` flag moved onto a holder object so TS control-flow doesn't narrow the catch-time read to always-false. --- packages/subagent/subagent-acp/src/run.ts | 71 ++++++++++++++----- .../subagent-acp/tests/mock-acp-server.ts | 14 ++++ .../subagent-acp/tests/subagent-acp.spec.ts | 68 +++++++++++++++--- 3 files changed, 126 insertions(+), 27 deletions(-) diff --git a/packages/subagent/subagent-acp/src/run.ts b/packages/subagent/subagent-acp/src/run.ts index 18f20c53c1..612fe87074 100644 --- a/packages/subagent/subagent-acp/src/run.ts +++ b/packages/subagent/subagent-acp/src/run.ts @@ -70,8 +70,17 @@ export interface AcpRunSpec { * the credential-scrub pattern (an explicit opt-in for the child's own creds). */ env: Record + /** + * Grace period (ms) between `SIGTERM` and the `SIGKILL` escalation in + * {@link SubagentRun.dispose}. Defaults to {@link DEFAULT_DISPOSE_GRACE_MS}; + * a test injects a small value to exercise the escalation without a long wait. + */ + disposeGraceMs?: number } +/** Default grace between SIGTERM and SIGKILL on dispose (mirrors the bash executor). */ +export const DEFAULT_DISPOSE_GRACE_MS = 3_000 + /** * Credential-shaped ambient env vars are NOT forwarded to the child by default * (the parent harness's own `DEEPSEEK_API_KEY`/secrets must not leak into a @@ -133,6 +142,9 @@ export function toAcpPrompt(prompt: ContentBlock[]): AcpContentBlock[] { /** Resolve once the child process exits (any code/signal); immediate if gone. */ function waitForExit(child: ChildProcess): Promise { + // Already-exited fast path: dispose guards on exitCode before calling, so in + // tests the child is always still alive here. + /* v8 ignore next */ if (child.exitCode !== null || child.signalCode !== null) return Promise.resolve() return new Promise(resolve => child.once('exit', () => { resolve() })) } @@ -151,6 +163,18 @@ function waitForExit(child: ChildProcess): Promise { export function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpec): SubagentRun { const id = AgentId(randomUUID()) + // A request already aborted before it starts never spawns the child at all — + // return an inert run that settled `aborted`, rather than launching the + // configured binary just to tear it down. `dispose`/`cancel` are no-ops. + if (request.signal?.aborted) { + return { + id, + result: Promise.resolve({ output: [], stopReason: 'aborted' }), + cancel(_reason?: string): void { /* nothing was started */ }, + dispose(): Promise { return Promise.resolve() }, + } + } + // Spawn the child ACP agent. stdin = ACP request channel, stdout = ACP // response channel, stderr = INHERIT so the child's diagnostics surface on the // parent's stderr (no separate capture to drain — we don't fold child stderr @@ -172,8 +196,11 @@ export function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpec): Su const output: string[] = [] // `cancelled` records that a cancel was requested (signal or cancel()), so a // run torn down before the prompt resolves settles `aborted` rather than the - // generic error mapping. - let cancelled = false + // generic error mapping. Held on a mutable object so the async closures that + // set it (the abort listener) and the IIFE that reads it don't fight TS's + // control-flow narrowing of a bare `let` (which would type the catch-time read + // as always-`false`). + const flags = { cancelled: false } const makeClient = (_agent: AcpAgent): Client => ({ sessionUpdate(params: SessionNotification): Promise { @@ -209,7 +236,7 @@ export function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpec): Su let sessionId: string | undefined const requestCancel = (): void => { - cancelled = true + flags.cancelled = true // Best-effort: tell the child to cancel the in-flight turn. Swallows a // rejection — the session may not exist yet, or the pipe may be gone; the // dispose path kills the process regardless. If the session has NOT been @@ -233,11 +260,6 @@ export function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpec): Su return text.length > 0 ? [{ type: 'text', text }] : [] } try { - // An already-aborted request never runs the child. - if (request.signal?.aborted) { - cancelled = true - return { output: [], stopReason: 'aborted' } - } // Race the ACP drive against a spawn failure: a bad command never speaks // ACP, so `initialize` would hang forever — the spawn `error` event is the // only signal, and a rejected race settles the run `error` via the catch. @@ -254,7 +276,7 @@ export function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpec): Su // send `session/cancel` (no session id yet). Honor it here: settle // `aborted` without ever issuing the prompt, rather than running the child // to completion and ignoring the cancel. - if (cancelled) return { output: collectOutput(), stopReason: 'aborted' } + if (flags.cancelled) return { output: collectOutput(), stopReason: 'aborted' } const promptResult = await conn.prompt({ sessionId, prompt: toAcpPrompt(request.prompt) }) return { output: collectOutput(), stopReason: acpStopReason(promptResult.stopReason) } } @@ -267,7 +289,7 @@ export function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpec): Su // failure. A spawn/transport/RPC error becomes an error/aborted result — // `aborted` if a cancel was requested (the failure is the cancellation // surfacing as a torn pipe / rejected RPC), else a genuine `error`. - return { output: collectOutput(), stopReason: cancelled ? 'aborted' : 'error' } + return { output: collectOutput(), stopReason: flags.cancelled ? 'aborted' : 'error' } } })() @@ -279,14 +301,29 @@ export function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpec): Su }, async dispose(): Promise { request.signal?.removeEventListener('abort', onAbort) - // Kill the subprocess and AWAIT its exit (quiescent teardown — dispose - // must reach quiescence, not merely request it). SIGTERM first; the child - // is our own short-lived ACP agent, so a graceful term is enough. Guard - // the kill: the process may already be gone. - if (child.exitCode === null && child.signalCode === null) { - child.kill('SIGTERM') + // Reach quiescence, not merely request it (dispose must AWAIT the child + // actually stopping). If the child is already gone, nothing to do. + if (child.exitCode !== null || child.signalCode !== null) return + // 1. Graceful: end the ACP request stream (stdin EOF). Our own acp-agent + // disposes its fiber on stdin 'end' — flushing persistence and stopping + // child-owned work (e.g. bash subprocesses) — then exits, which the + // server bridge's connection-close quiesce path drives. A child that + // ignores EOF is handled by the signal escalation below. + child.stdin.end() + // 2. SIGTERM, then escalate to SIGKILL if it does not exit within the + // grace period — a child that traps SIGTERM must not wedge dispose + // forever (the seam requires bounded quiescence). Race the exit against + // a grace timer; on timeout, SIGKILL and await the (now-certain) exit. + child.kill('SIGTERM') + const graceMs = spec.disposeGraceMs ?? DEFAULT_DISPOSE_GRACE_MS + const exited = await Promise.race([ + waitForExit(child).then(() => true), + new Promise(resolve => setTimeout(() => { resolve(false) }, graceMs).unref()), + ]) + if (!exited) { + child.kill('SIGKILL') + await waitForExit(child) } - await waitForExit(child) }, } } diff --git a/packages/subagent/subagent-acp/tests/mock-acp-server.ts b/packages/subagent/subagent-acp/tests/mock-acp-server.ts index e383bbe8eb..8260051f63 100644 --- a/packages/subagent/subagent-acp/tests/mock-acp-server.ts +++ b/packages/subagent/subagent-acp/tests/mock-acp-server.ts @@ -148,3 +148,17 @@ new AgentSideConnection( Readable.toWeb(process.stdin) as ReadableStream, ), ) + +// Under MOCK_TRAP_SIGTERM, ignore SIGTERM and keep stdin open so the process +// neither quiesces on EOF nor dies on the graceful signal — exercising the +// backend dispose path's SIGKILL escalation. Without this the process exits +// normally on SIGTERM / stdin end. Touch READY_FILE once the trap is armed, so +// a test waits for that CONDITION before disposing (the trap must be in place, +// not merely the process spawned — otherwise SIGTERM hits the default handler). +if (process.env.MOCK_TRAP_SIGTERM === '1') { + process.on('SIGTERM', () => { /* trapped: refuse to exit on the graceful signal */ }) + // Keep the event loop alive (a bare timer) so nothing else lets it exit. + setInterval(() => { /* stay alive until SIGKILL */ }, 1000) + if (READY_FILE !== undefined) writeFileSync(READY_FILE, 'trap-armed') +} + diff --git a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts b/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts index ac74ace7e8..0e4604e638 100644 --- a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts +++ b/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts @@ -8,7 +8,7 @@ import { fileURLToPath } from 'node:url' import SubagentService from '@deepseek-ai/dsh-subagent' import type { Agent } from '@deepseek-ai/dsh-agent' import * as acp from '../src/index.ts' -import { acpStopReason, acpContentText, buildChildEnv, SENSITIVE_ENV_PATTERN, toAcpPrompt } from '../src/run.ts' +import { acpStopReason, acpContentText, buildChildEnv, SENSITIVE_ENV_PATTERN, startAcpRun, toAcpPrompt, type AcpRunSpec } from '../src/run.ts' /** * Keyless integration tests for the ACP subagent backend. Each spawns a REAL @@ -159,15 +159,63 @@ describe('dsh-subagent-acp', () => { } }) - it('settles aborted without running the child when the signal is already aborted', async () => { - const controller = new AbortController() - controller.abort() - const ctx = await setup({ MOCK_TEXT: 'never seen' }) - const run = ctx.subagents.start('acp', { prompt: [{ type: 'text', text: 'p' }], parent: fakeParent, signal: controller.signal }) - const result = await run.result - expect(result.stopReason).toBe('aborted') - expect(result.output).toEqual([]) - await run.dispose() + it('settles aborted WITHOUT spawning the child when the signal is already aborted', async () => { + // A pre-aborted request must not even launch the configured binary. Point + // the command at one that would create a sentinel file if it ever ran, and + // assert the sentinel never appears. + const tmp = mkdtempSync(join(tmpdir(), 'acp-preabort-')) + const sentinel = join(tmp, 'spawned') + try { + const controller = new AbortController() + controller.abort() + const run = startAcpRun( + { prompt: [{ type: 'text', text: 'p' }], parent: fakeParent, signal: controller.signal }, + // `touch ` — runs only if the process is actually spawned. + { command: 'touch', args: [sentinel], cwd: tmp, permission: 'reject', env: {} }, + ) + const result = await run.result + expect(result.stopReason).toBe('aborted') + expect(result.output).toEqual([]) + // cancel/dispose on the inert run are safe no-ops. + run.cancel('noop') + await run.dispose() + // The binary was never launched — no sentinel. + expect(existsSync(sentinel)).toBe(false) + } finally { + rmSync(tmp, { recursive: true, force: true }) + } + }) + + it('dispose escalates SIGTERM → SIGKILL for a child that traps SIGTERM (bounded quiescence)', async () => { + // The child traps SIGTERM and keeps its event loop alive, so a graceful + // term alone would hang dispose forever. With a short grace, dispose must + // escalate to SIGKILL and return once the process is actually gone. + const tmp = mkdtempSync(join(tmpdir(), 'acp-trap-')) + const ready = join(tmp, 'trap-armed') + try { + const spec: AcpRunSpec = { + command: process.execPath, + args: ['--import', tsxLoader, mockServer], + cwd: process.cwd(), + permission: 'reject', + env: { MOCK_TRAP_SIGTERM: '1', MOCK_TEXT: 'x', MOCK_READY_FILE: ready, TSX_TSCONFIG_PATH: repoTsconfig }, + disposeGraceMs: 150, + } + const run = startAcpRun({ prompt: [{ type: 'text', text: 'p' }], parent: fakeParent }, spec) + // Wait until the child has BOOTED AND ARMED THE TRAP (a condition, not a + // sleep) — otherwise SIGTERM races the trap install and the default handler + // terminates the child, never exercising the escalation. + await waitForFile(ready) + // Don't await result (the child hangs). Dispose must still return promptly + // via the SIGKILL escalation — bound it so a regression (no escalation) + // fails loud instead of hanging the suite. + await expect(Promise.race([ + run.dispose(), + new Promise((_r, reject) => { setTimeout(() => { reject(new Error('dispose did not return — no SIGKILL escalation')) }, 4000) }), + ])).resolves.toBeUndefined() + } finally { + rmSync(tmp, { recursive: true, force: true }) + } }) it('honors a cancel that races AHEAD of newSession (no session id yet) without running the prompt', async () => {