From 4565161c64b86a7db497106a76f0d5dd81e2e119 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Mon, 22 Jun 2026 12:11:17 +0800 Subject: [PATCH] Give the ACP child an EOF window to quiesce before SIGTERM (Codex review round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dispose() ended stdin and sent SIGTERM in the same tick, so the child's EOF-driven quiesce had no window to run. The real acp-agent has no SIGTERM handler in a normal session — it flushes persistence and stops child-owned work via the server bridge's connection-close path (conn.closed → per-agent dispose → final session/flush), driven by 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 owes durable work when dispose runs; a same-tick default SIGTERM terminated it mid-flush, orphaning child-owned bash and dropping the flush. dispose now waits for the child's natural exit after stdin EOF first, then escalates SIGTERM (grace), then SIGKILL — a three-tier ladder. Add an `exitsWithin` helper for the bounded waits. Regression coverage: a new mock mode (MOCK_FLUSH_ON_EOF) flushes a marker asynchronously on EOF then self-exits; the tier-1 test asserts the marker lands (proven RED on the same-tick-SIGTERM ordering — child killed mid-flush). MOCK_IGNORE_EOF covers the middle tier (ignores EOF, dies on default SIGTERM); the existing MOCK_TRAP_SIGTERM test covers the SIGKILL tier. --- packages/subagent/subagent-acp/src/run.ts | 47 ++++++++------ .../subagent-acp/tests/mock-acp-server.ts | 38 ++++++++++++ .../subagent-acp/tests/subagent-acp.spec.ts | 61 +++++++++++++++++++ 3 files changed, 127 insertions(+), 19 deletions(-) diff --git a/packages/subagent/subagent-acp/src/run.ts b/packages/subagent/subagent-acp/src/run.ts index 612fe87074..c78e8ae08d 100644 --- a/packages/subagent/subagent-acp/src/run.ts +++ b/packages/subagent/subagent-acp/src/run.ts @@ -149,6 +149,15 @@ function waitForExit(child: ChildProcess): Promise { return new Promise(resolve => child.once('exit', () => { resolve() })) } +/** Resolve `true` if the child exits within `ms`, `false` on timeout. */ +function exitsWithin(child: ChildProcess, ms: number): Promise { + return Promise.race([ + waitForExit(child).then(() => true), + // `.unref()` so a pending grace timer never keeps the parent's loop alive. + new Promise(resolve => setTimeout(() => { resolve(false) }, ms).unref()), + ]) +} + /** * Start an out-of-process ACP child for `request` and return a {@link SubagentRun}. * @@ -304,26 +313,26 @@ 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 - // 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) - } + // 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 + // session — it tears down via the server bridge's connection-close path + // (conn.closed → per-agent dispose → final session/flush), driven by the + // 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. + child.stdin.end() + if (await exitsWithin(child, graceMs)) 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). + child.kill('SIGTERM') + if (await exitsWithin(child, graceMs)) return + // 3. Force-kill and await the (now-certain) exit. + child.kill('SIGKILL') + 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 8260051f63..b30492258a 100644 --- a/packages/subagent/subagent-acp/tests/mock-acp-server.ts +++ b/packages/subagent/subagent-acp/tests/mock-acp-server.ts @@ -14,6 +14,18 @@ * handler is in flight (it has streamed its chunk). A test * 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_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. * * 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 @@ -50,6 +62,7 @@ const NO_ALLOW = process.env.MOCK_NO_ALLOW === '1' const THOUGHT = process.env.MOCK_THOUGHT === '1' const CRASH_ON_CANCEL = process.env.MOCK_CRASH_ON_CANCEL === '1' const READY_FILE = process.env.MOCK_READY_FILE +const FLUSH_ON_EOF = process.env.MOCK_FLUSH_ON_EOF // When MOCK_NEWSESSION_READY/GO are set, newSession touches READY then blocks // until GO appears — letting a test cancel mid-newSession deterministically. const NEWSESSION_GATE = process.env.MOCK_NEWSESSION_READY !== undefined && process.env.MOCK_NEWSESSION_GO !== undefined @@ -162,3 +175,28 @@ if (process.env.MOCK_TRAP_SIGTERM === '1') { if (READY_FILE !== undefined) writeFileSync(READY_FILE, 'trap-armed') } +// 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. +if (FLUSH_ON_EOF !== undefined) { + process.stdin.on('end', () => { + setTimeout(() => { + writeFileSync(FLUSH_ON_EOF, 'flushed') + process.exit(0) + }, 150) + }) +} + +// 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. +if (process.env.MOCK_IGNORE_EOF === '1') { + setInterval(() => { /* stay alive past EOF; default SIGTERM still kills us */ }, 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 0e4604e638..96b749218d 100644 --- a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts +++ b/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts @@ -218,6 +218,67 @@ describe('dsh-subagent-acp', () => { } }) + it('dispose gives the child an EOF window to quiesce before escalating (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. + const tmp = mkdtempSync(join(tmpdir(), 'acp-eof-')) + const ready = join(tmp, 'ready') + const flushed = join(tmp, 'flushed') + try { + const spec: AcpRunSpec = { + command: process.execPath, + args: ['--import', tsxLoader, mockServer], + 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 }, + } + 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. + expect(existsSync(flushed)).toBe(true) + } finally { + rmSync(tmp, { recursive: true, force: true }) + } + }) + + 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. + const tmp = mkdtempSync(join(tmpdir(), 'acp-ignore-eof-')) + const ready = join(tmp, 'ready') + 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, + } + 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. + await expect(Promise.race([ + run.dispose(), + new Promise((_r, reject) => { setTimeout(() => { reject(new Error('dispose did not return')) }, 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 () => { // Gate the child at newSession: it signals `ready` and blocks until `go`. // We cancel WHILE newSession is pending (sessionId still undefined, so the