Skip to content

Commit e683793

Browse files
steipetesteipete
committed
fix: fence subagent cleanup deletion
1 parent 12c3e79 commit e683793

8 files changed

Lines changed: 117 additions & 3 deletions

‎src/agents/subagent-announce.test.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,25 @@ describe("subagent announce seam flow", () => {
336336
});
337337
});
338338

339+
it("skips delete cleanup when the lifecycle owner invalidates the attempt", async () => {
340+
const didAnnounce = await runSubagentAnnounceFlow({
341+
childSessionKey: "agent:main:subagent:test",
342+
childRunId: "run-invalidated-delete",
343+
requesterSessionKey: "agent:main:main",
344+
requesterDisplayKey: "main",
345+
task: "do thing",
346+
timeoutMs: 10,
347+
cleanup: "delete",
348+
waitForCompletion: false,
349+
outcome: { status: "ok" },
350+
roundOneReply: "ANNOUNCE_SKIP",
351+
onBeforeDeleteChildSession: () => false,
352+
});
353+
354+
expect(didAnnounce).toBe(true);
355+
expect(sessionsDeleteSpy).not.toHaveBeenCalled();
356+
});
357+
339358
it("warns when ANNOUNCE_SKIP suppresses a cron job completion", async () => {
340359
const logSpy = vi.spyOn(defaultRuntime, "log").mockImplementation(() => {});
341360

‎src/agents/subagent-announce.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,7 @@ export async function runSubagentAnnounceFlow(params: {
261261
signal?: AbortSignal;
262262
bestEffortDeliver?: boolean;
263263
onDeliveryResult?: (delivery: SubagentAnnounceDeliveryResult) => void;
264+
onBeforeDeleteChildSession?: () => boolean;
264265
}): Promise<boolean> {
265266
let didAnnounce = false;
266267
const expectsCompletionMessage = params.expectsCompletionMessage === true;
@@ -619,7 +620,7 @@ export async function runSubagentAnnounceFlow(params: {
619620
// Best-effort
620621
}
621622
}
622-
if (shouldDeleteChildSession) {
623+
if (shouldDeleteChildSession && (params.onBeforeDeleteChildSession?.() ?? true)) {
623624
await deleteSubagentSessionForCleanup({
624625
callGateway: subagentAnnounceDeps.callGateway,
625626
childSessionKey: params.childSessionKey,

‎src/agents/subagent-delivery-state.test.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,14 @@ describe("normalizeSubagentRunState", () => {
4141
expect(nonString.taskRunId).toBeUndefined();
4242
});
4343

44+
it("normalizes the durable delete-dispatch boundary", () => {
45+
const valid = normalizeSubagentRunState(baseRun({ deleteCleanupDispatchedAt: 200 }));
46+
const malformed = normalizeSubagentRunState(baseRun({ deleteCleanupDispatchedAt: Number.NaN }));
47+
48+
expect(valid.deleteCleanupDispatchedAt).toBe(200);
49+
expect(malformed.deleteCleanupDispatchedAt).toBeUndefined();
50+
});
51+
4452
it("preserves valid killed reconciliation ownership metadata", () => {
4553
const entry = normalizeSubagentRunState(
4654
baseRun({

‎src/agents/subagent-delivery-state.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,9 @@ export function normalizeSubagentRunState(entry: SubagentRunRecord): SubagentRun
5656
entry.generation > 0
5757
? entry.generation
5858
: undefined;
59+
entry.deleteCleanupDispatchedAt = Number.isFinite(entry.deleteCleanupDispatchedAt)
60+
? entry.deleteCleanupDispatchedAt
61+
: undefined;
5962
entry.suppressCompletionDelivery = entry.suppressCompletionDelivery === true ? true : undefined;
6063
const killReconciliation = entry.killReconciliation;
6164
if (

‎src/agents/subagent-registry-lifecycle.test.ts‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -762,6 +762,67 @@ describe("subagent registry lifecycle hardening", () => {
762762
},
763763
);
764764

765+
it("rejects a yield after direct delete cleanup has been dispatched", async () => {
766+
const entry = createRunEntry({ cleanup: "delete", expectsCompletionMessage: false });
767+
const runs = new Map([[entry.runId, entry]]);
768+
let releaseDelete: (() => void) | undefined;
769+
gatewayMocks.callGateway.mockImplementation((opts) => {
770+
if (opts.method !== "sessions.delete") {
771+
return Promise.resolve({});
772+
}
773+
return new Promise<Record<string, unknown>>((resolve) => {
774+
releaseDelete = () => resolve({});
775+
});
776+
});
777+
const controller = createLifecycleController({ entry, runs });
778+
779+
await controller.completeSubagentRun({
780+
runId: entry.runId,
781+
endedAt: 4_000,
782+
outcome: { status: "ok" },
783+
reason: SUBAGENT_ENDED_REASON_COMPLETE,
784+
triggerCleanup: true,
785+
});
786+
await vi.waitFor(() => expect(entry.deleteCleanupDispatchedAt).toBeTypeOf("number"));
787+
788+
expect(markSubagentRunPausedAfterYield({ entry, endedAt: 4_001 })).toBe(false);
789+
expect(entry.pauseReason).toBeUndefined();
790+
expect(entry.endedReason).toBe(SUBAGENT_ENDED_REASON_COMPLETE);
791+
792+
releaseDelete?.();
793+
await vi.waitFor(() => expect(runs.has(entry.runId)).toBe(false));
794+
});
795+
796+
it("rejects a yield after announce cleanup hands off delete dispatch", async () => {
797+
const entry = createRunEntry({ cleanup: "delete", expectsCompletionMessage: true });
798+
const runs = new Map([[entry.runId, entry]]);
799+
let releaseAnnounce: (() => void) | undefined;
800+
const runSubagentAnnounceFlow: LifecycleControllerParams["runSubagentAnnounceFlow"] = vi.fn(
801+
(announceParams) =>
802+
new Promise<boolean>((resolve) => {
803+
expect(announceParams.onBeforeDeleteChildSession?.()).toBe(true);
804+
releaseAnnounce = () => resolve(true);
805+
}),
806+
);
807+
const controller = createLifecycleController({ entry, runs, runSubagentAnnounceFlow });
808+
809+
await controller.completeSubagentRun({
810+
runId: entry.runId,
811+
endedAt: 4_000,
812+
outcome: { status: "ok" },
813+
reason: SUBAGENT_ENDED_REASON_COMPLETE,
814+
triggerCleanup: true,
815+
});
816+
await vi.waitFor(() => expect(entry.deleteCleanupDispatchedAt).toBeTypeOf("number"));
817+
818+
expect(markSubagentRunPausedAfterYield({ entry, endedAt: 4_001 })).toBe(false);
819+
expect(entry.pauseReason).toBeUndefined();
820+
expect(entry.endedReason).toBe(SUBAGENT_ENDED_REASON_COMPLETE);
821+
822+
releaseAnnounce?.();
823+
await vi.waitFor(() => expect(runs.has(entry.runId)).toBe(false));
824+
});
825+
765826
it("discards completion capture when an authoritative yield arrives during the await", async () => {
766827
const entry = createRunEntry({ expectsCompletionMessage: true });
767828
let finishCapture: ((result: string) => void) | undefined;

‎src/agents/subagent-registry-lifecycle.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1193,6 +1193,10 @@ export function createSubagentRegistryLifecycleController(params: {
11931193
return;
11941194
}
11951195
if (cleanup === "delete") {
1196+
// This durable boundary prevents a late yield from reviving a run
1197+
// after deletion may already have reached the gateway.
1198+
entry.deleteCleanupDispatchedAt ??= Date.now();
1199+
params.persist();
11961200
await deleteSubagentSessionForCleanup({
11971201
callGateway: params.callGateway,
11981202
childSessionKey: entry.childSessionKey,
@@ -1289,6 +1293,19 @@ export function createSubagentRegistryLifecycleController(params: {
12891293
spawnMode: pendingPayload.spawnMode,
12901294
expectsCompletionMessage: pendingPayload.expectsCompletionMessage,
12911295
wakeOnDescendantSettle: pendingPayload.wakeOnDescendantSettle === true,
1296+
onBeforeDeleteChildSession:
1297+
cleanup === "delete"
1298+
? () => {
1299+
if (!isCleanupAttemptCurrent(runId, entry, cleanupGeneration)) {
1300+
return false;
1301+
}
1302+
// Announce owns delete submission; fence late yields at the
1303+
// exact handoff instead of when cleanup merely starts.
1304+
entry.deleteCleanupDispatchedAt ??= Date.now();
1305+
params.persist();
1306+
return true;
1307+
}
1308+
: undefined,
12921309
onDeliveryResult: (delivery) => {
12931310
if (!isCleanupAttemptCurrent(runId, entry, cleanupGeneration)) {
12941311
void retireSupersededCleanupIfNeeded(runId, entry, cleanupGeneration);

‎src/agents/subagent-registry-run-manager.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,10 +107,12 @@ export function markSubagentRunPausedAfterYield(params: {
107107
const { entry } = params;
108108
if (
109109
entry.endedReason === SUBAGENT_ENDED_REASON_KILLED ||
110-
entry.suppressAnnounceReason === "killed"
110+
entry.suppressAnnounceReason === "killed" ||
111+
(entry.cleanup === "delete" && Number.isFinite(entry.deleteCleanupDispatchedAt))
111112
) {
112113
// agent.wait and lifecycle events can report the old yield after control
113-
// killed the run. Cleanup rechecks pauseReason before destructive work.
114+
// killed the run. Once delete dispatch starts, reviving the row would expose
115+
// a live run whose backing session may already be gone.
114116
return false;
115117
}
116118
let mutated = false;
@@ -655,6 +657,7 @@ export function createSubagentRunManager(params: {
655657
pauseReason: undefined,
656658
endedHookEmittedAt: undefined,
657659
browserCleanupDispatchedAt: undefined,
660+
deleteCleanupDispatchedAt: undefined,
658661
wakeOnDescendantSettle: undefined,
659662
outcome: undefined,
660663
execution: {

‎src/agents/subagent-registry.types.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,8 @@ export type SubagentRunRecord = {
137137
endedHookEmittedAt?: number;
138138
/** Set after cleanupBrowserSessionsForLifecycleEnd has been dispatched once. */
139139
browserCleanupDispatchedAt?: number;
140+
/** Set immediately before irreversible sessions.delete cleanup is dispatched. */
141+
deleteCleanupDispatchedAt?: number;
140142
/** Durable outbox marker for parent/external completion delivery. */
141143
delivery?: SubagentCompletionDeliveryState;
142144
attachmentsDir?: string;

0 commit comments

Comments
 (0)