Widen the dispose EOF grace past nested-teardown headroom; prove the SIGTERM rung (Codex review round 3)

Two round-3 findings:

(A) The EOF-quiesce window reused the 3000ms SIGTERM grace, the SAME value as
dsh-bash-local's own SIGTERM->SIGKILL grace. The child acp-agent's EOF teardown
disposes its loop, which stops child-owned bash -- and a SIGTERM-trapping bash
grandchild can hold that for up to ~3s before its own SIGKILL, then the child
still owes a final flush. With both graces equal, the parent's SIGTERM fired
exactly as the child reached its own SIGKILL+flush, cutting it off. Split the
EOF grace into its own knob (disposeEofGraceMs, default 6000ms) that exceeds a
single signal-grace of nested-teardown headroom. The child is an arbitrary ACP
agent, so the value is a standalone generous default, NOT derived from any
child's internals. Tier-1 test now uses a flush that outlasts the SIGTERM grace
but fits the EOF grace, so it lands only because the EOF tier honors its own
wider window (proven RED when tier 1 reuses the small SIGTERM grace).

(B) The middle-tier (SIGTERM) test only asserted dispose returned in time, so
an EOF->SIGKILL ladder with the rung removed would still pass. The mock's
MOCK_IGNORE_EOF mode now installs a SIGTERM handler that touches an observable
marker before exiting; SIGKILL is uncatchable, so removing the SIGTERM rung
leaves the marker absent (proven RED). The test asserts the marker exists.

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<string, string>
+  /**
+   * 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).

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')
 }
 

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 })
     }