diff --git a/packages/subagent/subagent-acp/src/run.ts b/packages/subagent/subagent-acp/src/run.ts index c78e8ae08d..8aeb136f2d 100644 --- a/packages/subagent/subagent-acp/src/run.ts +++ b/packages/subagent/subagent-acp/src/run.ts @@ -70,6 +70,13 @@ export interface AcpRunSpec { * the credential-scrub pattern (an explicit opt-in for the child's own creds). */ env: Record + /** + * Grace period (ms) for the child's EOF-driven quiesce in + * {@link SubagentRun.dispose} — the window to flush persistence and tear down + * its OWN nested subprocesses before the parent escalates to a signal. Defaults + * to {@link DEFAULT_DISPOSE_EOF_GRACE_MS}; a test injects a small value. + */ + disposeEofGraceMs?: number /** * Grace period (ms) between `SIGTERM` and the `SIGKILL` escalation in * {@link SubagentRun.dispose}. Defaults to {@link DEFAULT_DISPOSE_GRACE_MS}; @@ -78,6 +85,19 @@ export interface AcpRunSpec { disposeGraceMs?: number } +/** + * Default grace for the child's EOF-driven quiesce on dispose — the window for it + * to flush persistence and tear down its OWN nested subprocesses (which may run + * their own `SIGTERM`→`SIGKILL` escalation) before the parent escalates to a + * signal. Deliberately LARGER than {@link DEFAULT_DISPOSE_GRACE_MS}: a cooperative + * child whose teardown is itself waiting on a signal-trapping grandchild (e.g. a + * bash subprocess in its own ~3s SIGTERM→SIGKILL grace) plus a final flush needs + * MORE than a single signal-grace of headroom, or the parent's SIGTERM cuts it off + * exactly as it reaches its own SIGKILL+flush. The child is an arbitrary ACP agent, + * so this is a standalone generous default, NOT derived from any child's internals. + */ +export const DEFAULT_DISPOSE_EOF_GRACE_MS = 6_000 + /** Default grace between SIGTERM and SIGKILL on dispose (mirrors the bash executor). */ export const DEFAULT_DISPOSE_GRACE_MS = 3_000 @@ -313,6 +333,7 @@ export function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpec): Su // 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 + const eofGraceMs = spec.disposeEofGraceMs ?? DEFAULT_DISPOSE_EOF_GRACE_MS const graceMs = spec.disposeGraceMs ?? DEFAULT_DISPOSE_GRACE_MS // 1. Graceful: end the ACP request stream (stdin EOF) and let the child // quiesce ON ITS OWN. Our acp-agent has NO SIGTERM handler in a normal @@ -321,10 +342,13 @@ export function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpec): Su // stdin EOF, NOT by a signal. A prompt response can resolve from a // turn/end BEFORE that post-turn flush lands, so the child still has // durable work owed when dispose runs. Give the EOF-driven quiesce a real - // window to finish (flush persistence, stop child-owned bash) and EXIT; - // sending SIGTERM in the same tick would default-terminate it mid-flush. + // window — wider than a single signal-grace, since the child's own + // teardown may itself be awaiting a signal-trapping grandchild (a bash + // subprocess in its own SIGTERM→SIGKILL grace) plus a flush — and only + // escalate if it overruns. Sending SIGTERM in the same tick (or too soon) + // would default-terminate the child mid-flush, orphaning its nested work. child.stdin.end() - if (await exitsWithin(child, graceMs)) return + if (await exitsWithin(child, eofGraceMs)) return // 2. SIGTERM, then escalate to SIGKILL if it still does not exit within the // grace period — a child that ignores EOF and traps SIGTERM must not // wedge dispose forever (the seam requires bounded quiescence). diff --git a/packages/subagent/subagent-acp/tests/mock-acp-server.ts b/packages/subagent/subagent-acp/tests/mock-acp-server.ts index b30492258a..74ae340bde 100644 --- a/packages/subagent/subagent-acp/tests/mock-acp-server.ts +++ b/packages/subagent/subagent-acp/tests/mock-acp-server.ts @@ -15,17 +15,19 @@ * polls for this file to cancel on a CONDITION rather than * an arbitrary timeout (subprocess cold-start is variable). * - `MOCK_FLUSH_ON_EOF` — if set, on stdin EOF the agent takes an async beat - * (simulating the real acp-agent's EOF-driven - * quiesce+flush), then touches this path and exits ON ITS - * OWN — no signal. Stands in for a child whose durable - * flush completes only if dispose gives EOF a real window - * before escalating to SIGTERM. + * (MOCK_FLUSH_DELAY_MS, default 150) simulating the real + * acp-agent's EOF-driven quiesce+flush, then touches this + * path and exits ON ITS OWN — no signal. Stands in for a + * child whose durable flush completes only if dispose + * gives EOF a real window before escalating to SIGTERM. * - `MOCK_IGNORE_EOF` — if `1`, keep the event loop alive past stdin EOF (a bare - * timer) but leave SIGTERM at its DEFAULT handler, so the - * child ignores the graceful EOF window yet still dies on - * SIGTERM — exercising dispose's middle tier (exit during - * the SIGTERM grace, before the SIGKILL escalation). It - * touches MOCK_READY_FILE once the keepalive is armed. + * timer) but install a SIGTERM handler that exits (and, if + * MOCK_SIGTERM_FILE is set, touches it as an observable + * proof the SIGTERM rung fired). The child ignores the + * graceful EOF window yet dies cooperatively on SIGTERM — + * exercising dispose's middle tier (exit during the SIGTERM + * grace, before the SIGKILL escalation). Touches + * MOCK_READY_FILE once armed. * * It is NOT a test spec (no `describe`/`it`) — it is spawned BY the specs as the * child process the ACP backend drives. Kept as a `.ts` run under tsx by the @@ -177,26 +179,36 @@ if (process.env.MOCK_TRAP_SIGTERM === '1') { // Under MOCK_FLUSH_ON_EOF, model the real acp-agent's EOF-driven quiesce: on // stdin 'end' (the dispose path's `child.stdin.end()`), take an ASYNC beat to -// "flush", then touch the marker and exit ON OUR OWN — no signal involved. A -// dispose that sends SIGTERM in the same tick as the EOF (no graceful window) -// default-terminates this process before the beat completes, so the marker is -// missing; a dispose that waits for natural exit first lets the flush land. +// "flush", then touch the marker and exit ON OUR OWN — no signal involved. The +// beat is MOCK_FLUSH_DELAY_MS (default 150). A dispose that sends SIGTERM before +// the beat completes (no graceful window, or an EOF grace shorter than the +// flush) default-terminates this process and the marker is missing; a dispose +// that gives the EOF quiesce enough window first lets the flush land. if (FLUSH_ON_EOF !== undefined) { + const flushDelayMs = Number(process.env.MOCK_FLUSH_DELAY_MS ?? '150') process.stdin.on('end', () => { setTimeout(() => { writeFileSync(FLUSH_ON_EOF, 'flushed') process.exit(0) - }, 150) + }, flushDelayMs) }) } -// Under MOCK_IGNORE_EOF, keep the loop alive past stdin EOF but leave SIGTERM at -// its DEFAULT handler — the child ignores the graceful EOF window yet still dies -// on SIGTERM, exercising dispose's middle tier (exit during the SIGTERM grace, -// before the SIGKILL escalation). Touch the ready file once the keepalive is -// armed, so a test disposes on that condition rather than a timeout. +// Under MOCK_IGNORE_EOF, keep the loop alive past stdin EOF (so the graceful EOF +// window times out) but INSTALL A SIGTERM HANDLER that records it and exits — the +// child ignores the graceful EOF window yet dies cooperatively on SIGTERM, +// exercising dispose's MIDDLE tier (exit during the SIGTERM grace, before the +// SIGKILL escalation). When MOCK_SIGTERM_FILE is set the handler touches it, an +// OBSERVABLE proof that the SIGTERM rung fired: if dispose skipped the middle +// rung and jumped EOF→SIGKILL, SIGKILL is uncatchable so the handler never runs +// and the marker is missing. Touch READY_FILE once armed (a test waits on it). if (process.env.MOCK_IGNORE_EOF === '1') { - setInterval(() => { /* stay alive past EOF; default SIGTERM still kills us */ }, 1000) + const sigtermFile = process.env.MOCK_SIGTERM_FILE + process.on('SIGTERM', () => { + if (sigtermFile !== undefined) writeFileSync(sigtermFile, 'sigterm') + process.exit(0) + }) + setInterval(() => { /* stay alive past EOF until SIGTERM */ }, 1000) if (READY_FILE !== undefined) writeFileSync(READY_FILE, 'ignore-eof-armed') } diff --git a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts b/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts index 96b749218d..926319ad87 100644 --- a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts +++ b/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts @@ -199,6 +199,10 @@ describe('dsh-subagent-acp', () => { cwd: process.cwd(), permission: 'reject', env: { MOCK_TRAP_SIGTERM: '1', MOCK_TEXT: 'x', MOCK_READY_FILE: ready, TSX_TSCONFIG_PATH: repoTsconfig }, + // Short on BOTH tiers: the trap ignores EOF and SIGTERM, so dispose must + // burn the EOF window, then the SIGTERM window, then SIGKILL — keep each + // small so the whole ladder finishes well within the 4000ms bound. + disposeEofGraceMs: 150, disposeGraceMs: 150, } const run = startAcpRun({ prompt: [{ type: 'text', text: 'p' }], parent: fakeParent }, spec) @@ -218,13 +222,16 @@ describe('dsh-subagent-acp', () => { } }) - it('dispose gives the child an EOF window to quiesce before escalating (graceful flush)', async () => { + it('dispose gives the child an EOF window that outlasts the SIGTERM grace (graceful flush)', async () => { // The real acp-agent flushes ASYNCHRONOUSLY on stdin EOF (its bridge tears // down on connection close, NOT on a signal) — and it has no SIGTERM handler. - // The mock models that: on stdin 'end' it takes a beat to "flush", touches a - // marker, and exits on its own. dispose() must end stdin and WAIT for that - // natural exit before sending SIGTERM; a same-tick SIGTERM default-kills the - // child mid-flush and the marker never appears. + // Its EOF teardown can itself await a signal-trapping grandchild (a bash + // subprocess in its own SIGTERM→SIGKILL grace) plus a flush, so the EOF window + // must be a SEPARATE, WIDER grace than the SIGTERM tier — not the same value. + // The mock models a flush that takes LONGER than the SIGTERM grace but well + // under the EOF grace: it lands only because tier 1 waits eofGraceMs, not + // graceMs. (If dispose reused the small SIGTERM grace for the EOF wait — the + // round-2 bug — SIGTERM would fire mid-flush and the marker would be missing.) const tmp = mkdtempSync(join(tmpdir(), 'acp-eof-')) const ready = join(tmp, 'ready') const flushed = join(tmp, 'flushed') @@ -235,16 +242,23 @@ describe('dsh-subagent-acp', () => { cwd: process.cwd(), permission: 'reject', // MOCK_HANG so the prompt never resolves on its own — we tear down a live - // child. MOCK_FLUSH_ON_EOF is the marker the child writes iff its EOF - // quiesce was allowed to finish. - env: { MOCK_HANG: '1', MOCK_TEXT: 'x', MOCK_READY_FILE: ready, MOCK_FLUSH_ON_EOF: flushed, TSX_TSCONFIG_PATH: repoTsconfig }, + // child. The flush beat (400ms) outlasts the 50ms SIGTERM grace but fits + // the 2000ms EOF grace; the marker lands iff the EOF tier honored its own + // wider grace. + env: { + MOCK_HANG: '1', MOCK_TEXT: 'x', MOCK_READY_FILE: ready, + MOCK_FLUSH_ON_EOF: flushed, MOCK_FLUSH_DELAY_MS: '400', TSX_TSCONFIG_PATH: repoTsconfig, + }, + disposeEofGraceMs: 2000, + disposeGraceMs: 50, } const run = startAcpRun({ prompt: [{ type: 'text', text: 'p' }], parent: fakeParent }, spec) // Wait until the child is fully booted with its prompt in flight (its ACP // stdin reader is attached), so dispose's stdin EOF reaches a live child. await waitForFile(ready) await run.dispose() - // dispose returned via the natural-exit tier — the EOF-driven flush landed. + // dispose returned via the natural-exit tier — the EOF-driven flush landed + // despite taking longer than the SIGTERM grace. expect(existsSync(flushed)).toBe(true) } finally { rmSync(tmp, { recursive: true, force: true }) @@ -253,27 +267,38 @@ describe('dsh-subagent-acp', () => { it('escalates to SIGTERM for a child that ignores EOF but is not SIGTERM-trapping', async () => { // A child that keeps its loop alive past stdin EOF (so the graceful window - // times out) but leaves SIGTERM at the default handler must die on the - // SIGTERM tier — dispose returns there, never reaching the SIGKILL tier. + // times out) but exits cooperatively on SIGTERM must die on the SIGTERM tier + // — dispose returns there, never reaching the SIGKILL tier. The child touches + // a SIGTERM marker from its signal handler: SIGKILL is uncatchable, so if + // dispose had skipped the middle rung (EOF→SIGKILL) the handler would never + // run and the marker would be absent — making this a GENUINE middle-tier guard. const tmp = mkdtempSync(join(tmpdir(), 'acp-ignore-eof-')) const ready = join(tmp, 'ready') + const sigterm = join(tmp, 'sigterm') try { const spec: AcpRunSpec = { command: process.execPath, args: ['--import', tsxLoader, mockServer], cwd: process.cwd(), permission: 'reject', - env: { MOCK_HANG: '1', MOCK_IGNORE_EOF: '1', MOCK_TEXT: 'x', MOCK_READY_FILE: ready, TSX_TSCONFIG_PATH: repoTsconfig }, - disposeGraceMs: 150, + env: { + MOCK_HANG: '1', MOCK_IGNORE_EOF: '1', MOCK_TEXT: 'x', + MOCK_READY_FILE: ready, MOCK_SIGTERM_FILE: sigterm, TSX_TSCONFIG_PATH: repoTsconfig, + }, + // Tiny EOF grace so the ignored-EOF window elapses fast, then SIGTERM. + disposeEofGraceMs: 150, + disposeGraceMs: 2000, } const run = startAcpRun({ prompt: [{ type: 'text', text: 'p' }], parent: fakeParent }, spec) await waitForFile(ready) - // Bound it: a regression (no SIGTERM tier, only EOF + SIGKILL) would still - // pass, but a hang would fail loud rather than stall the suite. + // Bound it so a hang fails loud rather than stalling the suite. await expect(Promise.race([ run.dispose(), - new Promise((_r, reject) => { setTimeout(() => { reject(new Error('dispose did not return')) }, 4000) }), + new Promise((_r, reject) => { setTimeout(() => { reject(new Error('dispose did not return')) }, 5000) }), ])).resolves.toBeUndefined() + // The child caught SIGTERM and exited — proof the middle rung fired (not a + // jump straight to the uncatchable SIGKILL). + expect(existsSync(sigterm)).toBe(true) } finally { rmSync(tmp, { recursive: true, force: true }) }