diff --git a/src/ui/__tests__/preview-correctness.test.ts b/src/ui/__tests__/preview-correctness.test.ts index f493aa6..8f9e809 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 1f5439a..56bf882 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 () => {