fix(agents): dedupe subagent browser session cleanup

Feelw00 · web-flow · commit a9a86f788b08 · 2026-05-30T21:04:37.000+01:00
Deduplicate the browser lifecycle cleanup wrapper for embedded subagent completions while preserving retire and announce finalization for duplicate callers.\n\nAdds regression coverage for parallel completion callers and the held-first-cleanup duplicate-tail path.\n\nFixes #68668.\n\nCo-authored-by: Feelw00 <dhrtn1006@naver.com>
diff --git a/src/agents/subagent-registry-lifecycle.test.ts b/src/agents/subagent-registry-lifecycle.test.ts
@@ -1311,4 +1311,93 @@ describe("subagent registry lifecycle hardening", () => {
     ).not.toHaveBeenCalled();
     expect(runSubagentAnnounceFlow).not.toHaveBeenCalled();
   });
+
+  it("dedupes browser cleanup when two callers complete the same run in parallel", async () => {
+    // registerSubagentRun fires both an in-process listener (phase='end') and a
+    // gateway waitForSubagentCompletion RPC; in embedded mode both resolve to
+    // the same runId and call completeSubagentRun. Without a per-entry dispatch
+    // guard, cleanupBrowserSessionsForLifecycleEnd fires once per caller,
+    // duplicating browser driver tab-close IPC.
+    const entry = createRunEntry({
+      expectsCompletionMessage: false,
+    });
+    const runSubagentAnnounceFlow = vi.fn(async () => true);
+
+    const controller = createLifecycleController({
+      entry,
+      runSubagentAnnounceFlow,
+    });
+
+    const completeParams = {
+      runId: entry.runId,
+      endedAt: 4_000,
+      outcome: { status: "ok" as const },
+      reason: SUBAGENT_ENDED_REASON_COMPLETE,
+      triggerCleanup: true,
+    };
+
+    await Promise.all([
+      controller.completeSubagentRun(completeParams),
+      controller.completeSubagentRun(completeParams),
+    ]);
+
+    expect(
+      browserLifecycleCleanupMocks.cleanupBrowserSessionsForLifecycleEnd,
+    ).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
@@ -1247,20 +1247,29 @@ export function createSubagentRegistryLifecycleController(params: {
       return;
     }
 
-    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),
-      });
+    // registerSubagentRun fires both an in-process listener and a gateway
+    // waitForSubagentCompletion RPC; both can reach this point for the same
+    // 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 {
diff --git a/src/agents/subagent-registry-run-manager.ts b/src/agents/subagent-registry-run-manager.ts
@@ -594,6 +594,7 @@ export function createSubagentRunManager(params: {
       endedReason: undefined,
       pauseReason: undefined,
       endedHookEmittedAt: undefined,
+      browserCleanupDispatchedAt: undefined,
       wakeOnDescendantSettle: undefined,
       outcome: undefined,
       execution: {
diff --git a/src/agents/subagent-registry.types.ts b/src/agents/subagent-registry.types.ts
@@ -105,6 +105,8 @@ export type SubagentRunRecord = {
   completion?: SubagentCompletionState;
   /** Set after the subagent_ended hook has been emitted successfully once. */
   endedHookEmittedAt?: number;
+  /** Set after cleanupBrowserSessionsForLifecycleEnd has been dispatched once. */
+  browserCleanupDispatchedAt?: number;
   /** Durable outbox marker for parent/external completion delivery. */
   delivery?: SubagentCompletionDeliveryState;
   attachmentsDir?: string;
