Skip to content

Commit bad370b

Browse files
committed
Fix resumed-agent fail teardown and wait pin reuse
fail() of a live persisted agent must invoke the registered close so close_agent can recover after a resume followup throw. Mailbox register re-pins on call-id reuse, and a pruned mailbox member is a tombstone instead of eternal running. Wait/task cancel stays interrupted.
1 parent 70886b4 commit bad370b

8 files changed

Lines changed: 215 additions & 39 deletions

File tree

CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,11 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename
1313

1414
## [Unreleased]
1515

16+
### Changed
17+
18+
- Cancelling a `task` or `wait_agents` worker reports wait status `interrupted`,
19+
not `failed`.
20+
1621
### Fixed
1722

1823
- Codex ChatGPT subscription sessions no longer show a public-rate dollar

docs/ARCHITECTURE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ Every director package carries a required `tier: SubagentTier` field (`src/agent
219219
Enforcement is runtime code at the existing tool-mount point, not prompt wording — this is the fix for four prior mechanisms (`writePaths`, `report.requiredSections`, a `--config` comment, the thrash matcher) that were documented-as-enforced while enforcing nothing:
220220

221221
- **Mount-time gate — live today, and fails closed.** `task-tool.ts` resolves the caller's tier at dispatch time — a closed director's `DirectorPackage.tier` — and forwards it as `RunSubAgentParams.orchestratorTier`. `runSubAgent` (`src/subagent/run.ts`) then calls `assertTierMayMountFleetVerb(tier, toolName)` (`src/subagent/authority.ts`) before installing fleet verbs, treating a **missing** `orchestratorTier` as `"leaf"` — deny, not skip. This is the case that matters most: a project-local or plugin `AgentProfile` with `orchestrator: true` is outside the closed director set and is **not** trusted with fleet verbs just because `orchestrator: true` is set — there is no profile-level opt-in today, so the mount always throws `FleetAuthorityError` for a profile-sourced orchestrator. `FLEET_VERBS` in `authority.ts` names the live verbs (`task`, `spawn_agent`, `wait_agents`, `list_agents`, `send_input`, `interrupt_agent`, `close_agent`, `resume_agent`, `read_agent_trace`, `search_agents`) so every mount site inherits the same gate. `list_agents` is the non-blocking mailbox-scoped list of this install's own `spawn_agent` workers (same scope as `wait_agents`); nested orchestrators may mount it. Fleet discovery (`search_agents`) remains Tier 1 only.
222-
- **Subtree authority — wired for addressing verbs.** `assertCanTargetAgent(actor, targetId, nodes)` implements the "root owns its tree; a child manages only its own descendants" rule over the `{id, parentSessionId}` shape `SubAgentSessionStore` already tracks. Production call sites: `read_agent_trace`, `send_input`, `interrupt_agent`, `close_agent`, and `resume_agent`. Nested mounts pass `{actorId, tier, getNodes}` from `run.ts`; a missing `actorId` fails closed. Tier-1 primary omits authority and stays unrestricted. `spawn_agent` records `parentSessionId` on nested workers so `close_agent`'s descendant walk can see them. `wait_agents` with omitted targets waits only on that caller's per-install wait mailbox over the shared session store, not every running session. Wait JSON is a projection of stored lifecycle plus mailbox membership/pin/collected/interrupt override — not a second terminal store. `list_agents` reports that same mailbox without blocking. `interrupt_agent` / `send_input` with `interrupt:true` terminalize the wait mailbox immediately; the soft-interrupt wait path collects so a later followup cannot resurrect an already-observed interrupt. `close_agent` also terminalizes the wait mailbox before teardown. Operator cancel (`cancel` / `cancelAll`) projects wait status `interrupted`.
222+
- **Subtree authority — wired for addressing verbs.** `assertCanTargetAgent(actor, targetId, nodes)` implements the "root owns its tree; a child manages only its own descendants" rule over the `{id, parentSessionId}` shape `SubAgentSessionStore` already tracks. Production call sites: `read_agent_trace`, `send_input`, `interrupt_agent`, `close_agent`, and `resume_agent`. Nested mounts pass `{actorId, tier, getNodes}` from `run.ts`; a missing `actorId` fails closed. Tier-1 primary omits authority and stays unrestricted. `spawn_agent` records `parentSessionId` on nested workers so `close_agent`'s descendant walk can see them. `wait_agents` with omitted targets waits only on that caller's per-install wait mailbox over the shared session store, not every running session. Wait JSON is a projection of stored lifecycle plus mailbox membership/pin/collected/interrupt override — not a second terminal store. `list_agents` reports that same mailbox without blocking. `interrupt_agent` stamps the session `interrupted`; wait JSON projects that stored lifecycle and does not write a mailbox overlay. `send_input` with `interrupt:true` sets the mailbox interrupt overlay so wait unblocks while a queued followup may already be running. The wait path collects a terminal status so a later followup cannot resurrect an already-observed interrupt. `close_agent` terminalizes the wait mailbox before teardown. Operator cancel (`cancel` / `cancelAll`) projects wait status `interrupted`.
223223
- `task()` remains the deprecated fused spawn+wait fallback. `spawn_agent` + `wait_agents` is the supported parallel path. The tier check still gates which packages may mount any fleet verb.
224224

225225
#### Closed director fleet (`src/agent/directors/`)

src/subagent/agent-fleet.test.ts

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -415,6 +415,59 @@ describe("wait mailbox session tombstone and pin", () => {
415415
expect(results[0]!.report).toBeUndefined();
416416
expect(results[0]!.hint).toContain("read_agent_trace");
417417
});
418+
419+
test("spawn_agent call.id reuse still pins the new session", async () => {
420+
let t = 0;
421+
const sessions = createSubAgentSessionStore({
422+
maxCompleted: 1,
423+
now: () => ++t,
424+
});
425+
const deps = makeDeps(async () => ({ report: "ok" }), { sessions });
426+
const spawn = createSpawnAgentTool(deps);
427+
const wait = createWaitAgentsTool({ sessions, fleetRecords: deps.fleetRecords });
428+
if (spawn.kind !== "full") throw new Error("expected full tool");
429+
const args = { description: "job", prompt: "do it", intent: "explore" };
430+
const signal = new AbortController().signal;
431+
432+
await spawn.handler({ id: "reuse-id", name: "spawn_agent", arguments: args }, signal);
433+
await new Promise((resolve) => setTimeout(resolve, 20));
434+
await spawn.handler({ id: "reuse-id", name: "spawn_agent", arguments: args }, signal);
435+
await new Promise((resolve) => setTimeout(resolve, 20));
436+
437+
const extra1 = sessions.start({ description: "flood-1", agentId: "a", brief: "b" });
438+
sessions.complete(extra1.id, "flood-1");
439+
const extra2 = sessions.start({ description: "flood-2", agentId: "a", brief: "b" });
440+
sessions.complete(extra2.id, "flood-2");
441+
442+
expect(sessions.get("reuse-id")).toBeDefined();
443+
const waited = await callTool(wait, { targets: ["reuse-id"], timeout_ms: 1000 });
444+
expect(waited.timed_out).toBe(false);
445+
const results = waited.results as { status: string }[];
446+
expect(results[0]!.status).toBe("done");
447+
});
448+
449+
test("wait on a pruned mailbox member is tombstone not eternal running", () => {
450+
let t = 0;
451+
const sessions = createSubAgentSessionStore({
452+
maxCompleted: 1,
453+
now: () => ++t,
454+
});
455+
const mailbox = createFleetMailbox(sessions);
456+
sessions.start({ id: "reuse", description: "old", agentId: "a", brief: "b" });
457+
mailbox.register("reuse");
458+
sessions.complete("reuse", "old report");
459+
sessions.start({ id: "reuse", description: "new", agentId: "a", brief: "b" });
460+
sessions.complete("reuse", "new report");
461+
const extra = sessions.start({ description: "other", agentId: "a", brief: "b" });
462+
sessions.complete(extra.id, "other");
463+
const extra2 = sessions.start({ description: "prune", agentId: "a", brief: "b" });
464+
sessions.complete(extra2.id, "prune");
465+
expect(sessions.get("reuse")).toBeUndefined();
466+
const snap = mailbox.peek("reuse");
467+
expect(snap?.status).not.toBe("running");
468+
expect(snap?.tombstoned).toBe(true);
469+
expect(snap?.hint).toContain("read_agent_trace");
470+
});
418471
});
419472

420473
describe("spawn_agent parentage", () => {

src/subagent/agent-fleet.ts

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -129,10 +129,11 @@ class FleetMailbox {
129129

130130
register(id: string): void {
131131
const existing = this.records.get(id);
132-
const alreadyPinned =
133-
existing !== undefined && existing.collected !== true && existing.pinHeld === true;
132+
// start() drops pinCounts on call-id reuse. Re-pin whenever the overlay
133+
// thought it still held a pin, so wait cannot desync against an empty map.
134+
if (existing?.pinHeld === true) this.sessions.unpin(id);
134135
this.records.set(id, { pinHeld: true });
135-
if (!alreadyPinned) this.sessions?.pin(id);
136+
this.sessions.pin(id);
136137
this.enforceCap();
137138
}
138139

@@ -212,16 +213,29 @@ class FleetMailbox {
212213
private projectedStatus(id: string, overlay: FleetOverlay): WaitJSONStatus {
213214
if (overlay.frozenStatus !== undefined) return overlay.frozenStatus;
214215
if (overlay.forceInterrupted === true) return "interrupted";
215-
return this.sessionWaitStatus(id) ?? "running";
216+
return this.sessionWaitStatus(id) ?? "interrupted";
216217
}
217218

218219
snapshot(id: string): FleetRecord {
219220
const overlay = this.records.get(id);
220221
if (overlay === undefined) {
221222
return { status: "running" };
222223
}
224+
const session = this.sessions.get(id);
225+
if (
226+
session === undefined &&
227+
overlay.tombstoned !== true &&
228+
overlay.frozenStatus === undefined
229+
) {
230+
overlay.tombstoned = true;
231+
overlay.hint = RECOVERY_HINT;
232+
overlay.frozenStatus = "interrupted";
233+
if (overlay.pinHeld === true) {
234+
overlay.pinHeld = false;
235+
this.sessions.unpin(id);
236+
}
237+
}
223238
const status = this.projectedStatus(id, overlay);
224-
const session = this.sessions?.get(id);
225239
const sessionWait = this.sessionWaitStatus(id);
226240
const payload =
227241
overlay.tombstoned !== true && session !== undefined && sessionWait === status

src/subagent/lifecycle-tools.test.ts

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -132,9 +132,9 @@ describe("resume_agent", () => {
132132

133133
finish("done, history now 2 turns");
134134
await new Promise((resolve) => setTimeout(resolve, 0));
135-
expect(sessions.get(retained.id)?.lifecycleStatus).toBe("completed");
135+
expect(sessions.get(retained.id)?.lifecycleStatus).toBe("interrupted");
136136
expect(sessions.get(retained.id)?.id).toBe(retained.id);
137-
expect(sessions.get(retained.id)?.report).toBe("done, history now 2 turns");
137+
expect(sessions.get(retained.id)?.report).toBe("## Summary\nDone.");
138138

139139
if (resumeAgent.kind !== "full") throw new Error("expected full tool");
140140
const rejected = await resumeAgent.handler(
@@ -298,6 +298,40 @@ describe("resume_agent", () => {
298298
expect(results[0]!.status).toBe("done");
299299
expect(results[0]!.report).toBe("second report");
300300
});
301+
302+
test("resume followup rejection invokes close; close_agent tears down leftover", async () => {
303+
const sessions = createSubAgentSessionStore();
304+
const fleetRecords = createFleetMailbox(sessions);
305+
const worker = sessions.start({
306+
description: "worker",
307+
agentId: "a",
308+
brief: "b",
309+
retained: true,
310+
});
311+
let closeCalls = 0;
312+
sessions.registerClose(worker.id, async () => {
313+
closeCalls++;
314+
});
315+
sessions.registerFollowup(worker.id, async () => {
316+
throw new Error("send failed");
317+
});
318+
sessions.complete(worker.id, "first report");
319+
320+
const resumeAgent = createResumeAgentTool({ sessions, fleetRecords });
321+
const closeAgent = createCloseAgentTool({ sessions, fleetRecords });
322+
const resumed = await callTool(resumeAgent, { target: worker.id, message: "again" });
323+
expect(resumed.status).toBe("running");
324+
await new Promise((resolve) => setTimeout(resolve, 0));
325+
expect(closeCalls).toBe(1);
326+
expect(sessions.get(worker.id)?.lifecycle.state).toBe("failed");
327+
328+
const started = Date.now();
329+
const closed = await callTool(closeAgent, { target: worker.id });
330+
expect(Date.now() - started).toBeLessThan(1000);
331+
expect(closed.status).toBe("shutdown");
332+
expect(sessions.get(worker.id)?.lifecycle.state).toBe("failed");
333+
expect(closeCalls).toBe(1);
334+
});
301335
});
302336

303337
describe("interrupt_agent", () => {

src/subagent/lifecycle-tools.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ export type CloseAgentToolDeps = LifecycleToolDeps & {
121121
fleetRecords: FleetMailboxHandle;
122122
};
123123

124-
/** interrupt_agent always terminalizes the wait mailbox — no silent skip. */
124+
/** interrupt_agent stamps session interrupted; wait JSON projects that lifecycle. */
125125
export type InterruptAgentToolDeps = LifecycleToolDeps & {
126126
fleetRecords: FleetMailboxHandle;
127127
};

src/subagent/session-store.test.ts

Lines changed: 54 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -365,8 +365,9 @@ describe("CL-6943 reusable worker sessions", () => {
365365

366366
finish("later");
367367
await new Promise((resolve) => setTimeout(resolve, 0));
368-
expect(store.get(session.id)?.status).toBe("done");
369-
expect(store.get(session.id)?.lifecycleStatus).toBe("completed");
368+
expect(store.get(session.id)?.status).toBe("running");
369+
expect(store.get(session.id)?.lifecycleStatus).toBe("interrupted");
370+
expect(store.get(session.id)?.report).toBe("## Summary\nDone.");
370371
});
371372

372373
test("rejected followup restores strip status so interrupt_agent fails closed", async () => {
@@ -698,9 +699,9 @@ describe("interrupt stamps finishedAt once", () => {
698699
t = 5000;
699700
finish("later");
700701
await new Promise((resolve) => setTimeout(resolve, 0));
701-
expect(store.get(session.id)?.status).toBe("done");
702-
expect(store.get(session.id)?.lifecycleStatus).toBe("completed");
703-
expect(store.get(session.id)?.finishedAt).toBe(5000);
702+
expect(store.get(session.id)?.status).toBe("running");
703+
expect(store.get(session.id)?.lifecycleStatus).toBe("interrupted");
704+
expect(store.get(session.id)?.finishedAt).toBe(4000);
704705
});
705706

706707
test("a follow-up turn keeps the lane live past the linger window until it completes", async () => {
@@ -756,6 +757,54 @@ describe("interrupt stamps finishedAt once", () => {
756757
});
757758

758759
describe("CL-7269 one stored worker lifecycle", () => {
760+
test("complete() after interrupt_agent does not overwrite interrupted", () => {
761+
const store = createSubAgentSessionStore();
762+
const session = store.start({ description: "d", agentId: "a", brief: "b", retained: true });
763+
store.markRunning(session.id);
764+
store.registerInterrupt(session.id, () => {});
765+
expect(store.interruptOne(session.id).ok).toBe(true);
766+
store.complete(session.id, "late original send");
767+
const after = store.get(session.id);
768+
expect(after?.lifecycle.state).toBe("interrupted");
769+
expect(after?.lifecycleStatus).toBe("interrupted");
770+
expect(after?.report).toBeUndefined();
771+
});
772+
773+
test("fail() of a live persisted agent invokes the registered close", async () => {
774+
const store = createSubAgentSessionStore();
775+
const session = store.start({ description: "d", agentId: "a", brief: "b", retained: true });
776+
store.markRunning(session.id);
777+
let closeCalls = 0;
778+
store.registerClose(session.id, async () => {
779+
closeCalls++;
780+
});
781+
store.fail(session.id, "send failed");
782+
await new Promise((resolve) => setTimeout(resolve, 0));
783+
expect(closeCalls).toBe(1);
784+
expect(store.get(session.id)?.lifecycle.state).toBe("failed");
785+
const started = Date.now();
786+
const status = await store.closeOne(session.id, 5000);
787+
expect(Date.now() - started).toBeLessThan(200);
788+
expect(status).toBe("shutdown");
789+
expect(store.get(session.id)?.lifecycle.state).toBe("failed");
790+
expect(closeCalls).toBe(1);
791+
});
792+
793+
test("closeOne still tears down a leftover close handle after fail()", async () => {
794+
const store = createSubAgentSessionStore();
795+
const session = store.start({ description: "d", agentId: "a", brief: "b", retained: true });
796+
store.markRunning(session.id);
797+
let closeCalls = 0;
798+
store.fail(session.id, "send failed");
799+
store.registerClose(session.id, async () => {
800+
closeCalls++;
801+
});
802+
const status = await store.closeOne(session.id, 5000);
803+
expect(status).toBe("shutdown");
804+
expect(closeCalls).toBe(1);
805+
expect(store.get(session.id)?.lifecycle.state).toBe("failed");
806+
});
807+
759808
test("fail() stores failed, projects strip failed and verb shutdown", () => {
760809
const store = createSubAgentSessionStore();
761810
const session = store.start({ description: "d", agentId: "a", brief: "b" });

0 commit comments

Comments
 (0)