Skip to content

Commit 5903834

Browse files
Resolve fleetRecords on cancel so wait_agents sees salvage (CL-6915) (#672)
Operator cancel aborts the child signal, but run() can still return a salvage body. The spawn settle path used to drop that body when childCtl was aborted, leaving fleetRecords "running" forever so wait_agents never observed the salvage. Always resolve/reject fleetRecords; complete/fail already no-op when the strip is cancelled. Complementary to #668 (content salvage into the forced-stop envelope).
1 parent dbfe615 commit 5903834

2 files changed

Lines changed: 56 additions & 2 deletions

File tree

src/subagent/agent-fleet.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
} from "./agent-fleet.js";
1010
import { createSubAgentSessionStore } from "./session-store.js";
1111
import { createPermissionGate } from "../permission/gate.js";
12+
import { forcedStopReport } from "./stop-policy.js";
1213
import type { RunSubAgentParams, RunSubAgentResult } from "./types.js";
1314

1415
const testPermissionGate = createPermissionGate({
@@ -238,6 +239,53 @@ describe("spawn_agent + wait_agents", () => {
238239
expect(result.report).toBe("irrelevant");
239240
}
240241
});
242+
243+
// CL-6915: operator cancel aborts the child signal, but run() still returns a
244+
// salvage body (partial findings). Dropping that body left fleetRecords
245+
// "running" forever so wait_agents never saw the salvage.
246+
test("cancelled spawn_agent still resolves wait_agents with salvage findings", async () => {
247+
const deps = makeDeps(async (params) => {
248+
await new Promise<void>((resolve) => {
249+
if (params.signal?.aborted) {
250+
resolve();
251+
return;
252+
}
253+
params.signal?.addEventListener("abort", () => resolve(), { once: true });
254+
});
255+
await new Promise((r) => setTimeout(r, 10));
256+
return {
257+
report: forcedStopReport("cancelled", "Found path in gate.ts"),
258+
stopReason: "cancelled",
259+
};
260+
});
261+
const spawn = createSpawnAgentTool(deps);
262+
const wait = createWaitAgentsTool({ sessions: deps.sessions, fleetRecords: deps.fleetRecords });
263+
264+
const spawned = await callTool(spawn, {
265+
description: "cancel salvage",
266+
prompt: "probe",
267+
intent: "explore",
268+
});
269+
const id = spawned.agent_id as string;
270+
271+
expect(deps.sessions.cancel(id)).toBe(true);
272+
expect(deps.sessions.get(id)?.status).toBe("cancelled");
273+
274+
const waited = await callTool(wait, { targets: [id], timeout_ms: 5000 });
275+
expect(waited.timed_out).toBe(false);
276+
const results = waited.results as {
277+
agent_id: string;
278+
status: string;
279+
report?: string;
280+
}[];
281+
expect(results).toHaveLength(1);
282+
expect(results[0]!.status).toBe("done");
283+
expect(results[0]!.report).toContain("## Summary");
284+
expect(results[0]!.report).toContain("## Findings");
285+
expect(results[0]!.report).toContain("gate.ts");
286+
// Strip stays cancelled — salvage is for wait_agents, not a resurrection.
287+
expect(deps.sessions.get(id)?.status).toBe("cancelled");
288+
});
241289
});
242290

243291
describe("spawn_agent same-cwd concurrency", () => {

src/subagent/agent-fleet.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -467,25 +467,31 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool {
467467
deps
468468
.run(params)
469469
.then((result) => {
470-
if (childCtl.signal.aborted) return;
471470
// interrupt_agent already flipped this session to "interrupted"
472471
// synchronously (session-store.interruptOne) — do not let the
473472
// settling promise's normal bookkeeping overwrite that with a
474473
// "completed" status.
475474
if (result.interrupted === true) return;
475+
// Operator cancel may race after run resolves (childCtl aborted).
476+
// Keep strip status cancelled when sessions.cancel already flipped
477+
// it, but never discard a returned body (including salvage) —
478+
// wait_agents reads fleetRecords, not the strip.
476479
deps.fleetRecords.resolve(session.id, result.report);
477480
// result.agentRetained is only true on run.ts's clean-completion
478481
// path when persist actually skipped teardown — a deadline/cancel
479482
// salvage resolves through the same promise but always disposed
480483
// its agent first, so the store must not treat it as resumable
481484
// just because retained:true was requested at spawn.
485+
// complete() no-ops when status is already cancelled.
482486
deps.sessions.complete(session.id, result.report, {
483487
agentRetained: result.agentRetained === true,
484488
...(result.stopReason !== undefined ? { stopReason: result.stopReason } : {}),
485489
});
486490
})
487491
.catch((err) => {
488-
if (childCtl.signal.aborted) return;
492+
// Always terminalize fleetRecords — including pre-progress cancel that
493+
// rethrows with no salvage — so wait_agents does not hang. fail()
494+
// no-ops when cancel already flipped the strip status.
489495
const message = err instanceof Error ? err.message : String(err);
490496
deps.fleetRecords.reject(session.id, message);
491497
deps.sessions.fail(session.id, message);

0 commit comments

Comments
 (0)