diff --git a/src/subagent/run-shell-child-reap.test.ts b/src/subagent/run-shell-child-reap.test.ts index 436aff9c4..883854a6d 100644 --- a/src/subagent/run-shell-child-reap.test.ts +++ b/src/subagent/run-shell-child-reap.test.ts @@ -353,3 +353,54 @@ describe("CL-7990 shell-child reap: sessions holding a live shell child settle", { timeout: 60_000 }, ); }); + +/** + * CL-7997 kill proof: the tests above prove the run settles under a wedged + * child, but none proves a shell-guard-tracked child is actually KILLED on + * close/dispose. This drives a real `sleep` through `runGuardedShell` (the + * shell-guard tracking primitive) and the exact `reapLiveChildren` call the + * plugin dispose runs, then asserts on the ChildProcess handle itself that + * the process is dead — not just that the run settled. + */ +describe("CL-7997 shell-guard kill proof: dispose leaves the tracked child dead", () => { + test( + "reapLiveChildren kills a shell-guard-tracked sleep child", + async () => { + if (process.platform === "win32") return; + const { runGuardedShell, reapLiveChildren } = + await import("../plugins/shell-guard-plugin.js"); + const liveChildren = new Set(); + const controller = new AbortController(); + const running = runGuardedShell( + { command: "sleep 60" }, + controller.signal, + liveChildren, + ); + let child: ChildProcess | undefined; + try { + const started = Date.now(); + while (liveChildren.size === 0 && Date.now() - started < 5_000) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } + expect(liveChildren.size).toBe(1); + child = defined([...liveChildren][0]); + // Live before the reap, so the death assertion below is not vacuous. + expect(child.exitCode).toBeNull(); + expect(child.signalCode).toBeNull(); + await reapLiveChildren(liveChildren); + await waitForChildExit(child); + await running; + } finally { + controller.abort(); + if (child !== undefined) { + try { + child.kill("SIGKILL"); + } catch { + // Already dead — best-effort orphan guard. + } + } + } + }, + { timeout: 30_000 }, + ); +}); diff --git a/src/tui/correlation-acceptance.test.ts b/src/tui/correlation-acceptance.test.ts index 3dcc7b62b..ede94169d 100644 --- a/src/tui/correlation-acceptance.test.ts +++ b/src/tui/correlation-acceptance.test.ts @@ -74,6 +74,40 @@ describe("createCorrelationAcceptance", () => { expect(settled).toBe(true); }); + test("an approved hold survives abandon so a late tool.start settles the retry waiter", async () => { + const acceptance = createCorrelationAcceptance(); + const first = acceptance.wait("corr-1"); + let firstSettled = false; + void first.then(() => { + firstSettled = true; + }); + acceptance.observe({ + type: "message.correlated", + data: { correlationId: "corr-1", message: approvedMessage("corr-1") }, + }); + // Acceptance-timeout path: drops the waiter without resolving it, but the + // approved hold must survive so the retry waiter below still settles. + acceptance.abandon("corr-1"); + await new Promise((resolve) => setTimeout(resolve, 5)); + expect(firstSettled).toBe(false); + const retry = acceptance.wait("corr-1"); + acceptance.observe({ + type: "tool.start", + data: { call: { id: "call-1" } }, + }); + const settled = await Promise.race([ + retry.then(() => true), + new Promise((resolve) => setTimeout(() => resolve(false), 50)), + ]); + expect(settled).toBe(true); + }); + + test("late settle after abandon is a safe no-op", () => { + const acceptance = createCorrelationAcceptance(); + acceptance.abandon("corr-1"); + acceptance.settle("corr-1"); + }); + test("a rejected correlation settles at message.correlated", async () => { const acceptance = createCorrelationAcceptance(); const pending = acceptance.wait("corr-1"); diff --git a/src/tui/correlation-acceptance.ts b/src/tui/correlation-acceptance.ts index 9d50eeda4..f3dd9b339 100644 --- a/src/tui/correlation-acceptance.ts +++ b/src/tui/correlation-acceptance.ts @@ -81,10 +81,12 @@ export function createCorrelationAcceptance() { /** * Drop a waiter without resolving it. The acceptance deadline path uses * this instead of settle: settling would fulfill the very promise the - * deadline race is trying to reject, so the timeout could never fire. + * deadline race is trying to reject, so the timeout could never fire. The + * approved hold survives: the retry re-awaits acceptance for the same + * correlation id, and a late tool.start must still settle that waiter + * instead of finding an empty hold set and false-timing-out again. */ const abandon = (correlationId: string): void => { - holdUntilToolStart.delete(correlationId); waiters.delete(correlationId); };