From 9440d11268de34317cd5923a71bcd2fcf39b9f37 Mon Sep 17 00:00:00 2001 From: Kaushik Samadder Date: Sun, 16 Aug 2026 21:31:47 +0530 Subject: [PATCH] fix(browser): don't reuse a dead page/context lease in browser run/exec page.isClosed() can keep reporting false even after the underlying CDP connection has actually died (crashed renderer, killed process, dropped pipe), so getPage() was handing the same broken lease back out to every browser run/exec command on a Session, matching the repro in #314. - getPage()'s reuse fast path now wraps its existing liveness probe (assertOwnedWindow, which already makes a real CDP round trip) in a try/catch. On a closed-context error it invalidates the Profile runtime and falls through to acquire a fresh page instead of returning the stale one. - runBrowserProgram()'s post-run snapshot capture can hit the same closed-context error while still reporting ok: true (the script may have genuinely succeeded). It now signals that upward via a new onStaleContext option so the run action can invalidate the runtime instead of silently downgrading it to a warning. - Added CloakSessionManager.invalidateIfClosedContext() as the public hook actions.ts uses to wire that signal through. - Hoisted isClosedContextError() into run/types.ts as a shared helper instead of a private duplicate in session-manager.ts. Fixes #314 --- src/browser/run/runner.test.ts | 34 ++++++++++++++++ src/browser/run/runner.ts | 5 +++ src/browser/run/types.ts | 15 +++++++ src/browser/runtime/local-cloak/actions.ts | 1 + .../local-cloak/session-manager.test.ts | 33 ++++++++++++++++ .../runtime/local-cloak/session-manager.ts | 39 ++++++++++++++----- 6 files changed, 117 insertions(+), 10 deletions(-) diff --git a/src/browser/run/runner.test.ts b/src/browser/run/runner.test.ts index 9a93928b..ae371396 100644 --- a/src/browser/run/runner.test.ts +++ b/src/browser/run/runner.test.ts @@ -196,6 +196,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 6dcf9b4d..ebeff07b 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 { @@ -569,6 +570,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 fb1dd686..de22e415 100644 --- a/src/browser/run/types.ts +++ b/src/browser/run/types.ts @@ -86,6 +86,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 c12b35a5..5505f437 100644 --- a/src/browser/runtime/local-cloak/actions.ts +++ b/src/browser/runtime/local-cloak/actions.ts @@ -268,6 +268,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 ef74a144..2b1052fc 100644 --- a/src/browser/runtime/local-cloak/session-manager.test.ts +++ b/src/browser/runtime/local-cloak/session-manager.test.ts @@ -828,6 +828,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 49cef1ae..dc2d1ad7 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; @@ -170,11 +171,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); } @@ -272,11 +268,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, { @@ -651,6 +657,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) {