diff --git a/docs/IMPLEMENTATION.md b/docs/IMPLEMENTATION.md index afc875ef1..bf4b05af9 100644 --- a/docs/IMPLEMENTATION.md +++ b/docs/IMPLEMENTATION.md @@ -123,7 +123,7 @@ src/ permission-plugin.ts Tiered operator approval (owns catastrophic shell deny) shell/ run-shell-authz.ts Shared run_shell deny policy (gate-enforced) - background-shell.ts Background run_shell registry (start/collect/cancel/disposeAll) + background-shell.ts Background run_shell registry (start/cancel/disposeAll) verify-plugin.ts Write/edit verification (per-path lock) file-mutation-lock.ts Serialize mutations per file for verify lsp-hint-plugin.ts TS/JS LSP setup hint on unavailable server @@ -208,7 +208,7 @@ Listings are list-free, dumps are dump-locked: a bounded `ls`/`tree` prints name #### Background shell mode -`run_shell` accepts `background: true` (shell-guard plugin, after the permission chain — a denied command spawns nothing). The starting call resolves `cwd` as usual and applies a per-call `timeout` only when passed (no 120s default on background), skips the pwd probe, spawns a detached process group via the registry in `src/shell/background-shell.ts`, and returns `{shell_id, status: "running"}` immediately; the retained shell cwd is never mutated by a background run. Limits: 8 running, 8 completed entries (ring; evicted ids collect as not-found — truncated output is spilled to a `tool-output:///bg-shell-` blob named in the completion message). Completion is process-exit, not stdio-close: a grandchild holding the pipe does not delay the completion message. On process exit the host delivers `buildShellBackgroundMessage(exit)` (exit status, timed-out marker, ~2KB output preview, spill URI) through the same continuation channel as compaction — wired in all three loop hosts (TUI, exec, sub-agent). `run_shell` with `stop: ` kills the process group; it is ungated by design (cancel only kills the session's own child), and a finished run needs no poll because its result is delivered on its own. Timeout keeps its meaning: expiry kills the group and reports exit code 124 with `timed_out: true`. The tool watchdog exempts background starts because the process outlives the turn. A worker whose capability filter drops `run_shell` has no background registry wired — background starts fail closed. Parent abort, deadline, and `close_agent` release parked registry waiters and dispose live background groups; `interrupt_agent` only releases waiters so the session (and its children) stay resumable. Toolset dispose calls `disposeAll("session closed")` before the posix teardown, so `/clear`, interrupt, and reload kill every live background process group. +`run_shell` accepts `background: true` (shell-guard plugin, after the permission chain — a denied command spawns nothing). The starting call resolves `cwd` as usual and applies a per-call `timeout` only when passed (no 120s default on background), skips the pwd probe, spawns a detached process group via the registry in `src/shell/background-shell.ts`, and returns `{shell_id, status: "running"}` immediately; the retained shell cwd is never mutated by a background run. Limit: 8 running (a ninth start fails closed; truncated output is spilled to a `tool-output:///bg-shell-` blob named in the completion message). Completion is process-exit, not stdio-close: a grandchild holding the pipe does not delay the completion message. On process exit the host delivers `buildShellBackgroundMessage(exit)` (exit status, timed-out marker, ~2KB output preview, spill URI) through the same continuation channel as compaction — wired in all three loop hosts (TUI, exec, sub-agent). `run_shell` with `stop: ` kills the process group; it is ungated by design (cancel only kills the session's own child), and a finished run needs no poll because its result is delivered on its own. Timeout keeps its meaning: expiry kills the group and reports exit code 124 with `timed_out: true`. The tool watchdog exempts background starts because the process outlives the turn. A worker whose capability filter drops `run_shell` has no background registry wired — background starts fail closed. Parent abort, deadline, and `close_agent` dispose live background groups; `interrupt_agent` leaves them so the session (and its children) stay resumable. Toolset dispose calls `disposeAll("session closed")` before the posix teardown, so `/clear`, interrupt, and reload kill every live background process group. - **Alt+Enter** queues a follow-up (kind `"queue"`) delivered only on **session-idle** — parent-idle **and** no live fleet lanes (`run` goes idle). Session-idle Alt+Enter is a no-op. **Ctrl+C** stops the run. diff --git a/src/plugins/shell-guard-plugin.test.ts b/src/plugins/shell-guard-plugin.test.ts index d0654ceb3..df58509d4 100644 --- a/src/plugins/shell-guard-plugin.test.ts +++ b/src/plugins/shell-guard-plugin.test.ts @@ -10,6 +10,7 @@ import { spawnSync, type ChildProcess } from "node:child_process"; import { randomUUID } from "node:crypto"; import { createBackgroundShellRegistry } from "../shell/background-shell.js"; +import { createExitRecorder } from "../shell/background-shell-test-harness.js"; import { createShellOutputFeed } from "../session/shell-output-feed.js"; import { @@ -267,7 +268,10 @@ describe("background run_shell (shellGuardPlugin)", () => { } test("background:true returns a running handle immediately", async () => { - const registry = createBackgroundShellRegistry(); + const recorder = createExitRecorder(); + const registry = createBackgroundShellRegistry({ + onExit: recorder.onExit, + }); const start = Date.now(); const result = await runWith(registry, { id: "bg1", @@ -283,8 +287,7 @@ describe("background run_shell (shellGuardPlugin)", () => { expect(parsed.shell_id.length).toBeGreaterThan(0); // The START call resolved while the child was still sleeping. expect(Date.now() - start).toBeLessThan(400); - const snapshot = await registry.collect(parsed.shell_id, 5_000); - expect(snapshot.state).toBe("completed"); + await recorder.waitFor(parsed.shell_id); registry.disposeAll("test done"); }); @@ -334,7 +337,10 @@ describe("background run_shell (shellGuardPlugin)", () => { }); test("background omitted timeout skips the foreground default", async () => { - const registry = createBackgroundShellRegistry(); + const recorder = createExitRecorder(); + const registry = createBackgroundShellRegistry({ + onExit: recorder.onExit, + }); const handler = defined( shellGuardPlugin(process.cwd(), { defaultMs: 90 }, undefined, { getBackgroundShellRegistry: () => registry, @@ -353,12 +359,9 @@ describe("background run_shell (shellGuardPlugin)", () => { ); expect(result.isError).toBeUndefined(); const parsed = JSON.parse(String(result.content)) as { shell_id: string }; - const snapshot = await registry.collect(parsed.shell_id, 5_000); - expect(snapshot.state).toBe("completed"); - if (snapshot.state === "completed") { - expect(snapshot.exit.timedOut).toBe(false); - expect(snapshot.exit.output).toContain("survived"); - } + const exit = await recorder.waitFor(parsed.shell_id); + expect(exit.timedOut).toBe(false); + expect(exit.output).toContain("survived"); registry.disposeAll("test done"); }); diff --git a/src/shell/background-shell-test-harness.ts b/src/shell/background-shell-test-harness.ts new file mode 100644 index 000000000..b5b2d4669 --- /dev/null +++ b/src/shell/background-shell-test-harness.ts @@ -0,0 +1,25 @@ +import type { BackgroundShellExit } from "./background-shell.js"; + +/** Collects `onExit` deliveries so tests can await one shell's completion. */ +export function createExitRecorder(): { + onExit: (exit: BackgroundShellExit) => void; + waitFor: (id: string, timeoutMs?: number) => Promise; +} { + const exits = new Map(); + return { + onExit: (exit) => { + exits.set(exit.id, exit); + }, + waitFor: async (id, timeoutMs = 5_000) => { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + const exit = exits.get(id); + if (exit !== undefined) return exit; + await new Promise((resolve) => setTimeout(resolve, 20)); + } + throw new Error( + `no exit delivered for shell ${id} within ${timeoutMs}ms`, + ); + }, + }; +} diff --git a/src/shell/background-shell.test.ts b/src/shell/background-shell.test.ts index f201dcf15..2ead4276a 100644 --- a/src/shell/background-shell.test.ts +++ b/src/shell/background-shell.test.ts @@ -1,12 +1,11 @@ -import { defined } from "../../testkit/defined.js"; import { describe, expect, test } from "bun:test"; import { spawnSync } from "node:child_process"; import { randomUUID } from "node:crypto"; import { - MAX_COMPLETED_BACKGROUND_SHELLS, MAX_RUNNING_BACKGROUND_SHELLS, createBackgroundShellRegistry, } from "./background-shell.js"; +import { createExitRecorder } from "./background-shell-test-harness.js"; const tmpCwd = process.cwd(); @@ -22,66 +21,55 @@ async function waitUntilGone(token: string): Promise { } describe("background shell registry", () => { - test("start returns a handle immediately while the process runs", async () => { - const registry = createBackgroundShellRegistry(); + test("start returns a handle while the process runs, then delivers the exit", async () => { + const recorder = createExitRecorder(); + const registry = createBackgroundShellRegistry({ + onExit: recorder.onExit, + }); const started = registry.start({ command: "sleep 0.1; echo done", cwd: tmpCwd, }); if ("error" in started) throw new Error(started.error); - const snapshot = await registry.collect(started.id, 0); - expect(snapshot.state).toBe("running"); - const exited = await registry.collect(started.id, 5_000); - expect(exited.state).toBe("completed"); - if (exited.state !== "completed") return; - expect(exited.exit.exitCode).toBe(0); - expect(exited.exit.output).toContain("done"); - expect(exited.exit.timedOut).toBe(false); + expect(registry.runningCount()).toBe(1); + const exit = await recorder.waitFor(started.id); + expect(exit.exitCode).toBe(0); + expect(exit.output).toContain("done"); + expect(exit.timedOut).toBe(false); + expect(registry.runningCount()).toBe(0); }); - test("onExit fires with exit status and output", async () => { - const exits: unknown[] = []; + test("timeout kills the group and reports exit 124 + timedOut", async () => { + const recorder = createExitRecorder(); const registry = createBackgroundShellRegistry({ - onExit: (exit) => exits.push(exit), + onExit: recorder.onExit, }); - const started = registry.start({ command: "echo hi", cwd: tmpCwd }); - if ("error" in started) throw new Error(started.error); - await registry.collect(started.id, 5_000); - const deadline = Date.now() + 5_000; - while (exits.length === 0 && Date.now() < deadline) { - await new Promise((r) => setTimeout(r, 20)); - } - expect(exits).toHaveLength(1); - }); - - test("timeout kills the group and reports exit 124 + timedOut", async () => { - const registry = createBackgroundShellRegistry(); const started = registry.start({ command: "echo early; sleep 60", cwd: tmpCwd, timeoutMs: 150, }); if ("error" in started) throw new Error(started.error); - const exited = await registry.collect(started.id, 5_000); - expect(exited.state).toBe("completed"); - if (exited.state !== "completed") return; - expect(exited.exit.timedOut).toBe(true); - expect(exited.exit.exitCode).toBe(124); - expect(exited.exit.output).toContain("early"); + const exit = await recorder.waitFor(started.id); + expect(exit.timedOut).toBe(true); + expect(exit.exitCode).toBe(124); + expect(exit.output).toContain("early"); }); test("cancel kills the whole process group", async () => { if (process.platform === "win32") return; const token = `ic_bg_cancel_${randomUUID()}`; - const registry = createBackgroundShellRegistry(); + const recorder = createExitRecorder(); + const registry = createBackgroundShellRegistry({ + onExit: recorder.onExit, + }); const started = registry.start({ command: `bash -c 'TAG=${token} sleep 600 & TAG=${token} exec sleep 600'`, cwd: tmpCwd, }); if ("error" in started) throw new Error(started.error); expect(registry.cancel(started.id)).toBe(true); - const exited = await registry.collect(started.id, 5_000); - expect(exited.state).toBe("completed"); + await recorder.waitFor(started.id); await waitUntilGone(token); }); @@ -90,22 +78,6 @@ describe("background shell registry", () => { expect(registry.cancel("nope")).toBe(false); }); - test("completed ring evicts the oldest entry (collect reports not-found)", async () => { - const registry = createBackgroundShellRegistry(); - const ids: string[] = []; - for (let i = 0; i <= MAX_COMPLETED_BACKGROUND_SHELLS; i++) { - const started = registry.start({ command: "true", cwd: tmpCwd }); - if ("error" in started) throw new Error(started.error); - ids.push(started.id); - await registry.collect(started.id, 5_000); - } - expect(ids).toHaveLength(MAX_COMPLETED_BACKGROUND_SHELLS + 1); - const evicted = await registry.collect(defined(ids[0]), 0); - expect(evicted.state).toBe("not-found"); - const retained = await registry.collect(defined(ids[ids.length - 1]), 0); - expect(retained.state).toBe("completed"); - }); - test("running cap fails closed with an error instead of spawning", async () => { const registry = createBackgroundShellRegistry(); for (let i = 0; i < MAX_RUNNING_BACKGROUND_SHELLS; i++) { @@ -117,9 +89,12 @@ describe("background shell registry", () => { registry.disposeAll("test done"); }); - test("disposeAll kills running children", async () => { + test("disposeAll kills running children without delivering an exit", async () => { const token = `ic_bg_dispose_${randomUUID()}`; - const registry = createBackgroundShellRegistry(); + const exits: unknown[] = []; + const registry = createBackgroundShellRegistry({ + onExit: (exit) => exits.push(exit), + }); const started = registry.start({ command: `sleep 600 # ${token}`, cwd: tmpCwd, @@ -127,141 +102,19 @@ describe("background shell registry", () => { if ("error" in started) throw new Error(started.error); registry.disposeAll("session closed"); await waitUntilGone(token); - const after = await registry.collect(started.id, 0); - expect(after.state).toBe("not-found"); + expect(registry.runningCount()).toBe(0); + expect(exits).toHaveLength(0); }); - test("collect with an aborted signal releases as running without killing the child", async () => { - const token = `ic_bg_abort_${randomUUID()}`; - const registry = createBackgroundShellRegistry(); - const started = registry.start({ - command: `bash -c 'exec -a ${token} sleep 600'`, - cwd: tmpCwd, - }); - if ("error" in started) throw new Error(started.error); - try { - const aborted = new AbortController(); - aborted.abort(new Error("interrupted by interrupt_agent")); - const snapshot = await registry.collect( - started.id, - 60_000, - aborted.signal, - ); - expect(snapshot.state).toBe("running"); - const stillThere = await registry.collect(started.id, 0); - expect(stillThere.state).toBe("running"); - const probe = spawnSync("pgrep", ["-f", token], { encoding: "utf8" }); - expect(probe.status).toBe(0); - } finally { - registry.disposeAll("test done"); - } - }); - - test("releaseWaiters wakes a parked collect without killing the child", async () => { - const token = `ic_bg_release_${randomUUID()}`; - const registry = createBackgroundShellRegistry(); - const started = registry.start({ - command: `bash -c 'exec -a ${token} sleep 600'`, - cwd: tmpCwd, - }); - if ("error" in started) throw new Error(started.error); - try { - const pending = registry.collect(started.id, 60_000); - await new Promise((r) => setTimeout(r, 100)); - registry.releaseWaiters(); - const snapshot = await pending; - expect(snapshot.state).toBe("running"); - const stillThere = await registry.collect(started.id, 0); - expect(stillThere.state).toBe("running"); - const probe = spawnSync("pgrep", ["-f", token], { encoding: "utf8" }); - expect(probe.status).toBe(0); - } finally { - registry.disposeAll("test done"); - } - }); - - test("two concurrent collects on the same shell both resolve", async () => { - const registry = createBackgroundShellRegistry(); - const started = registry.start({ - command: "sleep 1; echo done", - cwd: tmpCwd, - }); - if ("error" in started) throw new Error(started.error); - try { - const [first, second] = await Promise.all([ - registry.collect(started.id, 5_000), - registry.collect(started.id, 5_000), - ]); - expect(first.state).toBe("completed"); - expect(second.state).toBe("completed"); - if (first.state !== "completed" || second.state !== "completed") return; - expect(first.exit.output).toContain("done"); - expect(second.exit.output).toContain("done"); - } finally { - registry.disposeAll("test done"); - } - }); - - test("one-sided release (timeout or abort) does not starve the other waiter", async () => { - const impatientCollect = ( - registry: ReturnType, - id: string, - mode: "timeout" | "abort", - ) => { - if (mode === "timeout") return registry.collect(id, 100); - const aborted = new AbortController(); - setTimeout(() => aborted.abort(new Error("stop waiting")), 100); - return registry.collect(id, 5_000, aborted.signal); - }; - - for (const mode of ["timeout", "abort"] as const) { - const registry = createBackgroundShellRegistry(); - const started = registry.start({ - command: "sleep 1; echo done", - cwd: tmpCwd, - }); - if ("error" in started) throw new Error(started.error); - try { - const [impatient, patient] = await Promise.all([ - impatientCollect(registry, started.id, mode), - registry.collect(started.id, 5_000), - ]); - expect(impatient.state).toBe("running"); - expect(patient.state).toBe("completed"); - if (patient.state !== "completed") return; - expect(patient.exit.output).toContain("done"); - } finally { - registry.disposeAll("test done"); - } - } - }); - - test("collect wait is capped so a huge or non-finite wait_ms cannot park unbounded", async () => { - for (const waitMs of [30_000, Number.POSITIVE_INFINITY]) { - const registry = createBackgroundShellRegistry({ maxCollectWaitMs: 80 }); - const started = registry.start({ - command: "sleep 30", - cwd: tmpCwd, - }); - if ("error" in started) throw new Error(started.error); - try { - const t0 = Date.now(); - const snapshot = await registry.collect(started.id, waitMs); - const elapsed = Date.now() - t0; - expect(snapshot.state).toBe("running"); - expect(elapsed).toBeLessThan(2_000); - } finally { - registry.disposeAll("test done"); - } - } - }); - - test("collect completes when the child exits even if stdio stays open", async () => { + test("the exit is delivered when the child exits even if stdio stays open", async () => { if (process.platform === "win32") return; const token = `ic_bg_stdio_hold_${randomUUID()}`; - const registry = createBackgroundShellRegistry(); + const recorder = createExitRecorder(); + const registry = createBackgroundShellRegistry({ + onExit: recorder.onExit, + }); // Grandchild inherits the piped stdout so Node's 'close' waits on it. - // The shell itself exits immediately; collect must not require close. + // The shell itself exits immediately; delivery must not require close. const started = registry.start({ command: `bash -c 'exec -a ${token} sleep 600 & exit 0'`, cwd: tmpCwd, @@ -269,11 +122,9 @@ describe("background shell registry", () => { if ("error" in started) throw new Error(started.error); try { const t0 = Date.now(); - const exited = await registry.collect(started.id, 5_000); + const exit = await recorder.waitFor(started.id); expect(Date.now() - t0).toBeLessThan(2_000); - expect(exited.state).toBe("completed"); - if (exited.state !== "completed") return; - expect(exited.exit.exitCode).toBe(0); + expect(exit.exitCode).toBe(0); } finally { registry.disposeAll("test done"); spawnSync("pkill", ["-f", token]); diff --git a/src/shell/background-shell.ts b/src/shell/background-shell.ts index b8bfe4c92..5a0d97b57 100644 --- a/src/shell/background-shell.ts +++ b/src/shell/background-shell.ts @@ -13,10 +13,8 @@ import { // cancels by id. export const MAX_RUNNING_BACKGROUND_SHELLS = 8; -export const MAX_COMPLETED_BACKGROUND_SHELLS = 8; -export const MAX_SHELL_COLLECT_WAIT_MS = 300_000; // Prefer close so trailing stdio is captured, but never require it: a grandchild -// holding the pipe must not park collect after the child has already exited. +// holding the pipe must not delay the completion message after the child exits. const STDIO_DRAIN_MS = 100; /** @@ -61,44 +59,18 @@ export interface BackgroundShellExit { spillUri?: string; } -export type BackgroundShellSnapshot = - | { state: "running" } - | { state: "completed"; exit: BackgroundShellExit } - | { state: "not-found" }; - export interface BackgroundShellRegistry { start: (args: StartBackgroundShellArgs) => { id: string } | { error: string }; - collect: ( - id: string, - waitMs?: number, - signal?: AbortSignal, - ) => Promise; cancel: (id: string) => boolean; disposeAll: (reason: string) => void; - releaseWaiters: () => void; runningCount: () => number; } -function resolveCollectWaitMs(waitMs: number, maxMs: number): number { - if (typeof waitMs !== "number" || Number.isNaN(waitMs) || waitMs <= 0) { - return 0; - } - if (!Number.isFinite(waitMs)) return maxMs; - return Math.min(waitMs, maxMs); -} - export function createBackgroundShellRegistry( - options: { - onExit?: (exit: BackgroundShellExit) => void; - maxCollectWaitMs?: number; - } = {}, + options: { onExit?: (exit: BackgroundShellExit) => void } = {}, ): BackgroundShellRegistry { const { onExit } = options; - const maxCollectWaitMs = - options.maxCollectWaitMs ?? MAX_SHELL_COLLECT_WAIT_MS; const running = new Map(); - const completed = new Map(); - const exitWaiters = new Map void>>(); const record = (id: string, command: string, collector: BoundedShellOutput) => @@ -113,19 +85,6 @@ export function createBackgroundShellRegistry( output, outputTruncated: truncated, }; - completed.set(id, exit); - // Ring eviction drops the oldest completed entry; the completion message - // already carried a spill URI for truncated output, so nothing is lost. - while (completed.size > MAX_COMPLETED_BACKGROUND_SHELLS) { - const oldest = completed.keys().next().value; - if (oldest === undefined) break; - completed.delete(oldest); - } - const waiters = exitWaiters.get(id); - if (waiters !== undefined) { - exitWaiters.delete(id); - for (const wake of waiters) wake(); - } onExit?.(exit); }; @@ -134,7 +93,7 @@ export function createBackgroundShellRegistry( ): { id: string } | { error: string } => { if (running.size >= MAX_RUNNING_BACKGROUND_SHELLS) { return { - error: `background shell limit reached (${MAX_RUNNING_BACKGROUND_SHELLS} running); collect or cancel one first`, + error: `background shell limit reached (${MAX_RUNNING_BACKGROUND_SHELLS} running); stop one first`, }; } const id = randomUUID(); @@ -184,64 +143,6 @@ export function createBackgroundShellRegistry( return { id }; }; - const collect = async ( - id: string, - waitMs = 0, - signal?: AbortSignal, - ): Promise => { - const done = completed.get(id); - if (done !== undefined) return { state: "completed", exit: done }; - if (!running.has(id)) return { state: "not-found" }; - const budget = resolveCollectWaitMs(waitMs, maxCollectWaitMs); - if (budget > 0) { - // An already-aborted collect releases immediately as still-running: - // interrupt must not park the session on a live descendant, and must - // not kill it either — the child belongs to the still-alive session. - if (signal?.aborted === true) return { state: "running" }; - // Concurrent collects on the same shell each park their own waiter so - // they resolve independently: a waiter removes only itself on - // timeout/abort/settle, never a sibling's registration. - let wake: (() => void) | undefined; - const forget = (): void => { - const waiters = exitWaiters.get(id); - if (wake !== undefined) waiters?.delete(wake); - if (waiters !== undefined && waiters.size === 0) exitWaiters.delete(id); - }; - await new Promise((resolve) => { - const timer = setTimeout(() => { - forget(); - signal?.removeEventListener("abort", onAbort); - resolve(); - }, budget); - const onAbort = (): void => { - clearTimeout(timer); - forget(); - resolve(); - }; - wake = (): void => { - clearTimeout(timer); - signal?.removeEventListener("abort", onAbort); - resolve(); - }; - signal?.addEventListener("abort", onAbort, { once: true }); - let waiters = exitWaiters.get(id); - if (waiters === undefined) { - waiters = new Set(); - exitWaiters.set(id, waiters); - } - waiters.add(wake); - }).finally(() => { - // Belt-and-braces: record() already dropped the whole entry when it - // woke us, and timeout/abort self-removed above — this only trims a - // waiter whose settle path raced out. - forget(); - }); - const finished = completed.get(id); - if (finished !== undefined) return { state: "completed", exit: finished }; - } - return { state: "running" }; - }; - const cancel = (id: string): boolean => { const child = running.get(id); if (child === undefined) return false; @@ -249,24 +150,9 @@ export function createBackgroundShellRegistry( return true; }; - /** - * Wake every parked `collect` waiter as still-running without touching the - * children. Interrupt paths call this so a live descendant releases the - * session instead of wedging teardown; close paths use `disposeAll`, which - * wakes waiters and then kills the trees. - */ - const releaseWaiters = (): void => { - for (const waiters of exitWaiters.values()) { - for (const wake of waiters) wake(); - } - exitWaiters.clear(); - }; - const disposeAll = (reason: string): void => { for (const child of running.values()) killProcessTree(child); running.clear(); - completed.clear(); - releaseWaiters(); // onExit is intentionally not fired for disposed shells: the session is // gone, so there is no later turn to deliver to (`reason` is for callers // that log it). @@ -275,10 +161,8 @@ export function createBackgroundShellRegistry( return { start, - collect, cancel, disposeAll, - releaseWaiters, runningCount: () => running.size, }; } diff --git a/src/subagent/run-persist-close.test.ts b/src/subagent/run-persist-close.test.ts index c2a2418cc..2b69b7f65 100644 --- a/src/subagent/run-persist-close.test.ts +++ b/src/subagent/run-persist-close.test.ts @@ -158,7 +158,6 @@ describe("worker persist reaps leftover registry children", () => { const token = `ic_worker_persist_${randomUUID()}`; let registry: BackgroundShellRegistry | undefined; const disposeReasons: string[] = []; - let leftoverId: string | undefined; await withMockedModuleDuring( import.meta.resolve("../shell/background-shell.js"), @@ -193,7 +192,6 @@ describe("worker persist reaps leftover registry children", () => { cwd, }); if ("error" in started) throw new Error(started.error); - leftoverId = started.id; expect(captured.runningCount()).toBe(1); if (process.platform !== "win32") { await waitUntilPresent(token); @@ -226,8 +224,6 @@ describe("worker persist reaps leftover registry children", () => { expect(disposeReasons).toEqual(["sub-agent closed"]); const captured = defined(registry); expect(captured.runningCount()).toBe(0); - const leftover = await captured.collect(defined(leftoverId), 0); - expect(leftover.state).toBe("not-found"); if (process.platform !== "win32") { await waitUntilGone(token); } diff --git a/src/subagent/run-shell-child-reap.test.ts b/src/subagent/run-shell-child-reap.test.ts index 741dd2eb5..b89beb15c 100644 --- a/src/subagent/run-shell-child-reap.test.ts +++ b/src/subagent/run-shell-child-reap.test.ts @@ -5,14 +5,12 @@ * awaitBoundedTeardown): posixTools.dispose() runs the shell-guard * reapLiveChildren (SIGKILL the child process groups, 2s reap wait), then * agent.close() and the session-stream drain are awaited — voided, not hung - * on, when the reap reports leftovers. Interrupt additionally releases - * parked registry waiters so a worker blocked in shell output - * collection comes back as still-running instead of wedging the run. + * on, when the reap reports leftovers. * * These tests drive the real `runSubAgent` with a stub agent whose `send` * holds a REAL live `sleep` child (killed on abort, like runGuardedShell's * onAbort) and whose `stream()` stays open until the session closes — the - * production shape of a worker parked in shell collection. + * production shape of a worker parked in a shell call. */ import { describe, expect, test } from "bun:test"; import { spawn, type ChildProcess } from "node:child_process"; @@ -53,7 +51,7 @@ async function waitForChildExit( /** * Stub agent modeling a worker parked behind a live shell child: `send` - * spawns a real `sleep` descendant and pends (like shell collection output); + * spawns a real `sleep` descendant and pends (like a pending shell call); * the abort listener kills the child first, mirroring runGuardedShell's * onAbort. `stream()` stays open until `releaseStream` — the session cannot * drain while the descendant is wedged. `leakOnAbort` models a stub that diff --git a/src/subagent/run.ts b/src/subagent/run.ts index ff30fb7af..6f4f72050 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -1506,10 +1506,8 @@ async function runSubAgentInner( // Aborting the send signal only rejects the promise; the child reactor keeps // running until close() (same hard-stop rule as the parent in runner.ts). closeOnAbort = (): void => { - // Parent abort / deadline / close_agent: unpark any registry waiters so - // teardown cannot wedge on one. interrupt_agent is - // the keep-alive path (releaseWaiters only, children stay). - backgroundShells.releaseWaiters(); + // interrupt_agent is the keep-alive path: it never reaches here, so its + // background children stay. backgroundShells.disposeAll("parent abort"); void (async () => { try { @@ -1560,11 +1558,9 @@ async function runSubAgentInner( } }; // Interrupt only fires interruptController — never runController/ - // close, so it cannot hang teardown on a wedged agent.close. Release - // parked registry waiters too: the interrupt settles the turn while the - // session (and its live shell children) stays alive. + // close, so it cannot hang teardown on a wedged agent.close. The session + // (and its live shell children) stays alive. const interrupt = (): void => { - backgroundShells.releaseWaiters(); if (!interruptController.signal.aborted) { interruptController.abort( new Error("interrupted by interrupt_agent"),