Give the ACP child an EOF window to quiesce before SIGTERM (Codex review round 2)

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.
2026-06-22 12:11:17 +08:00
parent 6801130f8c
commit 4565161c64
3 changed files with 127 additions and 19 deletions
--- 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<void> {
  return new Promise<void>(resolve => child.once('exit', () => { resolve() }))
 }

+/** Resolve `true` if the child exits within `ms`, `false` on timeout. */
+function exitsWithin(child: ChildProcess, ms: number): Promise<boolean> {
+  return Promise.race([
+    waitForExit(child).then(() => true),
+    // `.unref()` so a pending grace timer never keeps the parent's loop alive.
+    new Promise<boolean>(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<boolean>(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)
    },
  }
 }
--- 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')
+}
+
--- 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