Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions src/browser/run/runner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<BrowserContext['newCDPSession']>) => {
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<BrowserContext['newCDPSession']>) => {
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;');

Expand Down
5 changes: 5 additions & 0 deletions src/browser/run/runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
type BrowserRunResult,
type BrowserRunTimings,
type BrowserRunWarning,
isClosedContextError,
} from './types.js';

export interface BrowserRunSessionScope {
Expand Down Expand Up @@ -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);
}
Expand Down
15 changes: 15 additions & 0 deletions src/browser/run/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions src/browser/runtime/local-cloak/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
33 changes: 33 additions & 0 deletions src/browser/runtime/local-cloak/session-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof vi.fn> }).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();
Expand Down
39 changes: 29 additions & 10 deletions src/browser/runtime/local-cloak/session-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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, {
Expand Down Expand Up @@ -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<void> {
this.shuttingDown = true;
while (this.profileLaunches.size > 0) {
Expand Down