diff --git a/apps/extension/src/session-manager/__tests__/agent-window.test.ts b/apps/extension/src/session-manager/__tests__/agent-window.test.ts index e99118c6..265afdd9 100644 --- a/apps/extension/src/session-manager/__tests__/agent-window.test.ts +++ b/apps/extension/src/session-manager/__tests__/agent-window.test.ts @@ -23,7 +23,7 @@ describe("chromeAgentWindowApi.ensureActiveTab", () => { query.mockResolvedValue([{ id: 7, active: false }]); update.mockResolvedValue({}); - await chromeAgentWindowApi.ensureActiveTab(100, AGENT_WINDOW_HOME); + await chromeAgentWindowApi.ensureActiveTab(100, AGENT_WINDOW_HOME, new Set([7])); expect(query).toHaveBeenCalledWith({ windowId: 100 }); expect(update).toHaveBeenCalledWith(7, { active: true }); @@ -34,7 +34,7 @@ describe("chromeAgentWindowApi.ensureActiveTab", () => { query.mockResolvedValue([]); create.mockResolvedValue({ id: 8 }); - await chromeAgentWindowApi.ensureActiveTab(100, AGENT_WINDOW_HOME); + await chromeAgentWindowApi.ensureActiveTab(100, AGENT_WINDOW_HOME, new Set()); expect(create).toHaveBeenCalledWith({ windowId: 100, @@ -43,6 +43,15 @@ describe("chromeAgentWindowApi.ensureActiveTab", () => { }); expect(update).not.toHaveBeenCalled(); }); + + it("creates its own home tab instead of adopting a tab opened by the user", async () => { + query.mockResolvedValue([{ id: 99, active: true }]); + create.mockResolvedValue({ id: 8 }); + const home = await chromeAgentWindowApi.ensureActiveTab(100, AGENT_WINDOW_HOME, new Set([7])); + expect(home).toBe(8); + expect(update).not.toHaveBeenCalled(); + expect(create).toHaveBeenCalledWith({ windowId: 100, url: AGENT_WINDOW_HOME, active: true }); + }); }); describe("chromeAgentWindowApi.create", () => { @@ -60,6 +69,14 @@ describe("chromeAgentWindowApi.create", () => { vi.unstubAllGlobals(); }); + it("returns initial tab identities from the creation result", async () => { + create.mockResolvedValue({ id: 100, tabs: [{ id: 7 }, { id: 8 }] }); + expect(await chromeAgentWindowApi.create(AGENT_WINDOW_HOME)).toEqual({ + windowId: 100, + initialTabIds: [7, 8], + }); + }); + it("focuses Agent Windows by default", async () => { await chromeAgentWindowApi.create(AGENT_WINDOW_HOME); diff --git a/apps/extension/src/session-manager/__tests__/disconnect-cleanup.test.ts b/apps/extension/src/session-manager/__tests__/disconnect-cleanup.test.ts index f35a2a2b..a5d7c677 100644 --- a/apps/extension/src/session-manager/__tests__/disconnect-cleanup.test.ts +++ b/apps/extension/src/session-manager/__tests__/disconnect-cleanup.test.ts @@ -8,7 +8,7 @@ describe("disconnect session cleanup", () => { let nextWindowId = 100; const manager = new SessionManager({ agentWindow: { - create: vi.fn(async () => nextWindowId++), + create: vi.fn(async () => ({ windowId: nextWindowId++, initialTabIds: [] })), ensureActiveTab: vi.fn(async () => 1), remove, }, @@ -40,7 +40,7 @@ describe("disconnect session cleanup", () => { }); const manager = new SessionManager({ agentWindow: { - create: vi.fn(async () => 100), + create: vi.fn(async () => ({ windowId: 100, initialTabIds: [] })), ensureActiveTab: vi.fn(async () => 1), remove: vi.fn(() => removeGate), }, diff --git a/apps/extension/src/session-manager/__tests__/event-handler.test.ts b/apps/extension/src/session-manager/__tests__/event-handler.test.ts index 40ba6b10..e7af4ac3 100644 --- a/apps/extension/src/session-manager/__tests__/event-handler.test.ts +++ b/apps/extension/src/session-manager/__tests__/event-handler.test.ts @@ -48,7 +48,7 @@ describe("attachSessionEventHandler", () => { }; const manager = new SessionManager({ agentWindow: { - create: vi.fn(async () => 4242), + create: vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })), remove: vi.fn(path === "window" ? emitAndFlush : async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -74,7 +74,7 @@ describe("attachSessionEventHandler", () => { const events = fakeWindowEvents(); const manager = new SessionManager({ agentWindow: { - create: vi.fn(async () => 4242), + create: vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })), remove: vi.fn(async () => { throw new Error("close failed"); }), @@ -100,7 +100,7 @@ describe("attachSessionEventHandler", () => { it("drops the local session and emits session.window_closed when the agent window closes", async () => { const manager = new SessionManager({ agentWindow: { - create: vi.fn(async () => 4242), + create: vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -134,7 +134,7 @@ describe("attachSessionEventHandler", () => { it("reports borrowed tabs as return failures when the Agent Window was already closed", async () => { const manager = new SessionManager({ agentWindow: { - create: vi.fn(async () => 4242), + create: vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -175,7 +175,7 @@ describe("attachSessionEventHandler", () => { it("ignores non-agent windows", async () => { const manager = new SessionManager({ agentWindow: { - create: vi.fn(async () => 1), + create: vi.fn(async () => ({ windowId: 1, initialTabIds: [] })), remove: vi.fn(), ensureActiveTab: vi.fn(async () => 1), }, @@ -191,7 +191,7 @@ describe("attachSessionEventHandler", () => { it("dispose() removes the listener", () => { const manager = new SessionManager({ agentWindow: { - create: vi.fn(async () => 1), + create: vi.fn(async () => ({ windowId: 1, initialTabIds: [] })), remove: vi.fn(), ensureActiveTab: vi.fn(async () => 1), }, diff --git a/apps/extension/src/session-manager/__tests__/manager.test.ts b/apps/extension/src/session-manager/__tests__/manager.test.ts index 5abd2a78..357414e7 100644 --- a/apps/extension/src/session-manager/__tests__/manager.test.ts +++ b/apps/extension/src/session-manager/__tests__/manager.test.ts @@ -1,5 +1,9 @@ import { describe, expect, it, vi } from "vitest"; -import type { AgentWindowApi, AgentWindowCreateOptions } from "../agent-window"; +import type { + AgentWindowApi, + AgentWindowCreateOptions, + AgentWindowCreation, +} from "../agent-window"; import { isAgentControlledTab, SessionManager } from "../manager"; function fakeAgentWindow(): AgentWindowApi & { @@ -10,7 +14,7 @@ function fakeAgentWindow(): AgentWindowApi & { let nextId = 100; const createMock = vi.fn(async (_url: string, _opts?: AgentWindowCreateOptions) => { const id = nextId++; - return id; + return { windowId: id, initialTabIds: [] }; }); const removeMock = vi.fn(async (_id: number) => {}); const ensureActiveTabMock = vi.fn(async (_windowId: number, _url: string) => 0); @@ -32,7 +36,7 @@ describe("SessionManager", () => { expect(aw.createMock).toHaveBeenCalledOnce(); expect(aw.createMock).toHaveBeenCalledWith("about:blank", {}); expect(aw.ensureActiveTabMock).toHaveBeenCalledOnce(); - expect(aw.ensureActiveTabMock).toHaveBeenCalledWith(100, "about:blank"); + expect(aw.ensureActiveTabMock).toHaveBeenCalledWith(100, "about:blank", expect.any(Set)); expect(ctx.sessionId).toBe("aa11"); expect(ctx.agentWindowId).toBe(100); expect(ctx.createdAtMs).toBe(1700000000000); @@ -77,10 +81,10 @@ describe("SessionManager", () => { it("removes a newly created Agent Window when startup is aborted", async () => { const aw = fakeAgentWindow(); - let resolveCreate: (windowId: number) => void = () => {}; + let resolveCreate: (result: AgentWindowCreation) => void = () => {}; aw.createMock.mockImplementationOnce( () => - new Promise((resolve) => { + new Promise((resolve) => { resolveCreate = resolve; }), ); @@ -89,7 +93,7 @@ describe("SessionManager", () => { const pending = sm.start("aa11", { signal: controller.signal }); controller.abort(); - resolveCreate(777); + resolveCreate({ windowId: 777, initialTabIds: [7] }); await expect(pending).rejects.toMatchObject({ name: "AbortError" }); expect(aw.removeMock).toHaveBeenCalledWith(777); @@ -107,7 +111,7 @@ describe("SessionManager", () => { expect(sm.has("aa11")).toBe(false); }); - it("surfaces the orphan Agent Window id when startup cleanup fails", async () => { + it("retains a failed startup window so stop can retry cleanup", async () => { const aw = fakeAgentWindow(); aw.ensureActiveTabMock.mockRejectedValueOnce(new Error("tab setup failed")); aw.removeMock.mockRejectedValueOnce(new Error("window removal denied")); @@ -118,7 +122,12 @@ describe("SessionManager", () => { windowId: 100, message: expect.stringMatching(/cleanup of Agent Window 100 failed.*window removal denied/), }); + expect(sm.has("aa11")).toBe(true); + expect(sm.findByWindowId(100)?.sessionId).toBe("aa11"); + await sm.stop("aa11"); + expect(aw.removeMock).toHaveBeenCalledTimes(2); expect(sm.has("aa11")).toBe(false); + expect(sm.findByWindowId(100)).toBeNull(); }); it("stop() closes the Agent Window and forgets the session", async () => { diff --git a/apps/extension/src/session-manager/agent-window.ts b/apps/extension/src/session-manager/agent-window.ts index 1336dc70..a45d0492 100644 --- a/apps/extension/src/session-manager/agent-window.ts +++ b/apps/extension/src/session-manager/agent-window.ts @@ -8,7 +8,7 @@ */ export interface AgentWindowApi { - create(url: string, opts?: AgentWindowCreateOptions): Promise; + create(url: string, opts?: AgentWindowCreateOptions): Promise; remove(windowId: number): Promise; /** * Guarantee the Agent Window has an active, CDP-navigable tab. @@ -18,7 +18,13 @@ export interface AgentWindowApi { * Resolves with the id of the activated (or newly created) tab, so callers * can track the session's home tab without re-querying Chrome. */ - ensureActiveTab(windowId: number, url: string): Promise; + ensureActiveTab(windowId: number, url: string, ownedTabIds: ReadonlySet): Promise; +} + +/** Resource identities captured from the creation result, before initialization. */ +export interface AgentWindowCreation { + windowId: number; + initialTabIds: number[]; } /** Creation hints for a new Agent Window. */ @@ -33,7 +39,7 @@ export interface AgentWindowCreateOptions { export const AGENT_WINDOW_HOME = "about:blank"; export const chromeAgentWindowApi: AgentWindowApi = { - async create(url: string, opts: AgentWindowCreateOptions = {}): Promise { + async create(url: string, opts: AgentWindowCreateOptions = {}): Promise { const win = await chrome.windows.create({ type: "normal", focused: opts.focused ?? true, @@ -43,7 +49,12 @@ export const chromeAgentWindowApi: AgentWindowApi = { if (typeof win?.id !== "number") { throw new Error("[bh] chrome.windows.create returned no window id"); } - return win.id; + return { + windowId: win.id, + initialTabIds: (win.tabs ?? []).flatMap((tab) => + typeof tab.id === "number" ? [tab.id] : [], + ), + }; }, async remove(windowId: number): Promise { // Callers decide whether a missing/failed removal is benign. In @@ -52,9 +63,15 @@ export const chromeAgentWindowApi: AgentWindowApi = { // cancellation success while the Agent Window remains open. await chrome.windows.remove(windowId); }, - async ensureActiveTab(windowId: number, url: string): Promise { + async ensureActiveTab( + windowId: number, + url: string, + ownedTabIds: ReadonlySet, + ): Promise { const tabs = await chrome.tabs.query({ windowId }); - const first = tabs.find((t) => typeof t.id === "number"); + // A user may have opened a tab while window initialization was pending. + // Reuse only a tab whose identity came from our creation result. + const first = tabs.find((t) => t.id !== undefined && ownedTabIds.has(t.id)); if (first?.id !== undefined) { if (!first.active) { await chrome.tabs.update(first.id, { active: true }); diff --git a/apps/extension/src/session-manager/manager.ts b/apps/extension/src/session-manager/manager.ts index 72cd3823..bc2f220c 100644 --- a/apps/extension/src/session-manager/manager.ts +++ b/apps/extension/src/session-manager/manager.ts @@ -226,11 +226,19 @@ export class SessionManager { throwIfSessionStartAborted(opts.signal); let windowId: number | null = null; + const agentCreatedTabs = new Set(); try { const { signal: _signal, ...createOptions } = opts; - windowId = await this.agentWindow.create(AGENT_WINDOW_HOME, createOptions); + const created = await this.agentWindow.create(AGENT_WINDOW_HOME, createOptions); + windowId = created.windowId; + for (const tabId of created.initialTabIds) agentCreatedTabs.add(tabId); throwIfSessionStartAborted(opts.signal); - const homeTabId = await this.agentWindow.ensureActiveTab(windowId, AGENT_WINDOW_HOME); + const homeTabId = await this.agentWindow.ensureActiveTab( + windowId, + AGENT_WINDOW_HOME, + agentCreatedTabs, + ); + agentCreatedTabs.add(homeTabId); throwIfSessionStartAborted(opts.signal); const ctx: SessionContext = { @@ -239,10 +247,10 @@ export class SessionManager { agentWindowId: windowId, refStore: new RefStore(), borrowedTabs: new Map(), - // The home tab is the session's first explicit claim. Every other - // tab remains free until `tab_create` or `tab_borrow` identifies it - // by its concrete Chrome tab id. - agentCreatedTabs: new Set([homeTabId]), + // Capture ownership at creation, before initialization can fail. + // Later tabs remain free until `tab_create` or `tab_borrow` identifies + // them by their concrete Chrome tab id. + agentCreatedTabs, createdAtMs: this.now(), }; this.sessions.set(sessionId, ctx); @@ -253,6 +261,19 @@ export class SessionManager { try { await this.agentWindow.remove(windowId); } catch (cleanupError) { + // The daemon may retry stop after a failed startup rollback. Retain + // the exact window handle until closure is confirmed. + const pending: SessionContext = { + ...(this.remote() ? { remote: true } : {}), + sessionId, + agentWindowId: windowId, + refStore: new RefStore(), + borrowedTabs: new Map(), + agentCreatedTabs, + createdAtMs: this.now(), + }; + this.sessions.set(sessionId, pending); + this.windowIndex.set(windowId, sessionId); throw new SessionStartCleanupError(windowId, startupError, cleanupError); } } diff --git a/apps/extension/src/tools/__tests__/background-execution.browser.test.ts b/apps/extension/src/tools/__tests__/background-execution.browser.test.ts index 4155fd29..0e90930b 100644 --- a/apps/extension/src/tools/__tests__/background-execution.browser.test.ts +++ b/apps/extension/src/tools/__tests__/background-execution.browser.test.ts @@ -133,7 +133,7 @@ describe.skipIf(!process.env.BSK_BACKGROUND_CHROME)( ); const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 1, }, diff --git a/apps/extension/src/tools/__tests__/background-execution.test.ts b/apps/extension/src/tools/__tests__/background-execution.test.ts index 0afb2472..5eb73bb1 100644 --- a/apps/extension/src/tools/__tests__/background-execution.test.ts +++ b/apps/extension/src/tools/__tests__/background-execution.test.ts @@ -6,7 +6,7 @@ import { prepareBackgroundExecution } from "../background-execution"; async function fixture() { const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 1, }, diff --git a/apps/extension/src/tools/__tests__/background-screenshot.browser.test.ts b/apps/extension/src/tools/__tests__/background-screenshot.browser.test.ts index 864b4f90..c6116fcf 100644 --- a/apps/extension/src/tools/__tests__/background-screenshot.browser.test.ts +++ b/apps/extension/src/tools/__tests__/background-screenshot.browser.test.ts @@ -128,7 +128,7 @@ describe.skipIf(!process.env.BSK_BACKGROUND_CHROME)( ); const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 1, }, diff --git a/apps/extension/src/tools/__tests__/click.browser.test.ts b/apps/extension/src/tools/__tests__/click.browser.test.ts index 537359a2..b0e7bea8 100644 --- a/apps/extension/src/tools/__tests__/click.browser.test.ts +++ b/apps/extension/src/tools/__tests__/click.browser.test.ts @@ -81,7 +81,7 @@ describe.skipIf(!process.env.BSK_CLICK_CHROME)("real browser click readiness", ( const foreground = await page(false); const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 4, }, diff --git a/apps/extension/src/tools/__tests__/console.test.ts b/apps/extension/src/tools/__tests__/console.test.ts index 650bf0d7..bc7b9ebb 100644 --- a/apps/extension/src/tools/__tests__/console.test.ts +++ b/apps/extension/src/tools/__tests__/console.test.ts @@ -10,7 +10,7 @@ function fakeAgentWindow(ids: number[]) { create: vi.fn(async () => { const id = ids[i++]; if (id === undefined) throw new Error("ran out of fake ids"); - return id; + return { windowId: id, initialTabIds: [] }; }), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), diff --git a/apps/extension/src/tools/__tests__/dispatcher.test.ts b/apps/extension/src/tools/__tests__/dispatcher.test.ts index 5446d64b..f860f1f5 100644 --- a/apps/extension/src/tools/__tests__/dispatcher.test.ts +++ b/apps/extension/src/tools/__tests__/dispatcher.test.ts @@ -65,7 +65,7 @@ describe("ToolDispatcher", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 4242), + create: vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })), remove: vi.fn(), ensureActiveTab: vi.fn(), }, @@ -98,7 +98,7 @@ describe("ToolDispatcher", () => { const sessions = new SessionManager({ remote: () => true, agentWindow: { - create: vi.fn(async () => 4242), + create: vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })), remove: vi.fn(), ensureActiveTab: vi.fn(async () => 1), }, @@ -118,7 +118,7 @@ describe("ToolDispatcher", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 4242), + create: vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })), remove: vi.fn(), ensureActiveTab: vi.fn(async () => 1), }, @@ -180,7 +180,7 @@ describe("ToolDispatcher", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 4242), + create: vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })), remove: vi.fn(), ensureActiveTab: vi.fn(async () => 1), }, @@ -203,7 +203,7 @@ describe("ToolDispatcher", () => { it("forwards an unfocused session start to the Agent Window", async () => { const { transport, deliver } = fakeTransport(); - const create = vi.fn(async () => 4242); + const create = vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })); const sessions = new SessionManager({ agentWindow: { create, @@ -224,7 +224,7 @@ describe("ToolDispatcher", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 4242), + create: vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -258,7 +258,7 @@ describe("ToolDispatcher", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 4242), + create: vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })), remove: closeWindow, ensureActiveTab: vi.fn(async () => 1), }, @@ -290,7 +290,7 @@ describe("ToolDispatcher", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 4242), + create: vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -353,7 +353,7 @@ describe("ToolDispatcher", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 4242), + create: vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -446,7 +446,7 @@ describe("ToolDispatcher", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 4242), + create: vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -482,7 +482,7 @@ describe("ToolDispatcher", () => { const remove = vi.fn(async () => {}); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 4242), + create: vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })), remove, ensureActiveTab: vi.fn(async () => 1), }, @@ -530,7 +530,7 @@ describe("ToolDispatcher", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 1), + create: vi.fn(async () => ({ windowId: 1, initialTabIds: [] })), remove: vi.fn(), ensureActiveTab: vi.fn(async () => 1), }, @@ -550,7 +550,7 @@ describe("ToolDispatcher", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 1), + create: vi.fn(async () => ({ windowId: 1, initialTabIds: [] })), remove: vi.fn(), ensureActiveTab: vi.fn(async () => 1), }, @@ -574,7 +574,7 @@ describe("ToolDispatcher", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 1), + create: vi.fn(async () => ({ windowId: 1, initialTabIds: [] })), remove: vi.fn(), ensureActiveTab: vi.fn(async () => 1), }, @@ -591,7 +591,7 @@ describe("ToolDispatcher", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 1), + create: vi.fn(async () => ({ windowId: 1, initialTabIds: [] })), remove: vi.fn(), ensureActiveTab: vi.fn(async () => 1), }, @@ -608,7 +608,7 @@ describe("ToolDispatcher", () => { const { transport, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 1), + create: vi.fn(async () => ({ windowId: 1, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -642,7 +642,7 @@ describe("ToolDispatcher", () => { }); const sessions = new SessionManager({ agentWindow: { - create: async () => 4242, + create: async () => ({ windowId: 4242, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 7, }, @@ -736,7 +736,7 @@ describe("ToolDispatcher", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 1), + create: vi.fn(async () => ({ windowId: 1, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -777,7 +777,7 @@ describe("ToolDispatcher", () => { const { transport } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 1), + create: vi.fn(async () => ({ windowId: 1, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -859,7 +859,7 @@ describe("ToolDispatcher", () => { const { transport } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 1), + create: vi.fn(async () => ({ windowId: 1, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -926,7 +926,7 @@ describe("ToolDispatcher", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 100), + create: vi.fn(async () => ({ windowId: 100, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -981,7 +981,7 @@ describe("ToolDispatcher", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 100), + create: vi.fn(async () => ({ windowId: 100, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -1049,7 +1049,7 @@ describe("ToolDispatcher", () => { const { transport } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 1), + create: vi.fn(async () => ({ windowId: 1, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -1086,7 +1086,7 @@ describe("ToolDispatcher", () => { const { transport } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 4242), + create: vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -1138,7 +1138,7 @@ describe("ToolDispatcher", () => { const { transport } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 1), + create: vi.fn(async () => ({ windowId: 1, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -1172,7 +1172,7 @@ describe("ToolDispatcher", () => { const { transport, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 4242), + create: vi.fn(async () => ({ windowId: 4242, initialTabIds: [] })), remove: vi.fn(), ensureActiveTab: vi.fn(async () => 1), }, @@ -1194,7 +1194,7 @@ describe("ToolDispatcher", () => { const remove = vi.fn(async () => {}); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 5555), + create: vi.fn(async () => ({ windowId: 5555, initialTabIds: [] })), remove, ensureActiveTab: vi.fn(async () => 1), }, @@ -1222,7 +1222,7 @@ describe("ToolDispatcher", () => { }); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(() => createPromise), + create: vi.fn(async () => ({ windowId: await createPromise, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, @@ -1273,7 +1273,7 @@ describe("ToolDispatcher", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(async () => 1), + create: vi.fn(async () => ({ windowId: 1, initialTabIds: [] })), remove: vi.fn(), ensureActiveTab: vi.fn(async () => 1), }, @@ -1294,7 +1294,7 @@ describe("ToolDispatcher", () => { }); const sessions = new SessionManager({ agentWindow: { - create: vi.fn(() => createPromise), + create: vi.fn(async () => ({ windowId: await createPromise, initialTabIds: [] })), remove: vi.fn(), ensureActiveTab: vi.fn(async () => 1), }, @@ -1331,7 +1331,7 @@ describe("background execution dispatch integration", () => { const { transport, sent, deliver } = fakeTransport(); const sessions = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 1, }, diff --git a/apps/extension/src/tools/__tests__/emulate.test.ts b/apps/extension/src/tools/__tests__/emulate.test.ts index d8e27afe..7353edb8 100644 --- a/apps/extension/src/tools/__tests__/emulate.test.ts +++ b/apps/extension/src/tools/__tests__/emulate.test.ts @@ -17,7 +17,7 @@ function fakeAgentWindow(ids: number[]) { create: vi.fn(async () => { const id = ids[i++]; if (id === undefined) throw new Error("ran out of fake ids"); - return id; + return { windowId: id, initialTabIds: [] }; }), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), diff --git a/apps/extension/src/tools/__tests__/evaluate.test.ts b/apps/extension/src/tools/__tests__/evaluate.test.ts index 0065499f..7b9d7c92 100644 --- a/apps/extension/src/tools/__tests__/evaluate.test.ts +++ b/apps/extension/src/tools/__tests__/evaluate.test.ts @@ -9,7 +9,7 @@ function fakeAgentWindow(ids: number[]) { create: vi.fn(async () => { const id = ids[i++]; if (id === undefined) throw new Error("ran out of fake ids"); - return id; + return { windowId: id, initialTabIds: [] }; }), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), diff --git a/apps/extension/src/tools/__tests__/file-transfer.test.ts b/apps/extension/src/tools/__tests__/file-transfer.test.ts index 740630ee..9b2f2a1b 100644 --- a/apps/extension/src/tools/__tests__/file-transfer.test.ts +++ b/apps/extension/src/tools/__tests__/file-transfer.test.ts @@ -11,7 +11,7 @@ import { handleUpload } from "../upload"; function sessions() { return new SessionManager({ agentWindow: { - create: vi.fn(async () => 100), + create: vi.fn(async () => ({ windowId: 100, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }, diff --git a/apps/extension/src/tools/__tests__/fill.test.ts b/apps/extension/src/tools/__tests__/fill.test.ts index c7dba296..19f95503 100644 --- a/apps/extension/src/tools/__tests__/fill.test.ts +++ b/apps/extension/src/tools/__tests__/fill.test.ts @@ -13,7 +13,7 @@ async function setup(markup = '') { const element = document.body.firstElementChild as HTMLInputElement; const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 4, }, diff --git a/apps/extension/src/tools/__tests__/focus.browser.test.ts b/apps/extension/src/tools/__tests__/focus.browser.test.ts index d7e91d10..63510dcb 100644 --- a/apps/extension/src/tools/__tests__/focus.browser.test.ts +++ b/apps/extension/src/tools/__tests__/focus.browser.test.ts @@ -53,7 +53,7 @@ async function withFocusBrowser( ); const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 4, }, diff --git a/apps/extension/src/tools/__tests__/focus.test.ts b/apps/extension/src/tools/__tests__/focus.test.ts index 60d50938..8038c99f 100644 --- a/apps/extension/src/tools/__tests__/focus.test.ts +++ b/apps/extension/src/tools/__tests__/focus.test.ts @@ -20,7 +20,7 @@ async function setup(element?: HTMLElement, childSession = false) { const targetElement = element; const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 4, }, diff --git a/apps/extension/src/tools/__tests__/interaction.test.ts b/apps/extension/src/tools/__tests__/interaction.test.ts index 4ad2fd53..31479a7d 100644 --- a/apps/extension/src/tools/__tests__/interaction.test.ts +++ b/apps/extension/src/tools/__tests__/interaction.test.ts @@ -21,7 +21,7 @@ function fakeAgentWindow(ids: number[]) { create: vi.fn(async () => { const id = ids[i++]; if (id === undefined) throw new Error("ran out of fake ids"); - return id; + return { windowId: id, initialTabIds: [] }; }), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), diff --git a/apps/extension/src/tools/__tests__/navigation-recovery.test.ts b/apps/extension/src/tools/__tests__/navigation-recovery.test.ts index 442d0f22..3ee2fbdc 100644 --- a/apps/extension/src/tools/__tests__/navigation-recovery.test.ts +++ b/apps/extension/src/tools/__tests__/navigation-recovery.test.ts @@ -27,7 +27,7 @@ function event void>() { async function fixture() { const manager = new SessionManager({ agentWindow: { - create: vi.fn(async () => 100), + create: vi.fn(async () => ({ windowId: 100, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 4), }, diff --git a/apps/extension/src/tools/__tests__/navigation.test.ts b/apps/extension/src/tools/__tests__/navigation.test.ts index fef78586..641cd168 100644 --- a/apps/extension/src/tools/__tests__/navigation.test.ts +++ b/apps/extension/src/tools/__tests__/navigation.test.ts @@ -18,7 +18,7 @@ function fakeAgentWindow(ids: number[]) { create: vi.fn(async () => { const id = ids[i++]; if (id === undefined) throw new Error("ran out of fake ids"); - return id; + return { windowId: id, initialTabIds: [] }; }), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), diff --git a/apps/extension/src/tools/__tests__/network.test.ts b/apps/extension/src/tools/__tests__/network.test.ts index 8af1793a..41d1f02c 100644 --- a/apps/extension/src/tools/__tests__/network.test.ts +++ b/apps/extension/src/tools/__tests__/network.test.ts @@ -9,7 +9,7 @@ function fakeAgentWindow(ids: number[]) { create: vi.fn(async () => { const id = ids[i++]; if (id === undefined) throw new Error("ran out of fake ids"); - return id; + return { windowId: id, initialTabIds: [] }; }), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), diff --git a/apps/extension/src/tools/__tests__/observation.test.ts b/apps/extension/src/tools/__tests__/observation.test.ts index 3de5f5b3..dfcae6bc 100644 --- a/apps/extension/src/tools/__tests__/observation.test.ts +++ b/apps/extension/src/tools/__tests__/observation.test.ts @@ -53,7 +53,7 @@ function fakeAgentWindow(ids: number[]) { create: vi.fn(async () => { const id = ids[i++]; if (id === undefined) throw new Error("ran out of fake ids"); - return id; + return { windowId: id, initialTabIds: [] }; }), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), diff --git a/apps/extension/src/tools/__tests__/screenshot-full-page.test.ts b/apps/extension/src/tools/__tests__/screenshot-full-page.test.ts index 4b2de8dc..84a5bbff 100644 --- a/apps/extension/src/tools/__tests__/screenshot-full-page.test.ts +++ b/apps/extension/src/tools/__tests__/screenshot-full-page.test.ts @@ -28,7 +28,7 @@ afterEach(() => { async function setup() { const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 7, }, diff --git a/apps/extension/src/tools/__tests__/scroll.browser.test.ts b/apps/extension/src/tools/__tests__/scroll.browser.test.ts index deb1a416..b94eb7dd 100644 --- a/apps/extension/src/tools/__tests__/scroll.browser.test.ts +++ b/apps/extension/src/tools/__tests__/scroll.browser.test.ts @@ -41,7 +41,7 @@ async function harness(send: Send) { await send("Page.bringToFront", {}, rootSession); const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 4, }, diff --git a/apps/extension/src/tools/__tests__/scroll.test.ts b/apps/extension/src/tools/__tests__/scroll.test.ts index e41a119e..6314e193 100644 --- a/apps/extension/src/tools/__tests__/scroll.test.ts +++ b/apps/extension/src/tools/__tests__/scroll.test.ts @@ -16,7 +16,7 @@ interface Call { async function fixture(frame: "top" | "same-target" | "oopif" = "top") { const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 4, }, diff --git a/apps/extension/src/tools/__tests__/session.test.ts b/apps/extension/src/tools/__tests__/session.test.ts index 18c90853..c0a612a2 100644 --- a/apps/extension/src/tools/__tests__/session.test.ts +++ b/apps/extension/src/tools/__tests__/session.test.ts @@ -27,7 +27,7 @@ function fakeAgentWindow(ids: number[]) { const create = vi.fn(async () => { const id = ids[i++]; if (id === undefined) throw new Error("ran out of fake ids"); - return id; + return { windowId: id, initialTabIds: [] }; }); const remove = vi.fn(async () => {}); const ensureActiveTab = vi.fn(async () => 0); diff --git a/apps/extension/src/tools/__tests__/shared.test.ts b/apps/extension/src/tools/__tests__/shared.test.ts index c8a05795..970ecb4c 100644 --- a/apps/extension/src/tools/__tests__/shared.test.ts +++ b/apps/extension/src/tools/__tests__/shared.test.ts @@ -12,7 +12,7 @@ import { function fakeAgentWindow() { return { - create: vi.fn(async () => 100), + create: vi.fn(async () => ({ windowId: 100, initialTabIds: [] })), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), }; diff --git a/apps/extension/src/tools/__tests__/snapshot-ref.test.ts b/apps/extension/src/tools/__tests__/snapshot-ref.test.ts index 8c9b0ae7..13f43e97 100644 --- a/apps/extension/src/tools/__tests__/snapshot-ref.test.ts +++ b/apps/extension/src/tools/__tests__/snapshot-ref.test.ts @@ -8,7 +8,7 @@ function fakeAgentWindow(ids: number[]) { create: async () => { const id = ids[i++]; if (id === undefined) throw new Error("ran out of fake ids"); - return id; + return { windowId: id, initialTabIds: [] }; }, remove: async () => {}, ensureActiveTab: async () => 1, diff --git a/apps/extension/src/tools/__tests__/startup-cleanup.test.ts b/apps/extension/src/tools/__tests__/startup-cleanup.test.ts new file mode 100644 index 00000000..e8aef4c3 --- /dev/null +++ b/apps/extension/src/tools/__tests__/startup-cleanup.test.ts @@ -0,0 +1,104 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { SessionManager } from "@/session-manager/manager"; +import { handleSessionStop } from "../session"; + +afterEach(() => vi.unstubAllGlobals()); + +function browser() { + const liveTabs = new Map([ + [7, { id: 7, windowId: 100, active: true } as chrome.tabs.Tab], + ]); + const tabs = { + query: vi.fn(async () => [...liveTabs.values()]), + remove: vi.fn(async (id: number) => { + liveTabs.delete(id); + }), + update: vi.fn(), + create: vi.fn(), + get: vi.fn(async (id: number) => liveTabs.get(id)!), + move: vi.fn(), + }; + const windows = { + create: vi.fn(async () => ({ id: 100, tabs: [...liveTabs.values()] })), + remove: vi.fn(async () => { + liveTabs.clear(); + }), + }; + vi.stubGlobal("chrome", { tabs, windows }); + return { liveTabs, tabs, windows, manager: new SessionManager() }; +} + +describe("failed startup through the production stop handler", () => { + it.each([ + false, + true, + ])("retains the initial tab when startup fails (cancel=%s)", async (cancel) => { + const b = browser(); + const controller = new AbortController(); + if (cancel) { + b.windows.create.mockImplementationOnce(async () => { + controller.abort(); + return { id: 100, tabs: [...b.liveTabs.values()] }; + }); + } else { + b.tabs.query.mockRejectedValueOnce(new Error("initialization failed")); + } + b.windows.remove.mockRejectedValueOnce(new Error("rollback close failed")); + await expect(b.manager.start("broken", { signal: controller.signal })).rejects.toMatchObject({ + name: "SessionStartCleanupError", + }); + const result = await handleSessionStop( + b.manager, + { session_id: "broken" }, + { + tabManagement: { tabs: b.tabs }, + tabsQuery: b.tabs, + }, + ); + expect(result).not.toHaveProperty("window_released", true); + expect(b.tabs.remove).toHaveBeenCalledWith(7); + expect(b.windows.remove).toHaveBeenCalledTimes(2); + expect(b.liveTabs.size).toBe(0); + expect(b.manager.has("broken")).toBe(false); + }); + + it("removes the initial tab while preserving a later user tab", async () => { + const b = browser(); + b.tabs.query.mockRejectedValueOnce(new Error("initialization failed")); + b.windows.remove.mockRejectedValueOnce(new Error("rollback close failed")); + await expect(b.manager.start("broken")).rejects.toThrow(/cleanup/); + b.liveTabs.set(99, { id: 99, windowId: 100 } as chrome.tabs.Tab); + const result = await handleSessionStop( + b.manager, + { session_id: "broken" }, + { + tabManagement: { tabs: b.tabs }, + tabsQuery: b.tabs, + }, + ); + expect(result).toMatchObject({ window_released: true }); + expect([...b.liveTabs.keys()]).toEqual([99]); + expect(b.windows.remove).toHaveBeenCalledTimes(1); + expect(b.manager.has("broken")).toBe(false); + }); + + it("retains cleanup responsibility when an agent tab cannot close beside a user tab", async () => { + const b = browser(); + b.tabs.query.mockRejectedValueOnce(new Error("initialization failed")); + b.windows.remove.mockRejectedValueOnce(new Error("rollback close failed")); + await expect(b.manager.start("broken")).rejects.toThrow(/cleanup/); + b.liveTabs.set(99, { id: 99, windowId: 100 } as chrome.tabs.Tab); + b.tabs.remove.mockRejectedValueOnce(new Error("tab close failed")); + const deps = { tabManagement: { tabs: b.tabs }, tabsQuery: b.tabs }; + const failed = await handleSessionStop(b.manager, { session_id: "broken" }, deps); + expect(failed).toMatchObject({ code: "protocol_error", data: { reason: "cleanup_failed" } }); + expect(b.manager.has("broken")).toBe(true); + expect([...b.liveTabs.keys()]).toEqual([7, 99]); + expect(b.windows.remove).toHaveBeenCalledTimes(1); + expect(await handleSessionStop(b.manager, { session_id: "broken" }, deps)).toMatchObject({ + window_released: true, + }); + expect([...b.liveTabs.keys()]).toEqual([99]); + expect(b.manager.has("broken")).toBe(false); + }); +}); diff --git a/apps/extension/src/tools/__tests__/tabs.test.ts b/apps/extension/src/tools/__tests__/tabs.test.ts index 5a7239e3..976d63ba 100644 --- a/apps/extension/src/tools/__tests__/tabs.test.ts +++ b/apps/extension/src/tools/__tests__/tabs.test.ts @@ -23,7 +23,7 @@ function fakeAgentWindow(ids: number[]) { create: vi.fn(async () => { const id = ids[i++]; if (id === undefined) throw new Error("ran out of fake window ids"); - return id; + return { windowId: id, initialTabIds: [] }; }), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), diff --git a/apps/extension/src/tools/__tests__/visual-ref.test.ts b/apps/extension/src/tools/__tests__/visual-ref.test.ts index df686d91..c6d3b4a4 100644 --- a/apps/extension/src/tools/__tests__/visual-ref.test.ts +++ b/apps/extension/src/tools/__tests__/visual-ref.test.ts @@ -33,7 +33,7 @@ function visual(): VisualRefInput { async function setup() { const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 4, }, diff --git a/apps/extension/src/tools/__tests__/visual-screenshot.test.ts b/apps/extension/src/tools/__tests__/visual-screenshot.test.ts index c32fc53b..0ee769e2 100644 --- a/apps/extension/src/tools/__tests__/visual-screenshot.test.ts +++ b/apps/extension/src/tools/__tests__/visual-screenshot.test.ts @@ -447,7 +447,7 @@ describe("visual screenshot", () => { const f = fixture(); const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 4, }, @@ -471,7 +471,7 @@ async function pointFixture(child = false, oopif = false) { const f = fixture(child, oopif); const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 4, }, diff --git a/apps/extension/src/tools/__tests__/waits.test.ts b/apps/extension/src/tools/__tests__/waits.test.ts index ce0f3aaa..2165aa1e 100644 --- a/apps/extension/src/tools/__tests__/waits.test.ts +++ b/apps/extension/src/tools/__tests__/waits.test.ts @@ -9,7 +9,7 @@ function fakeAgentWindow(ids: number[]) { create: vi.fn(async () => { const id = ids[i++]; if (id === undefined) throw new Error("ran out of fake ids"); - return id; + return { windowId: id, initialTabIds: [] }; }), remove: vi.fn(async () => {}), ensureActiveTab: vi.fn(async () => 1), diff --git a/apps/extension/src/tools/__tests__/wheel.browser.test.ts b/apps/extension/src/tools/__tests__/wheel.browser.test.ts index 600d3dea..56499cd9 100644 --- a/apps/extension/src/tools/__tests__/wheel.browser.test.ts +++ b/apps/extension/src/tools/__tests__/wheel.browser.test.ts @@ -41,7 +41,7 @@ async function harness(send: Send) { await send("Page.bringToFront", {}, rootSession); const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 4, }, diff --git a/apps/extension/src/tools/__tests__/wheel.test.ts b/apps/extension/src/tools/__tests__/wheel.test.ts index b1733f31..b9fa8ecc 100644 --- a/apps/extension/src/tools/__tests__/wheel.test.ts +++ b/apps/extension/src/tools/__tests__/wheel.test.ts @@ -16,7 +16,7 @@ interface Call { async function fixture(frame: "top" | "same-target" | "oopif" = "top") { const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 4, }, diff --git a/apps/extension/src/tools/__tests__/window.test.ts b/apps/extension/src/tools/__tests__/window.test.ts index a52b4abc..cf4e8950 100755 --- a/apps/extension/src/tools/__tests__/window.test.ts +++ b/apps/extension/src/tools/__tests__/window.test.ts @@ -9,7 +9,7 @@ function fakeAgentWindow(ids: number[]) { const create = vi.fn(async () => { const id = ids[i++]; if (id === undefined) throw new Error("ran out of fake ids"); - return id; + return { windowId: id, initialTabIds: [] }; }); const remove = vi.fn(async () => {}); const ensureActiveTab = vi.fn(async () => 1); diff --git a/apps/extension/src/tools/session.ts b/apps/extension/src/tools/session.ts index 4e0220b8..000366d4 100644 --- a/apps/extension/src/tools/session.ts +++ b/apps/extension/src/tools/session.ts @@ -341,6 +341,19 @@ export async function handleSessionStop( const leakedAgentTabs = liveWindowTabs.filter( (t) => t.id !== undefined && ctx.agentCreatedTabs.has(t.id), ); + if (leakedAgentTabs.length > 0 && userTabs.length > 0) { + // Neither releasing ownership nor closing this mixed window is safe. + // Keep the binding so a later stop can retry only the agent tabs. + return rpcError( + "protocol_error", + "cleanup_failed", + "Agent tabs could not be closed; user tabs were preserved and cleanup can be retried", + { + resource_type: "agent_window", + resource_id: ctx.agentWindowId, + }, + ); + } if (leakedAgentTabs.length > 0) { console.warn( `[bsk session_stop] ${leakedAgentTabs.length} agent tab(s) failed to close; forcing window close instead of release`, diff --git a/apps/extension/src/tools/vom/__tests__/visual-observation.test.ts b/apps/extension/src/tools/vom/__tests__/visual-observation.test.ts index dc6d4a4d..7ee1db2f 100644 --- a/apps/extension/src/tools/vom/__tests__/visual-observation.test.ts +++ b/apps/extension/src/tools/vom/__tests__/visual-observation.test.ts @@ -461,7 +461,7 @@ it("observe discovers and registers visual refs from its single production captu const index = (s: string) => strings.indexOf(s); const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 4, }, @@ -702,7 +702,7 @@ it.each([ const f = fixture(35, 100); const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 4, }, @@ -866,7 +866,7 @@ it("preserves DOM labels for audit on the first and continuation pages", async ( f.output.render = prepareObservationRender(f.scene); const manager = new SessionManager({ agentWindow: { - create: async () => 100, + create: async () => ({ windowId: 100, initialTabIds: [] }), remove: async () => {}, ensureActiveTab: async () => 4, }, diff --git a/crates/bsk-cli/src/cli/session.rs b/crates/bsk-cli/src/cli/session.rs index fd66580b..1ecbdfb7 100644 --- a/crates/bsk-cli/src/cli/session.rs +++ b/crates/bsk-cli/src/cli/session.rs @@ -46,6 +46,8 @@ pub enum SessionSub { Stop(SessionStopArgs), /// List active sessions. List, + /// Inspect, claim, or cancel a recoverable start request. + Request(SessionRequestArgs), } #[derive(Debug, Clone, Args)] @@ -53,6 +55,9 @@ pub struct SessionStartArgs { /// Deprecated compatibility flag. Automation settings in the extension take precedence. #[arg(long)] pub unattended: bool, + /// Recoverable request token: :. Valid for at most ten minutes. + #[arg(long)] + pub request_id: Option, /// Optional task name displayed in local operation history. #[arg(long)] pub name: Option, @@ -101,8 +106,21 @@ pub struct SessionStopArgs { pub all: bool, } +#[derive(Debug, Clone, Args)] +pub struct SessionRequestArgs { + pub request_id: String, + #[arg(long, conflicts_with_all = ["claim", "prepare"])] + pub cancel: bool, + #[arg(long, conflicts_with = "claim")] + pub prepare: bool, + #[arg(long)] + pub claim: bool, +} + #[derive(Debug, Serialize)] struct StartParams { + #[serde(skip_serializing_if = "Option::is_none")] + request_id: Option, #[serde(skip_serializing_if = "Option::is_none")] task_name: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -159,11 +177,37 @@ pub fn dispatch(cmd: SessionCmd, format: Format) -> Result<(), CliError> { let info = ensure_daemon().context("ensure daemon is running")?; match cmd.sub { SessionSub::Start(args) => { - run_skill_sync_for_session_start(format); + // Embedded clients use their own tool instructions. Keep recoverable + // lifecycle requests free of unrelated harness filesystem writes. + if args.request_id.is_none() { + run_skill_sync_for_session_start(format); + } run_start(info.sock_path, args, format) } SessionSub::Stop(args) => run_stop(info.sock_path, args, format), SessionSub::List => run_list(info.sock_path, format), + SessionSub::Request(args) => { + let action = if args.prepare { + "prepare" + } else if args.cancel { + "cancel" + } else if args.claim { + "claim" + } else { + "status" + }; + let reply: serde_json::Value = call( + info.sock_path, + Method::SessionRequest, + Some(serde_json::json!({"request_id": args.request_id, "action": action})), + Duration::from_secs(40), + )?; + println!( + "{}", + serde_json::to_string_pretty(&reply).context("encode request status")? + ); + Ok(()) + } } } @@ -196,6 +240,7 @@ fn run_start(sock: PathBuf, args: SessionStartArgs, format: Format) -> Result<() sock, SessionStartOptions { name: args.name, + request_id: args.request_id, browser: args.browser, width: args.width, height: args.height, @@ -231,6 +276,7 @@ fn run_start(sock: PathBuf, args: SessionStartArgs, format: Format) -> Result<() /// (focused window, browser-chosen size). #[derive(Debug, Default, Clone)] pub struct SessionStartOptions { + pub request_id: Option, pub name: Option, pub browser: Option, pub width: Option, @@ -242,8 +288,13 @@ pub struct SessionStartOptions { pub fn start_session(sock: PathBuf, opts: SessionStartOptions) -> Result { call( sock, - Method::SessionStart, + if opts.request_id.is_some() { + Method::SessionStartTracked + } else { + Method::SessionStart + }, Some(StartParams { + request_id: opts.request_id, task_name: opts.name, browser_instance_id: opts.browser, width: opts.width, @@ -580,6 +631,7 @@ mod start_params_tests { fn start_params_send_task_name_without_policy_overrides() { for task_name in [None, Some("Check settings".to_string())] { let params = StartParams { + request_id: None, task_name: task_name.clone(), browser_instance_id: None, width: None, diff --git a/crates/bsk-cli/src/daemon/ipc.rs b/crates/bsk-cli/src/daemon/ipc.rs index 69daf58c..288a0d3f 100644 --- a/crates/bsk-cli/src/daemon/ipc.rs +++ b/crates/bsk-cli/src/daemon/ipc.rs @@ -44,7 +44,7 @@ use super::abort::AbortRegistry; use super::queue::{DEFAULT_TOOL_TIMEOUT, DispatchError}; use super::sessions::{ AgentWindowOptions, SessionId, StartSessionError, StopSessionError, snapshot_status_entries, - start_session, stop_session, + stop_session, }; use super::state::{DAEMON_VERSION, DaemonState, PROTOCOL_VERSION}; @@ -236,10 +236,16 @@ pub fn full_handler(status: DaemonStatus, state: Arc) -> RpcHandler Ok(v) => ResponseBody::Ok(v), Err(e) => ResponseBody::Err(e), }, - Method::SessionStart => match handle_session_start(&state, rpc_id, params).await { - Ok(v) => ResponseBody::Ok(v), - Err(e) => ResponseBody::Err(e), - }, + Method::SessionStartTracked => { + super::session_requests::start(&state, rpc_id, params).await + } + Method::SessionRequest => super::session_requests::operate(&state, params).await, + Method::SessionStart => { + match handle_session_start(&state, rpc_id, params, false).await { + Ok(v) => ResponseBody::Ok(v), + Err(e) => ResponseBody::Err(e), + } + } Method::SessionStop => match handle_session_stop(&state, rpc_id, params).await { Ok(v) => ResponseBody::Ok(v), Err(e) => ResponseBody::Err(e), @@ -906,10 +912,11 @@ fn clamp_browser_wait(wait_ms: Option) -> Option { )) } -async fn handle_session_start( +pub(super) async fn handle_session_start( state: &Arc, rpc_id: RpcId, params: Value, + recoverable: bool, ) -> Result { let task_name = params .get("task_name") @@ -951,7 +958,7 @@ async fn handle_session_start( }); } }; - match start_session( + match super::sessions::start_session_recoverable( &state.browsers, &state.sessions, &state.tool_queues, @@ -963,6 +970,7 @@ async fn handle_session_start( state.config.extension_connect_wait, DEFAULT_RPC_TIMEOUT, Some(cancel), + recoverable, ) .await { @@ -978,7 +986,12 @@ async fn handle_session_start( }; Ok(serde_json::to_value(result).unwrap_or(Value::Null)) } - Err(err) => Err(map_start_error(err)), + Err(err) => { + if recoverable && let StartSessionError::CleanupFailed { session_id, .. } = &err { + state.tool_queues.spawn(session_id.clone()); + } + Err(map_start_error(err)) + } } } diff --git a/crates/bsk-cli/src/daemon/mod.rs b/crates/bsk-cli/src/daemon/mod.rs index c8be0666..4f0c9cfd 100644 --- a/crates/bsk-cli/src/daemon/mod.rs +++ b/crates/bsk-cli/src/daemon/mod.rs @@ -14,6 +14,7 @@ pub(crate) mod probe; pub mod queue; pub mod remote; pub mod session_interrupt; +pub mod session_requests; pub mod sessions; pub mod start; pub mod state; diff --git a/crates/bsk-cli/src/daemon/session_requests.rs b/crates/bsk-cli/src/daemon/session_requests.rs new file mode 100644 index 00000000..56380349 --- /dev/null +++ b/crates/bsk-cli/src/daemon/session_requests.rs @@ -0,0 +1,586 @@ +//! Recoverable start operations. The caller knows the token before any browser +//! side effect; IPC delivery is never proof of ownership or cleanup. +//! +//! Tokens contain an admission deadline and a UUID. Expired tokens cannot start +//! again, so terminal tombstones can be collected without resurrecting delayed +//! requests. Unclaimed starts are cancelled at that deadline. Ordinary sessions +//! and claimed requests retain the existing session idle policy. + +use std::collections::HashMap; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, Mutex}; +use std::time::{Duration, SystemTime, UNIX_EPOCH}; + +use bsk_protocol::{ErrorCode, ResponseBody, RpcError}; +use serde_json::{Value, json}; +use tokio::sync::{Mutex as AsyncMutex, watch}; + +use super::sessions::{Session, SessionId, stop_session}; +use super::state::DaemonState; + +const MAX_ADMISSION_MS: u64 = 10 * 60 * 1000; +const MAX_REQUESTS: usize = 8192; + +#[derive(Debug, Default)] +pub struct SessionRequests(Mutex>>); + +#[derive(Debug)] +struct StartRequest { + id: String, + expires: u64, + params: Mutex, + started: AtomicBool, + data: Mutex, + finished: watch::Sender, + cleanup: AsyncMutex<()>, + reaping: AtomicBool, +} + +#[derive(Debug, Default)] +struct RequestData { + cancelled: bool, + claimed: bool, + closed: bool, + session: Option, + result: Option, + cleanup_error: Option, +} + +fn now_ms() -> u64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_millis() as u64 +} + +fn error(code: ErrorCode, message: impl Into) -> ResponseBody { + ResponseBody::Err(RpcError { + code, + message: message.into(), + data: None, + }) +} + +fn token_expiry(id: &str) -> Option { + let (expiry, nonce) = id.split_once(':')?; + uuid::Uuid::parse_str(nonce).ok()?; + expiry.parse().ok() +} + +impl StartRequest { + fn new(id: String, expires: u64, params: Value, cancelled: bool) -> Self { + Self { + id, + expires, + params: Mutex::new(params), + started: AtomicBool::new(false), + data: Mutex::new(RequestData { + cancelled, + closed: cancelled, + ..Default::default() + }), + finished: watch::channel(cancelled).0, + cleanup: AsyncMutex::new(()), + reaping: AtomicBool::new(false), + } + } + + fn rpc_id(&self) -> String { + format!("session-request:{}", self.id) + } + + fn snapshot(&self, state: &DaemonState) -> Value { + let mut data = self.data.lock().unwrap(); + if data.result.is_some() + && data + .session + .as_ref() + .is_some_and(|s| !same_session(state, s)) + { + data.session = None; + data.closed = true; + } + let phase = if data.closed { + "closed" + } else if data.cleanup_error.is_some() { + "cleanup_failed" + } else if data.cancelled { + "cancelling" + } else if !self.started.load(Ordering::SeqCst) { + "prepared" + } else if data.result.is_none() { + "starting" + } else if matches!(data.result, Some(ResponseBody::Err(_))) { + "failed" + } else if data.claimed { + "active" + } else { + "ready" + }; + json!({ + "request_id": self.id, "state": phase, + "session": data.session.as_ref().map(Session::status_entry), + "cleanup_error": data.cleanup_error, + }) + } +} + +fn same_session(state: &DaemonState, session: &Session) -> bool { + state.sessions.get(&session.id).is_some_and(|current| { + current.browser_id == session.browser_id + && current.created_at_ms == session.created_at_ms + && current.agent_window_id == session.agent_window_id + }) +} + +pub(super) async fn start( + state: &Arc, + rpc_id: String, + mut params: Value, +) -> ResponseBody { + let Some(id) = params + .get("request_id") + .and_then(Value::as_str) + .map(str::to_owned) + else { + return error(ErrorCode::InvalidParams, "request_id is required"); + }; + let Some(expires) = token_expiry(&id) else { + return error( + ErrorCode::InvalidParams, + "request_id must be :", + ); + }; + let now = now_ms(); + if expires <= now || expires > now.saturating_add(MAX_ADMISSION_MS) { + return error( + ErrorCode::InvalidParams, + "request expired or admission deadline exceeds ten minutes; use a new request_id", + ); + } + params.as_object_mut().unwrap().remove("request_id"); + let (entry, fresh) = { + let requests = state.session_requests.0.lock().unwrap(); + let Some(entry) = requests.get(&id) else { + return error( + ErrorCode::InvalidParams, + "request is not prepared or the daemon restarted; prepare a new request before starting", + ); + }; + let cancelled = entry.data.lock().unwrap().cancelled; + let fresh = !cancelled && !entry.started.swap(true, Ordering::SeqCst); + let mut original = entry.params.lock().unwrap(); + if fresh { + *original = params.clone(); + } else if !cancelled && *original != params { + return error( + ErrorCode::InvalidParams, + "request_id already used with different start parameters", + ); + } + (entry.clone(), fresh) + }; + if fresh { + let state = state.clone(); + let entry = entry.clone(); + // Owned by the daemon, not the IPC reader or a plugin process. + tokio::spawn(async move { + let cancelled = entry.data.lock().unwrap().cancelled; + let result = if cancelled { + error(ErrorCode::Cancelled, "start request cancelled") + } else { + match super::ipc::handle_session_start(&state, entry.rpc_id(), params, true).await { + Ok(value) => ResponseBody::Ok(value), + Err(err) => ResponseBody::Err(err), + } + }; + { + let mut data = entry.data.lock().unwrap(); + let session_id = match &result { + ResponseBody::Ok(value) => value.get("session_id").and_then(Value::as_str), + ResponseBody::Err(err) => err + .data + .as_ref() + .and_then(|v| v.get("session_id")) + .and_then(Value::as_str), + }; + data.session = session_id.and_then(|id| state.sessions.get(&SessionId(id.into()))); + if matches!(result, ResponseBody::Err(_)) { + data.cancelled = true; + } + data.result = Some(result); + } + entry.finished.send_replace(true); + if entry.data.lock().unwrap().cancelled { + cleanup(&state, &entry).await; + } + }); + } + let guard = match state.abort_registry.register(rpc_id) { + Ok(guard) => guard, + Err(_) => return error(ErrorCode::InvalidParams, "duplicate start RPC id"), + }; + let mut finished = entry.finished.subscribe(); + tokio::select! { + _ = async { let _ = finished.wait_for(|done| *done).await; } => {}, + _ = guard.token().cancelled() => { cancel(state, &entry).await; }, + } + entry.snapshot(state); + let data = entry.data.lock().unwrap(); + if let Some(ResponseBody::Err(err)) = &data.result { + return ResponseBody::Err(err.clone()); + } + if data.cancelled || data.closed { + error( + ErrorCode::Cancelled, + "start request cancelled; inspect session request for cleanup status", + ) + } else { + data.result + .clone() + .unwrap_or_else(|| error(ErrorCode::Cancelled, "start request cancelled")) + } +} + +pub(super) async fn operate(state: &Arc, params: Value) -> ResponseBody { + let Some(id) = params.get("request_id").and_then(Value::as_str) else { + return error(ErrorCode::InvalidParams, "request_id is required"); + }; + let Some(expires) = token_expiry(id) else { + return error(ErrorCode::InvalidParams, "invalid request_id"); + }; + let action = params + .get("action") + .and_then(Value::as_str) + .unwrap_or("status"); + if !matches!(action, "status" | "prepare" | "cancel" | "claim") { + return error( + ErrorCode::InvalidParams, + "action must be status, prepare, cancel, or claim", + ); + } + let entry = { + let mut requests = state.session_requests.0.lock().unwrap(); + if let Some(entry) = requests.get(id) { + Some(entry.clone()) + } else if matches!(action, "prepare" | "cancel") && expires > now_ms() { + if expires > now_ms().saturating_add(MAX_ADMISSION_MS) || requests.len() >= MAX_REQUESTS + { + return error( + ErrorCode::InvalidParams, + "cannot reserve cancellation tombstone", + ); + } + // Cancellation before start is terminal, not a no-op. + let entry = Arc::new(StartRequest::new( + id.into(), + expires, + Value::Null, + action == "cancel", + )); + requests.insert(id.into(), entry.clone()); + Some(entry) + } else { + None + } + }; + let Some(entry) = entry else { + return if action == "claim" { + error(ErrorCode::NotFound, "start request not found") + } else { + ResponseBody::Ok( + json!({"request_id": id, "state": if expires <= now_ms() {"closed"} else {"unknown"}, "session": null}), + ) + }; + }; + if action == "cancel" { + cancel(state, &entry).await; + } + if action == "claim" { + let mut data = entry.data.lock().unwrap(); + if data.cancelled + || data.closed + || (entry.expires <= now_ms() && !data.claimed) + || !matches!(data.result, Some(ResponseBody::Ok(_))) + || !data + .session + .as_ref() + .is_some_and(|s| same_session(state, s)) + { + return error(ErrorCode::Cancelled, "start request cannot be claimed"); + } + data.claimed = true; + } + ResponseBody::Ok(entry.snapshot(state)) +} + +async fn cancel(state: &Arc, entry: &Arc) { + { + let mut data = entry.data.lock().unwrap(); + data.cancelled = true; + if !entry.started.load(Ordering::SeqCst) { + data.closed = true; + entry.finished.send_replace(true); + } + } + state.abort_registry.cancel(&entry.rpc_id()); + let mut finished = entry.finished.subscribe(); + if tokio::time::timeout(Duration::from_secs(5), finished.wait_for(|done| *done)) + .await + .is_ok() + { + cleanup(state, entry).await; + } + // If start is still settling its worker will perform cleanup itself. +} + +async fn cleanup(state: &Arc, entry: &StartRequest) { + let _serial = entry.cleanup.lock().await; + let session = entry.data.lock().unwrap().session.clone(); + let result = match session { + Some(session) if same_session(state, &session) => stop_session( + &state.browsers, + &state.sessions, + &state.tool_queues, + &state.session_interrupts, + &session.id, + Duration::from_secs(10), + None, + ) + .await + .map(|_| state.transfers.release_session(&session.id.0)) + .map_err(|err| err.to_string()), + _ => Ok(()), + }; + let mut data = entry.data.lock().unwrap(); + match result { + Ok(()) => { + data.closed = true; + data.session = None; + data.cleanup_error = None; + } + Err(err) => { + data.cleanup_error = Some(err); + } + } +} + +/// Shares the daemon's existing reaper. Failed cleanup remains retryable, and a +/// dead caller cannot keep an unclaimed request alive by losing its reply. +pub(super) fn reap(state: &Arc) { + let now = now_ms(); + let entries: Vec<_> = state + .session_requests + .0 + .lock() + .unwrap() + .values() + .cloned() + .collect(); + for entry in entries { + entry.snapshot(state); + let (closed, needs_cleanup) = { + let data = entry.data.lock().unwrap(); + ( + data.closed, + !data.closed && (data.cancelled || (!data.claimed && entry.expires <= now)), + ) + }; + if closed && entry.expires <= now { + state.session_requests.0.lock().unwrap().remove(&entry.id); + } else if needs_cleanup && !entry.reaping.swap(true, Ordering::SeqCst) { + let state = state.clone(); + tokio::spawn(async move { + cancel(&state, &entry).await; + entry.reaping.store(false, Ordering::SeqCst); + }); + } + } +} + +#[cfg(test)] +mod tests { + use super::super::start::DaemonConfig; + use super::*; + + fn state() -> Arc { + Arc::new(DaemonState::new(DaemonConfig::new(0))) + } + fn id(expires: u64) -> String { + format!("{expires}:{}", uuid::Uuid::new_v4()) + } + fn value(body: ResponseBody) -> Value { + match body { + ResponseBody::Ok(v) => v, + _ => panic!("{body:?}"), + } + } + + #[tokio::test] + async fn cancelled_before_prepare_cannot_be_resurrected() { + let state = state(); + let id = id(now_ms() + 60_000); + assert_eq!( + value(operate(&state, json!({"request_id": id, "action":"cancel"})).await)["state"], + "closed" + ); + assert_eq!( + value(operate(&state, json!({"request_id": id, "action":"prepare"})).await)["state"], + "closed" + ); + assert!(matches!( + start(&state, "late".into(), json!({"request_id":id})).await, + ResponseBody::Err(RpcError { + code: ErrorCode::Cancelled, + .. + }) + )); + } + + #[tokio::test] + async fn daemon_restart_cannot_accept_an_old_prepared_start() { + let old = state(); + let id = id(now_ms() + 60_000); + assert_eq!( + value(operate(&old, json!({"request_id":id,"action":"prepare"})).await)["state"], + "prepared" + ); + let new = state(); + assert!(matches!( + start(&new, "late".into(), json!({"request_id":id})).await, + ResponseBody::Err(RpcError { + code: ErrorCode::InvalidParams, + .. + }) + )); + assert!(new.sessions.is_empty()); + } + + #[tokio::test] + async fn expired_unclaimed_requests_are_cancelled_and_tombstones_can_be_collected() { + let state = state(); + let expires = now_ms() - 1; + let id = id(expires); + let entry = Arc::new(StartRequest::new(id.clone(), expires, Value::Null, false)); + state + .session_requests + .0 + .lock() + .unwrap() + .insert(id.clone(), entry.clone()); + reap(&state); + tokio::task::yield_now().await; + assert_eq!(entry.snapshot(&state)["state"], "closed"); + reap(&state); + assert!(!state.session_requests.0.lock().unwrap().contains_key(&id)); + assert!(matches!( + start(&state, "late".into(), json!({"request_id":id})).await, + ResponseBody::Err(_) + )); + } + + #[tokio::test] + async fn malformed_and_far_future_tokens_never_allocate_requests() { + let state = state(); + for id in [ + "anything".to_string(), + id(now_ms() + MAX_ADMISSION_MS + 60_000), + ] { + assert!(matches!( + operate(&state, json!({"request_id":id,"action":"prepare"})).await, + ResponseBody::Err(_) + )); + assert!(matches!( + start(&state, "bad".into(), json!({"request_id":id})).await, + ResponseBody::Err(_) + )); + } + assert!(state.session_requests.0.lock().unwrap().is_empty()); + } +} + +#[cfg(test)] +mod ownership_tests { + use super::super::{browsers::BrowserId, start::DaemonConfig}; + use super::*; + + fn session(id: &str, window: i64) -> Session { + Session { + id: SessionId(id.into()), + browser_id: BrowserId("browser".into()), + agent_window_id: Some(window), + created_at_ms: window, + interaction: None, + } + } + + #[tokio::test] + async fn lease_reaps_ready_requests_but_keeps_claimed_sessions() { + let state = Arc::new(DaemonState::new(DaemonConfig::new(0))); + let mut entries = Vec::new(); + for claimed in [false, true] { + let session = session( + if claimed { "live" } else { "lost" }, + if claimed { 2 } else { 1 }, + ); + state.sessions.insert(session.clone()); + let expires = now_ms() - 1; + let id = format!("{expires}:{}", uuid::Uuid::new_v4()); + let entry = Arc::new(StartRequest::new(id.clone(), expires, Value::Null, false)); + entry.started.store(true, Ordering::SeqCst); + entry.finished.send_replace(true); + { + let mut data = entry.data.lock().unwrap(); + data.claimed = claimed; + data.session = Some(session); + data.result = Some(ResponseBody::Ok(json!({}))); + } + state + .session_requests + .0 + .lock() + .unwrap() + .insert(id, entry.clone()); + entries.push(entry); + } + reap(&state); + tokio::task::yield_now().await; + assert!(entries[0].data.lock().unwrap().cancelled); + // No browser connected in this unit test: preserve ownership and retry. + assert_eq!(entries[0].snapshot(&state)["state"], "cleanup_failed"); + assert!(!entries[1].data.lock().unwrap().cancelled); + assert_eq!(entries[1].snapshot(&state)["state"], "active"); + } + + #[tokio::test] + async fn stale_request_never_closes_a_reused_short_session_id() { + let state = Arc::new(DaemonState::new(DaemonConfig::new(0))); + let expires = now_ms() + 60_000; + let id = format!("{expires}:{}", uuid::Uuid::new_v4()); + let entry = Arc::new(StartRequest::new(id.clone(), expires, Value::Null, false)); + entry.started.store(true, Ordering::SeqCst); + entry.finished.send_replace(true); + { + let mut data = entry.data.lock().unwrap(); + data.session = Some(session("same", 1)); + data.result = Some(ResponseBody::Ok(json!({}))); + } + state.sessions.insert(session("same", 2)); + state + .session_requests + .0 + .lock() + .unwrap() + .insert(id.clone(), entry.clone()); + cancel(&state, &entry).await; + assert_eq!(entry.snapshot(&state)["state"], "closed"); + assert_eq!( + state + .sessions + .get(&SessionId("same".into())) + .unwrap() + .agent_window_id, + Some(2) + ); + } +} diff --git a/crates/bsk-cli/src/daemon/sessions.rs b/crates/bsk-cli/src/daemon/sessions.rs index 48863cff..c4201026 100644 --- a/crates/bsk-cli/src/daemon/sessions.rs +++ b/crates/bsk-cli/src/daemon/sessions.rs @@ -3,7 +3,7 @@ //! `bsk session stop`, `bsk session list` and the matching //! `tool.session_start` / `tool.session_stop` round-trips. -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::sync::Arc; use std::sync::Mutex; use std::time::{Instant, SystemTime, UNIX_EPOCH}; @@ -73,6 +73,8 @@ pub struct SessionRegistry { /// Operational metadata kept outside the public `Session` wire/domain /// shape so idle enforcement does not break external struct users. last_activity: Mutex>, + /// Managed creates whose original extension operation has not settled. + starting: Mutex>, } impl SessionRegistry { @@ -180,6 +182,7 @@ impl SessionRegistry { let session = guard.get_mut(session_id)?; session.agent_window_id = agent_window_id; let session = session.clone(); + self.starting.lock().unwrap().remove(session_id); drop(guard); if let Some(audit) = &self.audit { audit.session_started(&session); @@ -190,6 +193,7 @@ impl SessionRegistry { /// Drop a placeholder reservation, used on extension error/timeout /// paths so failed reservations do not accumulate. pub fn cancel_reservation(&self, session_id: &SessionId) { + self.starting.lock().unwrap().remove(session_id); self.inner .lock() .expect("session registry poisoned") @@ -201,6 +205,7 @@ impl SessionRegistry { } pub fn remove(&self, id: &SessionId) -> Option { + self.starting.lock().unwrap().remove(id); let removed = self .inner .lock() @@ -264,12 +269,14 @@ impl SessionRegistry { .last_activity .lock() .expect("session activity registry poisoned"); + let starting = self.starting.lock().unwrap(); sessions .values() .filter(|session| { - activity - .get(&session.id) - .is_some_and(|last| now.saturating_duration_since(*last) >= idle_for) + !starting.contains(&session.id) + && activity + .get(&session.id) + .is_some_and(|last| now.saturating_duration_since(*last) >= idle_for) }) .map(|session| session.id.clone()) .collect() @@ -284,6 +291,7 @@ impl SessionRegistry { .cloned() .collect(); for s in &drained { + self.starting.lock().unwrap().remove(&s.id); guard.remove(&s.id); } let mut activity = self @@ -468,6 +476,32 @@ pub async fn start_session( connect_wait: Duration, timeout_dur: Duration, cancel: Option, +) -> Result { + start_session_recoverable( + registry, + sessions, + queues, + requested, + window, + connect_wait, + timeout_dur, + cancel, + false, + ) + .await +} + +#[allow(clippy::too_many_arguments)] +pub(crate) async fn start_session_recoverable( + registry: &Arc, + sessions: &Arc, + queues: &Arc, + requested: Option<&str>, + window: AgentWindowOptions, + connect_wait: Duration, + timeout_dur: Duration, + cancel: Option, + preserve_cleanup: bool, ) -> Result { let selection = registry.select_with_connect_wait(requested, connect_wait); tokio::pin!(selection); @@ -496,6 +530,9 @@ pub async fn start_session( let session_id = sessions .reserve_id(client.id.clone(), SESSION_ID_MAX_RESERVE_ATTEMPTS, now_ms) .ok_or(StartSessionError::IdExhausted)?; + if preserve_cleanup { + sessions.starting.lock().unwrap().insert(session_id.clone()); + } let params = SessionStartParams { session_id: session_id.0.clone(), browser_instance_id: Some(client.id.0.clone()), @@ -554,8 +591,15 @@ pub async fn start_session( let response = match wait_outcome { StartWaitOutcome::Response(response) => response, StartWaitOutcome::WaiterClosed => { - sessions.cancel_reservation(&session_id); client.pending.lock().unwrap().cancel(&rpc_id); + if preserve_cleanup { + return Err(StartSessionError::CleanupFailed { + session_id, + agent_window_id: None, + message: "browser disconnected during start".into(), + }); + } + sessions.cancel_reservation(&session_id); return Err(StartSessionError::TransportClosed); } StartWaitOutcome::Cancelled => { @@ -566,6 +610,7 @@ pub async fn start_session( &rpc_id, waiter, StartAbortReason::Cancelled, + preserve_cleanup, ) .await); } @@ -577,6 +622,7 @@ pub async fn start_session( &rpc_id, waiter, StartAbortReason::Timeout, + preserve_cleanup, ) .await); } @@ -585,6 +631,14 @@ pub async fn start_session( ResponseBody::Ok(v) => match serde_json::from_value::(v) { Ok(parsed) => parsed, Err(_) => { + if preserve_cleanup { + sessions.commit_reservation(&session_id, None); + return Err(StartSessionError::CleanupFailed { + session_id, + agent_window_id: None, + message: "invalid tool.session_start payload; cleanup required".into(), + }); + } sessions.cancel_reservation(&session_id); return Err(StartSessionError::ExtensionError(RpcError { code: bsk_protocol::ErrorCode::ProtocolError, @@ -594,6 +648,23 @@ pub async fn start_session( } }, ResponseBody::Err(err) => { + if preserve_cleanup + && err.data.as_ref().is_some_and(|d| { + d.get("reason").and_then(serde_json::Value::as_str) == Some("cleanup_failed") + }) + { + let agent_window_id = err + .data + .as_ref() + .and_then(|d| d.get("resource_id")) + .and_then(serde_json::Value::as_i64); + sessions.commit_reservation(&session_id, agent_window_id); + return Err(StartSessionError::CleanupFailed { + session_id, + agent_window_id, + message: err.message, + }); + } sessions.cancel_reservation(&session_id); return Err(StartSessionError::ExtensionError(err)); } @@ -642,8 +713,11 @@ async fn finish_aborted_start( rpc_id: &RpcId, mut waiter: tokio::sync::oneshot::Receiver, reason: StartAbortReason, + preserve_cleanup: bool, ) -> StartSessionError { - sessions.cancel_reservation(session_id); + if !preserve_cleanup { + sessions.cancel_reservation(session_id); + } let cancel = Frame::Request(RequestFrame { id: format!("cancel-{rpc_id}"), method: Method::Cancel, @@ -651,16 +725,37 @@ async fn finish_aborted_start( }); if client.sink.send(cancel).is_err() { client.pending.lock().unwrap().cancel(rpc_id); - return StartSessionError::TransportClosed; + return if preserve_cleanup { + StartSessionError::CleanupFailed { + session_id: session_id.clone(), + agent_window_id: None, + message: "browser disconnected before cancellation".into(), + } + } else { + StartSessionError::TransportClosed + }; } - match timeout(CANCEL_CLEANUP_TIMEOUT, &mut waiter).await { + // Managed starts outlive the receiving CLI. Keep the original waiter until + // the extension settles: an early stop/not_found is not proof that a slow + // chrome.windows.create cannot still produce a window. + let settled = if preserve_cleanup { + Ok((&mut waiter).await) + } else { + timeout(CANCEL_CLEANUP_TIMEOUT, &mut waiter).await + }; + let outcome = match settled { Ok(Ok(response)) => match response.body { ResponseBody::Err(err) if matches!(err.code, ErrorCode::Cancelled | ErrorCode::UserAborted) => { reason.error() } + ResponseBody::Err(err) if preserve_cleanup => StartSessionError::CleanupFailed { + session_id: session_id.clone(), + agent_window_id: None, + message: err.message, + }, ResponseBody::Err(err) => StartSessionError::ExtensionError(err), ResponseBody::Ok(value) => { let agent_window_id = serde_json::from_value::(value) @@ -678,7 +773,15 @@ async fn finish_aborted_start( }, Ok(Err(_)) => { client.pending.lock().unwrap().cancel(rpc_id); - StartSessionError::TransportClosed + if preserve_cleanup { + StartSessionError::CleanupFailed { + session_id: session_id.clone(), + agent_window_id: None, + message: "browser disconnected during cancellation".into(), + } + } else { + StartSessionError::TransportClosed + } } Err(_) => { // The cancel frame is already queued behind session_start. Even @@ -687,7 +790,21 @@ async fn finish_aborted_start( client.pending.lock().unwrap().cancel(rpc_id); reason.error() } + }; + if preserve_cleanup { + match &outcome { + StartSessionError::Cancelled | StartSessionError::Timeout => { + sessions.cancel_reservation(session_id); + } + StartSessionError::CleanupFailed { + agent_window_id, .. + } => { + sessions.commit_reservation(session_id, *agent_window_id); + } + _ => {} + } } + outcome } async fn rollback_extension_session( @@ -750,6 +867,9 @@ pub async fn stop_session( timeout_dur: Duration, cancel: Option, ) -> Result { + if sessions.starting.lock().unwrap().contains(session_id) { + return Err(StopSessionError::SessionBusy); + } let session = sessions.get(session_id).ok_or(StopSessionError::NotFound)?; if registry.get(&session.browser_id).is_none() { return Err(StopSessionError::BrowserGone); diff --git a/crates/bsk-cli/src/daemon/start.rs b/crates/bsk-cli/src/daemon/start.rs index dd422c48..6d878c5c 100644 --- a/crates/bsk-cli/src/daemon/start.rs +++ b/crates/bsk-cli/src/daemon/start.rs @@ -536,6 +536,7 @@ pub(crate) fn spawn_session_idle_reaper(state: Arc) -> tokio::task: loop { ticker.tick().await; + super::session_requests::reap(&state); let idle_ids = state.sessions.idle_ids_at(session_idle, Instant::now()); for session_id in idle_ids { match stop_session( diff --git a/crates/bsk-cli/src/daemon/state.rs b/crates/bsk-cli/src/daemon/state.rs index ce28dc0a..dffb96d8 100644 --- a/crates/bsk-cli/src/daemon/state.rs +++ b/crates/bsk-cli/src/daemon/state.rs @@ -31,6 +31,7 @@ pub struct DaemonState { pub config: DaemonConfig, pub browsers: Arc, pub sessions: Arc, + pub session_requests: super::session_requests::SessionRequests, /// Per-session serial dispatch queues for `tool.*` RPCs (M6.5, /// design §5). Populated by `start_session`, drained by /// `stop_session` / browser disconnect. @@ -81,6 +82,7 @@ impl DaemonState { config, browsers, sessions, + session_requests: Default::default(), tool_queues, abort_registry, tool_inflight, diff --git a/crates/bsk-cli/tests/sessions_ipc.rs b/crates/bsk-cli/tests/sessions_ipc.rs index b94724ee..be4db29a 100644 --- a/crates/bsk-cli/tests/sessions_ipc.rs +++ b/crates/bsk-cli/tests/sessions_ipc.rs @@ -1505,3 +1505,510 @@ async fn borrow_reports_unknown_outcome_when_cancel_cleanup_never_finishes() { drop(ws); handle.shutdown().await; } + +mod recoverable_starts { + use super::*; + use serde_json::{Value, json}; + + fn token() -> String { + let expires = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_millis() + + 60_000; + format!("{expires}:{}", uuid::Uuid::new_v4()) + } + + async fn request(sock: &PathBuf, id: &str, action: &str) -> Value { + IpcClient::connect(sock) + .await + .unwrap() + .call( + "request", + Method::SessionRequest, + Some(json!({"request_id": id, "action": action})), + Duration::from_secs(15), + ) + .await + .unwrap() + .unwrap() + } + + async fn successful_start(ws: &mut TestWs) -> String { + let start = next_extension_request(ws).await; + assert_eq!(start.method, Method::ToolSessionStart); + let params: SessionStartParams = serde_json::from_value(start.params.unwrap()).unwrap(); + send_extension_response( + ws, + ResponseFrame { + id: start.id, + body: ResponseBody::Ok( + json!({"session_id": params.session_id, "agent_window_id": 101}), + ), + }, + ) + .await; + params.session_id + } + + async fn successful_stop(ws: &mut TestWs, expected: &str) { + let stop = next_extension_request(ws).await; + assert_eq!(stop.method, Method::ToolSessionStop); + assert_eq!(stop.params.as_ref().unwrap()["session_id"], expected); + send_extension_response( + ws, + ResponseFrame { + id: stop.id, + body: ResponseBody::Ok(serde_json::to_value(SessionStopResult::default()).unwrap()), + }, + ) + .await; + } + + #[tokio::test] + async fn cancellation_before_start_is_a_tombstone() { + let (daemon, sock) = spawn_daemon().await; + let id = token(); + assert_eq!(request(&sock, &id, "prepare").await["state"], "prepared"); + assert_eq!(request(&sock, &id, "cancel").await["state"], "closed"); + let mut client = IpcClient::connect(&sock).await.unwrap(); + let outcome = client + .call_with_id::<_, Value>( + "late-start".into(), + Method::SessionStartTracked, + Some(json!({"request_id": id})), + Duration::from_secs(2), + ) + .await + .unwrap(); + assert_eq!(outcome.unwrap_err().code, ErrorCode::Cancelled); + assert!(daemon.state().sessions.is_empty()); + daemon.shutdown().await; + } + + #[tokio::test] + async fn duplicate_start_and_lost_reply_are_recoverable_without_touching_foreign_session() { + let (daemon, sock) = spawn_daemon().await; + let mut ws = connect_ext(daemon.ws_addr()).await; + handshake_as_ext(&mut ws).await; + let id = token(); + assert_eq!(request(&sock, &id, "prepare").await["state"], "prepared"); + let start_sock = sock.clone(); + let start_id = id.clone(); + let original = tokio::spawn(async move { + IpcClient::connect(start_sock) + .await + .unwrap() + .call::<_, Value>( + "lost", + Method::SessionStartTracked, + Some(json!({"request_id": start_id})), + Duration::from_secs(10), + ) + .await + }); + let sid = successful_start(&mut ws).await; + // Drop the original receiving process/connection after the remote side effect. + original.abort(); + let mut retry = IpcClient::connect(&sock).await.unwrap(); + let reply: Value = retry + .call( + "retry", + Method::SessionStartTracked, + Some(json!({"request_id": id})), + Duration::from_secs(2), + ) + .await + .unwrap() + .unwrap(); + assert_eq!(reply["session_id"], sid); + assert_eq!(daemon.state().sessions.len(), 1); + assert_eq!(request(&sock, &id, "status").await["state"], "ready"); + assert_eq!(request(&sock, &id, "claim").await["state"], "active"); + let conflicting = retry + .call_with_id::<_, Value>( + "conflict".into(), + Method::SessionStartTracked, + Some(json!({"request_id": id, "width": 900, "height": 700})), + Duration::from_secs(2), + ) + .await + .unwrap(); + assert_eq!(conflicting.unwrap_err().code, ErrorCode::InvalidParams); + let foreign = bsk::daemon::sessions::Session { + id: bsk::daemon::sessions::SessionId("foreign".into()), + browser_id: bsk::daemon::browsers::BrowserId(TEST_EXT_ID.into()), + agent_window_id: Some(999), + created_at_ms: 1, + interaction: None, + }; + daemon.state().sessions.insert(foreign.clone()); + let transfer = daemon.state().transfers.begin_download(&sid).unwrap(); + let cancelling = request(&sock, &id, "cancel"); + let (_, cancelled) = tokio::join!(successful_stop(&mut ws, &sid), cancelling); + assert_eq!(cancelled["state"], "closed"); + assert_eq!(daemon.state().sessions.len(), 1); + assert!(daemon.state().sessions.get(&foreign.id).is_some()); + assert!( + !daemon + .state() + .transfers + .release(bsk_protocol::tools::TransferIdParams { + transfer_id: transfer.transfer_id, + }) + .released, + "managed stop must also release the session's transfer resources" + ); + assert_eq!(request(&sock, &id, "cancel").await["state"], "closed"); + daemon.shutdown().await; + } + + #[tokio::test] + async fn cancel_during_start_rolls_back_late_success() { + let (daemon, sock) = spawn_daemon().await; + let mut ws = connect_ext(daemon.ws_addr()).await; + handshake_as_ext(&mut ws).await; + let id = token(); + assert_eq!(request(&sock, &id, "prepare").await["state"], "prepared"); + let caller_sock = sock.clone(); + let caller_id = id.clone(); + let start = tokio::spawn(async move { + IpcClient::connect(caller_sock) + .await + .unwrap() + .call_with_id::<_, Value>( + "start".into(), + Method::SessionStartTracked, + Some(json!({"request_id": caller_id})), + Duration::from_secs(15), + ) + .await + .unwrap() + }); + let (seen_tx, seen_rx) = tokio::sync::oneshot::channel(); + let extension = tokio::spawn(async move { + respond_to_aborted_start(&mut ws, Some(seen_tx), 77).await; + ws + }); + seen_rx.await.unwrap(); + assert_eq!(request(&sock, &id, "cancel").await["state"], "closed"); + assert_eq!(start.await.unwrap().unwrap_err().code, ErrorCode::Cancelled); + let _ws = extension.await.unwrap(); + assert!(daemon.state().sessions.is_empty()); + daemon.shutdown().await; + } + + #[tokio::test] + async fn failed_cleanup_retains_ownership_for_retry() { + let (daemon, sock) = spawn_daemon().await; + let mut ws = connect_ext(daemon.ws_addr()).await; + handshake_as_ext(&mut ws).await; + let id = token(); + assert_eq!(request(&sock, &id, "prepare").await["state"], "prepared"); + let mut client = IpcClient::connect(&sock).await.unwrap(); + let (sid, start) = tokio::join!( + successful_start(&mut ws), + client.call::<_, Value>( + "start", + Method::SessionStartTracked, + Some(json!({"request_id": id})), + Duration::from_secs(5) + ) + ); + start.unwrap().unwrap(); + let fail_stop = async { + let stop = next_extension_request(&mut ws).await; + assert_eq!(stop.method, Method::ToolSessionStop); + send_extension_response( + &mut ws, + ResponseFrame { + id: stop.id, + body: ResponseBody::Err(RpcError { + code: ErrorCode::ProtocolError, + message: "injected close failure".into(), + data: None, + }), + }, + ) + .await; + }; + let (_, status) = tokio::join!(fail_stop, request(&sock, &id, "cancel")); + assert_eq!(status["state"], "cleanup_failed"); + assert_eq!(status["session"]["session_id"], sid); + assert_eq!(daemon.state().sessions.len(), 1); + let (_, status) = tokio::join!( + successful_stop(&mut ws, &sid), + request(&sock, &id, "cancel") + ); + assert_eq!(status["state"], "closed"); + assert!(daemon.state().sessions.is_empty()); + daemon.shutdown().await; + } + #[tokio::test] + async fn failed_startup_compensation_retains_the_window_for_retry() { + let (daemon, sock) = spawn_daemon().await; + let mut ws = connect_ext(daemon.ws_addr()).await; + handshake_as_ext(&mut ws).await; + let id = token(); + request(&sock, &id, "prepare").await; + let start_sock = sock.clone(); + let start_id = id.clone(); + let start = tokio::spawn(async move { + IpcClient::connect(start_sock) + .await + .unwrap() + .call::<_, Value>( + "start", + Method::SessionStartTracked, + Some(json!({"request_id": start_id})), + Duration::from_secs(5), + ) + .await + .unwrap() + }); + let create = next_extension_request(&mut ws).await; + let sid = create.params.as_ref().unwrap()["session_id"] + .as_str() + .unwrap() + .to_owned(); + send_extension_response( + &mut ws, + ResponseFrame { + id: create.id, + body: ResponseBody::Err(RpcError { + code: ErrorCode::ProtocolError, + message: "initialization and window rollback failed".into(), + data: Some(json!({"reason": "cleanup_failed", "resource_id": 123})), + }), + }, + ) + .await; + assert_eq!( + start.await.unwrap().unwrap_err().data.unwrap()["session_id"], + sid + ); + let stop = next_extension_request(&mut ws).await; + assert_eq!(stop.method, Method::ToolSessionStop); + assert_eq!(stop.params.unwrap()["session_id"], sid); + send_extension_response( + &mut ws, + ResponseFrame { + id: stop.id, + body: ResponseBody::Err(RpcError { + code: ErrorCode::ProtocolError, + message: "window still cannot close".into(), + data: None, + }), + }, + ) + .await; + tokio::time::timeout(Duration::from_secs(2), async { + loop { + let status = request(&sock, &id, "status").await; + if status["state"] == "cleanup_failed" { + assert_eq!(status["session"]["agent_window_id"], 123); + break; + } + tokio::task::yield_now().await; + } + }) + .await + .unwrap(); + assert_eq!(daemon.state().sessions.len(), 1); + let (_, status) = tokio::join!( + successful_stop(&mut ws, &sid), + request(&sock, &id, "cancel") + ); + assert_eq!(status["state"], "closed"); + assert!(daemon.state().sessions.is_empty()); + daemon.shutdown().await; + } + + #[tokio::test] + async fn real_cli_can_cancel_a_start_after_its_process_is_killed() { + let (daemon, sock) = spawn_daemon().await; + let home = tempfile::tempdir().unwrap(); + bsk::daemon::info::write_to_path( + &bsk::daemon::info::DaemonInfo::now( + std::process::id(), + sock.clone(), + daemon.ws_addr().port(), + env!("CARGO_PKG_VERSION"), + ), + &home.path().join("daemon.json"), + ) + .unwrap(); + let command = |args: Vec| { + let mut cmd = tokio::process::Command::new(env!("CARGO_BIN_EXE_bsk")); + cmd.args(args) + .arg("--json") + .env("BSK_HOME", home.path()) + .env("BSK_AUTO_START", "0") + .env("BSK_AUTO_UPDATE", "off") + .env_remove("BSK_CANCEL_ON_STDIN_CLOSE") + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .kill_on_drop(true); + cmd + }; + let mut ws = connect_ext(daemon.ws_addr()).await; + handshake_as_ext(&mut ws).await; + let id = token(); + let prepare = command(vec![ + "session".into(), + "request".into(), + id.clone(), + "--prepare".into(), + ]) + .output() + .await + .unwrap(); + assert!( + prepare.status.success(), + "{}", + String::from_utf8_lossy(&prepare.stderr) + ); + let mut child = command(vec![ + "session".into(), + "start".into(), + "--request-id".into(), + id.clone(), + ]) + .spawn() + .unwrap(); + let start = next_extension_request(&mut ws).await; + let sid = start.params.as_ref().unwrap()["session_id"] + .as_str() + .unwrap() + .to_owned(); + // Kill the receiving CLI after dispatch but before any successful reply. + child.kill().await.unwrap(); + send_extension_response( + &mut ws, + ResponseFrame { + id: start.id, + body: ResponseBody::Ok(json!({"agent_window_id": 88})), + }, + ) + .await; + let mut cancel = command(vec![ + "session".into(), + "request".into(), + id, + "--cancel".into(), + ]); + let (_, output) = tokio::join!(successful_stop(&mut ws, &sid), cancel.output()); + let output = output.unwrap(); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!( + serde_json::from_slice::(&output.stdout).unwrap()["state"], + "closed" + ); + assert!(daemon.state().sessions.is_empty()); + daemon.shutdown().await; + } + #[tokio::test] + async fn cancellation_waits_for_slow_creation_before_confirming_cleanup() { + let (daemon, sock) = spawn_daemon().await; + let mut ws = connect_ext(daemon.ws_addr()).await; + handshake_as_ext(&mut ws).await; + let id = token(); + request(&sock, &id, "prepare").await; + let start_sock = sock.clone(); + let start_id = id.clone(); + let start_task = tokio::spawn(async move { + IpcClient::connect(start_sock) + .await + .unwrap() + .call::<_, Value>( + "start", + Method::SessionStartTracked, + Some(json!({"request_id": start_id})), + Duration::from_secs(15), + ) + .await + .unwrap() + }); + let start = next_extension_request(&mut ws).await; + let sid = start.params.as_ref().unwrap()["session_id"] + .as_str() + .unwrap() + .to_owned(); + let cancel_sock = sock.clone(); + let cancel_id = id.clone(); + let cancellation = + tokio::spawn(async move { request(&cancel_sock, &cancel_id, "cancel").await }); + let cancel = next_extension_request(&mut ws).await; + acknowledge_extension_cancel(&mut ws, cancel, &start.id).await; + assert!( + tokio::time::timeout(Duration::from_millis(2200), next_extension_request(&mut ws)) + .await + .is_err(), + "must not issue stop before the original create settles" + ); + assert!(!cancellation.is_finished()); + assert!( + daemon + .state() + .sessions + .idle_ids_at(Duration::ZERO, std::time::Instant::now()) + .is_empty() + ); + send_extension_response( + &mut ws, + ResponseFrame { + id: start.id, + body: ResponseBody::Ok(json!({"agent_window_id": 777})), + }, + ) + .await; + successful_stop(&mut ws, &sid).await; + assert_eq!(cancellation.await.unwrap()["state"], "closed"); + assert_eq!( + start_task.await.unwrap().unwrap_err().code, + ErrorCode::Cancelled + ); + assert!(daemon.state().sessions.is_empty()); + daemon.shutdown().await; + } + + #[tokio::test] + async fn concurrent_retries_create_exactly_one_window() { + let (daemon, sock) = spawn_daemon().await; + let mut ws = connect_ext(daemon.ws_addr()).await; + handshake_as_ext(&mut ws).await; + let id = token(); + request(&sock, &id, "prepare").await; + let mut a = IpcClient::connect(&sock).await.unwrap(); + let mut b = IpcClient::connect(&sock).await.unwrap(); + let (sid, first, retry) = tokio::join!( + successful_start(&mut ws), + a.call::<_, Value>( + "first", + Method::SessionStartTracked, + Some(json!({"request_id":id})), + Duration::from_secs(5) + ), + b.call::<_, Value>( + "retry", + Method::SessionStartTracked, + Some(json!({"request_id":id})), + Duration::from_secs(5) + ) + ); + assert_eq!(first.unwrap().unwrap()["session_id"], sid); + assert_eq!(retry.unwrap().unwrap()["session_id"], sid); + assert_eq!(daemon.state().sessions.len(), 1); + // The next extension operation must be the single corresponding stop. + let (_, status) = tokio::join!( + successful_stop(&mut ws, &sid), + request(&sock, &id, "cancel") + ); + assert_eq!(status["state"], "closed"); + daemon.shutdown().await; + } +} diff --git a/crates/bsk-protocol/src/method.rs b/crates/bsk-protocol/src/method.rs index 2db8b20a..07b55e34 100644 --- a/crates/bsk-protocol/src/method.rs +++ b/crates/bsk-protocol/src/method.rs @@ -33,6 +33,11 @@ pub enum Method { #[serde(rename = "session.start")] SessionStart, + /// Recoverable CLI start; distinct method prevents unsafe fallback on old daemons. + #[serde(rename = "session.start_tracked")] + SessionStartTracked, + #[serde(rename = "session.request")] + SessionRequest, #[serde(rename = "session.stop")] SessionStop, #[serde(rename = "session.stop_all")] @@ -222,6 +227,8 @@ impl Method { // Session lifecycle — not gated. Method::SessionStart + | Method::SessionStartTracked + | Method::SessionRequest | Method::SessionStop | Method::SessionStopAll | Method::SessionList diff --git a/docs/recoverable-session-starts.md b/docs/recoverable-session-starts.md new file mode 100644 index 00000000..04315d22 --- /dev/null +++ b/docs/recoverable-session-starts.md @@ -0,0 +1,63 @@ +# Recoverable browser session starts + +The DSH plugin used to register a browser session only after receiving a successful CLI result. Cancellation, a lost reply, or a host crash could leave an Agent Window that the plugin could neither identify nor close (issue #245). A second leak occurred when initial navigation failed and a failed compensating stop caused the plugin to discard ownership. + +## Lifecycle contract + +Managed starts now have a caller-generated request token before any browser side effect. The plugin writes the token and DSH ownership lineage to an atomic, synced journal first. It prepares the request on the daemon, then starts it. The separate `session.start_tracked` RPC fails closed on an older daemon. A start without a prepared request also fails: a delayed CLI cannot recreate a cancelled window after a daemon restart. + +The CLI flow is: + +```text +bsk session request --prepare --json +bsk session start --request-id [window options] --json +bsk session request --claim --json +bsk session request --json +bsk session request --cancel --json +``` + +A token is `:`, generated with a random UUID. Admission deadlines must be in the next ten minutes; the plugin uses five minutes. Treat tokens as ownership handles: they are not labels, task names, or short session IDs. Ordinary `session start` remains unchanged and does not require these calls. Managed starts do not sync unrelated CLI harness skills; the plugin carries its own instructions. + +Preparation creates no browser window. Start transitions from prepared to starting, then ready. Claim happens after the plugin durably records the returned session and finishes initial navigation/emulation; it changes ready to active. A repeated start with the same token and parameters returns the existing result instead of opening another window; different parameters are rejected. Expired tokens cannot start again. + +Cancellation is monotonic, including cancellation **before** prepare/start. An early cancellation leaves a terminal tombstone. Cancelling a starting operation preserves its original extension waiter until creation settles; a premature `session not found` cannot prove that a delayed `chrome.windows.create` has finished. These reservations are excluded from idle cleanup and ordinary stop until creation settles. The caller's wait remains bounded, while daemon-owned reconciliation may continue. + +Cleanup failures retain the exact session/window identity and can be retried. The extension also retains a window whose startup compensation failed, so a later stop can actually close it. Cleanup uses the request's original session identity and verifies that a reused short session ID does not belong to another window. + +The extension captures the initial tab IDs directly from window creation, before initialization or cancellation can fail. Those IDs remain owned throughout failed compensation and subsequent stops. Retry closes only agent-created tabs; later user tabs are preserved. If an agent tab cannot close beside a user tab, stop reports a cleanup failure and retains the session binding instead of releasing a leaking window. + +The existing daemon reaper cancels unclaimed requests after their admission deadline (normally within the next 30-second tick), retries failed cleanup, and removes expired terminal tombstones. Claimed sessions retain the existing session idle policy. Browser disconnection still follows the existing session teardown behavior. An unresponsive browser can delay confirmed cleanup; that state remains owned and visible as pending rather than being reported as closed. + +## Plugin recovery + +The journal defaults to `$BSK_HOME/dsh-starts/` (or `~/.bsk/dsh-starts/`). The scope includes the working directory and configured CLI path. `sessionStateDirectory` can assign a dedicated durable directory to a host/profile. Each live plugin instance owns a separate process/UUID directory; recovery never takes another live instance's records. A reused PID is treated conservatively as live; daemon leases and idle cleanup still apply. + +On reload/startup, abandoned journal directories are claimed by atomic rename and their requests are cancelled. Interrupted recovery remains recoverable, including directories moved just before a crash. Failed cleanup records remain on disk and are retried by the plugin, list/stop actions, and the daemon. Unload and conversation archival cancel pending starts as well as completed sessions. New starts are refused during unload or from an archived conversation. + +The plugin distinguishes resource ownership from usability: registered starts are `starting`, become `active` only after initialization and claim succeed, and enter `cleanup` when cancellation starts. Starting/cleanup resources stay visible, occupy capacity, and accept stop, but cannot become current or accept ordinary commands. Queued commands recheck usability before execution, and background captures only run for active sessions. A failed start leaves an existing working session current; with none remaining, ordinary commands require a new active session. + +The model-facing list retains its owned-session view and adds `pendingCleanup`, each session's state, and its request ID. It never discovers ownership by diffing the daemon's global session list. A stop with no current session can retry a pending startup cleanup. When several targets are possible, callers must select one explicitly or use list to reconcile pending resources. + +## Stop admission and recovery + +Tool stops, overlay stops, startup failure, archival, unload, and reconciliation all use the same lifecycle owner and one cleanup job per request. Observation only tracks presentation, interruption, and captures; it does not execute a separate stop path. Every cancel RPC addresses the original request ID, including when a short session ID has been reused. + +An explicit stop first saves its cleanup intent and pending receipt to the journal, then makes the resource unusable and schedules cleanup. A pre-aborted call or failed admission write leaves a working session usable and dispatches nothing. Once admitted, the cleanup job owns its queue slot and timeout independently of the caller. Aborting the call ends only its wait; cleanup continues, and failures remain in the journal for the timer, list, and restart recovery. Automatic startup/archival cleanup also retains its intent in memory if a journal update fails; the original write-ahead ownership record remains recoverable. + +Ordinary commands may fall back from stopping B to active A. An implicit stop instead prioritizes the unacknowledged stop for B. If several stops await acknowledgement, it requires `session` or `requestId` rather than guessing. Explicit `requestId` also addresses a start whose short session ID was never received. These IDs must belong to this plugin. + +Confirmed closure releases the resource and capacity, but an explicit stop's completed receipt remains durable until a caller successfully receives its completion from the lifecycle manager. Background completion and restart preserve the receipt: retry acknowledges the original B without closing A. A receipt does not occupy browser capacity or trigger another cancel. Failed completion/acknowledgement writes retain a retryable receipt. Stop results include `requestId` and `alreadyClosed`, distinguishing acknowledgement of an earlier operation from a newly executed close. Stops reject unconfirmed or inconsistent responses instead of discarding ownership. + +Concurrent waiters cannot acknowledge a failed implicit stop on its caller's behalf. A durable revision records outstanding default retries: an explicit/overlay completion leaves that retry intact, and a concurrent implicit waiter cannot consume a newer failure. The next implicit retry acknowledges the original result before a different current session can be selected. + +CLI and daemon must both support the new request protocol. Preparation detects unsupported versions before opening a window. Update the extension as well to obtain retryable startup-compensation failures. No persistent daemon service, external database, or new package dependency is introduced. + +## Regression coverage + +- Plugin: lost/aborted/timed-out successful replies; failed navigation and failed cleanup without changing the current session; initialization and claim gating; late claim after cancellation; queued-operation rejection and capture suppression during cleanup; archive/unload during an in-flight start; write-ahead persistence failure; pending capacity limits; restart recovery with another live instance; unsupported preparation; stop without a current session. +- Stop entry points: failed normal-session stop followed by implicit retry; completion before retry; tool/HTTP/archive/reconcile convergence; pre-admission and queued/in-flight cancellation; autonomous timer retry; exact and ambiguous targets; anonymous requests; late prepare; short ID reuse; admission/completion/acknowledgement write failures; pending and completed receipts across disk recovery; unconfirmed/mismatched replies; completed receipt capacity accounting. +- Daemon: cancellation before prepare/start; expired tokens and tombstone collection; restart admission; unclaimed vs. claimed leases; reused short IDs; concurrent duplicate requests; delayed creation beyond the old cancellation grace; retryable close failures; foreign session isolation. +- Integration: actual CLI child killed after dispatch and before its successful response, followed by recovery through a new CLI process. IPC and WebSocket tests use a real daemon and a controlled extension peer, without operating the user's browser. +- Extension: the production stop handler retries failed startup compensation, closes initial agent tabs, preserves later user tabs, and retains mixed-window ownership when an agent tab still cannot close. + +These checks complement the existing session, cancellation, queue, plugin, and extension tests. They do not claim a Windows/DSH/Chrome GUI end-to-end run. diff --git a/packages/dsh-plugin-browserskill/README.md b/packages/dsh-plugin-browserskill/README.md index 23ea5e15..acf86870 100644 --- a/packages/dsh-plugin-browserskill/README.md +++ b/packages/dsh-plugin-browserskill/README.md @@ -77,6 +77,19 @@ One agent conversation can drive several browser sessions at once: - The number of concurrent sessions started through the plugin is capped (`maxSessions`, default 5). - Unloading the plugin stops every session it started and kills in-flight bsk processes. +Starts are journaled before creating a window. Lost replies and failed cleanup remain +recoverable on reload; `browser_session action=list` reports `pendingCleanup`, and stop +can retry it even when there is no current session. This requires a CLI and daemon +that support recoverable starts. See [the lifecycle contract](../../docs/recoverable-session-starts.md). + +Stops also retain their target and cleanup intent. A failed or interrupted stop can be +retried without accidentally stopping the next current session. Once the intent is saved, +cancelling the call only stops waiting; cleanup continues in the background. If it finishes +before a retry, that retry acknowledges the original result. With several unacknowledged +stops, pass `session` or the owned `requestId` explicitly (not both). List exposes request +IDs; stop results include `requestId` and `alreadyClosed`. Completed receipts survive reload +until acknowledged and do not occupy browser capacity. + **Ownership boundary**: the bsk daemon may be shared with other agents, terminals, or dsh instances. The plugin therefore only ever sees and operates on sessions it created itself — an explicit `session` argument naming a foreign or unknown id is rejected, the `list` action on @@ -121,6 +134,7 @@ All fields are optional; omitted fields use the defaults below: | Option | Default | Purpose | | --- | --- | --- | | `bskPath` | `bsk` | Path to the CLI binary. | +| `sessionStateDirectory` | Scoped under `$BSK_HOME/dsh-starts` (or `~/.bsk/dsh-starts`) | Durable recovery records; optionally isolate by host/profile. | | `defaultTimeoutMs` | `120000` | Default command timeout in milliseconds. | | `maxSessions` | `5` | Maximum concurrent sessions started by this plugin. | | `observationEnabled` | `true` | Enable live browser observation. | diff --git a/packages/dsh-plugin-browserskill/skill/SKILL.md b/packages/dsh-plugin-browserskill/skill/SKILL.md index 7a6047cb..584a7d33 100644 --- a/packages/dsh-plugin-browserskill/skill/SKILL.md +++ b/packages/dsh-plugin-browserskill/skill/SKILL.md @@ -81,6 +81,11 @@ unknown effects or switch backends to bypass limits. Borrow confirmation still a - Stale ref: observe, then retry the intended action once. - Unknown tab/session: list owned resources or start a session; never guess IDs. +- Failed or interrupted session stop: accepted cleanup continues in the background. + Retry the same stop; a completed previous stop returns `alreadyClosed: true`. + If several stops are pending, specify `session` or the owned `requestId` from the + result/list/error (not both). A request ID targets the original operation even if + the short session ID is reused. Never switch to another session just to retry cleanup. - Timeout/unknown effect: inspect before retrying; the action may have happened. - Unconfirmed fill: read the field. Formatting may satisfy the goal; correct only a remaining difference instead of blindly refilling or requesting help. diff --git a/packages/dsh-plugin-browserskill/src/archive-cleanup.ts b/packages/dsh-plugin-browserskill/src/archive-cleanup.ts index 5999e520..46d6bbb8 100644 --- a/packages/dsh-plugin-browserskill/src/archive-cleanup.ts +++ b/packages/dsh-plugin-browserskill/src/archive-cleanup.ts @@ -15,8 +15,7 @@ */ import type { Context } from "@deepseek-ai/cordis"; -import type { ObservationService } from "./observation"; -import type { SessionRegistry } from "./sessions"; +import type { SessionStarts } from "./session-starts"; /** The wire shape of a `domain/changed` frame (see dsh-storage-domain). */ interface DomainChange { @@ -65,8 +64,7 @@ export function ownerSessionIds(ctx: Context, agentId: string | undefined): stri */ export function armArchiveCleanup( ctx: Context, - registry: SessionRegistry, - observation: ObservationService, + starts: Pick, ): () => void { // 'domain/changed' lives outside the vendored Events type map, so the // listener goes through a structural view of the events mixin. @@ -75,7 +73,7 @@ export function armArchiveCleanup( ).on; if (typeof on !== "function") return () => {}; - /** Archived ids already accounted for; lazily seeded from the registry. */ + /** Archived ids already accounted for, seeded before the first change event. */ let seen: Set | undefined; const initialize = (): Set => { if (seen === undefined) { @@ -87,6 +85,9 @@ export function armArchiveCleanup( return seen; }; + // The host may update its registry before emitting domain/changed. + initialize(); + return on.call(ctx, "domain/changed", (change: DomainChange) => { if (change?.domain !== "workspace" || change?.table !== "") return; const archived = (change.value as WorkspaceGlobal | undefined)?.archivedSessionIds; @@ -97,12 +98,7 @@ export function armArchiveCleanup( ); seen = new Set(archived.filter((id): id is string => typeof id === "string")); for (const dshSessionId of fresh) { - for (const sessionId of registry.ownedByDsh(dshSessionId)) { - // stopSession owns the full teardown (kill in-flight tools, queue - // the daemon stop, drop registry + observation entries); a failure - // just leaves the session for idle timeout or unload cleanup. - void observation.stopSession(sessionId).catch(() => {}); - } + starts.archive(dshSessionId); } }); } diff --git a/packages/dsh-plugin-browserskill/src/browser-tools.ts b/packages/dsh-plugin-browserskill/src/browser-tools.ts index 72af073b..527cfcfe 100644 --- a/packages/dsh-plugin-browserskill/src/browser-tools.ts +++ b/packages/dsh-plugin-browserskill/src/browser-tools.ts @@ -6,7 +6,13 @@ */ import { defineTool, type ParameterSchemaSpec, type ToolDefinition } from "@deepseek-ai/dsh-tools"; -import { SESSION_PARAM, TAB_ID_PARAM, TIMEOUT_MS_PARAM, WAIT_UNTIL_PARAM } from "./tool-params"; +import { + SESSION_PARAM, + SESSION_STOP_PARAMS, + TAB_ID_PARAM, + TIMEOUT_MS_PARAM, + WAIT_UNTIL_PARAM, +} from "./tool-params"; import { createBrowserOperationDefinitions, type ToolDeps } from "./tools"; const DEVICE_PRESETS = [ @@ -99,14 +105,16 @@ const BROWSER_TOOL_SPECS: BrowserToolSpec[] = [ description: "Manage plugin-owned browser sessions. Actions: start opens an Agent Window; stop closes an " + "owned session; list returns owned sessions. For start, url/device/width/height/noFocus/browser " + - "are optional. For stop, session is optional and defaults to the current owned session.", + "are optional. For stop, specify session or requestId (not both), or omit both to retry an " + + "unacknowledged stop before selecting the current owned session. If several stops await " + + "acknowledgement, specify a target. Once accepted, cleanup continues if the call is aborted.", actions: { start: "session.start", stop: "session.stop", list: "session.list", }, parameters: { - session: SESSION_PARAM, + ...SESSION_STOP_PARAMS, url: { type: "string", description: "Initial URL for start." }, width: { type: "integer", description: "Agent Window width; start requires height too." }, height: { type: "integer", description: "Agent Window height; start requires width too." }, diff --git a/packages/dsh-plugin-browserskill/src/index.ts b/packages/dsh-plugin-browserskill/src/index.ts index df656fe9..145cb204 100644 --- a/packages/dsh-plugin-browserskill/src/index.ts +++ b/packages/dsh-plugin-browserskill/src/index.ts @@ -19,9 +19,11 @@ import { ObservationService } from "./observation"; import { registerObservationRoutes } from "./observation-http"; import { KeyedExecutor } from "./queue"; import { type BskRunner, createBskRunner } from "./runner"; +import { SessionStarts } from "./session-starts"; import { SessionRegistry } from "./sessions"; import { armAgentScopedBskSkill, registerBskSkill } from "./skill"; -import type { PluginConfig } from "./tools"; +import { DiskStartJournal, defaultStartJournalDirectory, type StartJournal } from "./start-journal"; +import type { PluginConfig, ToolDeps } from "./tools"; export const name = "@wxg-prc-cpg/browser-skill-dsh-plugin"; export const inject = ["tools"]; @@ -31,6 +33,9 @@ export const Config = Schema.object({ bskPath: Schema.string() .default("bsk") .description("Path to the bsk CLI binary (defaults to resolving `bsk` from PATH)."), + sessionStateDirectory: Schema.string().description( + "Directory for durable browser start recovery records; defaults to BSK_HOME/dsh-starts scoped by working directory and CLI.", + ), defaultTimeoutMs: Schema.number() .default(120_000) .description("Default per-command timeout in milliseconds."), @@ -59,6 +64,7 @@ export type Config = PluginConfig; /** Test seams: swap the process runner (unit tests never spawn a real bsk). */ export interface ApplyOptions { runnerFactory?: (bskPath: string) => BskRunner; + startJournal?: StartJournal; } export function apply( @@ -68,6 +74,7 @@ export function apply( ): void { const resolved: PluginConfig = { bskPath: config.bskPath ?? "bsk", + sessionStateDirectory: config.sessionStateDirectory, defaultTimeoutMs: config.defaultTimeoutMs ?? 120_000, maxSessions: config.maxSessions ?? 5, observationEnabled: config.observationEnabled ?? true, @@ -90,6 +97,16 @@ export function apply( }, }); + const deps: ToolDeps = { ctx, runner, registry, config: resolved, observation, queue }; + const journal = + options.startJournal ?? + new DiskStartJournal( + resolved.sessionStateDirectory ?? defaultStartJournalDirectory(resolved.bskPath), + ); + if (journal instanceof DiskStartJournal) journal.recover(); + const starts = (deps.starts = new SessionStarts(deps, journal)); + void starts.reconcile().catch((error) => console.warn("Browser start recovery failed", error)); + // Progressive disclosure of the BSK agent skill (catalog entry resident, // body on demand) through the official skill seam; silent no-op when the // composition lacks it. With lazyTools on, the skill entry is initially the @@ -101,21 +118,20 @@ export function apply( // exact agent context at startup so DSH always loads the browser_* protocol // instructions; the shared CLI skill remains untouched for other agents. const disarmAgentSkill = armAgentScopedBskSkill(ctx); - const registerSuite = () => - registerBrowserTools({ ctx, runner, registry, config: resolved, observation, queue }); + const registerSuite = () => registerBrowserTools(deps); const removeSuite = resolved.lazyTools ? armLazyTools(ctx, registerSuite) : registerSuite(); // Route registration rides ctx.inject: the webServer service may be provided // AFTER this plugin loads, and in headless compositions it never appears (the // callback simply never runs, leaving the rest of the plugin unaffected). let removeRoutes: () => void = () => {}; ctx.inject(["webServer"], (injected) => { - removeRoutes = registerObservationRoutes(injected, observation); + removeRoutes = registerObservationRoutes(injected, observation, starts); return () => removeRoutes(); }); // Reap a conversation's browsers when the conversation itself is archived: // archived sessions are hidden from every surface, so their Agent Windows // would otherwise linger unreachable until idle timeout or unload. - const disarmArchiveCleanup = armArchiveCleanup(ctx, registry, observation); + const disarmArchiveCleanup = armArchiveCleanup(ctx, starts); // Non-blocking install probe: warn early when bsk is missing instead of // failing the first tool call with a bare spawn error. Uses --version on @@ -143,11 +159,7 @@ export function apply( unregisterSkill(); removeRoutes(); disarmArchiveCleanup(); - runner.killAll(); - const stops = registry - .ownedIds() - .map((sessionId) => observation.stopSession(sessionId).catch(() => false)); - return Promise.all(stops).then(() => observation.dispose()); + return starts.dispose().then(() => observation.dispose()); }; }); } diff --git a/packages/dsh-plugin-browserskill/src/observation-http.ts b/packages/dsh-plugin-browserskill/src/observation-http.ts index 4b1a0080..1ae65b3b 100644 --- a/packages/dsh-plugin-browserskill/src/observation-http.ts +++ b/packages/dsh-plugin-browserskill/src/observation-http.ts @@ -25,6 +25,7 @@ import type { IncomingMessage, ServerResponse } from "node:http"; import type { Context } from "@deepseek-ai/cordis"; import type { ObservationService } from "./observation"; +import type { SessionStarts } from "./session-starts"; /** Structural view of the dsh-host-webserver route seam. */ interface WebServerLike { @@ -101,6 +102,7 @@ function fenceRejected(req: IncomingMessage, res: ServerResponse): boolean { export function registerObservationRoutes( ctx: Context, observation: ObservationService, + lifecycle: Pick, ): () => void { const webServer = ctx.get("webServer") as WebServerLike | undefined; if (webServer === undefined) { @@ -232,8 +234,8 @@ export function registerObservationRoutes( sendJson(res, 400, { error: "sessionId required" }); return; } - void observation.stopSession(sessionId).then( - (stopped) => sendJson(res, 200, { stopped }), + void lifecycle.stop({ sessionId }).then( + () => sendJson(res, 200, { stopped: true }), () => sendJson(res, 500, { error: "stop failed" }), ); }); diff --git a/packages/dsh-plugin-browserskill/src/observation.ts b/packages/dsh-plugin-browserskill/src/observation.ts index af1f90e8..7c803449 100644 --- a/packages/dsh-plugin-browserskill/src/observation.ts +++ b/packages/dsh-plugin-browserskill/src/observation.ts @@ -19,7 +19,7 @@ import { join } from "node:path"; import type { Context } from "@deepseek-ai/cordis"; import { sniffImageMediaType } from "./image"; import type { KeyedExecutor } from "./queue"; -import { BskError, type BskRunner, parseBskJson, runWithSessionBusyRetry } from "./runner"; +import type { BskRunner } from "./runner"; import type { SessionRegistry } from "./sessions"; /** One owned session's live observation record (wire-stable shape). */ @@ -203,7 +203,7 @@ export class ObservationService { this.put({ sessionId, ...(url !== undefined ? { url } : {}), - action: "idle", + action: this.restingAction(sessionId), since: this.scheduler.now(), ...(dshSessionIds.length > 0 ? { dshSessionIds } : {}), }); @@ -281,7 +281,11 @@ export class ObservationService { if (entry === undefined || entry.dead === true) return; const now = this.scheduler.now(); this.lastActivity.set(sessionId, now); - const next: SessionObservation = { ...entry, action: "idle", since: now }; + const next: SessionObservation = { + ...entry, + action: this.restingAction(sessionId), + since: now, + }; if (error !== undefined) next.lastError = error; else delete next.lastError; this.put(next); @@ -309,55 +313,6 @@ export class ObservationService { return this.deps.runner.killFor(target) > 0; } - /** - * Stop one owned session and close its Agent Window (the overlay's stop - * button — same end state as `browser_session` action=stop). Never waits behind a - * hung in-flight command: tool children are killed first so the session's - * keyed queue drains immediately, and no further captures queue up. A - * session the daemon already forgot stops idempotently — the goal state - * (entry gone) is identical. - * @returns false only for a foreign session; bsk failures reject so callers - * can preserve the structured error instead of silently leaving a ghost. - */ - async stopSession(sessionId: string, signal?: AbortSignal): Promise { - if (!this.deps.registry.isOwned(sessionId)) return false; - const releaseForeground = this.acquireForeground(sessionId); - let actionError: string | undefined; - this.beginAction(sessionId, "stopping"); - try { - this.deps.runner.killFor(sessionId); - const result = await this.deps.queue.run( - sessionId, - () => - runWithSessionBusyRetry( - () => - this.deps.runner.run(["session", "stop", sessionId], { - signal, - timeoutMs: 30_000, - tag: sessionId, - }), - signal, - ), - signal, - ); - if (result.aborted) throw abortError(); - try { - parseBskJson(result, "session stop"); - } catch (error) { - if (!isSessionNotFoundError(error)) throw error; - } - this.deps.registry.remove(sessionId); - this.removeSession(sessionId); - return true; - } catch (error) { - actionError = error instanceof Error ? error.message.split("\n")[0] : String(error); - throw error; - } finally { - this.endAction(sessionId, actionError); - releaseForeground(); - } - } - /** * Read one captured thumbnail from the in-process ring. Powers the plugin's * own HTTP thumbnail route — frames are plugin-owned runtime data, never @@ -408,6 +363,7 @@ export class ObservationService { const entry = this.observations.get(sessionId); return ( this.deps.options.enabled && + this.deps.registry.isUsable(sessionId) && !this.disposed && this.thumbnailViewers > 0 && !this.foregroundDepth.has(sessionId) && @@ -416,6 +372,11 @@ export class ObservationService { ); } + private restingAction(sessionId: string): string { + const state = this.deps.registry.stateFor(sessionId); + return state === "cleanup" ? "awaiting cleanup" : state === "starting" ? "starting" : "idle"; + } + /** Schedule the next capture for a session; `delayMs` 0 means "as soon as the event loop allows". */ private scheduleCapture(sessionId: string, delayMs?: number): void { if (!this.canCapture(sessionId)) return; @@ -535,16 +496,6 @@ function isSessionNotFoundCode(code: string | undefined): boolean { return code === "not_found" || code === "session_not_found"; } -function isSessionNotFoundError(error: unknown): boolean { - return error instanceof BskError && isSessionNotFoundCode(error.code); -} - -function abortError(): Error { - const error = new Error("tool call aborted"); - error.name = "AbortError"; - return error; -} - /** Map a bsk command label onto its observation action verb. */ export function actionForLabel(label: string): string { switch (label) { diff --git a/packages/dsh-plugin-browserskill/src/session-starts.ts b/packages/dsh-plugin-browserskill/src/session-starts.ts new file mode 100644 index 00000000..213de8b9 --- /dev/null +++ b/packages/dsh-plugin-browserskill/src/session-starts.ts @@ -0,0 +1,457 @@ +import { randomUUID } from "node:crypto"; +import { + bskInstallMessage, + isCommandNotFound, + parseBskJson, + runWithSessionBusyRetry, +} from "./runner"; +import { memoryStartJournal, type StartJournal, type StartRecord } from "./start-journal"; +import type { ToolDeps } from "./tools"; + +interface RequestStatus { + state: + | "prepared" + | "unknown" + | "starting" + | "ready" + | "active" + | "cancelling" + | "cleanup_failed" + | "closed" + | "failed"; + session?: { session_id: string; browser_instance_id: string } | null; + cleanup_error?: string | null; +} + +export interface StopOptions { + sessionId?: string; + /** Exact target for recovery, including starts that never returned a session ID. */ + requestId?: string; + signal?: AbortSignal; +} + +export interface StopResult { + stopped: string; + requestId: string; + alreadyClosed: boolean; +} + +/** The one lifecycle owner for pending, live, and failed-cleanup starts. */ +export class SessionStarts { + private closing = false; + private timer?: ReturnType; + private readonly cleaning = new Map>(); + private readonly reserved = new Set(); + + constructor( + private readonly deps: ToolDeps, + readonly journal: StartJournal = memoryStartJournal(), + ) {} + + begin(owners: string[]): StartRecord { + if (this.closing) throw new Error("browser plugin is unloading"); + const archived = this.deps.ctx.get("workspaceRegistry") as + | { archivedSessionIds?: string[] } + | undefined; + if (owners.some((id) => archived?.archivedSessionIds?.includes(id))) + throw new Error("browser conversation is archived"); + // Recovered requests may still own windows even without a returned session + // ID. Count them before admitting more work into this plugin's capacity. + const recoveredPending = [...this.journal.records.values()].filter( + (r) => + r.stop !== "closed" && !this.reserved.has(r.requestId) && !this.ownsRegisteredSession(r), + ).length; + this.deps.registry.reserveStart(recoveredPending); + const record: StartRecord = { + requestId: `${Date.now() + 5 * 60_000}:${randomUUID()}`, + owners, + startedAtMs: Date.now(), + cleanup: false, + }; + this.reserved.add(record.requestId); + this.journal.records.set(record.requestId, record); + try { + this.journal.save(); + } catch (error) { + this.forget(record); + throw error; + } + return record; + } + + async prepare(record: StartRecord, signal: AbortSignal): Promise { + try { + const result = await this.deps.runner.run( + ["session", "request", record.requestId, "--prepare"], + { signal, timeoutMs: 30_000 }, + ); + if (signal.aborted || result.aborted) + throw new DOMException("tool call aborted", "AbortError"); + const status = parseBskJson(result, "session request prepare") as RequestStatus; + if (status.state !== "prepared") + throw new Error("Browser start preparation failed; use matching CLI and daemon versions."); + this.assertStarting(record); + } catch (error) { + // No start has been sent. An independently accepted stop still owns its + // cancellation job/receipt; a late prepare must not discard that intent. + if (!record.cleanup && !record.stop) this.forget(record); + if (isCommandNotFound(error)) throw new Error(bskInstallMessage(this.deps.config.bskPath)); + throw error; + } + } + + assertStarting(record: StartRecord): void { + if ( + this.closing || + record.cleanup || + record.stop || + !this.journal.records.has(record.requestId) + ) + throw new Error("browser start was cancelled during cleanup"); + } + + register(record: StartRecord, reply: { session_id: string; browser_instance_id: string }): void { + this.assertStarting(record); + if (typeof reply.session_id !== "string" || typeof reply.browser_instance_id !== "string") + throw new Error("invalid browser start result"); + record.session = { sessionId: reply.session_id, browserInstanceId: reply.browser_instance_id }; + this.journal.save(); + this.adopt(record); + } + + private adopt(record: StartRecord): void { + if (!record.session) return; + if (this.deps.registry.isOwned(record.session.sessionId)) { + if (this.ownsRegisteredSession(record)) return; + throw new Error("browser session id conflicts with another owned start"); + } + if (!this.reserved.has(record.requestId)) this.deps.registry.reserveStart(); + this.deps.registry.trackStart( + { + ...record.session, + requestId: record.requestId, + startedAtMs: record.startedAtMs, + }, + record.cleanup ? "cleanup" : "starting", + ); + this.reserved.delete(record.requestId); + this.deps.registry.trackOwner(record.session.sessionId, record.owners); + this.deps.observation.addSession(record.session.sessionId); + } + + async claim(record: StartRecord, signal: AbortSignal): Promise { + this.assertStarting(record); + if (!record.session || !this.ownsRegisteredSession(record)) + throw new Error("browser start must be registered before claiming it"); + const result = await this.deps.runner.run(["session", "request", record.requestId, "--claim"], { + timeoutMs: 30_000, + signal, + }); + if (signal.aborted || result.aborted) throw new DOMException("tool call aborted", "AbortError"); + const status = parseBskJson(result, "session request") as RequestStatus; + if (status.state !== "active") throw new Error("browser start could not be claimed"); + this.assertStarting(record); + this.deps.registry.activate(record.session.sessionId); + this.deps.observation.endAction(record.session.sessionId); + } + + /** Accept a durable stop; aborting its caller only cancels waiting, never cleanup. */ + async stop({ sessionId, requestId, signal }: StopOptions = {}): Promise { + if (signal?.aborted) throw abortError(); + if (this.closing) throw new Error("browser plugin is unloading"); + const record = this.resolveStop(sessionId, requestId); + const stopped = record.session?.sessionId ?? "pending browser starts"; + const alreadyClosed = record.stop === "closed"; + const implicit = !sessionId?.trim() && requestId === undefined; + let admitted = false; + try { + if (!alreadyClosed || (implicit && record.defaultStopRevision === undefined)) + this.requestCleanup(record, true, implicit); + admitted = true; + const revision = implicit ? record.defaultStopRevision : undefined; + if (!alreadyClosed) await waitForCleanup(this.cancel(record), signal); + if (signal?.aborted) throw abortError(); + // An explicit/overlay caller, or a concurrent waiter that predates another + // caller's failure, must not consume that caller's default retry target. + if (record.defaultStopRevision === undefined || revision === record.defaultStopRevision) { + this.journal.records.delete(record.requestId); + try { + this.journal.save(); + } catch (error) { + this.journal.records.set(record.requestId, record); + throw error; + } + } + return { stopped, requestId: record.requestId, alreadyClosed }; + } catch (error) { + if (implicit && admitted) { + // Restore even if another waiter just acknowledged completion. Keeping + // this receipt ensures the failed call can never fall through to A. + record.defaultStopRevision = (record.defaultStopRevision ?? 0) + 1; + this.journal.records.set(record.requestId, record); + try { + this.journal.save(); + } catch (saveError) { + console.warn("Browser stop retry receipt write failed", saveError); + } + } + throw error; + } + } + + private resolveStop(sessionId?: string, requestId?: string): StartRecord { + if (requestId !== undefined) { + if (sessionId !== undefined) throw new Error("Specify either session or requestId, not both"); + const record = this.journal.records.get(requestId); + if (!record) throw new Error("browser stop request does not belong to this plugin"); + return record; + } + const records = [...this.journal.records.values()]; + const receipts = records.filter((r) => r.stop !== undefined); + if (sessionId?.trim()) { + // Prefer a previous stop even if Chrome has since reused its short ID. + const receipt = receipts.find((r) => r.session?.sessionId === sessionId); + if (receipt) return receipt; + } else { + if (receipts.length === 1) return receipts[0]; + if (receipts.length > 1) { + throw new Error( + `Several stops await acknowledgement (${receipts.map((r) => `${r.session?.sessionId ?? "pending start"}: ${r.requestId}`).join(", ")}); specify session or requestId to retry one`, + ); + } + if (this.deps.registry.current() === undefined) { + const pending = records.filter((r) => r.cleanup); + if (pending.length === 1) return pending[0]; + if (pending.length > 1) + throw new Error( + `Several browser starts await cleanup (${pending.map((r) => r.requestId).join(", ")}); use list to retry cleanup or specify requestId`, + ); + } + } + const id = this.deps.registry.resolveForStop(sessionId); + const ownedRequestId = this.deps.registry.requestFor(id); + const record = ownedRequestId ? this.journal.records.get(ownedRequestId) : undefined; + if (!record || !this.ownsRegisteredSession(record)) + throw new Error(`browser session ${id} has no owned lifecycle request`); + return record; + } + + private requestCleanup(record: StartRecord, explicitStop = false, implicit = false): void { + const previous = { + cleanup: record.cleanup, + stop: record.stop, + defaultStopRevision: record.defaultStopRevision, + }; + if (record.stop !== "closed") { + record.cleanup = true; + if (explicitStop) record.stop = "pending"; + } + if (implicit) record.defaultStopRevision ??= 0; + try { + this.journal.save(); + } catch (error) { + if (explicitStop) { + // Nothing has been dispatched or made unusable yet: reject admission. + record.cleanup = previous.cleanup; + record.stop = previous.stop; + record.defaultStopRevision = previous.defaultStopRevision; + throw error; + } + // Startup/archival cleanup must continue even if the disk is unavailable. + // Its original ownership record still enables recovery after restart. + console.warn("Browser cleanup journal write failed", error); + } + if ( + record.session && + record.stop !== "closed" && + this.ownsRegisteredSession(record) && + this.deps.registry.stateFor(record.session.sessionId) !== "cleanup" + ) { + this.deps.registry.markForCleanup(record.session.sessionId); + this.deps.observation.endAction(record.session.sessionId); + } + this.schedule(); + } + + async fail(record: StartRecord): Promise { + if (!this.journal.records.has(record.requestId) || record.stop === "closed") return; + this.requestCleanup(record); + await this.cancel(record); + } + + private cancel(record: StartRecord): Promise { + const existing = this.cleaning.get(record.requestId); + if (existing) return existing; + // Publish the shared job before observation callbacks or runner hooks run. + const work = Promise.resolve() + .then(() => this.cancelOnce(record)) + .finally(() => { + this.cleaning.delete(record.requestId); + this.schedule(); + }); + this.cleaning.set(record.requestId, work); + return work; + } + + private async cancelOnce(record: StartRecord): Promise { + const sessionId = this.ownsRegisteredSession(record) ? record.session?.sessionId : undefined; + const release = sessionId ? this.deps.observation.acquireForeground(sessionId) : undefined; + let actionError: string | undefined; + try { + if (sessionId) { + this.deps.observation.beginAction(sessionId, "stopping"); + this.deps.runner.killFor(sessionId); + } + // All entry points share one job per request. Neither its queue slot nor + // its process belongs to the caller's abort signal or ordinary tool tag. + const run = () => + runWithSessionBusyRetry(() => + this.deps.runner.run(["session", "request", record.requestId, "--cancel"], { + timeoutMs: 30_000, + tag: `cleanup:${record.requestId}`, + }), + ); + const result = sessionId ? await this.deps.queue.run(sessionId, run) : await run(); + const status = parseBskJson(result, "session request cancel") as RequestStatus; + if (result.aborted) throw new Error("browser cleanup was interrupted"); + if (status.session) { + if ( + typeof status.session.session_id !== "string" || + typeof status.session.browser_instance_id !== "string" + ) + throw new Error("Invalid browser cleanup session identity; ownership retained"); + if ( + record.session && + (record.session.sessionId !== status.session.session_id || + record.session.browserInstanceId !== status.session.browser_instance_id) + ) + throw new Error( + "Browser cleanup returned a different session identity; ownership retained", + ); + } + if (status.state === "closed" || status.state === "failed") { + this.completeCleanup(record); + return; + } + if (status.session) { + record.session = { + sessionId: status.session.session_id, + browserInstanceId: status.session.browser_instance_id, + }; + this.journal.save(); + this.adopt(record); + } + throw new Error(status.cleanup_error ?? "browser start is still being cancelled"); + } catch (error) { + actionError = error instanceof Error ? error.message.split("\n")[0] : String(error); + throw error; + } finally { + if (sessionId && this.ownsRegisteredSession(record)) + this.deps.observation.endAction(sessionId, actionError); + release?.(); + } + } + + private releaseResource(record: StartRecord): void { + if (record.session && this.ownsRegisteredSession(record)) { + this.deps.registry.remove(record.session.sessionId); + this.deps.observation.removeSession(record.session.sessionId); + } + if (this.reserved.delete(record.requestId)) this.deps.registry.abandonStart(); + } + + private completeCleanup(record: StartRecord): void { + if (record.stop) { + record.stop = "closed"; + record.cleanup = false; + this.releaseResource(record); + this.journal.save(); + } else this.forget(record); + } + + private forget(record: StartRecord): void { + this.releaseResource(record); + this.journal.records.delete(record.requestId); + this.journal.save(); + } + + private ownsRegisteredSession(record: StartRecord): boolean { + return ( + record.session !== undefined && + this.deps.registry.requestFor(record.session.sessionId) === record.requestId + ); + } + + /** Forget passive session disappearance, preserving unacknowledged stop receipts. */ + forgetStopped(): void { + for (const record of this.journal.records.values()) { + if (!record.cleanup && !record.stop && record.session && !this.ownsRegisteredSession(record)) + this.forget(record); + } + } + + async reconcile(): Promise { + this.forgetStopped(); + await Promise.allSettled( + [...this.journal.records.values()].filter((r) => r.cleanup).map((r) => this.cancel(r)), + ); + } + + pendingCleanup(): number { + return [...this.journal.records.values()].filter((r) => r.cleanup).length; + } + + archive(owner: string): void { + for (const record of this.journal.records.values()) { + if (record.owners.includes(owner)) void this.fail(record).catch(() => {}); + } + } + + private schedule(): void { + if (this.closing || this.timer || this.pendingCleanup() === 0) return; + this.timer = setTimeout(() => { + this.timer = undefined; + void this.reconcile().catch((error) => { + console.warn("Browser start reconciliation failed", error); + this.schedule(); + }); + }, 15_000); + this.timer.unref(); + } + + async dispose(): Promise { + this.closing = true; + if (this.timer) clearTimeout(this.timer); + this.deps.runner.killAll(); + await Promise.allSettled([...this.journal.records.values()].map((r) => this.fail(r))); + this.journal.release(); + } +} + +function abortError(): Error { + return new DOMException("tool call aborted", "AbortError"); +} + +/** Detach one waiter without cancelling shared cleanup or leaking its rejection. */ +function waitForCleanup(work: Promise, signal?: AbortSignal): Promise { + if (!signal) return work; + return new Promise((resolve, reject) => { + const onAbort = () => { + signal.removeEventListener("abort", onAbort); + reject(abortError()); + }; + signal.addEventListener("abort", onAbort, { once: true }); + work.then( + () => { + signal.removeEventListener("abort", onAbort); + if (signal.aborted) reject(abortError()); + else resolve(); + }, + (error: unknown) => { + signal.removeEventListener("abort", onAbort); + reject(error); + }, + ); + if (signal.aborted) onAbort(); + }); +} diff --git a/packages/dsh-plugin-browserskill/src/sessions.ts b/packages/dsh-plugin-browserskill/src/sessions.ts index f14d7399..3a7120fd 100644 --- a/packages/dsh-plugin-browserskill/src/sessions.ts +++ b/packages/dsh-plugin-browserskill/src/sessions.ts @@ -15,12 +15,17 @@ export interface TrackedSession { sessionId: string; + /** Stable lifecycle handle, retained even if the short session ID is reused. */ + requestId?: string; browserInstanceId?: string; startedAtMs: number; /** Always true: only plugin-created sessions enter the registry at all. */ owned: boolean; + state: "starting" | "active" | "cleanup"; } +type NewSession = Omit; + export class SessionRegistry { private readonly sessions = new Map(); private currentId: string | undefined; @@ -44,8 +49,8 @@ export class SessionRegistry { * Reserve a start slot synchronously, BEFORE spawning. * @throws when the configured concurrency cap (tracked + in-flight) is reached. */ - reserveStart(): void { - if (this.sessions.size + this.pendingStarts >= this.maxSessions) { + reserveStart(recoveredPending = 0): void { + if (this.sessions.size + this.pendingStarts + recoveredPending >= this.maxSessions) { throw new Error( `session limit reached (${this.maxSessions} concurrent sessions); ` + "stop one with browser_session action=stop before starting another", @@ -63,7 +68,13 @@ export class SessionRegistry { * Register a freshly started session, consuming its reservation, and make * it current. */ - completeStart(session: Omit): void { + completeStart(session: NewSession): void { + this.trackStart(session); + this.activate(session.sessionId); + } + + /** Own a resource without exposing it to ordinary browser commands. */ + trackStart(session: NewSession, state: "starting" | "cleanup" = "starting"): void { this.pendingStarts = Math.max(0, this.pendingStarts - 1); // Backstop only: with the reservation protocol above this never fires. if (!this.sessions.has(session.sessionId) && this.sessions.size >= this.maxSessions) { @@ -72,8 +83,47 @@ export class SessionRegistry { "stop one with browser_session action=stop before starting another", ); } - this.sessions.set(session.sessionId, { ...session, owned: true }); - this.currentId = session.sessionId; + this.sessions.set(session.sessionId, { ...session, owned: true, state }); + } + + /** Publish only after initialization and the daemon claim have succeeded. */ + activate(sessionId: string): void { + const session = this.sessions.get(sessionId); + if (session?.state !== "starting") + throw new Error("browser start is no longer awaiting activation"); + session.state = "active"; + this.touch(sessionId); + } + + markForCleanup(sessionId: string): void { + const session = this.sessions.get(sessionId); + if (!session) return; + session.state = "cleanup"; + if (this.currentId === sessionId) this.selectCurrent(); + } + + private selectCurrent(): void { + this.currentId = [...this.sessions.values()] + .filter((s) => s.state === "active") + .at(-1)?.sessionId; + } + + isUsable(sessionId: string): boolean { + return this.sessions.get(sessionId)?.state === "active"; + } + + stateFor(sessionId: string): TrackedSession["state"] | undefined { + return this.sessions.get(sessionId)?.state; + } + + assertUsable(sessionId: string, toolName: string): void { + if (!this.isOwned(sessionId)) throw this.foreignError(sessionId, toolName); + if (!this.isUsable(sessionId)) { + const reason = this.stateFor(sessionId) === "cleanup" ? "awaiting cleanup" : "not ready"; + throw new Error( + `${toolName}: session "${sessionId}" is ${reason}; only stop is available until the session is active`, + ); + } } /** Forget a session; falls back to the most recent remaining one. */ @@ -81,8 +131,7 @@ export class SessionRegistry { this.sessions.delete(sessionId); this.dshOwners.delete(sessionId); if (this.currentId === sessionId) { - const rest = [...this.sessions.values()]; - this.currentId = rest.length > 0 ? rest[rest.length - 1].sessionId : undefined; + this.selectCurrent(); } } @@ -141,6 +190,10 @@ export class SessionRegistry { return this.sessions.get(sessionId)?.owned === true; } + requestFor(sessionId: string): string | undefined { + return this.sessions.get(sessionId)?.requestId; + } + size(): number { return this.sessions.size; } @@ -161,7 +214,7 @@ export class SessionRegistry { */ resolve(explicit: string | undefined, toolName: string): string { if (explicit !== undefined && explicit.trim().length > 0) { - if (!this.isOwned(explicit)) throw this.foreignError(explicit, toolName); + this.assertUsable(explicit, toolName); this.touch(explicit); return explicit; } @@ -176,12 +229,15 @@ export class SessionRegistry { } /** - * Resolve the session a STOP call acts on. Same ownership rule as every - * other tool; a rejected stop never moves the current pointer. + * Stop can also reach starting/cleanup resources. If no usable session is + * current, default to the most recent owned resource so cleanup is retryable. + * A rejected stop never moves the current pointer. */ resolveForStop(explicit: string | undefined): string { const candidate = - explicit !== undefined && explicit.trim().length > 0 ? explicit : this.current(); + explicit !== undefined && explicit.trim().length > 0 + ? explicit + : (this.current() ?? this.list().at(-1)?.sessionId); if (candidate === undefined) { throw new Error( "browser_session action=stop needs a session but none is active — use action=start first", diff --git a/packages/dsh-plugin-browserskill/src/start-journal.ts b/packages/dsh-plugin-browserskill/src/start-journal.ts new file mode 100644 index 00000000..d9ade915 --- /dev/null +++ b/packages/dsh-plugin-browserskill/src/start-journal.ts @@ -0,0 +1,158 @@ +/** Write-ahead ownership for starts, including ones whose CLI never replies. */ +import { createHash, randomUUID } from "node:crypto"; +import { + closeSync, + existsSync, + fsyncSync, + mkdirSync, + openSync, + readdirSync, + readFileSync, + renameSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { homedir } from "node:os"; +import { join, resolve } from "node:path"; + +export interface StartRecord { + requestId: string; + owners: string[]; + startedAtMs: number; + cleanup: boolean; + /** An explicit stop stays retryable until its caller acknowledges completion. */ + stop?: "pending" | "closed"; + /** A failed implicit waiter advances this revision; other callers cannot consume its retry. */ + defaultStopRevision?: number; + session?: { sessionId: string; browserInstanceId: string }; +} + +export interface StartJournal { + records: Map; + save(): void; + release(): void; +} + +export function memoryStartJournal(): StartJournal { + return { records: new Map(), save() {}, release() {} }; +} + +const liveKey = Symbol.for("browser-skill.live-start-journals"); +const globals = globalThis as typeof globalThis & { [liveKey]?: Set }; +const live = (globals[liveKey] ??= new Set()); + +export function defaultStartJournalDirectory(bskPath: string): string { + const scope = createHash("sha256") + .update(JSON.stringify([process.cwd(), bskPath])) + .digest("hex") + .slice(0, 24); + return join(process.env.BSK_HOME ?? join(homedir(), ".bsk"), "dsh-starts", scope); +} + +function processAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + return (error as NodeJS.ErrnoException).code !== "ESRCH"; + } +} + +/** Separate owner directories avoid overwriting another live plugin's ledger. + * Stale directories are claimed by atomic rename before reading their records. + * A reused PID is conservatively treated as live; daemon leases/idle reaping + * still bound the abandoned browser lifetime in that case. + */ +export class DiskStartJournal implements StartJournal { + readonly records = new Map(); + private readonly owner = `${process.pid}-${randomUUID()}`; + private readonly directory: string; + private initialized = false; + + constructor(private readonly root: string) { + this.directory = join(resolve(root), this.owner); + live.add(this.owner); + } + + recover(): void { + if (this.initialized) return; + mkdirSync(this.root, { recursive: true, mode: 0o700 }); + mkdirSync(this.directory, { mode: 0o700 }); + this.initialized = true; + for (const dir of readdirSync(this.root, { withFileTypes: true })) { + const match = /^(\d+)-([a-f0-9-]{36})$/.exec(dir.name); + if (!dir.isDirectory() || !match || dir.name === this.owner) continue; + const pid = Number(match[1]); + if (pid === process.pid ? live.has(dir.name) : processAlive(pid)) continue; + const claimed = join(this.directory, `recovered-${dir.name}`); + try { + renameSync(join(this.root, dir.name), claimed); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") continue; + throw error; + } + this.readRecovered(claimed); + this.save(); + rmSync(claimed, { recursive: true }); + } + } + + private readRecovered(directory: string): void { + const file = join(directory, "requests.json"); + if (existsSync(file)) { + const records: unknown = JSON.parse(readFileSync(file, "utf8")); + if (!Array.isArray(records)) throw new Error(`Invalid browser start journal: ${file}`); + for (const item of records) { + if ( + !item || + typeof item.requestId !== "string" || + !/^\d+:[a-f0-9-]{36}$/.test(item.requestId) || + !Array.isArray(item.owners) || + !item.owners.every((id: unknown) => typeof id === "string") || + typeof item.startedAtMs !== "number" || + (item.stop !== undefined && item.stop !== "pending" && item.stop !== "closed") || + (item.defaultStopRevision !== undefined && + (!Number.isSafeInteger(item.defaultStopRevision) || + item.defaultStopRevision < 0 || + item.stop === undefined)) + ) + throw new Error(`Invalid browser start journal: ${file}`); + this.records.set(item.requestId, { ...item, cleanup: item.stop !== "closed" }); + } + } + // A crash during adoption must not lose the already-renamed source. + for (const entry of readdirSync(directory, { withFileTypes: true })) { + if (entry.isDirectory() && entry.name.startsWith("recovered-")) + this.readRecovered(join(directory, entry.name)); + } + } + + save(): void { + if (!this.initialized) this.recover(); + const temporary = join(this.directory, "requests.tmp"); + const fd = openSync(temporary, "w", 0o600); + try { + writeFileSync(fd, JSON.stringify([...this.records.values()])); + fsyncSync(fd); + } finally { + closeSync(fd); + } + renameSync(temporary, join(this.directory, "requests.json")); + // Directory fsync is supported on Unix; Windows atomic rename remains the + // durability boundary and does not allow opening directories this way. + if (process.platform !== "win32") { + const dir = openSync(this.directory, "r"); + try { + fsyncSync(dir); + } finally { + closeSync(dir); + } + } + } + + release(): void { + live.delete(this.owner); + if (this.initialized && this.records.size === 0) + rmSync(this.directory, { recursive: true, force: true }); + } +} diff --git a/packages/dsh-plugin-browserskill/src/tool-params.ts b/packages/dsh-plugin-browserskill/src/tool-params.ts index 188549ab..de3f776f 100644 --- a/packages/dsh-plugin-browserskill/src/tool-params.ts +++ b/packages/dsh-plugin-browserskill/src/tool-params.ts @@ -7,6 +7,21 @@ export const SESSION_PARAM = { "Omit to use the current session (the one most recently started or used).", } as const; +export const SESSION_STOP_PARAMS = { + session: { + type: "string", + description: + "For stop: owned session ID; mutually exclusive with requestId. Omit both targets to retry " + + "an unacknowledged stop before using the current session.", + }, + requestId: { + type: "string", + description: + "For stop: owned lifecycle request ID to stop or acknowledge; mutually exclusive with session. " + + "Targets the original start even if its short session ID is reused or was never received.", + }, +} as const; + export const TAB_ID_PARAM = { type: "integer", description: "Target tab id. Omit to use the Agent Window's active tab.", diff --git a/packages/dsh-plugin-browserskill/src/tools.ts b/packages/dsh-plugin-browserskill/src/tools.ts index 5d6a3e07..ad18dc9e 100644 --- a/packages/dsh-plugin-browserskill/src/tools.ts +++ b/packages/dsh-plugin-browserskill/src/tools.ts @@ -28,12 +28,15 @@ import { parseBskJson, runWithSessionBusyRetry, } from "./runner"; +import { SessionStarts } from "./session-starts"; import type { SessionRegistry } from "./sessions"; -import { SESSION_PARAM } from "./tool-params"; +import { SESSION_PARAM, SESSION_STOP_PARAMS } from "./tool-params"; /** Plugin configuration resolved from the Schemastery schema in index.ts. */ export interface PluginConfig { bskPath: string; + /** Optional durable start journal directory (isolated by host/profile when configured). */ + sessionStateDirectory?: string; defaultTimeoutMs: number; maxSessions: number; observationEnabled: boolean; @@ -56,6 +59,7 @@ export interface ToolDeps { observation: ObservationService; /** Per-session FIFO: the daemon rejects a second command while one is unfinished. */ queue: KeyedExecutor; + starts?: SessionStarts; } /** Device presets supported by `bsk emulate --device`. */ @@ -83,6 +87,7 @@ async function runBsk( label: string, observeSession?: string, runnerTimeoutMs?: number, + initializing = false, ): Promise { const releaseForeground = observeSession !== undefined ? deps.observation.acquireForeground(observeSession) : undefined; @@ -99,15 +104,19 @@ async function runBsk( began = true; deps.observation.beginAction(observeSession, actionForLabel(label)); } - return runWithSessionBusyRetry( - () => - deps.runner.run(args, { - signal: exec.signal, - timeoutMs: runnerTimeoutMs ?? deps.config.defaultTimeoutMs, - ...(observeSession !== undefined ? { tag: observeSession } : {}), - }), - exec.signal, - ); + return runWithSessionBusyRetry(async () => { + if ( + observeSession !== undefined && + !(initializing && deps.registry.stateFor(observeSession) === "starting") + ) { + deps.registry.assertUsable(observeSession, label); + } + return deps.runner.run(args, { + signal: exec.signal, + timeoutMs: runnerTimeoutMs ?? deps.config.defaultTimeoutMs, + ...(observeSession !== undefined ? { tag: observeSession } : {}), + }); + }, exec.signal); }; result = observeSession !== undefined @@ -228,8 +237,10 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): } // Reserve the slot BEFORE spawning: check-and-reserve is synchronous, // so concurrent starts can never both pass the cap and leak a session. - registry.reserveStart(); - const startArgs = ["session", "start"]; + const starts = (deps.starts ??= new SessionStarts(deps)); + await starts.reconcile(); + const record = starts.begin(ownerSessionIds(deps.ctx, exec.agent?.id)); + const startArgs = ["session", "start", "--request-id", record.requestId]; if (args.width !== undefined && args.height !== undefined) { startArgs.push("--width", String(args.width), "--height", String(args.height)); } @@ -237,21 +248,15 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): if (args.browser !== undefined) startArgs.push("--browser", args.browser); let reply: { session_id: string; browser_instance_id: string }; try { + await starts.prepare(record, exec.signal); reply = (await runBsk(deps, exec, startArgs, "session start")) as typeof reply; } catch (error) { - registry.abandonStart(); + await starts.fail(record).catch(() => {}); throw error; } - registry.completeStart({ - sessionId: reply.session_id, - browserInstanceId: reply.browser_instance_id, - startedAtMs: Date.now(), - }); - // Ownership for archive cleanup: the calling conversation and its - // ancestors reap this session when any of them is archived. - registry.trackOwner(reply.session_id, ownerSessionIds(deps.ctx, exec.agent?.id)); - deps.observation.addSession(reply.session_id, args.url); try { + starts.register(record, reply); + deps.observation.addSession(reply.session_id, args.url); if (args.device !== undefined) { await runBsk( deps, @@ -259,6 +264,8 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): ["emulate", "--session", reply.session_id, "--device", args.device], "emulate", reply.session_id, + undefined, + true, ); } if (args.url !== undefined) { @@ -268,19 +275,13 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): ["navigate", "--session", reply.session_id, args.url], "navigate", reply.session_id, + undefined, + true, ); } + await starts.claim(record, exec.signal); } catch (error) { - // A half-initialized session must not leak: stop it before surfacing. - // Reuse the same capture-preempting, idempotent path as explicit and - // overlay stops; local ownership is dropped even when daemon cleanup - // itself fails, because this start never becomes usable to the model. - try { - await deps.observation.stopSession(reply.session_id); - } catch { - registry.remove(reply.session_id); - deps.observation.removeSession(reply.session_id); - } + await starts.fail(record).catch(() => {}); throw error; } return { @@ -309,35 +310,51 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): name: "session.stop", description: "Stop a browser session and close its Agent Window. Stops the given session, or the " + - "current session when `session` is omitted. Only plugin-created sessions " + - "can be stopped — sessions owned by other programs sharing the bsk daemon are refused.", - parameters: { session: SESSION_PARAM }, + "current session when `session` is omitted. An unacknowledged stop is retried before " + + "selecting another session; specify session or requestId if several stops are pending. " + + "A requestId identifies the original start even if its short session ID is reused. Once accepted, " + + "cleanup continues if this call is aborted. Only plugin-created sessions can be stopped.", + parameters: SESSION_STOP_PARAMS, output: { schema: { type: "object", additionalProperties: false, - properties: { stopped: { type: "string", required: true } }, + properties: { + stopped: { type: "string", required: true }, + requestId: { type: "string", required: true }, + alreadyClosed: { type: "boolean", required: true }, + }, }, render: (_args, value) => [ - { type: "text", text: `stopped browser session ${value.stopped}` }, + { + type: "text", + text: `${value.alreadyClosed ? "previous stop completed for" : "stopped browser session"} ${value.stopped} (request ${value.requestId})`, + }, ], }, async execute(args, exec) { - const sessionId = registry.resolveForStop(args.session); try { - const stopped = await deps.observation.stopSession(sessionId, exec.signal); - if (!stopped) throw new Error(`browser session ${sessionId} is not owned by this plugin`); + const starts = (deps.starts ??= new SessionStarts(deps)); + return await starts.stop({ + sessionId: args.session, + requestId: args.requestId, + signal: exec.signal, + }); } catch (error) { if (isCommandNotFound(error)) { throw new Error(bskInstallMessage(deps.config.bskPath)); } throw error; } - return { stopped: sessionId }; }, presentCall: (args) => ({ card: "terminal", - title: cmdline(deps, ["session", "stop", args.session ?? "(current session)"]), + title: cmdline( + deps, + args.requestId + ? ["session", "request", args.requestId, "--cancel"] + : ["session", "stop", args.session ?? "(current or pending stop)"], + ), description: "Stop a browser session", }), presentResult: presentTerminalResult, @@ -357,6 +374,7 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): type: "object", additionalProperties: false, properties: { + pendingCleanup: { type: "integer", required: true }, sessions: { type: "array", required: true, @@ -366,7 +384,13 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): properties: { sessionId: { type: "string", required: true }, browserInstanceId: { type: "string", required: true }, + requestId: { type: "string" }, current: { type: "boolean", required: true }, + state: { + type: "string", + enum: ["starting", "active", "cleanup"], + required: true, + }, }, }, }, @@ -377,26 +401,31 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): type: "text", text: value.sessions.length === 0 - ? "no active browser sessions" + ? value.pendingCleanup > 0 + ? `${value.pendingCleanup} browser start(s) awaiting cleanup; retry list or stop` + : "no active browser sessions" : value.sessions .map( (s) => - `${s.sessionId} (browser ${s.browserInstanceId})${s.current ? " [current]" : ""}`, + `${s.sessionId} (browser ${s.browserInstanceId})${s.current ? " [current]" : ""}${s.state !== "active" ? ` [${s.state}]` : ""}${s.requestId ? ` (request ${s.requestId})` : ""}`, ) .join("\n"), }, ], }, isConcurrencySafe: () => true, - // Registry-only by design: no daemon call, so foreign sessions on a - // shared daemon can never even be SEEN through this tool. + // Reconcile only our journaled requests; never enumerate foreign sessions. async execute() { + await deps.starts?.reconcile(); const current = registry.current(); return { + pendingCleanup: deps.starts?.pendingCleanup() ?? 0, sessions: registry.list().map((entry) => ({ sessionId: entry.sessionId, browserInstanceId: entry.browserInstanceId ?? "", + ...(entry.requestId ? { requestId: entry.requestId } : {}), current: entry.sessionId === current, + state: entry.state, })), }; }, diff --git a/packages/dsh-plugin-browserskill/tests/archive-cleanup.test.ts b/packages/dsh-plugin-browserskill/tests/archive-cleanup.test.ts index 9115faf7..67a68715 100644 --- a/packages/dsh-plugin-browserskill/tests/archive-cleanup.test.ts +++ b/packages/dsh-plugin-browserskill/tests/archive-cleanup.test.ts @@ -4,7 +4,6 @@ import { describe, expect, it, vi } from "vitest"; import { armArchiveCleanup, ownerSessionIds } from "../src/archive-cleanup"; -import type { ObservationService } from "../src/observation"; import { SessionRegistry } from "../src/sessions"; /** A ctx stub carrying a session store with the given lineage headers. */ @@ -71,8 +70,7 @@ describe("SessionRegistry owner tracking", () => { describe("armArchiveCleanup", () => { function harness(opts: { archived?: string[] } = {}) { - const registry = new SessionRegistry(5); - const observation = { stopSession: vi.fn(async () => true) }; + const lifecycle = { archive: vi.fn() }; const listeners = new Set<(change: unknown) => void>(); const ctx = { get: (key: string) => @@ -86,33 +84,22 @@ describe("armArchiveCleanup", () => { for (const listener of [...listeners]) listener(change); }; return { - registry, - observation: observation as unknown as ObservationService, - stopSession: observation.stopSession, + archive: lifecycle.archive, emit, - arm: () => - armArchiveCleanup(ctx as never, registry, observation as unknown as ObservationService), + arm: () => armArchiveCleanup(ctx as never, lifecycle), }; } - function startOwned(registry: SessionRegistry, bskId: string, owners: string[]): void { - registry.reserveStart(); - registry.completeStart({ sessionId: bskId, startedAtMs: 1 }); - registry.trackOwner(bskId, owners); - } - - it("stops every bsk session owned by a freshly archived conversation, until disarmed", () => { + it("forwards fresh archive ownership to the lifecycle manager, until disarmed", () => { const h = harness(); - startOwned(h.registry, "bsk1", ["conv-a", "root"]); - startOwned(h.registry, "bsk2", ["conv-b"]); const disarm = h.arm(); h.emit({ domain: "workspace", table: "", value: { archivedSessionIds: ["root"] }, }); - expect(h.stopSession).toHaveBeenCalledTimes(1); - expect(h.stopSession).toHaveBeenCalledWith("bsk1"); + expect(h.archive).toHaveBeenCalledTimes(1); + expect(h.archive).toHaveBeenCalledWith("root"); // After the disposer runs the watcher is silent again. disarm(); h.emit({ @@ -120,38 +107,44 @@ describe("armArchiveCleanup", () => { table: "", value: { archivedSessionIds: ["root", "conv-b"] }, }); - expect(h.stopSession).toHaveBeenCalledTimes(1); + expect(h.archive).toHaveBeenCalledTimes(1); }); it("ignores pre-archived ids, foreign domains, and malformed frames", () => { const h = harness({ archived: ["old-conv"] }); - startOwned(h.registry, "bsk1", ["old-conv"]); - startOwned(h.registry, "bsk2", ["conv-a"]); h.arm(); // Seeded from the registry: the pre-archived id must not retro-fire. h.emit({ domain: "workspace", table: "", value: { archivedSessionIds: ["old-conv"] } }); h.emit({ domain: "settings", table: "", value: { archivedSessionIds: ["conv-a"] } }); h.emit({ domain: "workspace", table: "rows", value: { archivedSessionIds: ["conv-a"] } }); h.emit({ domain: "workspace", table: "", value: {} }); - expect(h.stopSession).not.toHaveBeenCalled(); + expect(h.archive).not.toHaveBeenCalled(); // A genuinely new archive still fires. h.emit({ domain: "workspace", table: "", value: { archivedSessionIds: ["old-conv", "conv-a"] }, }); - expect(h.stopSession).toHaveBeenCalledWith("bsk2"); + expect(h.archive).toHaveBeenCalledWith("conv-a"); + }); + + it("handles the host updating its registry before broadcasting the first archive", () => { + const options = { archived: [] as string[] }; + const h = harness(options); + h.arm(); + options.archived = ["conv-a"]; + h.emit({ domain: "workspace", table: "", value: { archivedSessionIds: ["conv-a"] } }); + expect(h.archive).toHaveBeenCalledWith("conv-a"); }); it("treats a re-archived session as fresh again after unarchive", () => { const h = harness(); - startOwned(h.registry, "bsk1", ["conv-a"]); h.arm(); h.emit({ domain: "workspace", table: "", value: { archivedSessionIds: ["conv-a"] } }); - expect(h.stopSession).toHaveBeenCalledTimes(1); + expect(h.archive).toHaveBeenCalledTimes(1); // Unarchive, then re-archive: the second archival cleans up again. h.emit({ domain: "workspace", table: "", value: { archivedSessionIds: [] } }); h.emit({ domain: "workspace", table: "", value: { archivedSessionIds: ["conv-a"] } }); - expect(h.stopSession).toHaveBeenCalledTimes(2); + expect(h.archive).toHaveBeenCalledTimes(2); }); }); diff --git a/packages/dsh-plugin-browserskill/tests/dispose.test.ts b/packages/dsh-plugin-browserskill/tests/dispose.test.ts index e0cdbb7d..00f422a4 100644 --- a/packages/dsh-plugin-browserskill/tests/dispose.test.ts +++ b/packages/dsh-plugin-browserskill/tests/dispose.test.ts @@ -7,6 +7,7 @@ import type { ToolDefinition, ToolRunContext } from "@deepseek-ai/dsh-tools"; import { describe, expect, it } from "vitest"; import { apply } from "../src/index"; import type { BskRunOptions, BskRunResult } from "../src/runner"; +import { memoryStartJournal } from "../src/start-journal"; interface FakeCall { args: string[]; @@ -46,6 +47,14 @@ describe("dispose cleanup ownership", () => { calls.push({ args }); const joined = args.join(" "); if (joined.startsWith("status")) return ok({}); + if (joined.startsWith("session request")) + return ok({ + state: args.includes("--prepare") + ? "prepared" + : args.includes("--claim") + ? "active" + : "closed", + }); if (joined.startsWith("session start")) return ok(startReplies.shift()); if (joined.startsWith("snapshot")) { return ok({ text: "x", ref_count: 1, tab_id: 7, truncated: false }); @@ -71,7 +80,11 @@ describe("dispose cleanup ownership", () => { disposers.push(fn()); }, }; - apply(ctx as never, { maxSessions: 5, lazyTools: false }, { runnerFactory: () => runner }); + apply( + ctx as never, + { maxSessions: 5, lazyTools: false }, + { runnerFactory: () => runner, startJournal: memoryStartJournal() }, + ); const session = tools.get("browser_session"); const inspect = tools.get("browser_inspect"); @@ -87,8 +100,9 @@ describe("dispose cleanup ownership", () => { const stops = calls .map((call) => call.args.join(" ")) - .filter((joined) => joined.startsWith("session stop")); - expect(stops.sort()).toEqual(["session stop own1", "session stop own2"]); + .filter((joined) => joined.startsWith("session request") && joined.endsWith("--cancel")); + expect(stops).toHaveLength(2); + expect(new Set(stops).size).toBe(2); expect(stops.some((joined) => joined.includes("ext9"))).toBe(false); }); }); diff --git a/packages/dsh-plugin-browserskill/tests/lazy-tools.test.ts b/packages/dsh-plugin-browserskill/tests/lazy-tools.test.ts index e70a6323..ab557bff 100644 --- a/packages/dsh-plugin-browserskill/tests/lazy-tools.test.ts +++ b/packages/dsh-plugin-browserskill/tests/lazy-tools.test.ts @@ -8,6 +8,7 @@ import { describe, expect, it, vi } from "vitest"; import { apply } from "../src/index"; import { armLazyTools, hasSuccessfulSkillInvocation } from "../src/lazy-tools"; import type { BskRunOptions, BskRunResult } from "../src/runner"; +import { memoryStartJournal } from "../src/start-journal"; function fakeEventCtx(sessions?: { list(): { events: unknown[] }[] }) { const listeners = new Map void>(); @@ -205,7 +206,10 @@ describe("lazyTools wiring in apply()", () => { killAll() {}, killFor: () => 0, }; - apply(ctx as never, config, { runnerFactory: () => runner as never }); + apply(ctx as never, config, { + runnerFactory: () => runner as never, + startJournal: memoryStartJournal(), + }); return { tools, listeners }; } diff --git a/packages/dsh-plugin-browserskill/tests/observation.test.ts b/packages/dsh-plugin-browserskill/tests/observation.test.ts index af857b2d..bd42161a 100644 --- a/packages/dsh-plugin-browserskill/tests/observation.test.ts +++ b/packages/dsh-plugin-browserskill/tests/observation.test.ts @@ -142,6 +142,9 @@ function setup(opts: { queue?: KeyedExecutor; }) { const registry = opts.registry ?? new SessionRegistry(5); + // Ordinary observation fixtures represent an initialized, usable session. + // Lifecycle tests supply their own registry to exercise other states. + if (!opts.registry) own(registry, "s1"); const runner = opts.runner ?? fakeRunner(); const scheduler = opts.scheduler ?? fakeScheduler(); const service = new ObservationService({ @@ -178,6 +181,35 @@ async function waitFor(cond: () => boolean, timeoutMs = 10_000): Promise { } describe("state machine", () => { + it("shows pending resources without capturing until activation, and stops captures during cleanup", async () => { + const registry = new SessionRegistry(5); + registry.trackStart({ sessionId: "pending", startedAtMs: 1 }); + const { service, scheduler, runner } = setup({ registry }); + service.addSession("pending"); + expect(service.getState()[0].action).toBe("starting"); + expect(scheduler.pending()).toEqual([]); + expect(runner.calls).toEqual([]); + registry.activate("pending"); + service.endAction("pending"); + expect(service.getState()[0].action).toBe("idle"); + expect(scheduler.pending()).toEqual([0]); + scheduler.runNext(); + await waitFor( + () => + service.getState()[0].thumbnailAttachmentId !== undefined && + scheduler.pending().length === 1, + ); + registry.markForCleanup("pending"); + service.endAction("pending"); + expect(service.getState()[0].action).toBe("awaiting cleanup"); + // Even a previously scheduled capture must recheck usability before running. + scheduler.runNext(); + expect(runner.calls).toHaveLength(1); + expect(scheduler.pending()).toEqual([]); + expect(registry.isOwned("pending")).toBe(true); + service.dispose(); + }); + it("tracks add/action/end/url/remove and emits upsert/remove/reset events", () => { const { service, events } = setup({}); service.addSession("s1"); @@ -516,58 +548,15 @@ describe("interrupt routing", () => { }); }); -describe("stopSession", () => { - it("stops an owned session: kills in-flight tools, runs session stop, removes the entry", async () => { - const registry = new SessionRegistry(5); - own(registry, "s1"); - const runner = fakeRunner(); - const { service, events } = setup({ registry, runner }); - service.addSession("s1"); - expect(registry.isOwned("s1")).toBe(true); - - await expect(service.stopSession("s1")).resolves.toBe(true); - expect(runner.killed).toEqual(["observation:s1", "s1"]); - const stop = runner.calls.find((c) => c.args[0] === "session" && c.args[1] === "stop"); - expect(stop?.args).toEqual(["session", "stop", "s1"]); - expect(registry.isOwned("s1")).toBe(false); - expect(service.getState()).toEqual([]); - expect(events[events.length - 1]).toMatchObject({ type: "remove" }); - }); - - it("refuses foreign sessions without touching the runner", async () => { - const registry = new SessionRegistry(5); - own(registry, "s1"); - const runner = fakeRunner(); - const { service } = setup({ registry, runner }); - await expect(service.stopSession("foreign")).resolves.toBe(false); - expect(runner.killed).toEqual([]); - expect(runner.calls).toEqual([]); - }); - - it("stops idempotently when the daemon already forgot the session (dead entries)", async () => { - const registry = new SessionRegistry(5); - own(registry, "s1"); - const runner = fakeRunner({ stopNotFound: true }); - const { service, events } = setup({ registry, runner }); - service.addSession("s1"); - await expect(service.stopSession("s1")).resolves.toBe(true); - expect(registry.isOwned("s1")).toBe(false); - expect(events[events.length - 1]).toMatchObject({ type: "remove" }); - }); - - it("rejects and keeps the entry when the stop itself fails", async () => { - const registry = new SessionRegistry(5); - own(registry, "s1"); - const runner = fakeRunner({ stopFails: true }); - const { service } = setup({ registry, runner }); - service.addSession("s1"); - await expect(service.stopSession("s1")).rejects.toThrow(/boom/); - expect(registry.isOwned("s1")).toBe(true); - expect(service.getState().map((s) => s.sessionId)).toEqual(["s1"]); - }); -}); - describe("HTTP/SSE interface", () => { + // Route tests verify transport/forwarding; lifecycle entry tests exercise real cleanup. + const lifecycleFor = (service: ObservationService) => ({ + stop: vi.fn(async ({ sessionId: id }: { sessionId?: string } = {}) => { + if (!id) throw new Error("session required"); + service.removeSession(id); + return { stopped: id, requestId: `request-${id}`, alreadyClosed: false }; + }), + }); interface RecordedRoute { path: string; handler: (req: IncomingMessage, res: ServerResponse) => void | Promise; @@ -637,7 +626,7 @@ describe("HTTP/SSE interface", () => { const { routes, webServer } = routeHarness(); const ctx = { get: (key: string) => (key === "webServer" ? webServer : undefined) } as never; - const dispose = registerObservationRoutes(ctx, service); + const dispose = registerObservationRoutes(ctx, service, lifecycleFor(service)); // state const stateRoute = routes.get("/bsk-observation/state"); @@ -688,7 +677,11 @@ describe("HTTP/SSE interface", () => { const { service, scheduler } = setup({ thumbnails: false }); service.addSession("s1"); const { routes, webServer } = routeHarness(); - const dispose = registerObservationRoutes({ get: () => webServer } as never, service); + const dispose = registerObservationRoutes( + { get: () => webServer } as never, + service, + lifecycleFor(service), + ); const route = routes.get("/bsk-observation/events")!; const metadata = fakeRes(); await route.handler(fakeReq({ url: "/bsk-observation/events?thumbnails=0" }), metadata.res); @@ -712,7 +705,11 @@ describe("HTTP/SSE interface", () => { const { service, scheduler } = setup({ thumbnails: false }); service.addSession("s1"); const { routes, webServer } = routeHarness(); - const dispose = registerObservationRoutes({ get: () => webServer } as never, service); + const dispose = registerObservationRoutes( + { get: () => webServer } as never, + service, + lifecycleFor(service), + ); const response = fakeRes(); vi.spyOn(response.res, "write").mockImplementation(() => { throw new Error("closed socket"); @@ -731,7 +728,7 @@ describe("HTTP/SSE interface", () => { const { service } = setup({ registry }); const { routes, webServer } = routeHarness(); const ctx = { get: (key: string) => (key === "webServer" ? webServer : undefined) } as never; - const dispose = registerObservationRoutes(ctx, service); + const dispose = registerObservationRoutes(ctx, service, lifecycleFor(service)); const stopRoute = routes.get("/bsk-observation/stop"); const res = fakeRes(); @@ -758,7 +755,7 @@ describe("HTTP/SSE interface", () => { service.addSession("s1"); const { routes, webServer } = routeHarness(); const ctx = { get: (key: string) => (key === "webServer" ? webServer : undefined) } as never; - const dispose = registerObservationRoutes(ctx, service); + const dispose = registerObservationRoutes(ctx, service, lifecycleFor(service)); const stateRoute = routes.get("/bsk-observation/state"); const interruptRoute = routes.get("/bsk-observation/interrupt"); @@ -805,7 +802,7 @@ describe("HTTP/SSE interface", () => { it("registers nothing when no webServer is mounted", () => { const { service } = setup({}); const ctx = { get: () => undefined } as never; - const dispose = registerObservationRoutes(ctx, service); + const dispose = registerObservationRoutes(ctx, service, lifecycleFor(service)); expect(typeof dispose).toBe("function"); dispose(); service.dispose(); diff --git a/packages/dsh-plugin-browserskill/tests/session-lifecycle-harness.ts b/packages/dsh-plugin-browserskill/tests/session-lifecycle-harness.ts new file mode 100644 index 00000000..dd1adbfd --- /dev/null +++ b/packages/dsh-plugin-browserskill/tests/session-lifecycle-harness.ts @@ -0,0 +1,101 @@ +import type { ToolDefinition, ToolRunContext } from "@deepseek-ai/dsh-tools"; +import { afterEach, vi } from "vitest"; +import { registerBrowserTools } from "../src/browser-tools"; +import { ObservationService } from "../src/observation"; +import { KeyedExecutor } from "../src/queue"; +import type { BskRunOptions, BskRunResult } from "../src/runner"; +import { SessionStarts } from "../src/session-starts"; +import { SessionRegistry } from "../src/sessions"; +import { memoryStartJournal, type StartJournal } from "../src/start-journal"; +import type { ToolDeps } from "../src/tools"; + +export const cleanups: Array<() => void | Promise> = []; +afterEach(async () => { + try { + for (const cleanup of cleanups.splice(0).reverse()) await cleanup(); + } finally { + vi.useRealTimers(); + } +}); +export const ok = (body: unknown): BskRunResult => ({ + code: 0, + stdout: JSON.stringify(body), + stderr: "", + aborted: false, + timedOut: false, +}); +export const failed = (message: string): BskRunResult => ({ + code: 2, + stdout: JSON.stringify({ code: "protocol_error", message }), + stderr: "", + aborted: false, + timedOut: false, +}); +export const exec = (signal = new AbortController().signal) => + ({ + callId: "call", + name: "browser_session", + signal, + agent: { id: "conversation" }, + }) as ToolRunContext; + +export function harness( + run: (args: string[], options: BskRunOptions) => Promise, + journal: StartJournal = memoryStartJournal(), +) { + const calls: Array<{ args: string[]; options: BskRunOptions }> = []; + const tools = new Map(); + const ctx = { + tools: { + register: (tool: ToolDefinition) => { + tools.set(tool.name, tool); + return () => {}; + }, + }, + get: () => undefined, + }; + const prepare = vi.fn(async () => ok({ state: "prepared" })); + const runner = { + run: async (args: string[], options: BskRunOptions = {}) => { + calls.push({ args, options }); + if (args.includes("--prepare")) return prepare(); + return run(args, options); + }, + killAll: vi.fn(), + killFor: vi.fn((_tag: string) => 0), + }; + const registry = new SessionRegistry(5); + const queue = new KeyedExecutor(); + const observation = new ObservationService({ + ctx: ctx as never, + runner, + registry, + queue, + options: { enabled: true, thumbnailIntervalMs: 1500, idleIntervalMs: 8000 }, + }); + const deps: ToolDeps = { + ctx: ctx as never, + runner, + registry, + queue, + observation, + config: { + bskPath: "bsk", + defaultTimeoutMs: 120000, + maxSessions: 5, + observationEnabled: true, + thumbnailIntervalMs: 1500, + idleIntervalMs: 8000, + lazyTools: false, + }, + }; + const starts = (deps.starts = new SessionStarts(deps, journal)); + registerBrowserTools(deps); + const session = (args: Record, context = exec()) => + tools.get("browser_session")!.execute(args, context); + cleanups.push(async () => { + await starts.dispose(); + observation.dispose(); + }); + return { starts, registry, journal, calls, session, runner, prepare, tools, observation, queue }; +} diff --git a/packages/dsh-plugin-browserskill/tests/session-starts.test.ts b/packages/dsh-plugin-browserskill/tests/session-starts.test.ts new file mode 100644 index 00000000..8f90c8ab --- /dev/null +++ b/packages/dsh-plugin-browserskill/tests/session-starts.test.ts @@ -0,0 +1,405 @@ +import { mkdtempSync, readdirSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, it, vi } from "vitest"; +import type { BskRunResult } from "../src/runner"; +import { DiskStartJournal, memoryStartJournal } from "../src/start-journal"; +import { cleanups, exec, failed, harness, ok } from "./session-lifecycle-harness"; + +describe("recoverable plugin starts", () => { + it("publishes a session only after both initialization and claim succeed", async () => { + let finishNavigation!: (result: BskRunResult) => void; + let finishClaim!: (result: BskRunResult) => void; + const h = harness(async (args) => { + if (args[1] === "start") + return ok({ session_id: "starting", browser_instance_id: "browser" }); + if (args[0] === "navigate") + return new Promise((resolve) => { + finishNavigation = resolve; + }); + if (args.includes("--claim")) + return new Promise((resolve) => { + finishClaim = resolve; + }); + return ok({ state: "closed" }); + }); + h.registry.completeStart({ sessionId: "working", startedAtMs: 1 }); + const starting = h.session({ + action: "start", + device: "iphone-14", + url: "https://example.com", + }); + await vi.waitFor(() => expect(finishNavigation).toBeTypeOf("function")); + const assertUnavailable = async () => { + expect(h.registry.current()).toBe("working"); + expect(h.registry.size()).toBe(2); + expect(await h.session({ action: "list" })).toMatchObject({ + sessions: [ + { sessionId: "working", state: "active", current: true }, + { sessionId: "starting", state: "starting", current: false }, + ], + }); + const before = h.calls.length; + await expect( + h.tools.get("browser_page")!.execute( + { + action: "navigate", + session: "starting", + url: "https://example.org", + }, + exec(), + ), + ).rejects.toThrow(/not ready/); + expect(h.calls).toHaveLength(before); + }; + await assertUnavailable(); + finishNavigation(ok({ url: "https://example.com", reached: "load", tab_id: 1 })); + await vi.waitFor(() => expect(finishClaim).toBeTypeOf("function")); + await assertUnavailable(); + finishClaim(ok({ state: "active" })); + await expect(starting).resolves.toMatchObject({ sessionId: "starting" }); + expect(h.registry.current()).toBe("starting"); + expect(h.registry.resolve("starting", "tool")).toBe("starting"); + expect(h.observation.getState()).toContainEqual( + expect.objectContaining({ + sessionId: "starting", + action: "idle", + }), + ); + }); + + it("refuses a late claim after a stop fails during initialization", async () => { + let finishClaim!: (result: BskRunResult) => void; + const h = harness(async (args) => { + if (args[1] === "start") + return ok({ session_id: "starting", browser_instance_id: "browser" }); + if (args.includes("--claim")) + return new Promise((resolve) => { + finishClaim = resolve; + }); + return failed("close unavailable"); + }); + const starting = h.session({ action: "start" }); + const rejected = expect(starting).rejects.toThrow(/cancelled|activation/); + await vi.waitFor(() => expect(finishClaim).toBeTypeOf("function")); + await expect(h.session({ action: "stop", session: "starting" })).rejects.toThrow( + /close unavailable/, + ); + finishClaim(ok({ state: "active" })); + await rejected; + expect(h.registry.current()).toBeUndefined(); + expect(h.registry.isOwned("starting")).toBe(true); + expect(h.registry.stateFor("starting")).toBe("cleanup"); + expect(h.starts.pendingCleanup()).toBe(1); + }); + + it("rejects already queued ordinary work when its session enters cleanup", async () => { + const h = harness(async (args) => { + if (args[1] === "start") return ok({ session_id: "created", browser_instance_id: "browser" }); + if (args.includes("--claim")) return ok({ state: "active" }); + return failed("close unavailable"); + }); + await h.session({ action: "start" }); + let release!: () => void; + const blocker = h.queue.run( + "created", + () => + new Promise((resolve) => { + release = resolve; + }), + ); + await vi.waitFor(() => expect(release).toBeTypeOf("function")); + const queued = h.tools.get("browser_page")!.execute( + { + action: "navigate", + session: "created", + url: "https://example.com", + }, + exec(), + ); + const rejected = expect(queued).rejects.toThrow(/awaiting cleanup/); + h.starts.archive("conversation"); + expect(h.registry.current()).toBeUndefined(); + release(); + await blocker; + await rejected; + expect(h.calls.some(({ args }) => args[0] === "navigate")).toBe(false); + expect(h.registry.isOwned("created")).toBe(true); + }); + + it("adopts recovered cleanup resources without making them usable", async () => { + const journal = memoryStartJournal(); + journal.records.set("recovered", { + requestId: "recovered", + startedAtMs: 1, + owners: [], + cleanup: true, + }); + const h = harness( + async () => + ok({ + state: "cleanup_failed", + session: { session_id: "broken", browser_instance_id: "browser" }, + cleanup_error: "close unavailable", + }), + journal, + ); + h.registry.completeStart({ sessionId: "working", startedAtMs: 1 }); + await h.starts.reconcile(); + expect(h.registry.current()).toBe("working"); + expect(h.registry.stateFor("broken")).toBe("cleanup"); + expect(h.registry.resolveForStop("broken")).toBe("broken"); + expect(() => h.registry.resolve("broken", "tool")).toThrow(/awaiting cleanup/); + expect(h.registry.size()).toBe(2); + h.registry.remove("working"); + expect(h.registry.current()).toBeUndefined(); + expect(() => h.registry.resolve(undefined, "tool")).toThrow(/none is active/); + expect(h.registry.resolveForStop(undefined)).toBe("broken"); + }); + + it("keeps the working session current when another start and its cleanup fail", async () => { + let count = 0; + let canClose = false; + const h = harness(async (args) => { + if (args[1] === "start") + return ok({ + session_id: count++ === 0 ? "working" : "broken", + browser_instance_id: "browser", + }); + if (args.includes("--claim")) return ok({ state: "active" }); + if (args.includes("--cancel")) + return canClose ? ok({ state: "closed" }) : failed("close unavailable"); + if (args[0] === "navigate" && args.includes("broken")) return failed("navigation failed"); + return ok({ url: "https://example.com", reached: "load", tab_id: 1 }); + }); + await h.session({ action: "start" }); + await expect(h.session({ action: "start", url: "https://example.com" })).rejects.toThrow( + /navigation failed/, + ); + expect(h.registry.current()).toBe("working"); + expect(h.registry.ownedIds()).toEqual(["working", "broken"]); + expect(h.registry.size()).toBe(2); + expect(h.starts.pendingCleanup()).toBe(1); + const page = h.tools.get("browser_page")!; + await page.execute({ action: "navigate", url: "https://example.com" }, exec()); + expect(h.calls.at(-1)?.args).toContain("working"); + const before = h.calls.length; + await expect( + page.execute({ action: "navigate", url: "https://example.com", session: "broken" }, exec()), + ).rejects.toThrow(/cleanup|not ready/); + expect(h.calls).toHaveLength(before); + canClose = true; + await h.session({ action: "stop", session: "broken" }); + expect(h.registry.current()).toBe("working"); + expect(h.registry.ownedIds()).toEqual(["working"]); + }); + it.each([ + "lost reply", + "aborted success", + "timed out success", + ])("reaps only its own request after %s", async (mode) => { + const backend = new Map(); + backend.set("foreign-owner", "foreign-session"); + const h = harness(async (args, options) => { + if (args[1] === "start") { + const id = args[3]; + expect(h.journal.records.has(id)).toBe(true); // write-ahead ownership + backend.set(id, "created"); + return { + ...ok({ session_id: "created", browser_instance_id: "browser" }), + stdout: + mode === "lost reply" + ? "" + : ok({ session_id: "created", browser_instance_id: "browser" }).stdout, + aborted: mode === "aborted success", + timedOut: mode === "timed out success", + }; + } + expect(args).toEqual(["session", "request", expect.any(String), "--cancel"]); + expect(options.signal).toBeUndefined(); + backend.delete(args[2]); + return ok({ state: "closed" }); + }); + await expect(h.session({ action: "start" })).rejects.toThrow(); + expect([...backend.values()]).toEqual(["foreign-session"]); + expect(h.registry.ownedIds()).toEqual([]); + expect(h.journal.records.size).toBe(0); + }); + + it("retains failed navigation cleanup, exposes the session, and retries by request", async () => { + let canClose = false; + const h = harness(async (args) => { + if (args[1] === "start") return ok({ session_id: "created", browser_instance_id: "browser" }); + if (args[0] === "navigate") return failed("navigation failed"); + return ok( + canClose + ? { state: "closed" } + : { + state: "cleanup_failed", + session: { session_id: "created", browser_instance_id: "browser" }, + cleanup_error: "window close failed", + }, + ); + }); + await expect(h.session({ action: "start", url: "https://example.com" })).rejects.toThrow( + /navigation failed/, + ); + expect(h.registry.ownedIds()).toEqual(["created"]); + expect(h.starts.pendingCleanup()).toBe(1); + expect(await h.session({ action: "list" })).toMatchObject({ + pendingCleanup: 1, + sessions: [{ sessionId: "created", state: "cleanup", current: false }], + }); + canClose = true; + await h.session({ action: "list" }); + expect(h.registry.ownedIds()).toEqual([]); + expect(h.journal.records.size).toBe(0); + expect(h.calls.every(({ args }) => args[1] !== "stop")).toBe(true); + }); + + it.each([ + "archive", + "unload", + ])("cancels a pending start on %s and refuses its late success", async (mode) => { + let resolveStart!: (result: BskRunResult) => void; + const h = harness(async (args) => { + if (args[1] === "start") + return new Promise((resolve) => { + resolveStart = resolve; + }); + return ok({ state: "closed" }); + }); + const pending = h.session({ action: "start" }); + const rejected = expect(pending).rejects.toThrow(/cancelled/); + await vi.waitFor(() => expect(resolveStart).toBeTypeOf("function")); + if (mode === "archive") h.starts.archive("conversation"); + else await h.starts.dispose(); + await vi.waitFor(() => expect(h.journal.records.size).toBe(0)); + resolveStart(ok({ session_id: "late", browser_instance_id: "browser" })); + await rejected; + expect(h.registry.ownedIds()).toEqual([]); + expect(h.calls.filter(({ args }) => args.includes("--cancel"))).toHaveLength(1); + }); + + it("does not start any process when the write-ahead record cannot be saved", async () => { + const journal = memoryStartJournal(); + journal.save = () => { + throw new Error("disk full"); + }; + const h = harness(async () => { + throw new Error("must not spawn"); + }, journal); + await expect(h.session({ action: "start" })).rejects.toThrow(/disk full/); + expect(h.calls).toHaveLength(0); + expect(h.registry.size()).toBe(0); + }); + + it("fails before creating a window when the CLI/daemon cannot prepare requests", async () => { + const h = harness(async () => { + throw new Error("must not create"); + }); + h.prepare.mockRejectedValueOnce(new Error("unsupported session request")); + await expect(h.session({ action: "start" })).rejects.toThrow(/unsupported/); + expect(h.calls).toHaveLength(1); + expect(h.journal.records.size).toBe(0); + expect(h.registry.size()).toBe(0); + }); + + it("stop without a current session retries a lost start's cleanup", async () => { + let ready = false; + const h = harness(async (args) => + args[1] === "start" + ? { ...ok({}), aborted: true } + : ready + ? ok({ state: "closed" }) + : failed("temporarily unavailable"), + ); + await expect(h.session({ action: "start" })).rejects.toThrow(); + expect(h.registry.current()).toBeUndefined(); + ready = true; + await expect(h.session({ action: "stop" })).resolves.toMatchObject({ + stopped: "pending browser starts", + }); + expect(h.journal.records.size).toBe(0); + }); + + it("keeps capacity reserved while a lost reply's cleanup is unavailable", async () => { + const h = harness(async (args) => + args[1] === "start" ? { ...ok({}), aborted: true } : failed("daemon unavailable"), + ); + for (let n = 0; n < 5; n++) await expect(h.session({ action: "start" })).rejects.toThrow(); + await expect(h.session({ action: "start" })).rejects.toThrow(/session limit/); + expect(h.calls.filter(({ args }) => args[1] === "start")).toHaveLength(5); + expect(h.journal.records.size).toBe(5); + }); + + it("never stops another request when a short session id has been reused", async () => { + const h = harness(async (args) => { + if (args[1] === "start") return ok({ session_id: "reused", browser_instance_id: "browser" }); + expect(args[2]).not.toBe("other-request"); + return ok({ state: "closed" }); + }); + h.registry.reserveStart(); + h.registry.completeStart({ + sessionId: "reused", + browserInstanceId: "browser", + requestId: "other-request", + startedAtMs: 1, + }); + await expect(h.session({ action: "start" })).rejects.toThrow(/conflicts/); + expect(h.registry.requestFor("reused")).toBe("other-request"); + expect(h.registry.ownedIds()).toEqual(["reused"]); + expect(h.journal.records.size).toBe(0); + }); + + it("counts recovered requests with unknown sessions against the start limit", async () => { + const journal = memoryStartJournal(); + for (let n = 0; n < 5; n++) { + journal.records.set(`recovered-${n}`, { + requestId: `recovered-${n}`, + owners: [], + startedAtMs: 1, + cleanup: true, + }); + } + const h = harness(async () => failed("cleanup unavailable"), journal); + await expect(h.session({ action: "start" })).rejects.toThrow(/session limit/); + expect(h.calls.every(({ args }) => args.includes("--cancel"))).toBe(true); + }); +}); + +describe("durable recovery ownership", () => { + it("recovers released/crashed owners without adopting a live plugin's records", async () => { + const directory = mkdtempSync(join(tmpdir(), "bsk-start-journal-")); + cleanups.push(() => rmSync(directory, { recursive: true, force: true })); + const abandoned = new DiskStartJournal(directory); + abandoned.recover(); + const live = new DiskStartJournal(directory); + live.recover(); + const first = harness(async () => failed("host interrupted"), abandoned); + const other = harness(async () => failed("host interrupted"), live); + const ours = first.starts.begin(["ours"]); + const theirs = other.starts.begin(["theirs"]); + const owner = readdirSync(directory).find((name) => + readFileSync(join(directory, name, "requests.json"), "utf8").includes(ours.requestId), + )!; + expect( + JSON.parse(readFileSync(join(directory, owner, "requests.json"), "utf8"))[0].requestId, + ).toBe(ours.requestId); + abandoned.release(); // process/plugin ceased to own its journal, without running cleanup + const recovered = new DiskStartJournal(directory); + recovered.recover(); + expect([...recovered.records.keys()]).toEqual([ours.requestId]); + expect(recovered.records.get(ours.requestId)?.cleanup).toBe(true); + expect(live.records.has(theirs.requestId)).toBe(true); + const h = harness(async (args) => { + expect(args).toEqual(["session", "request", ours.requestId, "--cancel"]); + return ok({ state: "closed" }); + }, recovered); + await h.starts.reconcile(); + expect(recovered.records.size).toBe(0); + // The abandoned JS objects no longer execute in a real crashed process. + first.journal.records.clear(); + other.journal.records.clear(); + }); +}); diff --git a/packages/dsh-plugin-browserskill/tests/session-stops.test.ts b/packages/dsh-plugin-browserskill/tests/session-stops.test.ts new file mode 100644 index 00000000..a8f1388e --- /dev/null +++ b/packages/dsh-plugin-browserskill/tests/session-stops.test.ts @@ -0,0 +1,587 @@ +import { mkdtempSync, readdirSync, readFileSync, rmSync } from "node:fs"; +import type { IncomingMessage, ServerResponse } from "node:http"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, it, vi } from "vitest"; +import { armArchiveCleanup } from "../src/archive-cleanup"; +import { registerObservationRoutes } from "../src/observation-http"; +import type { BskRunOptions, BskRunResult } from "../src/runner"; +import { DiskStartJournal, memoryStartJournal } from "../src/start-journal"; +import { cleanups, exec, failed, harness, ok } from "./session-lifecycle-harness"; + +function managed( + close: (id: string, options: BskRunOptions) => Promise = async () => + ok({ state: "closed" }), + journal = memoryStartJournal(), + names = ["A", "B", "C", "D", "E", "F"], +) { + let count = 0; + const requests = new Map(); + const targets: string[] = []; + const h = harness(async (args, options) => { + if (args[1] === "start") { + const id = names[count++]; + requests.set(args[3], id); + return ok({ session_id: id, browser_instance_id: "browser" }); + } + if (args.includes("--claim")) return ok({ state: "active" }); + if (args.includes("--cancel")) { + const id = + requests.get(args[2]) ?? journal.records.get(args[2])?.session?.sessionId ?? "unknown"; + targets.push(id); + return close(id, options); + } + return ok({ url: "https://example.com", reached: "load", tab_id: 1 }); + }, journal); + return { ...h, targets, requests }; +} + +/** Real HTTP route and lifecycle manager; only the browser transport is faked. */ +function overlay(h: ReturnType) { + const routes = new Map< + string, + (req: IncomingMessage, res: ServerResponse) => void | Promise + >(); + const webServer = { + register: (route: { + path: string; + handler: (req: IncomingMessage, res: ServerResponse) => void | Promise; + }) => { + routes.set(route.path, route.handler); + return () => routes.delete(route.path); + }, + }; + cleanups.push( + registerObservationRoutes({ get: () => webServer } as never, h.observation, h.starts), + ); + return (sessionId: string) => + new Promise<{ status: number; body: unknown }>((resolve) => { + let status = 0; + const req = { + method: "POST", + headers: { host: "127.0.0.1:3999", "content-type": "application/json" }, + on(event: string, callback: (data?: string) => void) { + if (event === "data") callback(JSON.stringify({ sessionId })); + if (event === "end") callback(); + }, + }; + const res = { + writeHead(code: number) { + status = code; + }, + end(body: string) { + resolve({ status, body: JSON.parse(body) }); + }, + }; + void routes.get("/bsk-observation/stop")!( + req as IncomingMessage, + res as unknown as ServerResponse, + ); + }); +} + +describe("managed session stops", () => { + it.each([ + "implicit", + "explicit", + "overlay", + ])("preserves a cancelled default caller's retry when a concurrent %s stop succeeds", async (mode) => { + let finish!: (result: BskRunResult) => void; + const h = managed(async (id) => + id === "B" + ? new Promise((resolve) => { + finish = resolve; + }) + : ok({ state: "closed" }), + ); + await h.session({ action: "start" }); + await h.session({ action: "start" }); + const controller = new AbortController(); + const first = h.session({ action: "stop" }, exec(controller.signal)); + const interrupted = expect(first).rejects.toMatchObject({ name: "AbortError" }); + await vi.waitFor(() => expect(finish).toBeTypeOf("function")); + const other = + mode === "overlay" + ? overlay(h)("B") + : h.session({ action: "stop", ...(mode === "explicit" ? { session: "B" } : {}) }); + controller.abort(); + await interrupted; + finish(ok({ state: "closed" })); + await other; + expect(h.registry.current()).toBe("A"); + expect(h.registry.isOwned("B")).toBe(false); + await expect(h.session({ action: "stop" })).resolves.toMatchObject({ + stopped: "B", + alreadyClosed: true, + }); + expect(h.targets).toEqual(["B"]); + expect(h.registry.isUsable("A")).toBe(true); + }); + + it("supports exact request targeting and rejects foreign or conflicting targets", async () => { + const h = managed(); + await h.session({ action: "start" }); + const requestId = h.registry.requestFor("A")!; + await expect(h.session({ action: "stop", requestId: "foreign" })).rejects.toThrow( + /does not belong/, + ); + await expect(h.session({ action: "stop", session: "A", requestId })).rejects.toThrow( + /not both/, + ); + expect(h.targets).toEqual([]); + expect(await h.session({ action: "list" })).toMatchObject({ sessions: [{ requestId }] }); + await expect(h.session({ action: "stop", requestId })).resolves.toEqual({ + stopped: "A", + requestId, + alreadyClosed: false, + }); + }); + + it("retains anonymous stop receipts when a cancelled prepare replies late", async () => { + const h = managed(); + let finishPrepare!: (result: BskRunResult) => void; + h.prepare.mockImplementationOnce( + () => + new Promise((resolve) => { + finishPrepare = resolve; + }), + ); + const start = h.session({ action: "start" }); + const cancelled = expect(start).rejects.toThrow(/cancelled/); + await vi.waitFor(() => expect(finishPrepare).toBeTypeOf("function")); + const requestId = [...h.journal.records.keys()][0]; + const controller = new AbortController(); + const stop = h.session({ action: "stop", requestId }, exec(controller.signal)); + const interrupted = expect(stop).rejects.toMatchObject({ name: "AbortError" }); + controller.abort(); + await interrupted; + await h.starts.reconcile(); + finishPrepare(ok({ state: "prepared" })); + await cancelled; + expect(h.calls.some(({ args }) => args[1] === "start")).toBe(false); + expect(h.journal.records.get(requestId)?.stop).toBe("closed"); + await h.session({ action: "start" }); + await expect(h.session({ action: "stop", requestId })).resolves.toEqual({ + stopped: "pending browser starts", + requestId, + alreadyClosed: true, + }); + expect(h.registry.current()).toBe("A"); + expect(h.targets).toEqual(["unknown"]); + }); + + it("can acknowledge multiple anonymous completed stops by request ID", async () => { + const journal = memoryStartJournal(); + for (const requestId of ["first", "second"]) + journal.records.set(requestId, { + requestId, + owners: [], + startedAtMs: 1, + cleanup: false, + stop: "closed", + }); + const h = managed(undefined, journal); + await h.session({ action: "start" }); + await expect(h.session({ action: "stop" })).rejects.toThrow(/specify session or requestId/); + await expect(h.session({ action: "stop", requestId: "first" })).resolves.toMatchObject({ + requestId: "first", + alreadyClosed: true, + }); + await expect(h.session({ action: "stop" })).resolves.toMatchObject({ + requestId: "second", + alreadyClosed: true, + }); + expect(h.registry.current()).toBe("A"); + expect(h.targets).toEqual([]); + }); + + it.each([ + "unknown", + "cancelling", + "cleanup_failed", + "not_found", + ])("does not release ownership for an unconfirmed %s result", async (state) => { + let ready = false; + const h = managed(async () => + ready + ? ok({ state: "closed" }) + : state === "not_found" + ? { + ...failed("request not found"), + stdout: JSON.stringify({ code: "not_found", message: "request not found" }), + } + : ok({ state }), + ); + await h.session({ action: "start" }); + await expect(h.session({ action: "stop" })).rejects.toThrow(); + expect(h.registry.stateFor("A")).toBe("cleanup"); + expect(h.starts.pendingCleanup()).toBe(1); + ready = true; + await h.session({ action: "stop" }); + expect(h.registry.size()).toBe(0); + }); + + it.each([ + "cleanup_failed", + "closed", + ])("retains the original identity if a %s reply names a different session", async (state) => { + let ready = false; + const h = managed(async () => + ready + ? ok({ state: "closed" }) + : ok({ state, session: { session_id: "B", browser_instance_id: "browser" } }), + ); + await h.session({ action: "start" }); + await h.session({ action: "start" }); + const requestId = h.registry.requestFor("A")!; + await expect(h.session({ action: "stop", session: "A" })).rejects.toThrow( + /different session identity/, + ); + expect(h.journal.records.get(requestId)?.session?.sessionId).toBe("A"); + expect(h.registry.stateFor("A")).toBe("cleanup"); + expect(h.registry.isUsable("B")).toBe(true); + ready = true; + await h.session({ action: "stop", requestId }); + expect(h.registry.ownedIds()).toEqual(["B"]); + }); + + it("does not accept an already cancelled stop or change the current session", async () => { + const h = managed(); + await h.session({ action: "start" }); + const controller = new AbortController(); + controller.abort(); + const before = JSON.stringify([...h.journal.records.values()]); + await expect(h.session({ action: "stop" }, exec(controller.signal))).rejects.toMatchObject({ + name: "AbortError", + }); + expect(JSON.stringify([...h.journal.records.values()])).toBe(before); + expect(h.registry.current()).toBe("A"); + expect(h.registry.stateFor("A")).toBe("active"); + expect(h.targets).toEqual([]); + expect(h.runner.killFor).not.toHaveBeenCalled(); + }); + + it("rejects stop admission when its intent cannot be saved, keeping the session usable", async () => { + const h = managed(); + await h.session({ action: "start" }); + const save = vi.spyOn(h.journal, "save").mockImplementationOnce(() => { + throw new Error("disk full"); + }); + await expect(h.session({ action: "stop" })).rejects.toThrow(/disk full/); + expect(h.registry.resolve(undefined, "tool")).toBe("A"); + expect([...h.journal.records.values()][0]).toMatchObject({ cleanup: false }); + expect([...h.journal.records.values()][0].stop).toBeUndefined(); + expect(h.targets).toEqual([]); + expect(h.runner.killFor).not.toHaveBeenCalled(); + save.mockRestore(); + await expect(h.session({ action: "stop" })).resolves.toMatchObject({ stopped: "A" }); + }); + + it("continues dispatched cleanup after a caller aborts and joins the same request on retry", async () => { + let finish!: (result: BskRunResult) => void; + const h = managed(async (_id, options) => { + expect(options.signal).toBeUndefined(); + expect(options.tag).toMatch(/^cleanup:/); + return new Promise((resolve) => { + finish = resolve; + }); + }); + await h.session({ action: "start" }); + const controller = new AbortController(); + const first = h.session({ action: "stop" }, exec(controller.signal)); + const aborted = expect(first).rejects.toMatchObject({ name: "AbortError" }); + await vi.waitFor(() => expect(finish).toBeTypeOf("function")); + controller.abort(); + await aborted; + const second = h.session({ action: "stop" }); + expect(h.starts.pendingCleanup()).toBe(1); + finish(ok({ state: "closed" })); + await expect(second).resolves.toMatchObject({ stopped: "A" }); + expect(h.targets).toEqual(["A"]); + expect(h.journal.records.size).toBe(0); + }); + + it.each([ + "lost reply", + "timeout", + "interrupted", + ])("retains ownership after %s until terminal cleanup is confirmed", async (mode) => { + let attempt = 0; + const h = managed(async () => { + if (attempt++ > 0) return ok({ state: "closed" }); + return { + ...ok({}), + stdout: mode === "lost reply" ? "" : "{}", + timedOut: mode === "timeout", + aborted: mode === "interrupted", + }; + }); + await h.session({ action: "start" }); + await expect(h.session({ action: "stop" })).rejects.toThrow(); + expect(h.registry.stateFor("A")).toBe("cleanup"); + expect(h.registry.size()).toBe(1); + expect(h.starts.pendingCleanup()).toBe(1); + await h.starts.reconcile(); + expect(h.registry.size()).toBe(0); + await expect(h.session({ action: "stop" })).resolves.toMatchObject({ stopped: "A" }); + expect(h.targets).toEqual(["A", "A"]); + }); + + it("retries failed stops on the timer without another tool call", async () => { + vi.useFakeTimers(); + let ready = false; + const h = managed(async () => (ready ? ok({ state: "closed" }) : failed("offline"))); + await h.session({ action: "start" }); + await expect(h.session({ action: "stop" })).rejects.toThrow(/offline/); + ready = true; + await vi.advanceTimersByTimeAsync(15_000); + expect(h.targets).toEqual(["A", "A"]); + expect(h.registry.size()).toBe(0); + expect(h.starts.pendingCleanup()).toBe(0); + await expect(h.session({ action: "stop" })).resolves.toMatchObject({ stopped: "A" }); + }); + + it("routes overlay failures into durable recovery and never stops a foreign session", async () => { + let ready = false; + const h = managed(async () => (ready ? ok({ state: "closed" }) : failed("offline"))); + await h.session({ action: "start" }); + const stop = overlay(h); + expect(await stop("foreign")).toMatchObject({ status: 500 }); + expect(h.targets).toEqual([]); + expect(await stop("A")).toMatchObject({ status: 500 }); + expect(h.starts.pendingCleanup()).toBe(1); + expect([...h.journal.records.values()][0]).toMatchObject({ cleanup: true, stop: "pending" }); + ready = true; + await h.session({ action: "list" }); + expect(await stop("A")).toEqual({ status: 200, body: { stopped: true } }); + expect(h.targets).toEqual(["A", "A"]); + expect(h.journal.records.size).toBe(0); + }); + + it("shares one cleanup across tool, overlay, archive, and reconciliation", async () => { + let finish!: (result: BskRunResult) => void; + const h = managed( + async () => + new Promise((resolve) => { + finish = resolve; + }), + ); + await h.session({ action: "start" }); + const tool = h.session({ action: "stop" }); + const http = overlay(h)("A"); + let archive!: (change: unknown) => void; + cleanups.push( + armArchiveCleanup( + { + get: () => undefined, + on: (_event: string, fn: typeof archive) => { + archive = fn; + return () => {}; + }, + } as never, + h.starts, + ), + ); + archive({ domain: "workspace", table: "", value: { archivedSessionIds: ["conversation"] } }); + const reconcile = h.starts.reconcile(); + await vi.waitFor(() => expect(finish).toBeTypeOf("function")); + finish(ok({ state: "closed" })); + await expect(tool).resolves.toMatchObject({ stopped: "A" }); + expect(await http).toMatchObject({ status: 200 }); + await reconcile; + expect(h.targets).toEqual(["A"]); + expect(h.runner.killFor.mock.calls.filter(([tag]) => tag === "A")).toEqual([["A"]]); + expect(h.journal.records.size).toBe(0); + }); + + it("requires an explicit target when several stops are unacknowledged", async () => { + let ready = false; + const h = managed(async () => (ready ? ok({ state: "closed" }) : failed("offline"))); + for (let n = 0; n < 3; n++) await h.session({ action: "start" }); + await expect(h.session({ action: "stop", session: "B" })).rejects.toThrow(); + await expect(h.session({ action: "stop", session: "C" })).rejects.toThrow(); + expect(h.registry.current()).toBe("A"); + await expect(h.session({ action: "stop" })).rejects.toThrow(/specify session/); + expect(h.targets).toEqual(["B", "C"]); + ready = true; + await h.session({ action: "stop", session: "B" }); + await expect(h.session({ action: "stop" })).resolves.toMatchObject({ stopped: "C" }); + expect(h.registry.ownedIds()).toEqual(["A"]); + }); + + it("does not cancel a new session that reuses a completed stop's short ID", async () => { + let ready = false; + const h = managed( + async () => (ready ? ok({ state: "closed" }) : failed("offline")), + memoryStartJournal(), + ["A", "B", "B"], + ); + await h.session({ action: "start" }); + await h.session({ action: "start" }); + const oldRequest = h.registry.requestFor("B"); + await expect(h.session({ action: "stop" })).rejects.toThrow(); + ready = true; + await h.session({ action: "list" }); + await h.session({ action: "start" }); + expect(h.registry.requestFor("B")).not.toBe(oldRequest); + const before = h.calls.length; + await expect(h.session({ action: "stop" })).resolves.toMatchObject({ stopped: "B" }); + expect(h.calls).toHaveLength(before); + expect(h.registry.isUsable("B")).toBe(true); + expect(h.registry.size()).toBe(2); + }); + + it.each([ + "completion", + "acknowledgement", + ])("keeps the same receipt when %s persistence fails", async (phase) => { + const h = managed(); + await h.session({ action: "start" }); + await h.session({ action: "start" }); + const requestId = h.registry.requestFor("B")!; + let failOnce = true; + vi.spyOn(h.journal, "save").mockImplementation(() => { + const record = h.journal.records.get(requestId); + if (failOnce && (phase === "completion" ? record?.stop === "closed" : !record)) { + failOnce = false; + throw new Error("receipt write failed"); + } + }); + await expect(h.session({ action: "stop" })).rejects.toThrow(/receipt write failed/); + expect(h.registry.current()).toBe("A"); + expect(h.registry.isOwned("B")).toBe(false); + expect(h.journal.records.get(requestId)?.stop).toBe("closed"); + await expect(h.session({ action: "stop" })).resolves.toMatchObject({ stopped: "B" }); + expect(h.targets).toEqual(["B"]); + }); + + it.each([ + false, + true, + ])("recovers a stop receipt across restart (already closed=%s)", async (closed) => { + const directory = mkdtempSync(join(tmpdir(), "bsk-stop-recovery-")); + cleanups.push(() => rmSync(directory, { recursive: true, force: true })); + const journal = new DiskStartJournal(directory); + journal.recover(); + let ready = false; + const first = managed( + async () => (ready ? ok({ state: "closed" }) : failed("offline")), + journal, + ); + await first.session({ action: "start" }); + await expect(first.session({ action: "stop" })).rejects.toThrow(); + ready = true; + if (closed) await first.starts.reconcile(); + const snapshot = readdirSync(directory).flatMap((name) => + JSON.parse(readFileSync(join(directory, name, "requests.json"), "utf8")), + ); + expect(snapshot[0]).toMatchObject({ cleanup: !closed, stop: closed ? "closed" : "pending" }); + journal.release(); + journal.records.clear(); // the old process no longer runs; leave its durable ledger intact + const recovered = new DiskStartJournal(directory); + recovered.recover(); + const second = managed(undefined, recovered, ["new"]); + await second.starts.reconcile(); + expect(second.targets).toEqual(closed ? [] : ["A"]); + await second.session({ action: "start" }); + await expect(second.session({ action: "stop" })).resolves.toMatchObject({ stopped: "A" }); + expect(second.registry.current()).toBe("new"); + expect(second.registry.isUsable("new")).toBe(true); + }); + + it("does not count completed receipts as browser capacity", async () => { + let ready = false; + const h = managed(async () => (ready ? ok({ state: "closed" }) : failed("offline"))); + for (const id of ["A", "B", "C", "D", "E"]) { + await h.session({ action: "start" }); + ready = false; + await expect(h.session({ action: "stop", session: id })).rejects.toThrow(); + ready = true; + await h.starts.reconcile(); + } + expect(h.journal.records.size).toBe(5); + expect(h.registry.size()).toBe(0); + await expect(h.session({ action: "start" })).resolves.toMatchObject({ sessionId: "F" }); + }); + + it("retries the same default stop after failure instead of stopping another active session", async () => { + const ids = new Map(); + const targets: string[] = []; + let canClose = false; + const h = harness(async (args) => { + if (args[1] === "start") { + const id = ids.size === 0 ? "A" : "B"; + ids.set(args[3], id); + return ok({ session_id: id, browser_instance_id: "browser" }); + } + if (args.includes("--claim")) return ok({ state: "active" }); + targets.push(ids.get(args[2])!); + return canClose ? ok({ state: "closed" }) : failed("close unavailable"); + }); + await h.session({ action: "start" }); + await h.session({ action: "start" }); + await expect(h.session({ action: "stop" })).rejects.toThrow(/close unavailable/); + expect(h.registry.current()).toBe("A"); + canClose = true; + await expect(h.session({ action: "stop" })).resolves.toMatchObject({ stopped: "B" }); + expect(targets).toEqual(["B", "B"]); + expect(h.registry.ownedIds()).toEqual(["A"]); + }); + + it("reconciles a failed active-session stop and retains its completion for default retry", async () => { + let count = 0; + let canClose = false; + const h = harness(async (args) => { + if (args[1] === "start") + return ok({ session_id: count++ ? "B" : "A", browser_instance_id: "browser" }); + if (args.includes("--claim")) return ok({ state: "active" }); + return canClose ? ok({ state: "closed" }) : failed("close unavailable"); + }); + await h.session({ action: "start" }); + await h.session({ action: "start" }); + await expect(h.session({ action: "stop" })).rejects.toThrow(); + const record = [...h.journal.records.values()].find((r) => r.session?.sessionId === "B")!; + expect(record.cleanup).toBe(true); + expect(h.starts.pendingCleanup()).toBe(1); + canClose = true; + await h.session({ action: "list" }); + expect(h.registry.ownedIds()).toEqual(["A"]); + expect(h.starts.pendingCleanup()).toBe(0); + const before = h.calls.length; + await expect(h.session({ action: "stop" })).resolves.toMatchObject({ stopped: "B" }); + expect(h.calls).toHaveLength(before); + expect(h.registry.current()).toBe("A"); + }); + + it("keeps an accepted stop recoverable when its caller aborts while queued", async () => { + const h = harness(async (args, options) => { + if (args[1] === "start") return ok({ session_id: "B", browser_instance_id: "browser" }); + if (args.includes("--claim")) return ok({ state: "active" }); + expect(options.signal).toBeUndefined(); + return ok({ state: "closed" }); + }); + await h.session({ action: "start" }); + let release!: () => void; + const blocker = h.queue.run( + "B", + () => + new Promise((resolve) => { + release = resolve; + }), + ); + await vi.waitFor(() => expect(release).toBeTypeOf("function")); + const controller = new AbortController(); + const stop = h.session({ action: "stop" }, exec(controller.signal)); + const rejected = expect(stop).rejects.toMatchObject({ name: "AbortError" }); + controller.abort(); + await rejected; + // Always drain the fixture queue, even when the regression assertion fails. + release(); + await blocker; + expect([...h.journal.records.values()][0].cleanup).toBe(true); + await h.starts.reconcile(); + expect(h.calls.filter(({ args }) => args.includes("--cancel"))).toHaveLength(1); + expect(h.registry.ownedIds()).toEqual([]); + await expect(h.session({ action: "stop" })).resolves.toMatchObject({ stopped: "B" }); + }); +}); diff --git a/packages/dsh-plugin-browserskill/tests/sessions.test.ts b/packages/dsh-plugin-browserskill/tests/sessions.test.ts index 32600c45..0246ec68 100644 --- a/packages/dsh-plugin-browserskill/tests/sessions.test.ts +++ b/packages/dsh-plugin-browserskill/tests/sessions.test.ts @@ -8,6 +8,27 @@ function start(registry: SessionRegistry, sessionId: string): void { } describe("SessionRegistry", () => { + it.each([ + "starting", + "cleanup", + ] as const)("counts %s resources against capacity without selecting them", (state) => { + const registry = new SessionRegistry(2); + start(registry, "working"); + registry.reserveStart(); + registry.trackStart({ sessionId: "pending", startedAtMs: 1 }, state); + expect(registry.current()).toBe("working"); + expect(registry.size()).toBe(2); + expect(() => registry.reserveStart()).toThrow(/session limit/); + expect(() => registry.resolve("pending", "tool")).toThrow(/not ready|awaiting cleanup/); + expect(registry.resolveForStop("pending")).toBe("pending"); + registry.remove("working"); + expect(registry.current()).toBeUndefined(); + expect(() => registry.resolve(undefined, "tool")).toThrow(/none is active/); + expect(registry.resolveForStop(undefined)).toBe("pending"); + registry.remove("pending"); + expect(() => registry.reserveStart()).not.toThrow(); + }); + it("tracks the current session across start and remove", () => { const registry = new SessionRegistry(5); expect(registry.current()).toBeUndefined(); diff --git a/packages/dsh-plugin-browserskill/tests/tools.test.ts b/packages/dsh-plugin-browserskill/tests/tools.test.ts index 64a9a918..79861912 100644 --- a/packages/dsh-plugin-browserskill/tests/tools.test.ts +++ b/packages/dsh-plugin-browserskill/tests/tools.test.ts @@ -48,7 +48,23 @@ function fakeRunner(responses: Record) { calls, runner: { async run(args: string[], options: BskRunOptions = {}): Promise { - calls.push({ args, options }); + // Generic tool tests omit the claim acknowledgement from their business-call log; lifecycle tests exercise it explicitly. + if (!args.includes("--claim") && !args.includes("--prepare")) calls.push({ args, options }); + if (args[0] === "session" && args[1] === "request") { + return { + code: 0, + stdout: JSON.stringify({ + state: args.includes("--prepare") + ? "prepared" + : args.includes("--claim") + ? "active" + : "closed", + }), + stderr: "", + timedOut: false, + aborted: false, + }; + } if (options.signal?.aborted) { return { code: null, stdout: "", stderr: "", timedOut: false, aborted: true }; } @@ -231,6 +247,23 @@ describe("tool registration", () => { } }); + it("exposes optional stop targets and retry guidance in the public session schema", () => { + const { tools } = setup({}); + const tool = tools.get("browser_session")!; + const properties = tool.parameters.properties as Record | undefined; + expect(properties?.requestId).toMatchObject({ + type: "string", + description: expect.stringMatching(/stop.*mutually exclusive with session/i), + }); + expect(properties?.session).toMatchObject({ + type: "string", + description: expect.stringMatching(/unacknowledged stop.*current session/i), + }); + expect(tool.parameters.required).toEqual(["action"]); + expect(tool.description).toMatch(/session or requestId/); + expect(tool.description).toMatch(/unacknowledged stop/); + }); + it("requires an action on every public tool", async () => { const { tools } = setup({}); for (const name of Object.keys(EXPECTED_ACTIONS)) { @@ -277,7 +310,7 @@ describe("action dispatch", () => { expect(registry.current()).toBe("s1"); expect(calls.map(({ args }) => args)).toEqual([ - ["session", "start"], + ["session", "start", "--request-id", expect.any(String)], ["navigate", "--session", "s1", "https://example.test/"], ["observe", "--session", "s1"], ["fill", "--session", "s1", "--value", "hello", "@e1"], @@ -321,6 +354,8 @@ describe("session.start", () => { expect(calls[0].args).toEqual([ "session", "start", + "--request-id", + expect.any(String), "--width", "1280", "--height", @@ -347,7 +382,7 @@ describe("session.start", () => { await expect(tool?.execute({ url: "https://example.com" }, makeExec())).rejects.toThrow( /navigate/, ); - expect(calls.some((c) => c.args.join(" ") === "session stop s1")).toBe(true); + expect(calls.some((c) => c.args[1] === "request" && c.args.includes("--cancel"))).toBe(true); expect(registry.current()).toBeUndefined(); }); }); @@ -459,7 +494,7 @@ describe("session.stop / list", () => { const stop = tools.get("session.stop"); const value = (await stop?.execute({}, makeExec())) as { stopped: string }; expect(value.stopped).toBe("s1"); - expect(calls[1].args).toEqual(["session", "stop", "s1"]); + expect(calls[1].args).toEqual(["session", "request", expect.any(String), "--cancel"]); expect(registry.current()).toBeUndefined(); }); @@ -474,8 +509,20 @@ describe("session.stop / list", () => { sessions: { sessionId: string; current: boolean }[]; }; expect(value.sessions).toEqual([ - { sessionId: "s1", browserInstanceId: "chrome-1", current: false }, - { sessionId: "s2", browserInstanceId: "chrome-1", current: true }, + { + sessionId: "s1", + browserInstanceId: "chrome-1", + current: false, + state: "active", + requestId: expect.any(String), + }, + { + sessionId: "s2", + browserInstanceId: "chrome-1", + current: true, + state: "active", + requestId: expect.any(String), + }, ]); // Registry-only: listing must not call the daemon at all. expect(calls.some((c) => c.args.join(" ").startsWith("session list"))).toBe(false); @@ -1238,7 +1285,17 @@ describe("screenshot scratch file lifecycle", () => { if (args[0] === "session") { return { code: 0, - stdout: JSON.stringify({ session_id: "s1", browser_instance_id: "chrome-1" }), + stdout: JSON.stringify( + args[1] === "request" + ? { + state: args.includes("--prepare") + ? "prepared" + : args.includes("--claim") + ? "active" + : "closed", + } + : { session_id: "s1", browser_instance_id: "chrome-1" }, + ), stderr: "", timedOut: false, aborted: false, @@ -1345,7 +1402,17 @@ describe("observation action instrumentation timing", () => { if (args[0] === "session") { return { code: 0, - stdout: JSON.stringify({ session_id: "s1", browser_instance_id: "chrome-1" }), + stdout: JSON.stringify( + args[1] === "request" + ? { + state: args.includes("--prepare") + ? "prepared" + : args.includes("--claim") + ? "active" + : "closed", + } + : { session_id: "s1", browser_instance_id: "chrome-1" }, + ), stderr: "", timedOut: false, aborted: false, @@ -1491,7 +1558,7 @@ describe("wheel action", () => { it("inspect.observe forwards continuation cursors and exposes the next cursor", async () => { const { tools, calls } = setup({ - "session start": { session_id: "s1", agent_window_id: 100 }, + "session start": { session_id: "s1", browser_instance_id: "b1", agent_window_id: 100 }, observe: { ...SNAPSHOT_REPLY, truncated: true, next_cursor: "page-three" }, }); await startSession(tools);