Pine/deepseek-harness
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases 1 Wiki Activity

Bound ACP dispose with SIGKILL escalation; skip spawn when pre-aborted (Codex review round 1)

Browse Source 
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 <sentinel>` 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.
This commit is contained in:
Tianyi Cui
2026-06-22 11:39:30 +08:00
parent f393043b03
commit 6801130f8c
3 changed files with 126 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/subagent/subagent-acp/src/run.ts
+54 -17
View File
@@ -70,8 +70,17 @@ export interface AcpRunSpec {
* the credential-scrub pattern (an explicit opt-in for the child's own creds).
*/
env: Record<string, string>
/**
* 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<void> {
// 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<void>(resolve => child.once('exit', () => { resolve() }))
}
@@ -151,6 +163,18 @@ function waitForExit(child: ChildProcess): Promise<void> {
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<void> { 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<void> {
@@ -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<void> {
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<boolean>(resolve => setTimeout(() => { resolve(false) }, graceMs).unref()),
])
if (!exited) {
child.kill('SIGKILL')
await waitForExit(child)
}
await waitForExit(child)
},
}
}
packages/subagent/subagent-acp/tests/mock-acp-server.ts
+14
View File
@@ -148,3 +148,17 @@ new AgentSideConnection(
Readable.toWeb(process.stdin) as ReadableStream<Uint8Array>,
),
)
// 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')
}
packages/subagent/subagent-acp/tests/subagent-acp.spec.ts
+58 -10
View File
@@ -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 <sentinel>` — 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 () => {