Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion extensions/background-terminals/src/manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void>;
/** 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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -999,6 +1003,7 @@ function* makeManager(maxSpillBytesPerSession: number) {
terminationFailed: false,
exitCleanupStarted: false,
settled,
awaitingKillReport: false,
};

if (options.timeoutSeconds !== undefined) {
Expand Down Expand Up @@ -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",
Expand All @@ -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);
Expand All @@ -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();
}),
),
Expand Down
111 changes: 111 additions & 0 deletions tests/extensions/background-terminals/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, CapturedTool>();
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<string, unknown>,
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> | 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");
}
});
107 changes: 107 additions & 0 deletions tests/extensions/background-terminals/manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }> =
Expand Down
Loading