From c257e3f0dba17c99e95acc193fd3477b4e8121bb Mon Sep 17 00:00:00 2001 From: ROHAN <123131rkorohan@gmail.com> Date: Sun, 16 Aug 2026 20:45:01 +0530 Subject: [PATCH 1/2] fix(browser): evict dead Profile runtime after a closed-target run failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit raw `browser run` leased pages directly and ran the program with no recovery path: `CloakSessionManager.getPage()` treats a lease as healthy whenever `page.isClosed()` is false, but a disconnected context/CDP session can still report that. `dispatchCloakAction`'s 'run' case called `runBrowserProgram` with no try/catch, so a closed-target failure never invalidated the Profile runtime the way `navigatePage`/`newPage` already do — every subsequent raw browser command on that Session kept leasing the same dead page (#314). Wrap the run in try/catch: on the closed-target signature, evict the Profile runtime (new `CloakSessionManager.evictDeadRuntime`, guarded to no-op if the runtime was already replaced) so the next command gets a fresh context/page. We don't retry the program itself here — it may have already caused side effects, so blind replay isn't safe; the caller sees the original error and the next command recovers. Co-Authored-By: Claude Sonnet 5 --- src/browser/runtime/local-cloak/actions.ts | 37 +++-- .../runtime/local-cloak/run-recovery.test.ts | 154 ++++++++++++++++++ .../runtime/local-cloak/session-manager.ts | 13 +- 3 files changed, 190 insertions(+), 14 deletions(-) create mode 100644 src/browser/runtime/local-cloak/run-recovery.test.ts 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..42c69da8 --- /dev/null +++ b/src/browser/runtime/local-cloak/run-recovery.test.ts @@ -0,0 +1,154 @@ +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(); + allPages.push(created); + 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); + }); +}); diff --git a/src/browser/runtime/local-cloak/session-manager.ts b/src/browser/runtime/local-cloak/session-manager.ts index 49cef1ae..111799f9 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,17 @@ 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 runtime = this.profiles.get(profileId); + if (runtime && runtime.context === context) this.invalidateProfileRuntime(profileId, runtime); + } + private async newPageAttempt(input: SessionKeyInput & { url?: string }, attempt: number): Promise { const profileId = normalizeProfileId(input.profileId); const session = requireSession(input.session); From 1cb305f45cd5587254a28d99bd62408ee91dc4ee Mon Sep 17 00:00:00 2001 From: ROHAN <123131rkorohan@gmail.com> Date: Sun, 16 Aug 2026 21:51:06 +0530 Subject: [PATCH 2/2] fix(browser): normalize profileId in evictDeadRuntime, fix test mock double-push MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR review feedback on #323: - evictDeadRuntime looked up this.profiles by the raw profileId param, but the map is keyed by normalizeProfileId(...) everywhere else in this class. As a public method it could silently no-op on an un-normalized id (whitespace/case). Normalize before the lookup. - run-recovery.test.ts's Target.createTarget mock pushed the page created by context.newPage() into allPages a second time — newPage()'s own mock already does that, so this duplicated pages in context.pages(). Adds a regression test proving evictDeadRuntime still finds the runtime when called with an un-normalized profileId. Co-Authored-By: Claude Sonnet 5 --- .../runtime/local-cloak/run-recovery.test.ts | 18 +++++++++++++++++- .../runtime/local-cloak/session-manager.ts | 5 +++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/browser/runtime/local-cloak/run-recovery.test.ts b/src/browser/runtime/local-cloak/run-recovery.test.ts index 42c69da8..a03bbf71 100644 --- a/src/browser/runtime/local-cloak/run-recovery.test.ts +++ b/src/browser/runtime/local-cloak/run-recovery.test.ts @@ -46,7 +46,6 @@ function fakeContext() { send: vi.fn(async (command: string, params?: { targetId?: string }) => { if (command === 'Target.createTarget') { const created = await context.newPage(); - allPages.push(created); queueMicrotask(() => emit('page', created)); return { targetId: targetIds.get(created) }; } @@ -151,4 +150,21 @@ describe('browser run recovery from a dead context (#314)', () => { 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 111799f9..1bb319be 100644 --- a/src/browser/runtime/local-cloak/session-manager.ts +++ b/src/browser/runtime/local-cloak/session-manager.ts @@ -429,8 +429,9 @@ export class CloakSessionManager { * raced ahead of the failed lease (see #314). */ evictDeadRuntime(profileId: string, context: BrowserContext): void { - const runtime = this.profiles.get(profileId); - if (runtime && runtime.context === context) this.invalidateProfileRuntime(profileId, runtime); + 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 {