mirror of
https://github.com/openclaw/openclaw.git
synced 2026-10-03 09:39:25 +00:00
fix(browser): preserve successor tabs during stale cleanup (#155307)
Carry the current subagent cleanup owner through Browser activation, dashboard reconciliation, and tab claims. Let already admitted closes settle against their captured tab ownership without removing later registrations. Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
parent
2bf25ef263
commit
cc69772410
15 changed files with 395 additions and 35 deletions
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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<void>();
|
||||
const finish = createDeferred<void>();
|
||||
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) => {
|
||||
|
|
|
|||
|
|
@ -658,7 +658,11 @@ async function stopMaterializedDashboard(
|
|||
|
||||
/** Existing cleanup cycle reconciles dashboard removal, replacement, and explicit stop. */
|
||||
export async function reconcileBrowserDashboards(
|
||||
params: { sessionKeys?: Array<string | undefined>; onWarn?: (message: string) => void } = {},
|
||||
params: {
|
||||
sessionKeys?: Array<string | undefined>;
|
||||
isCurrent?: () => boolean;
|
||||
onWarn?: (message: string) => void;
|
||||
} = {},
|
||||
): Promise<number> {
|
||||
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 &&
|
||||
|
|
|
|||
|
|
@ -35,6 +35,8 @@ type CloseTab = (tab: {
|
|||
profile?: string;
|
||||
}) => Promise<void>;
|
||||
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<number> {
|
||||
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);
|
||||
|
|
|
|||
|
|
@ -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<void>();
|
||||
const finish = createDeferred<void>();
|
||||
const closeTab = vi.fn<CloseTab>(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");
|
||||
|
|
|
|||
|
|
@ -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<void>();
|
||||
const finish = createDeferred<void>();
|
||||
let current = true;
|
||||
const closeTab = vi.fn(async (_tab: { targetId: string }) => {
|
||||
started.resolve();
|
||||
await finish.promise;
|
||||
});
|
||||
const closeDurableTab: NonNullable<
|
||||
Parameters<typeof registry.closeTrackedBrowserTabsForSessions>[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) => {
|
||||
|
|
|
|||
|
|
@ -42,6 +42,7 @@ export type CloseTab = (tab: {
|
|||
}) => Promise<void>;
|
||||
|
||||
type CleanupParams = {
|
||||
isCurrent?: () => boolean;
|
||||
closeTab?: CloseTab;
|
||||
closeDurableTab?: (
|
||||
tab: DurableTab,
|
||||
|
|
|
|||
|
|
@ -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<string | undefined>; now?: number },
|
||||
): Promise<number> {
|
||||
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,
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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<void> {
|
||||
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<typeof createSubagentRegistryMockState>,
|
||||
"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<typeof surface>();
|
||||
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");
|
||||
});
|
||||
}
|
||||
|
|
@ -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([
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<void> {
|
||||
|
|
@ -35,6 +36,7 @@ export async function cleanupBrowserSessionsForLifecycleEnd(params: {
|
|||
cleanup: async () => {
|
||||
await closeTrackedBrowserTabsForSessions({
|
||||
sessionKeys,
|
||||
isCurrent: params.isCurrent,
|
||||
onWarn: params.onWarn,
|
||||
});
|
||||
},
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -6,6 +6,8 @@ export { movePathToTrash, type MovePathToTrashOptions } from "./browser-trash.js
|
|||
|
||||
type CloseTrackedBrowserTabsParams = {
|
||||
sessionKeys: Array<string | undefined>;
|
||||
/** Gates new cleanup claims; already claimed tabs retain their cleanup owner. */
|
||||
isCurrent?: () => boolean;
|
||||
closeTab?: (tab: { targetId: string; baseUrl?: string; profile?: string }) => Promise<void>;
|
||||
onWarn?: (message: string) => void;
|
||||
};
|
||||
|
|
@ -22,7 +24,7 @@ function hasRequestedSessionKeys(sessionKeys: Array<string | undefined>): boolea
|
|||
export async function closeTrackedBrowserTabsForSessions(
|
||||
params: CloseTrackedBrowserTabsParams,
|
||||
): Promise<number> {
|
||||
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);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue