Skip to content

Commit 9b1b04d

Browse files
committed
fix(subagent): keep failure records without delaying spawn failure
1 parent 694fbd2 commit 9b1b04d

7 files changed

Lines changed: 221 additions & 56 deletions

‎src/subagent/agent-fleet.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@ import { formatSubAgentSpawnAuthFailureMessage } from "./inference-auth-failure.
113113
import {
114114
classifySubAgentFailure,
115115
isProviderFailureClass,
116+
isSubAgentLoopGuardError,
116117
type SubAgentFailureRecord,
117118
} from "./terminal-failure.js";
118119
import { errorMessage } from "../agent/error-message.js";
@@ -1690,6 +1691,7 @@ export function createSpawnAgentTool(deps: AgentFleetDeps): AgentTool {
16901691
...(settlement?.teardown_failed === true
16911692
? { teardownFailed: true }
16921693
: {}),
1694+
...(isSubAgentLoopGuardError(err) ? { cause: err.cause } : {}),
16931695
});
16941696
})
16951697
.finally(() => {

‎src/subagent/fleet-terminal-failure.test.ts‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
waitUntilMailboxTerminal,
1616
} from "./fleet-test-harness.js";
1717
import { pollUntil } from "./run-test-harness.js";
18+
import { createSubAgentLoopGuardError } from "./terminal-failure.js";
1819
import type { RunSubAgentParams, RunSubAgentResult } from "./types.js";
1920

2021
type Row = Record<string, unknown> & {
@@ -85,6 +86,38 @@ describe("failure_class drives the existing provider markers", () => {
8586
},
8687
);
8788

89+
test("a doom-loop over a retryable provider failure stays loop_guard and continuable", async () => {
90+
const deps = createFleetDeps(async () => {
91+
throw createSubAgentLoopGuardError(retryable());
92+
});
93+
const tools = fleetTools(deps);
94+
const id = await spawnAgentId(tools.spawn, spawnArgs);
95+
const [row] = await waitRows(tools, [id]);
96+
expect(row?.status).toBe("failed");
97+
expect(row?.failure?.failure_class).toBe("loop_guard");
98+
expect(row?.failure?.recovery).toEqual({
99+
available: true,
100+
reason: "eligible",
101+
});
102+
expect(row?.provider_failure).toBeUndefined();
103+
expect(row?.continuable).toBe(true);
104+
});
105+
106+
test("a doom-loop without a retryable cause stays loop_guard and not continuable", async () => {
107+
const deps = createFleetDeps(async () => {
108+
throw createSubAgentLoopGuardError(new Error("reactor error: doom"));
109+
});
110+
const tools = fleetTools(deps);
111+
const id = await spawnAgentId(tools.spawn, spawnArgs);
112+
const [row] = await waitRows(tools, [id]);
113+
expect(row?.failure?.failure_class).toBe("loop_guard");
114+
expect(row?.failure?.recovery).toEqual({
115+
available: false,
116+
reason: "not_retryable",
117+
});
118+
expect(row?.continuable).toBeUndefined();
119+
});
120+
88121
test("a permission suspension after an observed inference.error is error, not provider", async () => {
89122
const deps = createFleetDeps(async (params) => {
90123
params.onEvent?.(inferenceError);
@@ -199,6 +232,39 @@ describe("terminal failure cleanup", () => {
199232
expect(deps.fleetRecords.peek(secondId)?.status).toBe("done");
200233
});
201234

235+
test("a failed spawn with a hanging close frees its admission slot before close", async () => {
236+
const admission = createAdmissionQueue({ capacity: 1 });
237+
let started = 0;
238+
const deps = createFleetDeps(
239+
async (params: RunSubAgentParams): Promise<RunSubAgentResult> => {
240+
started += 1;
241+
if (started > 1) return { report: "second done" };
242+
params.onAgentReady?.({
243+
close: () => new Promise<void>(() => undefined),
244+
interrupt: () => undefined,
245+
followup: async () => "",
246+
deliver: () => undefined,
247+
});
248+
throw new Error("spawn failed");
249+
},
250+
);
251+
deps.admission = admission;
252+
const tools = fleetTools(deps);
253+
const first = await spawnAgentId(tools.spawn, spawnArgs);
254+
const second = await callFleetTool(tools.spawn, spawnArgs);
255+
expect(second.status).toBe("queued");
256+
257+
await waitUntilMailboxTerminal(deps.fleetRecords, deps.sessions, first);
258+
const [row] = await waitRows(tools, [first]);
259+
expect(row?.status).toBe("failed");
260+
expect(deps.sessions.get(first)?.lifecycle.state).toBe("failed");
261+
await pollUntil(() => !admission.occupied(first));
262+
263+
const secondId = second.agent_id as string;
264+
await waitUntilMailboxTerminal(deps.fleetRecords, deps.sessions, secondId);
265+
expect(deps.fleetRecords.peek(secondId)?.status).toBe("done");
266+
});
267+
202268
test("a failure with an open ask releases the wait, the ask, and the admission slot once", async () => {
203269
const admission = createAdmissionQueue({ capacity: 1 });
204270
const failFirst = deferred<undefined>();

‎src/subagent/lifecycle-tools.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,10 @@ import {
2121
UNSTRUCTURED_DIGEST_CHARS,
2222
} from "./fleet-dry-drive.js";
2323
import { parseSubAgentReport } from "./report.js";
24-
import { classifySubAgentFailure } from "./terminal-failure.js";
24+
import {
25+
classifySubAgentFailure,
26+
isSubAgentLoopGuardError,
27+
} from "./terminal-failure.js";
2528
import {
2629
DEFAULT_MAX_ENTRY_CHARS,
2730
type AgentLifecycleStatus,
@@ -353,6 +356,7 @@ export function createResumeAgentTool(deps: ResumeAgentToolDeps): AgentTool {
353356
onFail: (err) =>
354357
deps.sessions.fail(target, errorMessage(err), {
355358
failure_class: classifySubAgentFailure(err),
359+
...(isSubAgentLoopGuardError(err) ? { cause: err.cause } : {}),
356360
}),
357361
});
358362
if (!outcome.ok) {

‎src/subagent/session-store-failure-record.test.ts‎

Lines changed: 64 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,36 @@ describe("terminal failure record", () => {
125125
expect(Object.isFrozen(snap?.failure)).toBe(true);
126126
});
127127

128+
test("a loop_guard with a retryable provider cause is recovery-eligible", () => {
129+
const { sessions } = store();
130+
const id = startRunning(sessions);
131+
sessions.fail(id, "upstream overloaded", {
132+
failure_class: "loop_guard",
133+
cause: createResolvedProviderFailureError("test-provider", {
134+
category: "retryable",
135+
message: "upstream overloaded",
136+
}),
137+
});
138+
const failure = sessions.get(id)?.failure;
139+
expect(failure?.failure_class).toBe("loop_guard");
140+
expect(failure?.recovery).toEqual({ available: true, reason: "eligible" });
141+
expect("cause" in (failure ?? {})).toBe(false);
142+
});
143+
144+
test("a loop_guard without a retryable cause is not recovery-eligible", () => {
145+
const { sessions } = store();
146+
const id = startRunning(sessions);
147+
sessions.fail(id, "doom", {
148+
failure_class: "loop_guard",
149+
cause: new Error("doom"),
150+
});
151+
expect(sessions.get(id)?.failure?.failure_class).toBe("loop_guard");
152+
expect(sessions.get(id)?.failure?.recovery).toEqual({
153+
available: false,
154+
reason: "not_retryable",
155+
});
156+
});
157+
128158
test("fail without a classification records error", () => {
129159
const { sessions } = store();
130160
const id = startRunning(sessions);
@@ -415,30 +445,47 @@ describe("backstop close honesty", () => {
415445
expect(sessions.get(id)?.failure?.cleanup).toBe("released");
416446
});
417447

418-
test("a rejecting backstop close records partial cleanup", async () => {
448+
test("a rejecting backstop close does not delay the failed lane", async () => {
419449
const { sessions } = store();
420-
const id = retainedWithClose(sessions, async () => {
421-
throw new Error("child still live");
422-
});
423-
await sessions.fail(id, "boom", { failure_class: "error" });
450+
let rejectClose: (err: Error) => void = () => undefined;
451+
const id = retainedWithClose(
452+
sessions,
453+
() =>
454+
new Promise<void>((_resolve, reject) => {
455+
rejectClose = reject;
456+
}),
457+
);
458+
const failing = sessions.fail(id, "boom", { failure_class: "error" });
424459
expect(sessions.get(id)?.lifecycle.state).toBe("failed");
425-
expect(sessions.get(id)?.failure?.cleanup).toBe("partial");
460+
const record = sessions.get(id)?.failure;
461+
expect(record?.cleanup).toBe("released");
462+
rejectClose(new Error("child still live"));
463+
await failing;
464+
expect(sessions.get(id)?.failure).toBe(
465+
record as NonNullable<typeof record>,
466+
);
426467
});
427468

428-
test("a backstop close that hangs past its deadline records partial cleanup", async () => {
429-
const sessions = createSubAgentSessionStore({
430-
now: () => T0,
431-
backstopCloseDeadlineMs: 20,
432-
});
469+
test("a hanging backstop close does not delay the failed lane", async () => {
470+
const { sessions } = store();
433471
const id = retainedWithClose(
434472
sessions,
435473
() => new Promise<void>(() => undefined),
436474
);
437-
await sessions.fail(id, "boom", { failure_class: "error" });
438-
expect(sessions.get(id)?.failure?.cleanup).toBe("partial");
475+
const settled = await Promise.race([
476+
sessions
477+
.fail(id, "boom", { failure_class: "error" })
478+
.then(() => "settled" as const),
479+
new Promise<"stalled">((resolve) =>
480+
setTimeout(() => resolve("stalled"), 200),
481+
),
482+
]);
483+
expect(settled).toBe("settled");
484+
expect(sessions.get(id)?.lifecycle.state).toBe("failed");
485+
expect(sessions.get(id)?.failure?.cleanup).toBe("released");
439486
});
440487

441-
test("the record is not written until the close settles, and never changes after", async () => {
488+
test("the record is written before the close settles, and never changes after", async () => {
442489
const { sessions } = store();
443490
let finishClose: () => void = () => undefined;
444491
const id = retainedWithClose(
@@ -449,11 +496,11 @@ describe("backstop close honesty", () => {
449496
}),
450497
);
451498
const failing = sessions.fail(id, "boom", { failure_class: "error" });
452-
expect(sessions.get(id)?.failure).toBeUndefined();
453-
finishClose();
454-
await failing;
499+
expect(sessions.get(id)?.lifecycle.state).toBe("failed");
455500
const record = sessions.get(id)?.failure;
456501
expect(record?.cleanup).toBe("released");
502+
finishClose();
503+
await failing;
457504
await settleTick();
458505
expect(sessions.get(id)?.failure).toBe(
459506
record as NonNullable<typeof record>,

‎src/subagent/session-store.ts‎

Lines changed: 10 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import { canonicalToolName } from "../agent/canonical-tool-name.js";
3333
import {
3434
classifySubAgentFailure,
3535
failureClassForStopReason,
36+
isSubAgentLoopGuardError,
3637
recoveryFor,
3738
type SubAgentFailureClass,
3839
type SubAgentFailureInput,
@@ -193,9 +194,6 @@ export interface SubAgentSessionStoreOptions {
193194
createId?: () => string;
194195
/** When set, resume/followup inference is admitted through this queue. */
195196
admission?: AdmissionQueue;
196-
// How long fail() waits on a live close before recording partial cleanup.
197-
// Defaults to close_agent's deadline: the same teardown, so no tighter or
198-
// looser bound is justified.
199197
backstopCloseDeadlineMs?: number;
200198
}
201199

@@ -217,8 +215,6 @@ export interface SubAgentSessionStore {
217215
report: string,
218216
opts?: { agentRetained?: boolean; stopReason?: ForcedStopReason },
219217
): void;
220-
// Resolves once the lane is terminal and its failure record is written. A
221-
// live backstop close is awaited first so the record's cleanup is true.
222218
fail(
223219
id: string,
224220
error: string,
@@ -846,6 +842,7 @@ export function createSubAgentSessionStore(
846842
failureClass,
847843
attempt,
848844
followupTurn: followupTurns.has(id),
845+
cause: pendingFailures.get(id)?.cause,
849846
}),
850847
),
851848
attempt,
@@ -951,8 +948,6 @@ export function createSubAgentSessionStore(
951948

952949
const releaseHandles = (id: string): void => {
953950
cancelAskInternal(id, "session handles released");
954-
// Eviction and prune have no record left to keep honest, so the close
955-
// stays fire-and-forget here; fail() awaits it through detachClose.
956951
void detachClose(id)?.catch((err: unknown) => {
957952
log.warn("session close during handle release failed: {error}", {
958953
error: errorMessage(err),
@@ -1262,7 +1257,10 @@ export function createSubAgentSessionStore(
12621257
// Only the interrupted restore stamps a record of its own; a completed
12631258
// restore stays a clean lane, as before.
12641259
if (failLifecycle === "interrupted") {
1265-
pendingFailures.set(id, { failure_class: classifySubAgentFailure(err) });
1260+
pendingFailures.set(id, {
1261+
failure_class: classifySubAgentFailure(err),
1262+
...(isSubAgentLoopGuardError(err) ? { cause: err.cause } : {}),
1263+
});
12661264
}
12671265
endFollowupTurn(id, failLifecycle);
12681266
pendingFailures.delete(id);
@@ -1732,39 +1730,13 @@ export function createSubAgentSessionStore(
17321730
}
17331731
},
17341732

1735-
async fail(
1733+
fail(
17361734
id: string,
17371735
error: string,
17381736
failure?: SubAgentFailureInput,
17391737
): Promise<void> {
17401738
if (failure !== undefined) pendingFailures.set(id, failure);
17411739
settleCancelsAsks(id, "session failed");
1742-
const current = sessions.get(id);
1743-
const failable =
1744-
current !== undefined &&
1745-
isLiveStrip(current.lifecycle) &&
1746-
current.lifecycle.state !== "cancelled";
1747-
// The record is written once and never amended, so the close it reports
1748-
// on has to finish first. A close that rejects or outlives its deadline
1749-
// is a cleanup fault, not a released worker.
1750-
const closing = failable ? detachClose(id) : undefined;
1751-
if (closing !== undefined && failure?.teardownFailed === true) {
1752-
// The worker's own bounded teardown already timed out on this agent,
1753-
// so the record is partial either way; waiting on a second close
1754-
// would only hold the lane and its admission slot for another
1755-
// deadline.
1756-
void closing.catch((err: unknown) => {
1757-
log.warn("backstop close after failed teardown failed: {error}", {
1758-
error: errorMessage(err),
1759-
});
1760-
});
1761-
} else if (closing !== undefined) {
1762-
try {
1763-
await closing;
1764-
} catch {
1765-
releaseFaults.add(id);
1766-
}
1767-
}
17681740
mutate(id, (session) => {
17691741
if (
17701742
!isLiveStrip(session.lifecycle) ||
@@ -1773,7 +1745,8 @@ export function createSubAgentSessionStore(
17731745
return;
17741746
// Spawn-path throws already dispose in run.ts's finally; a resumed
17751747
// persisted agent does not, so the live close handle is the only
1776-
// teardown, awaited above. A disposed spawn close is idempotent.
1748+
// teardown. Invoke it fire-and-forget (as prune/evict) without
1749+
// marking shutdown — a disposed spawn close is idempotent.
17771750
session.lifecycle = { state: "failed", error };
17781751
session.retained = false;
17791752
session.finishedAt = now();
@@ -1789,6 +1762,7 @@ export function createSubAgentSessionStore(
17891762
pruneCompleted();
17901763
});
17911764
pendingFailures.delete(id);
1765+
return Promise.resolve();
17921766
},
17931767

17941768
registerCancel(id: string, abort: () => void): void {

0 commit comments

Comments
 (0)