From 512d7d615ac4eefe29c80760b60400d50af9bcbd Mon Sep 17 00:00:00 2001 From: Sparkle <2705263765@qq.com> Date: Thu, 15 Oct 2026 15:06:36 +0800 Subject: [PATCH] fix(background-terminals): redeliver settled results when a multi-id bg_kill is aborted A terminal that settles while a bg_kill holds interest skips the automatic follow-up because that kill is expected to report it. If the kill is then interrupted (Esc while another id waits for SIGKILL), it reports nothing and the settled result was lost. Remember which settlements were suppressed for a kill report and, when an interrupted kill releases the last interest in them, hand them back to the normal delivery path. Co-Authored-By: Claude Opus 4.8 --- .../background-terminals/src/manager.ts | 30 ++++- .../background-terminals/index.test.ts | 111 ++++++++++++++++++ .../background-terminals/manager.test.ts | 107 +++++++++++++++++ 3 files changed, 247 insertions(+), 1 deletion(-) diff --git a/extensions/background-terminals/src/manager.ts b/extensions/background-terminals/src/manager.ts index fa43e83ad..9b88e0f93 100644 --- a/extensions/background-terminals/src/manager.ts +++ b/extensions/background-terminals/src/manager.ts @@ -125,6 +125,9 @@ interface Entry { /** Completed exactly once when the entry settles. Kill callers and the scope * finalizer can all await the same result without missing a notification. */ settled: Deferred.Deferred; + /** Settled while a bg_kill held interest, so the automatic follow-up was + * skipped, and no kill has returned that result yet. */ + awaitingKillReport: boolean; } interface SpillFile { @@ -761,6 +764,7 @@ function* makeManager(maxSpillBytesPerSession: number) { // ensuring blocks release interest. Snapshot consumption first so the // settle hook observes the interest that existed when settlement won. const consumed = (killInterest.get(s.id) ?? 0) > 0; + entry.awaitingKillReport = consumed; Deferred.doneUnsafe(entry.settled, Effect.void); notify(s.id); try { @@ -999,6 +1003,7 @@ function* makeManager(maxSpillBytesPerSession: number) { terminationFailed: false, exitCleanupStarted: false, settled, + awaitingKillReport: false, }; if (options.timeoutSeconds !== undefined) { @@ -1213,6 +1218,7 @@ function* makeManager(maxSpillBytesPerSession: number) { // Mark consumed before signaling so this kill's settlements are not // ALSO queued as automatic follow-up messages to the model. addKillInterest(runningIds); + let reported = false; const work = Effect.gen(function* () { yield* Effect.forEach(running, killEntry, { concurrency: "unbounded", @@ -1227,7 +1233,7 @@ function* makeManager(maxSpillBytesPerSession: number) { ); // Capture the report BEFORE the ensuring below releases interest and // prunes — a just-settled entry must not vanish out from under it. - return unique.map((id): KillResult => { + const report = unique.map((id): KillResult => { const sourceEntry = byId.get(id); const snapshot = sourceEntry?.snapshot; const history = settledHistory.get(id); @@ -1252,11 +1258,33 @@ function* makeManager(maxSpillBytesPerSession: number) { : (history?.exit ?? "unknown"), }; }); + for (const entry of running) entry.awaitingKillReport = false; + reported = true; + return report; }); return work.pipe( Effect.ensuring( Effect.sync(() => { releaseKillInterest(runningIds); + // An interrupted kill (Esc while another id waits for SIGKILL) + // returns nothing. Hand every result it suppressed back to the + // automatic follow-up path, unless another kill still owns it. + if (!reported && !disposed) { + for (const entry of running) { + if ( + !entry.awaitingKillReport || + killInterest.has(entry.snapshot.id) + ) { + continue; + } + entry.awaitingKillReport = false; + try { + onSettled?.(entry.snapshot, false); + } catch { + // The parent session may be unavailable; settlement stays final. + } + } + } pruneSettled(); }), ), diff --git a/tests/extensions/background-terminals/index.test.ts b/tests/extensions/background-terminals/index.test.ts index ce5e77b29..a3009fb58 100644 --- a/tests/extensions/background-terminals/index.test.ts +++ b/tests/extensions/background-terminals/index.test.ts @@ -132,3 +132,114 @@ test("a successful start exposes lifecycle tools but a rejected start does not", } } }); + +test("FAIL: an aborted multi-id bg_kill still delivers the terminal that already settled", { + skip: process.platform === "win32", +}, async () => { + const tools = new Map(); + const handlers = new Map< + string, + Array<(event: unknown, ctx: ExtensionContext) => unknown> + >(); + const sent: Array<{ + content: string; + details: { id?: string; results?: Array<{ id: string }> }; + }> = []; + let active: string[] = []; + const pi = { + on(event: string, handler: unknown) { + handlers.set(event, [ + ...(handlers.get(event) ?? []), + handler as (event: unknown, ctx: ExtensionContext) => unknown, + ]); + }, + registerTool(tool: CapturedTool) { + tools.set(tool.name, tool); + }, + getActiveTools: () => [...active], + setActiveTools(names: string[]) { + active = [...names]; + }, + registerMessageRenderer() {}, + registerCommand() {}, + sendMessage(message: (typeof sent)[number]) { + sent.push(message); + }, + } as unknown as ExtensionAPI; + let idle = false; + const ctx = { + cwd: process.cwd(), + hasUI: false, + isIdle: () => idle, + sessionManager: { getSessionId: () => "bg-kill-abort" }, + } as unknown as ExtensionContext; + const emit = async (event: string) => { + for (const handler of handlers.get(event) ?? []) await handler({}, ctx); + }; + const call = ( + name: string, + params: Record, + signal?: AbortSignal, + ) => + tools.get(name)!.execute(name, params, signal, undefined, ctx) as Promise<{ + details: { + id: string; + terminals?: Array<{ id: string; status: string }>; + }; + }>; + // bg_list, unlike bg_status, never hands a settlement to the model itself. + const statusOf = async (id: string) => + (await call("bg_list", {})).details.terminals?.find( + (terminal) => terminal.id === id, + )?.status; + const waitFor = async (check: () => Promise | boolean) => { + const deadline = Date.now() + 10_000; + while (!(await check())) { + if (Date.now() > deadline) throw new Error("timed out"); + await new Promise((resolve) => setTimeout(resolve, 25)); + } + }; + + backgroundTerminals(pi); + await emit("session_start"); + try { + const fast = await call("bg_start", { command: "sleep 30", title: "fast" }); + const slow = await call("bg_start", { + command: "trap '' TERM; sleep 30", + title: "slow", + }); + await new Promise((resolve) => setTimeout(resolve, 200)); + + const abort = new AbortController(); + const killing = call( + "bg_kill", + { ids: [fast.details.id, slow.details.id] }, + abort.signal, + ); + await waitFor(async () => (await statusOf(fast.details.id)) === "killed"); + abort.abort(); + await assert.rejects(killing, /termination continues in the background/); + + await waitFor(async () => (await statusOf(slow.details.id)) !== "running"); + idle = true; + await emit("agent_settled"); + const deliveredIds = () => + sent.flatMap(({ details }) => + details.results + ? details.results.map((result) => result.id) + : details.id + ? [details.id] + : [], + ); + await waitFor(() => + [fast.details.id, slow.details.id].every((id) => + deliveredIds().includes(id), + ), + ); + // Leave room for a duplicate batch to show up before counting. + await new Promise((resolve) => setTimeout(resolve, 500)); + assert.deepEqual(deliveredIds().sort(), [fast.details.id, slow.details.id]); + } finally { + await emit("session_shutdown"); + } +}); diff --git a/tests/extensions/background-terminals/manager.test.ts b/tests/extensions/background-terminals/manager.test.ts index f01e80e6c..28b1ab34b 100644 --- a/tests/extensions/background-terminals/manager.test.ts +++ b/tests/extensions/background-terminals/manager.test.ts @@ -789,6 +789,113 @@ test("a settle during an in-flight kill reports consumed: true", async () => { }); }); +test("FAIL: an aborted multi-id kill hands already-settled results back exactly once", { + skip: process.platform === "win32", +}, async () => { + await withManager(async (manager, runtime) => { + const settled: Array<{ id: string; status: string; consumed: boolean }> = + []; + manager.view.setOnSettled((snap, consumed) => + settled.push({ id: snap.id, status: snap.status, consumed }), + ); + const fast = await runTool( + runtime, + manager.start({ + command: nodeCmd("setInterval(() => {}, 1000)"), + title: "fast", + cwd, + }), + ); + const slow = await runTool( + runtime, + manager.start({ + command: `exec ${nodeCmd( + 'process.on("SIGTERM", () => {}); process.stdout.write("ready\\n"); setInterval(() => {}, 1000);', + )}`, + title: "slow", + cwd, + }), + ); + assert.ok( + await pollUntil(() => + (manager.view.get(slow.id)?.stdout.text ?? "").includes("ready"), + ), + ); + + // `fast` dies on SIGTERM; `slow` holds the kill open until SIGKILL. The + // user aborts the tool call inside that window. + const abort = new AbortController(); + const killing = runTool(runtime, manager.kill([fast.id, slow.id]), { + signal: abort.signal, + }); + assert.ok( + await pollUntil(() => manager.view.get(fast.id)?.status === "killed"), + ); + abort.abort(); + await assert.rejects(killing); + + // The aborted kill returned nothing, so `fast` must flow back as an + // automatic result instead of disappearing. + assert.deepEqual(settled, [ + { id: fast.id, status: "killed", consumed: true }, + { id: fast.id, status: "killed", consumed: false }, + ]); + await settlement(manager, slow.id); + assert.ok(await pollUntil(() => settled.length === 3)); + assert.deepEqual(settled[2], { + id: slow.id, + status: "killed", + consumed: false, + }); + }); +}); + +test("an aborted kill does not redeliver results another kill returned", { + skip: process.platform === "win32", +}, async () => { + await withManager(async (manager, runtime) => { + const settled: Array<{ id: string; consumed: boolean }> = []; + manager.view.setOnSettled((snap, consumed) => + settled.push({ id: snap.id, consumed }), + ); + const fast = await runTool( + runtime, + manager.start({ + command: nodeCmd("setInterval(() => {}, 1000)"), + title: "fast", + cwd, + }), + ); + const slow = await runTool( + runtime, + manager.start({ + command: `exec ${nodeCmd( + 'process.on("SIGTERM", () => {}); process.stdout.write("ready\\n"); setInterval(() => {}, 1000);', + )}`, + title: "slow", + cwd, + }), + ); + assert.ok( + await pollUntil(() => + (manager.view.get(slow.id)?.stdout.text ?? "").includes("ready"), + ), + ); + + const abort = new AbortController(); + const aborted = runTool(runtime, manager.kill([fast.id, slow.id]), { + signal: abort.signal, + }); + // A second kill of `fast` alone returns its result to its own caller. + const [returned] = await runTool(runtime, manager.kill([fast.id])); + assert.equal(returned.status, "killed"); + abort.abort(); + await assert.rejects(aborted); + + assert.deepEqual(settled, [{ id: fast.id, consumed: true }]); + }); +}); + test("UI requestKill settles as killed and is NOT consumed", async () => { await withManager(async (manager, runtime) => { const settled: Array<{ id: string; status: string; consumed: boolean }> =