diff --git a/src/browser/run/runner.test.ts b/src/browser/run/runner.test.ts index 138c7eea..505444fb 100644 --- a/src/browser/run/runner.test.ts +++ b/src/browser/run/runner.test.ts @@ -236,6 +236,40 @@ afterAll(async () => { })); }); + it('signals a stale context when the post-snapshot fails with a closed-context error (webcmd#314)', async () => { + const newCDPSession = context.newCDPSession.bind(context); + let calls = 0; + context.newCDPSession = ((...args: Parameters) => { + calls += 1; + if (calls === 2) return Promise.reject(new Error('Target page, context or browser has been closed')); + return newCDPSession(...args); + }) as BrowserContext['newCDPSession']; + const onStaleContext = vi.fn(); + + const output = await run('return 7;', { snapshotDiff: true, onStaleContext }); + + expect(output.result).toBe(7); + expect(output.warnings).toContainEqual(expect.objectContaining({ + code: 'BROWSER_RUN_SNAPSHOT_FAILED', + })); + expect(onStaleContext).toHaveBeenCalledTimes(1); + }); + + it('does not signal a stale context for an unrelated post-snapshot failure', async () => { + const newCDPSession = context.newCDPSession.bind(context); + let calls = 0; + context.newCDPSession = ((...args: Parameters) => { + calls += 1; + if (calls === 2) return Promise.reject(new Error('post snapshot failed')); + return newCDPSession(...args); + }) as BrowserContext['newCDPSession']; + const onStaleContext = vi.fn(); + + await run('return 7;', { snapshotDiff: true, onStaleContext }); + + expect(onStaleContext).not.toHaveBeenCalled(); + }); + it('does not expose page.snapshotForAI inside browser-run code', async () => { const output = await run('return typeof page.snapshotForAI;'); diff --git a/src/browser/run/runner.ts b/src/browser/run/runner.ts index b6d8432d..6a5cc08a 100644 --- a/src/browser/run/runner.ts +++ b/src/browser/run/runner.ts @@ -33,6 +33,7 @@ import { type BrowserRunResult, type BrowserRunTimings, type BrowserRunWarning, + isClosedContextError, } from './types.js'; export interface BrowserRunSessionScope { @@ -650,6 +651,10 @@ export async function runBrowserProgram( code: 'BROWSER_RUN_SNAPSHOT_FAILED', message: normalizeExecutionError(snapshotError).message, }); + // The run itself already succeeded, so keep ok: true — but a closed-context + // signature here means the connection is dying underneath it. Signal the + // caller out-of-band so the dead Profile runtime doesn't get reused (#314). + if (isClosedContextError(snapshotError)) options.onStaleContext?.(); } finally { timings.snapshot_ms = (timings.snapshot_ms ?? 0) + Math.max(0, Date.now() - snapshotStartedAt); } diff --git a/src/browser/run/types.ts b/src/browser/run/types.ts index a964c1fb..09fa8861 100644 --- a/src/browser/run/types.ts +++ b/src/browser/run/types.ts @@ -88,6 +88,21 @@ export interface BrowserRunOptions { snapshotMode?: SnapshotTreeMode; snapshotBaselineStore?: SnapshotBaselineStore; signal?: AbortSignal; + /** + * Called when a closed-context signature ("Target page, context or browser + * has been closed") surfaces somewhere that doesn't itself fail the run — + * currently, the post-run snapshot capture (webcmd#314). The run still + * completes with `ok: true` (the program itself may have genuinely + * succeeded), but the caller should treat the underlying Profile runtime as + * dead so the next command on this Session isn't handed the same lease. + */ + onStaleContext?: () => void; +} + +/** Matches Playwright's "Target page, context or browser has been closed" error family. */ +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); } export interface BrowserRunLogEntry { diff --git a/src/browser/runtime/local-cloak/actions.ts b/src/browser/runtime/local-cloak/actions.ts index 19f7d499..41f6c169 100644 --- a/src/browser/runtime/local-cloak/actions.ts +++ b/src/browser/runtime/local-cloak/actions.ts @@ -279,6 +279,7 @@ export async function dispatchCloakAction(manager: CloakSessionManager, command: snapshotDiff: command.noSnapshotDiff ? false : command.snapshotDiff, snapshotMode: command.snapshotMode === 'tree' ? 'tree' : 'act', snapshotBaselineStore: snapshotBaselineStore(manager), + onStaleContext: () => manager.invalidateIfClosedContext(lease.profileId, scope.context), ...(signal ? { signal } : {}), }); return { diff --git a/src/browser/runtime/local-cloak/session-manager.test.ts b/src/browser/runtime/local-cloak/session-manager.test.ts index 786042ec..ab1a9f21 100644 --- a/src/browser/runtime/local-cloak/session-manager.test.ts +++ b/src/browser/runtime/local-cloak/session-manager.test.ts @@ -850,6 +850,39 @@ describe('CloakSessionManager', () => { expect(launchPersistentContext).toHaveBeenCalledTimes(2); }); + it('invalidates a reused getPage() lease when the CDP liveness probe finds a dead context (webcmd#314)', async () => { + const first = fakeContext(); + const replacement = fakeContext(); + replacement.context.pages.mockReturnValue([]); + const launchPersistentContext = vi.fn() + .mockResolvedValueOnce(first.context) + .mockResolvedValueOnce(replacement.context); + const manager = new CloakSessionManager({ baseDir: '/tmp/webcmd-test', launchPersistentContext }); + + const firstLease = await manager.getPage({ profileId: 'default', session: 'work', surface: 'browser' }); + expect(firstLease.context).toBe(first.context); + + // Simulate the issue's repro: isClosed() still reports false, but the + // underlying CDP connection is dead, so the liveness probe used on reuse + // (Browser.getWindowForTarget, via assertOwnedWindow) fails. + first.cdp.send.mockImplementation(async (command: string) => { + if (command === 'Browser.getWindowForTarget') { + throw new Error('Target page, context or browser has been closed'); + } + return {}; + }); + + // isClosed() still reports false right up to the reuse attempt — the fast + // path's precondition holds; only the liveness probe catches the dead lease. + expect(firstLease.page.isClosed()).toBe(false); + const secondLease = await manager.getPage({ profileId: 'default', session: 'work', surface: 'browser' }); + + expect(secondLease.context).toBe(replacement.context); + expect(secondLease.page).not.toBe(firstLease.page); + expect((firstLease.page as unknown as { close: ReturnType }).close).toHaveBeenCalled(); + expect(launchPersistentContext).toHaveBeenCalledTimes(2); + }); + it('retries explicit newPage page creation once after a closed-context failure', async () => { const closed = new Error('browserContext.newPage: Target page, context or browser has been closed'); const first = fakeContext(); diff --git a/src/browser/runtime/local-cloak/session-manager.ts b/src/browser/runtime/local-cloak/session-manager.ts index 04dca882..8778add2 100644 --- a/src/browser/runtime/local-cloak/session-manager.ts +++ b/src/browser/runtime/local-cloak/session-manager.ts @@ -11,6 +11,7 @@ import { findPackageRoot } from '../../../package-paths.js'; import { findExactCloakProfileProcesses } from './process-matcher.js'; import { log } from '../../../logger.js'; import { CliError, EXIT_CODES } from '../../../errors.js'; +import { isClosedContextError } from '../../run/types.js'; const UNRESOLVED = Symbol('unresolved'); const TARGET_PAGE_MATCH_TIMEOUT_MS = 1_000; @@ -180,11 +181,6 @@ function pageIsClosed(page: PlaywrightPage): boolean { return page.isClosed?.() === true; } -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); -} - function errorMessage(error: unknown): string { return error instanceof Error ? error.message : String(error); } @@ -282,11 +278,21 @@ export class CloakSessionManager { const sessionRuntime = this.getSessionRuntime(runtime, sessionId); const existing = sessionRuntime.pages.get(leaseKey); if (existing && !pageIsClosed(existing.page) && !freshPage) { - await this.assertOwnedWindow(runtime, sessionId, existing); - runtime.lastSeenAt = Date.now(); - existing.idleTimeout = input.idleTimeout; - this.refreshIdleTimer(runtime, sessionRuntime, leaseKey, existing); - return { profileId, leaseKey, context: runtime.context, page: existing.page, pageId: existing.pageId }; + try { + await this.assertOwnedWindow(runtime, sessionId, existing); + runtime.lastSeenAt = Date.now(); + existing.idleTimeout = input.idleTimeout; + this.refreshIdleTimer(runtime, sessionRuntime, leaseKey, existing); + return { profileId, leaseKey, context: runtime.context, page: existing.page, pageId: existing.pageId }; + } catch (error) { + if (!isClosedContextError(error)) throw error; + // isClosed() reported false, but the liveness probe above shows the + // underlying CDP connection is actually dead. Invalidate the Profile + // runtime and fall through to acquire a fresh page instead of handing + // the same broken lease back out (webcmd#314). + this.invalidateProfileRuntime(profileId, runtime); + if (!pageIsClosed(existing.page)) await existing.page.close().catch(() => {}); + } } const acquired = await this.acquireSessionPage(profileId, sessionId, input.windowMode); const entry = await this.registerOwnedPage(acquired.runtime, acquired.session, acquired.page, { @@ -667,6 +673,19 @@ export class CloakSessionManager { return entries.length; } + /** + * Invalidates the Profile runtime backing `context`, if it's still the active + * one, without evicting or retrying the command that observed it. Used by the + * `run` action (webcmd#314) when a post-run snapshot capture surfaces a + * closed-context signature: the run itself may have genuinely succeeded, but + * the connection is dying, so the next command on this Session shouldn't be + * handed the same lease. + */ + invalidateIfClosedContext(profileId: string, context: BrowserContext): void { + const runtime = this.profiles.get(profileId); + if (runtime?.context === context) this.invalidateProfileRuntime(profileId, runtime); + } + async shutdown(): Promise { this.shuttingDown = true; while (this.profileLaunches.size > 0) {