From dbba25464cc2a8c98c17a0562f2a669e1ab55975 Mon Sep 17 00:00:00 2001 From: truffle Date: Thu, 18 Jun 2026 20:06:03 +0000 Subject: [PATCH] fix(preview): relaunch the shared browser after it disconnects (#146) getOrCreateBrowser and getOrCreatePreviewContext both returned their cached handle unconditionally. When the shared Chromium process dies out of band -- a renderer crash that propagates, an OOM kill, or an external process sweep -- the Browser disconnects but the module-level reference stays non-null, so every later phantom_preview_page and browser_* call hands back a dead handle and fails for the rest of the process lifetime. That is the #146 symptom: "Target/Page crashed" or "...has been closed" across the whole tool surface with no recovery short of a restart. Gate both warm-cache returns on liveness. getOrCreateBrowser checks browser.isConnected() and drops the stale reference so the existing launch path relaunches a fresh process. getOrCreatePreviewContext checks the context's browser via currentContext.browser()?.isConnected() and drops the dead context so it rebuilds on the relaunched browser. The recovery is lazy (next call), which matches the module's existing lazy-singleton shape and needs no disconnected-event listener lifecycle. Two regression tests drive the crash path with a fake browser whose __disconnect() flips isConnected() to false without going through close(): one asserts getOrCreateBrowser relaunches, one asserts getOrCreatePreviewContext rebuilds on a fresh browser. The fakes gain isConnected() and context.browser() to cover the new call surface. --- src/ui/__tests__/preview-correctness.test.ts | 71 ++++++++++++++++++-- src/ui/preview.ts | 33 ++++++--- 2 files changed, 92 insertions(+), 12 deletions(-) diff --git a/src/ui/__tests__/preview-correctness.test.ts b/src/ui/__tests__/preview-correctness.test.ts index f493aa69..8f9e8098 100644 --- a/src/ui/__tests__/preview-correctness.test.ts +++ b/src/ui/__tests__/preview-correctness.test.ts @@ -22,12 +22,15 @@ import { getSessionCount, revokeAllSessions } from "../session.ts"; type Cookie = { name: string; value: string; domain: string; path: string }; type FakeContext = BrowserContext & { __cookies: Cookie[]; __closed: boolean }; -type FakeBrowser = Browser & { __contexts: FakeContext[]; __closed: boolean }; +type FakeBrowser = Browser & { __contexts: FakeContext[]; __closed: boolean; __disconnect: () => void }; -function makeFakeContext(): FakeContext { +function makeFakeContext(parent: FakeBrowser): FakeContext { const state = { __cookies: [] as Cookie[], __closed: false }; const ctx = { ...state, + // preview.ts reads context.browser() to test liveness before reusing a + // cached context, so the fake must report its parent (#146). + browser: () => parent, addCookies: async (cookies: Cookie[]) => { // Match Playwright semantics: replace any cookie with the same // name+domain+path in place, otherwise append. @@ -50,19 +53,30 @@ function makeFakeContext(): FakeContext { function makeFakeBrowser(): FakeBrowser { const contexts: FakeContext[] = []; - const state = { __closed: false }; + const state = { __closed: false, __connected: true }; const b = { + // preview.ts gates cache reuse on isConnected(); a crashed/killed + // browser process reports false and must trigger a relaunch (#146). + isConnected: () => state.__connected, newContext: async () => { - const ctx = makeFakeContext(); + const ctx = makeFakeContext(b); contexts.push(ctx); return ctx; }, close: async () => { state.__closed = true; + state.__connected = false; }, } as unknown as FakeBrowser; Object.defineProperty(b, "__contexts", { get: () => contexts }); Object.defineProperty(b, "__closed", { get: () => state.__closed }); + // Simulate an out-of-band process death (renderer crash, OOM, kill) that + // disconnects the Browser without going through close(). + Object.defineProperty(b, "__disconnect", { + value: () => { + state.__connected = false; + }, + }); return b; } @@ -115,6 +129,55 @@ describe("getOrCreatePreviewContext launch-failure recovery", () => { }); }); +describe("crashed-browser recovery (#146)", () => { + test("getOrCreateBrowser relaunches after the cached browser disconnects", async () => { + const spy = spyOn(chromium, "launch"); + try { + const browsers: FakeBrowser[] = []; + spy.mockImplementation(async () => { + const b = makeFakeBrowser(); + browsers.push(b); + return b; + }); + + const first = (await getOrCreateBrowser()) as FakeBrowser; + // A warm cache hit while still connected must not relaunch. + expect(await getOrCreateBrowser()).toBe(first); + expect(browsers.length).toBe(1); + + // Simulate the process dying out from under us (the #146 symptom). + first.__disconnect(); + + const second = (await getOrCreateBrowser()) as FakeBrowser; + expect(second).not.toBe(first); + expect(second.isConnected()).toBe(true); + expect(browsers.length).toBe(2); + } finally { + spy.mockRestore(); + } + }); + + test("getOrCreatePreviewContext rebuilds on a relaunched browser after a crash", async () => { + const spy = spyOn(chromium, "launch"); + try { + spy.mockImplementation(async () => makeFakeBrowser()); + + const firstCtx = (await getOrCreatePreviewContext()) as FakeContext; + // Warm hit while the browser is alive returns the same context. + expect(await getOrCreatePreviewContext()).toBe(firstCtx); + + // The browser behind the cached context dies. + (firstCtx.browser() as FakeBrowser).__disconnect(); + + const secondCtx = (await getOrCreatePreviewContext()) as FakeContext; + expect(secondCtx).not.toBe(firstCtx); + expect((secondCtx.browser() as FakeBrowser).isConnected()).toBe(true); + } finally { + spy.mockRestore(); + } + }); +}); + describe("shuttingDown flag (review F7)", () => { test("getOrCreateBrowser throws after closePreviewResources", async () => { await closePreviewResources(); diff --git a/src/ui/preview.ts b/src/ui/preview.ts index 1f5439ab..56bf8827 100644 --- a/src/ui/preview.ts +++ b/src/ui/preview.ts @@ -70,7 +70,16 @@ async function injectFreshPreviewCookie(ctx: BrowserContext): Promise { export async function getOrCreateBrowser(): Promise { if (shuttingDown) throw new Error("preview subsystem is shutting down"); - if (browser) return browser; + if (browser) { + // A cached Browser is only reusable while its process is still alive. + // A renderer/browser crash, an OOM kill, or an external process sweep + // disconnects the Browser without clearing this reference, and every + // later call would otherwise hand back a dead handle that fails for the + // rest of the process lifetime (#146). Drop the stale reference on + // disconnect so the launch path below relaunches a fresh process. + if (browser.isConnected()) return browser; + browser = null; + } if (browserPromise) return browserPromise; // try/finally pattern: on either success OR failure we clear the cached // promise. If we did not, a transient chromium.launch() throw would leave @@ -94,14 +103,22 @@ export async function getOrCreateBrowser(): Promise { export async function getOrCreatePreviewContext(): Promise { if (shuttingDown) throw new Error("preview subsystem is shutting down"); if (currentContext) { - // Warm cache path: rotate the preview cookie if we are inside the 2 - // minute safety margin before the 10 minute TTL expires. Playwright's - // addCookies replaces cookies with the same name+domain+path in place, - // so this is an O(1) refresh of the cached context (review F6, Codex P1). - if (Date.now() - lastCookieMintAt >= COOKIE_ROTATE_AFTER_MS) { - await injectFreshPreviewCookie(currentContext); + // A cached context dies with its browser: once the underlying process + // disconnects (crash, OOM, kill) the context handle is permanently + // dead, so drop it and fall through to rebuild a fresh context on a + // relaunched browser rather than returning a corpse (#146). + const ctxBrowser = currentContext.browser(); + if (ctxBrowser?.isConnected()) { + // Warm cache path: rotate the preview cookie if we are inside the 2 + // minute safety margin before the 10 minute TTL expires. Playwright's + // addCookies replaces cookies with the same name+domain+path in place, + // so this is an O(1) refresh of the cached context (review F6, Codex P1). + if (Date.now() - lastCookieMintAt >= COOKIE_ROTATE_AFTER_MS) { + await injectFreshPreviewCookie(currentContext); + } + return currentContext; } - return currentContext; + currentContext = null; } if (currentContextPromise) return currentContextPromise; currentContextPromise = (async () => {