diff --git a/src/browser/runtime/local-cloak/actions.ts b/src/browser/runtime/local-cloak/actions.ts index c12b35a5..617ecf50 100644 --- a/src/browser/runtime/local-cloak/actions.ts +++ b/src/browser/runtime/local-cloak/actions.ts @@ -10,7 +10,7 @@ import { import { redactText, redactUrl } from '../../../observation/redaction.js'; import { articleHtmlToMarkdown } from '../../../download/article-download.js'; import { waitForDownload } from './downloads.js'; -import type { CloakSessionManager } from './session-manager.js'; +import { isClosedContextError, type CloakSessionManager } from './session-manager.js'; import type { BrowserContext, Frame, Page as PlaywrightPage } from 'playwright-core'; import { runBrowserProgram } from '../../run/runner.js'; import { BROWSER_RUN_MAX_SOURCE_BYTES } from '../../run/types.js'; @@ -258,18 +258,29 @@ export async function dispatchCloakAction(manager: CloakSessionManager, command: idleTimeout: command.idleTimeout, windowMode: command.windowMode, }, lease.page); - const data = await runBrowserProgram({ - ...scope, - pageId: lease.pageId, - }, command.source, { - timeoutMs: command.timeoutMs, - maxOutputChars: command.maxOutputChars, - memoryLimitBytes: command.memoryLimitBytes, - snapshotDiff: command.noSnapshotDiff ? false : command.snapshotDiff, - snapshotMode: command.snapshotMode === 'tree' ? 'tree' : 'act', - snapshotBaselineStore: snapshotBaselineStore(manager), - ...(signal ? { signal } : {}), - }); + let data: Awaited>; + try { + data = await runBrowserProgram({ + ...scope, + pageId: lease.pageId, + }, command.source, { + timeoutMs: command.timeoutMs, + maxOutputChars: command.maxOutputChars, + memoryLimitBytes: command.memoryLimitBytes, + snapshotDiff: command.noSnapshotDiff ? false : command.snapshotDiff, + snapshotMode: command.snapshotMode === 'tree' ? 'tree' : 'act', + snapshotBaselineStore: snapshotBaselineStore(manager), + ...(signal ? { signal } : {}), + }); + } catch (error) { + // A dead context can still report `page.isClosed() === false`, so a lease + // can look healthy right up until the program tries to use it. Evict the + // Profile runtime so the next command gets a fresh page instead of + // repeatedly leasing the same invalid one (#314). We don't retry here: + // the program may already have caused side effects, so replay isn't safe. + if (isClosedContextError(error)) manager.evictDeadRuntime(lease.profileId, lease.context); + throw error; + } return { id: command.id, ok: true, diff --git a/src/browser/runtime/local-cloak/run-recovery.test.ts b/src/browser/runtime/local-cloak/run-recovery.test.ts new file mode 100644 index 00000000..a03bbf71 --- /dev/null +++ b/src/browser/runtime/local-cloak/run-recovery.test.ts @@ -0,0 +1,170 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { CloakSessionManager } from './session-manager.js'; +import { dispatchCloakAction } from './actions.js'; + +const runBrowserProgram = vi.hoisted(() => vi.fn()); + +vi.mock('../../run/runner.js', () => ({ + runBrowserProgram, +})); + +// Trimmed copy of the fakeContext helper in session-manager.test.ts — only the +// surface `browser run` recovery touches (getPage/browserRunScope/CDP window +// bookkeeping) needs to be here. +function fakeContext() { + const listeners = new Map void>>(); + const targetIds = new WeakMap(); + const windowIds = new Map(); + let targetCounter = 0; + let windowCounter = 0; + const fakePage = () => { + let closed = false; + let currentUrl = 'https://example.com/'; + const page: any = { + goto: vi.fn().mockImplementation(async (url: string) => { currentUrl = url; }), + evaluate: vi.fn().mockResolvedValue('ok'), + title: vi.fn().mockResolvedValue('Title'), + url: vi.fn(() => currentUrl), + isClosed: vi.fn(() => closed), + close: vi.fn(async () => { closed = true; }), + opener: vi.fn().mockResolvedValue(null), + on() {}, + once() {}, + off() {}, + }; + const targetId = `target-${++targetCounter}`; + targetIds.set(page, targetId); + windowIds.set(targetId, ++windowCounter); + return page; + }; + const page = fakePage(); + const allPages = [page]; + const emit = (event: string, ...args: unknown[]) => { + for (const listener of listeners.get(event) ?? []) listener(...args); + }; + const cdp = { + send: vi.fn(async (command: string, params?: { targetId?: string }) => { + if (command === 'Target.createTarget') { + const created = await context.newPage(); + queueMicrotask(() => emit('page', created)); + return { targetId: targetIds.get(created) }; + } + if (command === 'Browser.getWindowForTarget') return { windowId: windowIds.get(params?.targetId ?? '') }; + if (command === 'Target.closeTarget') return { success: true }; + return {}; + }), + on: vi.fn(), + detach: vi.fn().mockResolvedValue(undefined), + }; + let context: any; + return { + context: context = { + on(event: string, listener: (...args: unknown[]) => void) { + const bucket = listeners.get(event) ?? new Set(); + bucket.add(listener); + listeners.set(event, bucket); + }, + off(event: string, listener: (...args: unknown[]) => void) { + listeners.get(event)?.delete(listener); + }, + emit, + pages: vi.fn(() => allPages.filter((candidate) => !candidate.isClosed())), + newPage: vi.fn(async () => { + const created = fakePage(); + allPages.push(created); + return created; + }), + newCDPSession: vi.fn(async (target: object) => ({ + send: vi.fn(async (command: string, params?: { targetId?: string }) => { + if (command === 'Target.getTargetInfo') return { targetInfo: { targetId: targetIds.get(target) } }; + if (command === 'Browser.getWindowForTarget') return { windowId: windowIds.get(params?.targetId ?? '') }; + return {}; + }), + detach: vi.fn().mockResolvedValue(undefined), + })), + browser: vi.fn().mockReturnValue({ newBrowserCDPSession: vi.fn().mockResolvedValue(cdp) }), + cookies: vi.fn().mockResolvedValue([]), + close: vi.fn().mockResolvedValue(undefined), + }, + page, + }; +} + +describe('browser run recovery from a dead context (#314)', () => { + afterEach(() => { + runBrowserProgram.mockReset(); + }); + + it('evicts the dead Profile runtime after a closed-target run failure so the next command gets a fresh page', async () => { + const dead = fakeContext(); + const replacement = fakeContext(); + const launchPersistentContext = vi.fn() + .mockResolvedValueOnce(dead.context) + .mockResolvedValueOnce(replacement.context); + const manager = new CloakSessionManager({ baseDir: '/tmp/webcmd-run-recovery-test', launchPersistentContext }); + const command = (id: string, extra: Record = {}) => ({ + id, + action: 'run' as const, + profileId: 'default', + session: 'work', + surface: 'browser' as const, + source: "return 'ok';", + ...extra, + }); + + // Existing page still reports isClosed() === false, but the program that + // uses it hits the closed-target signature — the same shape the issue's + // diagnostic trace showed for a dead-but-not-yet-noticed runtime. + runBrowserProgram.mockRejectedValueOnce(new Error('Target page, context or browser has been closed')); + const failed = await dispatchCloakAction(manager, command('run-1')); + + expect(failed.ok).toBe(false); + expect(dead.page.isClosed()).toBe(false); + + runBrowserProgram.mockResolvedValueOnce({ ok: true, result: 'ok' }); + const recovered = await dispatchCloakAction(manager, command('run-2')); + + expect(recovered).toMatchObject({ ok: true, data: { ok: true, result: 'ok' } }); + expect(launchPersistentContext).toHaveBeenCalledTimes(2); + }); + + it('does not evict the runtime for an unrelated failure', async () => { + const context = fakeContext(); + const launchPersistentContext = vi.fn().mockResolvedValue(context.context); + const manager = new CloakSessionManager({ baseDir: '/tmp/webcmd-run-recovery-test-2', launchPersistentContext }); + const command = (id: string) => ({ + id, + action: 'run' as const, + profileId: 'default', + session: 'work', + surface: 'browser' as const, + source: "throw new Error('boom');", + }); + + runBrowserProgram.mockRejectedValueOnce(new Error('boom')); + const failed = await dispatchCloakAction(manager, command('run-1')); + expect(failed.ok).toBe(false); + + runBrowserProgram.mockResolvedValueOnce({ ok: true, result: 'ok' }); + await dispatchCloakAction(manager, command('run-2')); + + expect(launchPersistentContext).toHaveBeenCalledTimes(1); + }); + + it('normalizes the profileId before evicting, so an un-normalized id still finds the runtime', async () => { + const dead = fakeContext(); + const launchPersistentContext = vi.fn().mockResolvedValue(dead.context); + const manager = new CloakSessionManager({ baseDir: '/tmp/webcmd-run-recovery-test-3', launchPersistentContext }); + const lease = await manager.getPage({ profileId: 'default', session: 'work', surface: 'browser' }); + + // The manager keys its runtime map by normalizeProfileId('default') === 'default'; + // callers may reasonably pass an equivalent but un-normalized id. + manager.evictDeadRuntime(' default ', lease.context); + + const replacement = fakeContext(); + launchPersistentContext.mockResolvedValueOnce(replacement.context); + await manager.getPage({ profileId: 'default', session: 'work', surface: 'browser' }); + + expect(launchPersistentContext).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/browser/runtime/local-cloak/session-manager.ts b/src/browser/runtime/local-cloak/session-manager.ts index 49cef1ae..1bb319be 100644 --- a/src/browser/runtime/local-cloak/session-manager.ts +++ b/src/browser/runtime/local-cloak/session-manager.ts @@ -170,7 +170,7 @@ function pageIsClosed(page: PlaywrightPage): boolean { return page.isClosed?.() === true; } -function isClosedContextError(error: unknown): boolean { +export function isClosedContextError(error: unknown): boolean { const message = error instanceof Error ? error.message : String(error); return /Target page, context or browser has been closed/i.test(message); } @@ -422,6 +422,18 @@ export class CloakSessionManager { return this.navigatePageAttempt(input, url, waitUntil, 0); } + /** + * Drop a Profile runtime that a caller has independently proven dead (e.g. a + * `browser run` program observed the closed-target signature). No-ops if the + * Profile has already been replaced, so it never evicts a healthy runtime that + * raced ahead of the failed lease (see #314). + */ + evictDeadRuntime(profileId: string, context: BrowserContext): void { + const normalized = normalizeProfileId(profileId); + const runtime = this.profiles.get(normalized); + if (runtime && runtime.context === context) this.invalidateProfileRuntime(normalized, runtime); + } + private async newPageAttempt(input: SessionKeyInput & { url?: string }, attempt: number): Promise { const profileId = normalizeProfileId(input.profileId); const session = requireSession(input.session);