diff --git a/docs/tools/subagents/thread-bound-sessions.md b/docs/tools/subagents/thread-bound-sessions.md index cfee9aa9f407..8f04e5d91eab 100644 --- a/docs/tools/subagents/thread-bound-sessions.md +++ b/docs/tools/subagents/thread-bound-sessions.md @@ -103,5 +103,9 @@ remain spawnable while inheriting defaults. - Auto-archive applies equally at every sub-agent depth. - Browser cleanup is separate from archive cleanup: tracked browser tabs/processes are best-effort closed when the run finishes, even if the transcript/session record is kept. +If a newer run takes over the same session, the older run stops claiming tabs for +cleanup. Cleanup already admitted for a tab still settles against that tab's +captured ownership; it does not remove a later registration. + The `subagent_ended` plugin hook is best-effort. Hook execution or plugin runtime loading failures are logged and do not abort sub-agent cleanup. diff --git a/extensions/browser/src/browser-dashboard.test.ts b/extensions/browser/src/browser-dashboard.test.ts index 4581f6380cca..a8418e06aa19 100644 --- a/extensions/browser/src/browser-dashboard.test.ts +++ b/extensions/browser/src/browser-dashboard.test.ts @@ -305,6 +305,56 @@ describe("Browser dashboard lifetime", () => { expect(browser.open).not.toHaveBeenCalled(); }); + it.each(["active tab", "Stop intent"] as const)( + "preserves a dashboard %s and successor tabs when cleanup becomes stale during a board read", + async (state) => { + if (state === "active tab") { + await requestBrowserDashboard(request); + } else { + await stopBrowserDashboard(request); + } + fixture.widgets = []; + const reading = createDeferred(); + const finish = createDeferred(); + fixture.readBoard.mockImplementationOnce(async () => { + reading.resolve(); + await finish.promise; + return { sessionKey, widgets: [] }; + }); + let current = true; + const closeTab = vi.fn(async () => {}); + const cleanup = closeTrackedBrowserTabsForSessions({ + sessionKeys: [sessionKey], + isCurrent: () => current, + closeTab, + }); + try { + await reading.promise; + current = false; + trackSessionBrowserTab({ + sessionKey, + targetId: "successor-tab", + profile: "openclaw", + ownership: durableOwnership("successor-tab"), + }); + const retained = getBrowserSessionTabStore().entries(); + finish.resolve(); + await expect(cleanup).resolves.toBe(0); + expect(getBrowserSessionTabStore().entries()).toEqual(retained); + expect(closeTab).not.toHaveBeenCalled(); + expect(browser.closeOwned).not.toHaveBeenCalled(); + + await expect( + closeTrackedBrowserTabsForSessions({ sessionKeys: [sessionKey], closeTab }), + ).resolves.toBe(state === "active tab" ? 2 : 1); + expect(getBrowserSessionTabStore().entries()).toEqual([]); + } finally { + finish.resolve(); + await cleanup; + } + }, + ); + it.each(["removed", "invalid URL", "invalid profile"] as const)( "reconciles %s definitions even when ordinary tab cleanup is disabled", async (condition) => { diff --git a/extensions/browser/src/browser-dashboard.ts b/extensions/browser/src/browser-dashboard.ts index 23839e3862be..f87c034f59d6 100644 --- a/extensions/browser/src/browser-dashboard.ts +++ b/extensions/browser/src/browser-dashboard.ts @@ -658,7 +658,11 @@ async function stopMaterializedDashboard( /** Existing cleanup cycle reconciles dashboard removal, replacement, and explicit stop. */ export async function reconcileBrowserDashboards( - params: { sessionKeys?: Array; onWarn?: (message: string) => void } = {}, + params: { + sessionKeys?: Array; + isCurrent?: () => boolean; + onWarn?: (message: string) => void; + } = {}, ): Promise { if (!getOptionalBrowserStateRuntime()?.gateway) { return 0; @@ -675,6 +679,9 @@ export async function reconcileBrowserDashboards( const definition = await readBrowserDashboardDefinition({ ...tab.dashboard, }); + if (params.isCurrent?.() === false) { + return closed; + } if (!definitionOwnsTab(definition, tab) || tab.dashboard.state === "released") { closed += (await releaseTab(tab, params)).closed; } else if (definition && tab.dashboard.state === "stopping") { @@ -701,6 +708,9 @@ export async function reconcileBrowserDashboards( } try { const definition = await readBrowserDashboardDefinition(intent); + if (params.isCurrent?.() === false) { + return closed; + } if ( !sameBrowserDashboardDefinition(intent, definition) || (definition && diff --git a/extensions/browser/src/browser/session-tab-cleanup-claim.ts b/extensions/browser/src/browser/session-tab-cleanup-claim.ts index efa54b1f1f00..e7e9a06aa131 100644 --- a/extensions/browser/src/browser/session-tab-cleanup-claim.ts +++ b/extensions/browser/src/browser/session-tab-cleanup-claim.ts @@ -35,6 +35,8 @@ type CloseTab = (tab: { profile?: string; }) => Promise; export type CloseParams = { + /** Gates new cleanup claims, without revoking an already admitted close. */ + isCurrent?: () => boolean; closeTab?: CloseTab; closeDurableTab?: ( tab: DurableTab, @@ -177,7 +179,11 @@ export async function closeDurableTab( now: number, cleanupKind: CleanupKind, ): Promise { - if (candidate.dashboard?.state === "active" || candidate.dashboard?.state === "stopped") { + if ( + params.isCurrent?.() === false || + candidate.dashboard?.state === "active" || + candidate.dashboard?.state === "stopped" + ) { return 0; } const tab = claimCleanup(candidate, now, cleanupKind); diff --git a/extensions/browser/src/browser/session-tab-registry.concurrent.test.ts b/extensions/browser/src/browser/session-tab-registry.concurrent.test.ts index ab5af1b1d692..85b521998716 100644 --- a/extensions/browser/src/browser/session-tab-registry.concurrent.test.ts +++ b/extensions/browser/src/browser/session-tab-registry.concurrent.test.ts @@ -1,3 +1,4 @@ +import { createDeferred } from "openclaw/plugin-sdk/extension-shared"; import { importFreshModule } from "openclaw/plugin-sdk/test-fixtures"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { CloseTab, RegistryModule } from "./session-tab-registry.sqlite.test-helpers.js"; @@ -30,6 +31,43 @@ describe("volatile session tab cleanup across Browser plugin bundles", () => { beforeEach(clearProcessLocalTabState); afterEach(clearProcessLocalTabState); + it("keeps a replacement registration when a waiting cleanup caller becomes stale", async () => { + const first = await freshRegistry("first-owner"); + const follower = await freshRegistry("waiting-owner"); + const tab = { + sessionKey: "agent:main:main", + targetId: "bridge-tab", + route: { kind: "browser-control", baseUrl: "http://127.0.0.1:9999" } as const, + profile: "remote", + }; + first.trackSessionBrowserTab(tab); + const started = createDeferred(); + const finish = createDeferred(); + const closeTab = vi.fn(async () => { + started.resolve(); + await finish.promise; + }); + let current = true; + const params = { sessionKeys: [tab.sessionKey], closeTab, isCurrent: () => current }; + const closing = first.closeTrackedBrowserTabsForSessions(params); + await started.promise; + follower.trackSessionBrowserTab(tab); + const waiting = follower.closeTrackedBrowserTabsForSessions(params); + try { + current = false; + finish.resolve(); + await expect(Promise.all([closing, waiting])).resolves.toEqual([1, 0]); + expect(closeTab).toHaveBeenCalledOnce(); + await expect( + follower.closeTrackedBrowserTabsForSessions({ sessionKeys: [tab.sessionKey], closeTab }), + ).resolves.toBe(1); + expect(closeTab).toHaveBeenCalledTimes(2); + } finally { + finish.resolve(); + await Promise.all([closing, waiting]); + } + }); + it("shares one close attempt and releases a failed reservation for retry", async () => { const first = await freshRegistry("first"); const duplicate = await freshRegistry("duplicate"); diff --git a/extensions/browser/src/browser/session-tab-registry.lifecycle-retry.test.ts b/extensions/browser/src/browser/session-tab-registry.lifecycle-retry.test.ts index 4dd5bbf1a69f..6e4de7fa4d74 100644 --- a/extensions/browser/src/browser/session-tab-registry.lifecycle-retry.test.ts +++ b/extensions/browser/src/browser/session-tab-registry.lifecycle-retry.test.ts @@ -1,10 +1,75 @@ -import { describe, expect, it } from "vitest"; +import { createDeferred } from "openclaw/plugin-sdk/extension-shared"; +import { describe, expect, it, vi } from "vitest"; import { installSessionTabRegistrySqliteHarness } from "./session-tab-registry.sqlite.test-harness.js"; import { durableOwnership as ownership } from "./session-tab-registry.sqlite.test-helpers.js"; -describe("durable session tab lifecycle retries", () => { +describe("session tab lifecycle cleanup", () => { const { freshRegistry, openStore } = installSessionTabRegistrySqliteHarness(); + it.each(["durable", "volatile"] as const)( + "settles admitted %s cleanup but stops claiming tabs when its caller changes", + async (kind) => { + const registry = await freshRegistry(`caller-generation-${kind}`); + const sessionKey = "agent:subagent:ended"; + for (const targetId of ["tab-a", "tab-b"]) { + registry.trackSessionBrowserTab({ + sessionKey, + targetId, + profile: "remote", + ...(kind === "durable" + ? { ownership: ownership(targetId) } + : { route: { kind: "browser-control", baseUrl: "http://127.0.0.1:9999" } as const }), + }); + } + const started = createDeferred(); + const finish = createDeferred(); + let current = true; + const closeTab = vi.fn(async (_tab: { targetId: string }) => { + started.resolve(); + await finish.promise; + }); + const closeDurableTab: NonNullable< + Parameters[0]["closeDurableTab"] + > = async (tab, options) => { + await closeTab({ targetId: tab.nativeTargetId }); + expect(options.shouldClose()).toBe(true); + return { status: "closed" }; + }; + const cleanup = registry.closeTrackedBrowserTabsForSessions({ + sessionKeys: [sessionKey], + isCurrent: () => current, + closeTab, + closeDurableTab, + }); + try { + await started.promise; + current = false; + finish.resolve(); + await expect(cleanup).resolves.toBe(1); + expect(closeTab).toHaveBeenCalledOnce(); + if (kind === "durable") { + expect(openStore().entries()).toHaveLength(1); + expect(openStore().entries()[0]?.value).not.toHaveProperty("cleanupAttemptToken"); + } + await expect( + registry.closeTrackedBrowserTabsForSessions({ + sessionKeys: [sessionKey], + closeTab, + closeDurableTab, + }), + ).resolves.toBe(1); + expect(closeTab.mock.calls.map(([tab]) => tab.targetId).toSorted()).toEqual([ + "tab-a", + "tab-b", + ]); + expect(openStore().entries()).toEqual([]); + } finally { + finish.resolve(); + await cleanup; + } + }, + ); + it.each([true, false])( "retries pending lifecycle cleanup with ordinary cleanup %s", async (ordinaryCleanup) => { diff --git a/extensions/browser/src/browser/session-tab-registry.sqlite.test-helpers.ts b/extensions/browser/src/browser/session-tab-registry.sqlite.test-helpers.ts index 0266977fa113..654d048b8de9 100644 --- a/extensions/browser/src/browser/session-tab-registry.sqlite.test-helpers.ts +++ b/extensions/browser/src/browser/session-tab-registry.sqlite.test-helpers.ts @@ -42,6 +42,7 @@ export type CloseTab = (tab: { }) => Promise; type CleanupParams = { + isCurrent?: () => boolean; closeTab?: CloseTab; closeDurableTab?: ( tab: DurableTab, diff --git a/extensions/browser/src/browser/session-tab-registry.ts b/extensions/browser/src/browser/session-tab-registry.ts index 0178986d5908..e6889e69f5d0 100644 --- a/extensions/browser/src/browser/session-tab-registry.ts +++ b/extensions/browser/src/browser/session-tab-registry.ts @@ -50,6 +50,9 @@ async function performVolatileCleanup( : undefined; }; while (true) { + if (params.isCurrent?.() === false) { + return 0; + } const current = resolveCurrent(); if (!current) { return 0; @@ -156,6 +159,9 @@ async function closeTrackedTabs( export async function closeTrackedBrowserTabsForSessions( params: CloseParams & { sessionKeys: Array; now?: number }, ): Promise { + if (params.isCurrent?.() === false) { + return 0; + } let dashboardClosed = 0; if ( readDurableTabs(params.onWarn).some((tab) => tab.dashboard) || @@ -164,6 +170,9 @@ export async function closeTrackedBrowserTabsForSessions( const { reconcileBrowserDashboards } = await import("../browser-dashboard.js"); dashboardClosed = await reconcileBrowserDashboards(params); } + if (params.isCurrent?.() === false) { + return dashboardClosed; + } const tabs = selectTrackedTabsForSessions({ durable: readDurableTabs(params.onWarn), sessionKeys: params.sessionKeys, diff --git a/src/agents/subagents/registry/subagent-registry-lifecycle-cleanup.ts b/src/agents/subagents/registry/subagent-registry-lifecycle-cleanup.ts index 72752db6692c..fba6d4311ef8 100644 --- a/src/agents/subagents/registry/subagent-registry-lifecycle-cleanup.ts +++ b/src/agents/subagents/registry/subagent-registry-lifecycle-cleanup.ts @@ -598,6 +598,8 @@ async function completeTerminalCleanup( try { await cleanupBrowserSessions({ sessionKeys: [entry.childSessionKey], + isCurrent: () => + isSessionEffectsOwnerCurrent() && !context.shouldSuppressSessionEffects(entry), onWarn: (msg) => params.warn(msg, { runId: entry.runId }), }); } catch (error) { diff --git a/src/agents/subagents/registry/subagent-registry.browser-cleanup.test-support.ts b/src/agents/subagents/registry/subagent-registry.browser-cleanup.test-support.ts new file mode 100644 index 000000000000..bd4ce30b035b --- /dev/null +++ b/src/agents/subagents/registry/subagent-registry.browser-cleanup.test-support.ts @@ -0,0 +1,160 @@ +import { expect, it, vi, type Mock } from "vitest"; +import { createDeferred } from "../../../../test/helpers/promise.js"; +import * as gatewayWorkAdmission from "../../../process/gateway-work-admission.js"; +import { AsyncWorkScope } from "../../../shared/async-work-scope.js"; +import type { SubagentRegistryHarness } from "../../subagent-test-fixtures.test-helpers.js"; +import type { createSubagentRegistryMockState } from "./subagent-registry.mock-state.test-support.js"; +import type { SubagentRunRecord } from "./subagent-registry.types.js"; + +function observeRootWork(): () => Promise { + const observations = [ + vi.spyOn(gatewayWorkAdmission, "runWithGatewayIndependentRootWorkContinuation"), + // Detached completion resolves its result before this scope drains and releases its root. + vi.spyOn(AsyncWorkScope.prototype, "run"), + ]; + return async () => { + const failures: unknown[] = []; + try { + for (const observation of observations) { + for (const result of observation.mock.results) { + try { + if (result.type === "throw") { + throw result.value; + } + await result.value; + } catch (error) { + failures.push(error); + } + } + } + } finally { + for (const observation of observations) { + observation.mockRestore(); + } + } + if (failures.length > 0) { + throw new AggregateError(failures, "Failed to settle subagent cleanup roots"); + } + }; +} + +export function registerBrowserCleanupBoundaryTests({ + getRegistry, + mocks, + loadBrowserMaintenanceSurface, + mockPendingAgentWait, + findRequesterRun, +}: { + getRegistry: () => SubagentRegistryHarness; + mocks: Pick< + ReturnType, + "cleanupBrowserSessionsForLifecycleEnd" | "runSubagentAnnounceFlow" + >; + loadBrowserMaintenanceSurface: Mock; + mockPendingAgentWait: () => void; + findRequesterRun: (runId: string) => SubagentRunRecord | undefined; +}): void { + it.each(["current owner", "replacement row", "newer child generation", "session reset"] as const)( + "rechecks the %s after browser activation in registered run completion", + async (owner) => { + const mod = getRegistry(); + const deps = await import("./subagent-registry-deps.js"); + const fixtureDeps = deps.subagentRegistryDeps; + mod.testing.setDepsForTest(); + mod.testing.setDepsForTest({ + ...fixtureDeps, + cleanupBrowserSessionsForLifecycleEnd: + deps.subagentRegistryDeps.cleanupBrowserSessionsForLifecycleEnd, + }); + const closeTrackedBrowserTabsForSessions = vi + .fn< + typeof import("../../../plugin-sdk/browser-maintenance.js").closeTrackedBrowserTabsForSessions + >() + .mockResolvedValue(1); + const surface = { closeTrackedBrowserTabsForSessions }; + const activation = createDeferred(); + const activationEntered = createDeferred(); + loadBrowserMaintenanceSurface.mockImplementationOnce(() => { + activationEntered.resolve(); + return activation.promise; + }); + const settleRootWork = observeRootWork(); + const childSessionKey = "agent:main:subagent:child"; + const runId = "run-browser-activation-old"; + const successorRunId = owner === "replacement row" ? runId : "run-browser-activation-new"; + + try { + mod.registerSubagentRun({ runId, childSessionKey, task: "finish browser work" }); + await activationEntered.promise; + expect(loadBrowserMaintenanceSurface).toHaveBeenCalledOnce(); + expect(gatewayWorkAdmission.getActiveGatewayRootWorkCount()).toBeGreaterThan(0); + + if (owner === "session reset") { + mod.prepareSubagentSessionCleanupRevocation(childSessionKey)(); + } else if (owner !== "current owner") { + mockPendingAgentWait(); + mod.registerSubagentRun({ + runId: successorRunId, + childSessionKey, + task: "continue using the same browser session", + }); + } + } finally { + activation.resolve(surface); + await settleRootWork(); + } + + expect(gatewayWorkAdmission.getActiveGatewayRootWorkCount()).toBe(0); + expect(closeTrackedBrowserTabsForSessions).toHaveBeenCalledTimes( + owner === "current owner" ? 1 : 0, + ); + if (owner === "current owner") { + expect(closeTrackedBrowserTabsForSessions).toHaveBeenCalledWith( + expect.objectContaining({ sessionKeys: [childSessionKey] }), + ); + expect(findRequesterRun(runId)?.cleanupCompletedAt).toBeTypeOf("number"); + } else if (owner === "session reset") { + expect(findRequesterRun(runId)?.execution).toMatchObject({ + status: "terminal", + suppressSessionEffects: true, + }); + } else { + expect(findRequesterRun(successorRunId)).toMatchObject({ + childSessionKey, + execution: { status: "running" }, + }); + expect(findRequesterRun(successorRunId)?.cleanupCompletedAt).toBeUndefined(); + } + }, + ); + + it("continues completion announce cleanup when lifecycle cleanup fails", async () => { + const cleanupEntered = createDeferred(); + mocks.cleanupBrowserSessionsForLifecycleEnd.mockImplementationOnce(async () => { + cleanupEntered.resolve(); + throw new Error("browser cleanup unavailable"); + }); + const settleRootWork = observeRootWork(); + try { + getRegistry().registerSubagentRun({ + runId: "run-cleanup-warning", + task: "finish despite cleanup warning", + }); + await cleanupEntered.promise; + } finally { + await settleRootWork(); + } + + expect(gatewayWorkAdmission.getActiveGatewayRootWorkCount()).toBe(0); + expect(mocks.cleanupBrowserSessionsForLifecycleEnd).toHaveBeenCalledOnce(); + expect(mocks.runSubagentAnnounceFlow).toHaveBeenCalledOnce(); + expect(mocks.runSubagentAnnounceFlow).toHaveBeenCalledWith( + expect.objectContaining({ + childSessionKey: "agent:main:subagent:child", + childRunId: "run-cleanup-warning", + task: "finish despite cleanup warning", + }), + ); + expect(findRequesterRun("run-cleanup-warning")?.cleanupCompletedAt).toBeTypeOf("number"); + }); +} diff --git a/src/agents/subagents/registry/subagent-registry.test.ts b/src/agents/subagents/registry/subagent-registry.test.ts index b2010afe87c6..e28fe8b30c5f 100644 --- a/src/agents/subagents/registry/subagent-registry.test.ts +++ b/src/agents/subagents/registry/subagent-registry.test.ts @@ -73,6 +73,7 @@ import { persistSubagentRunsToDiskOrThrow, restoreSubagentRunsFromDisk, } from "./subagent-registry-state.js"; +import { registerBrowserCleanupBoundaryTests } from "./subagent-registry.browser-cleanup.test-support.js"; import { findRecordCallArg } from "./subagent-registry.mock-call.test-support.js"; import { saveSubagentRegistryChangesToSqlite } from "./subagent-registry.store.sqlite.js"; import type { @@ -88,6 +89,12 @@ const mocks = await vi.hoisted(async () => { return createSubagentRegistryMockState(); }); +const loadBrowserMaintenanceSurface = vi.hoisted(() => vi.fn()); + +vi.mock("../../../plugin-sdk/facade-runtime.js", () => ({ + tryLoadActivatedBundledPluginPublicSurfaceModule: loadBrowserMaintenanceSurface, +})); + vi.mock("../../../gateway/call.js", () => ({ callGateway: mocks.callGateway, })); @@ -375,6 +382,7 @@ describe("subagent registry seam flow", () => { mocks.callGateway.mockReset(); mocks.captureSubagentCompletionReply.mockReset().mockResolvedValue("final completion reply"); mocks.cleanupBrowserSessionsForLifecycleEnd.mockReset().mockResolvedValue(undefined); + loadBrowserMaintenanceSurface.mockReset().mockResolvedValue(null); mocks.persistSubagentRunsToDisk.mockReset(); mocks.persistSubagentRunsToDiskOrThrow.mockReset(); mocks.restoreSubagentRunsFromDisk.mockReset().mockReturnValue(0); @@ -5733,33 +5741,12 @@ describe("subagent registry seam flow", () => { expect(findTaskByRunIdForStatus(runId)).toMatchObject({ status: "running" }); }); - it("continues completion announce cleanup when lifecycle cleanup fails", async () => { - mocks.cleanupBrowserSessionsForLifecycleEnd.mockRejectedValueOnce( - new Error("browser cleanup unavailable"), - ); - - mod.registerSubagentRun({ - runId: "run-cleanup-warning", - task: "finish despite cleanup warning", - }); - - await waitForFast(() => { - expect(mocks.runSubagentAnnounceFlow).toHaveBeenCalledTimes(1); - }); - - expect(mocks.cleanupBrowserSessionsForLifecycleEnd).toHaveBeenCalledTimes(1); - expectRecordFields( - getMockCallArg(mocks.runSubagentAnnounceFlow, 0, 0, "completion announce"), - { - childSessionKey: "agent:main:subagent:child", - childRunId: "run-cleanup-warning", - task: "finish despite cleanup warning", - }, - "completion announce params", - ); - - const run = findRequesterRun("run-cleanup-warning"); - expect(run?.cleanupCompletedAt).toBeTypeOf("number"); + registerBrowserCleanupBoundaryTests({ + getRegistry: () => mod, + mocks, + loadBrowserMaintenanceSurface, + mockPendingAgentWait, + findRequesterRun, }); it.each([ diff --git a/src/browser-lifecycle-cleanup.test.ts b/src/browser-lifecycle-cleanup.test.ts index a07d3be5f1e9..8796ee6c6541 100644 --- a/src/browser-lifecycle-cleanup.test.ts +++ b/src/browser-lifecycle-cleanup.test.ts @@ -17,16 +17,19 @@ describe("cleanupBrowserSessionsForLifecycleEnd", () => { it("normalizes session keys before closing browser sessions", async () => { const onWarn = vi.fn(); + const isCurrent = () => true; await expect( cleanupBrowserSessionsForLifecycleEnd({ sessionKeys: ["", " session-a ", "session-a", "session-b"], + isCurrent, onWarn, }), ).resolves.toBeUndefined(); expect(closeTrackedBrowserTabsForSessions).toHaveBeenCalledWith({ sessionKeys: ["session-a", "session-b"], + isCurrent, onWarn, }); }); diff --git a/src/browser-lifecycle-cleanup.ts b/src/browser-lifecycle-cleanup.ts index e021ac540eb9..8254f0901184 100644 --- a/src/browser-lifecycle-cleanup.ts +++ b/src/browser-lifecycle-cleanup.ts @@ -21,6 +21,7 @@ function isBrowserCleanupDisabled(cfg: OpenClawConfig | undefined): boolean { export async function cleanupBrowserSessionsForLifecycleEnd(params: { cfg?: OpenClawConfig; sessionKeys: string[]; + isCurrent?: () => boolean; onWarn?: (message: string) => void; onError?: (error: unknown) => void; }): Promise { @@ -35,6 +36,7 @@ export async function cleanupBrowserSessionsForLifecycleEnd(params: { cleanup: async () => { await closeTrackedBrowserTabsForSessions({ sessionKeys, + isCurrent: params.isCurrent, onWarn: params.onWarn, }); }, diff --git a/src/plugin-sdk/browser-maintenance.test.ts b/src/plugin-sdk/browser-maintenance.test.ts index 2ef4b8388d83..0fff9c2f340f 100644 --- a/src/plugin-sdk/browser-maintenance.test.ts +++ b/src/plugin-sdk/browser-maintenance.test.ts @@ -5,6 +5,7 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { createDeferred } from "../../test/helpers/promise.js"; const closeTrackedBrowserTabsForSessionsImpl = vi.hoisted(() => vi.fn()); const tryLoadActivatedBundledPluginPublicSurfaceModule = vi.hoisted(() => vi.fn()); @@ -142,13 +143,32 @@ describe("browser maintenance", () => { expect(closeTrackedBrowserTabsForSessionsImpl).toHaveBeenCalledTimes(1); }); - it("delegates cleanup through the browser maintenance surface", async () => { + it("does not dispatch cleanup after its owner changes during plugin activation", async () => { + const { promise, resolve } = createDeferred(); + tryLoadActivatedBundledPluginPublicSurfaceModule.mockImplementationOnce(async () => { + await promise; + return { closeTrackedBrowserTabsForSessions: closeTrackedBrowserTabsForSessionsImpl }; + }); + const { closeTrackedBrowserTabsForSessions } = await import("./browser-maintenance.js"); + let current = true; + const cleanup = closeTrackedBrowserTabsForSessions({ + sessionKeys: ["agent:main:test"], + isCurrent: () => current, + }); + expect(tryLoadActivatedBundledPluginPublicSurfaceModule).toHaveBeenCalledOnce(); + current = false; + resolve(); + await expect(cleanup).resolves.toBe(0); + expect(closeTrackedBrowserTabsForSessionsImpl).not.toHaveBeenCalled(); + }); + + it.each([undefined, () => true])("delegates cleanup with owner guard %s", async (isCurrent) => { closeTrackedBrowserTabsForSessionsImpl.mockResolvedValue(2); const { closeTrackedBrowserTabsForSessions } = await import("./browser-maintenance.js"); await expect( - closeTrackedBrowserTabsForSessions({ sessionKeys: ["agent:main:test"] }), + closeTrackedBrowserTabsForSessions({ sessionKeys: ["agent:main:test"], isCurrent }), ).resolves.toBe(2); expect(tryLoadActivatedBundledPluginPublicSurfaceModule).toHaveBeenCalledWith({ dirName: "browser", @@ -156,6 +176,7 @@ describe("browser maintenance", () => { }); expect(closeTrackedBrowserTabsForSessionsImpl).toHaveBeenCalledWith({ sessionKeys: ["agent:main:test"], + isCurrent, }); }); diff --git a/src/plugin-sdk/browser-maintenance.ts b/src/plugin-sdk/browser-maintenance.ts index 6220c573e93e..51fad2bdab44 100644 --- a/src/plugin-sdk/browser-maintenance.ts +++ b/src/plugin-sdk/browser-maintenance.ts @@ -6,6 +6,8 @@ export { movePathToTrash, type MovePathToTrashOptions } from "./browser-trash.js type CloseTrackedBrowserTabsParams = { sessionKeys: Array; + /** Gates new cleanup claims; already claimed tabs retain their cleanup owner. */ + isCurrent?: () => boolean; closeTab?: (tab: { targetId: string; baseUrl?: string; profile?: string }) => Promise; onWarn?: (message: string) => void; }; @@ -22,7 +24,7 @@ function hasRequestedSessionKeys(sessionKeys: Array): boolea export async function closeTrackedBrowserTabsForSessions( params: CloseTrackedBrowserTabsParams, ): Promise { - if (!hasRequestedSessionKeys(params.sessionKeys)) { + if (params.isCurrent?.() === false || !hasRequestedSessionKeys(params.sessionKeys)) { return 0; } @@ -37,7 +39,7 @@ export async function closeTrackedBrowserTabsForSessions( params.onWarn?.(`browser cleanup unavailable: ${String(error)}`); return 0; } - if (!surface) { + if (!surface || params.isCurrent?.() === false) { return 0; } return await surface.closeTrackedBrowserTabsForSessions(params);