fix(agents): scope browser-cleanup dispatch guard to the cleanup wrapper only

Feelw00 · Feelw00 · commit abbb467d63cf · 2026-05-29T16:13:58.000+09:00
ClawSweeper re-review flagged [P2] "Keep duplicate completions on the
finalization path": the browserCleanupDispatchedAt guard returned from
completeSubagentRun entirely, so a duplicate completion caller skipped
retireRunModeBundleMcpRuntime and startSubagentAnnounceCleanupFlow, not just
the browser-cleanup wrapper. On pre-fix main the duplicate caller still ran
that tail because its own cleanupBrowserSessionsForLifecycleEnd returns
immediately once takeTrackedTabsForSessionKeys has drained the tabs.

Scope the check-then-set guard to the cleanupBrowserSessions wrapper only.
retireRunModeBundleMcpRuntime (idempotent) and startSubagentAnnounceCleanupFlow
(single-caller via beginSubagentCleanup) now run for every completion caller,
restoring pre-fix tail behavior while still firing the tab-close IPC once.

Add a held-first-cleanup regression test: with the first caller parked inside
a pending cleanup promise, a second completeSubagentRun must still reach
retireSessionMcpRuntimeForSessionKey and runSubagentAnnounceFlow.

[AI-assisted]
diff --git a/src/agents/subagent-registry-lifecycle.test.ts b/src/agents/subagent-registry-lifecycle.test.ts
@@ -1346,4 +1346,58 @@ describe("subagent registry lifecycle hardening", () => {
     ).toHaveBeenCalledTimes(1);
     expect(entry.browserCleanupDispatchedAt).toBeTypeOf("number");
   });
+
+  it("drains the retire + announce tail for a duplicate completion held behind a slow first browser cleanup", async () => {
+    // The dispatch flag dedupes only the browser tab-close IPC. A duplicate
+    // completion caller must still reach retireRunModeBundleMcpRuntime and
+    // startSubagentAnnounceCleanupFlow while the first caller's cleanup
+    // promise is still pending, so a slow browser driver cannot strand
+    // completion delivery behind it.
+    const entry = createRunEntry({
+      expectsCompletionMessage: true,
+    });
+    const runSubagentAnnounceFlow = vi.fn(async () => true);
+    const controller = createLifecycleController({ entry, runSubagentAnnounceFlow });
+
+    let releaseFirstCleanup: (() => void) | undefined;
+    let firstCleanupEntered: (() => void) | undefined;
+    const firstCleanupEnteredPromise = new Promise<void>((resolve) => {
+      firstCleanupEntered = resolve;
+    });
+    browserLifecycleCleanupMocks.cleanupBrowserSessionsForLifecycleEnd.mockImplementationOnce(
+      () => {
+        firstCleanupEntered?.();
+        return new Promise<void>((resolve) => {
+          releaseFirstCleanup = resolve;
+        });
+      },
+    );
+
+    const completeParams = {
+      runId: entry.runId,
+      endedAt: 4_000,
+      outcome: { status: "ok" as const },
+      reason: SUBAGENT_ENDED_REASON_COMPLETE,
+      triggerCleanup: true,
+    };
+
+    // First caller takes the dispatch flag and parks inside the cleanup wrapper.
+    const firstCompletion = controller.completeSubagentRun(completeParams);
+    await firstCleanupEnteredPromise;
+
+    // Second caller observes the flag set, skips the cleanup wrapper, and must
+    // still drain the retire + announce tail without waiting on the first
+    // caller's still-pending cleanup.
+    await controller.completeSubagentRun(completeParams);
+
+    expect(
+      browserLifecycleCleanupMocks.cleanupBrowserSessionsForLifecycleEnd,
+    ).toHaveBeenCalledTimes(1);
+    expect(bundleMcpRuntimeMocks.retireSessionMcpRuntimeForSessionKey).toHaveBeenCalled();
+    expect(runSubagentAnnounceFlow).toHaveBeenCalled();
+
+    // Release the held first cleanup so the first caller can settle too.
+    releaseFirstCleanup?.();
+    await expect(firstCompletion).resolves.toBeUndefined();
+  });
 });
diff --git a/src/agents/subagent-registry-lifecycle.ts b/src/agents/subagent-registry-lifecycle.ts
@@ -1249,27 +1249,27 @@ export function createSubagentRegistryLifecycleController(params: {
 
     // registerSubagentRun fires both an in-process listener and a gateway
     // waitForSubagentCompletion RPC; both can reach this point for the same
-    // runId in embedded mode. Guard with a sync check-then-set so the browser
-    // driver tab-close IPC only fires once per completion.
-    if (entry.browserCleanupDispatchedAt !== undefined) {
-      return;
-    }
-    entry.browserCleanupDispatchedAt = Date.now();
-
-    try {
-      const cleanupBrowserSessions =
-        params.cleanupBrowserSessionsForLifecycleEnd ??
-        (await loadCleanupBrowserSessionsForLifecycleEnd());
-      await cleanupBrowserSessions({
-        sessionKeys: [entry.childSessionKey],
-        onWarn: (msg) => params.warn(msg, { runId: entry.runId }),
-      });
-    } catch (error) {
-      params.warn("failed to cleanup browser sessions for completed subagent", {
-        error: buildSafeLifecycleErrorMeta(error),
-        runId: maskRunId(completeParams.runId),
-        childSessionKey: maskSessionKey(entry.childSessionKey),
-      });
+    // runId in embedded mode. Dedupe only the browser driver tab-close IPC
+    // with a sync check-then-set. The retire + announce tail below must still
+    // run for every caller, so a slow or held first browser cleanup cannot
+    // strand a duplicate caller's completion behind it.
+    if (entry.browserCleanupDispatchedAt === undefined) {
+      entry.browserCleanupDispatchedAt = Date.now();
+      try {
+        const cleanupBrowserSessions =
+          params.cleanupBrowserSessionsForLifecycleEnd ??
+          (await loadCleanupBrowserSessionsForLifecycleEnd());
+        await cleanupBrowserSessions({
+          sessionKeys: [entry.childSessionKey],
+          onWarn: (msg) => params.warn(msg, { runId: entry.runId }),
+        });
+      } catch (error) {
+        params.warn("failed to cleanup browser sessions for completed subagent", {
+          error: buildSafeLifecycleErrorMeta(error),
+          runId: maskRunId(completeParams.runId),
+          childSessionKey: maskSessionKey(entry.childSessionKey),
+        });
+      }
     }
 
     try {
