From 60cbaebee5dd11befa8348212d6a6041b6acf354 Mon Sep 17 00:00:00 2001 From: Andrei Vershinin Date: Sat, 17 Oct 2026 16:11:20 +0000 Subject: [PATCH] fix(browse): keep a busy tab's last good storage in saved state (#3247) Review on #3250: with BROWSE_PERSIST_STATE=1 the periodic snapshot used to hang on a stuck tab, so session-state.json kept its last good copy. The 5s storage bound let the snapshot finish and write `storage: null` for that tab over it, so a restart lost its localStorage and sessionStorage even with persistence on. saveState() now records the storage each page answers with, together with its origin. A read that times out returns that last good storage when the origin still matches, and null otherwise. A failed read stays null, as before. The storage read also joins the page's pending read instead of queueing a new one on every persist tick. busy-active-tab.test.ts takes a good snapshot, hangs the tab, and checks that persistSessionState() still writes its localStorage. Live, with BROWSE_PERSIST_STATE=1 and a 2s interval, session-state.json kept {"probe":"kept"} for the busy tab, and its savedAt stayed current through 12s of busy page. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Wb6Wntntmwau19fSuSbuaL --- browse/src/browser-manager.ts | 52 +++++++++++++++++++++-------- browse/test/busy-active-tab.test.ts | 18 ++++++---- 2 files changed, 51 insertions(+), 19 deletions(-) diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts index e89a4a7c6..7ad480813 100644 --- a/browse/src/browser-manager.ts +++ b/browse/src/browser-manager.ts @@ -302,12 +302,17 @@ export const TAB_PROBE_TIMEOUT_MS = 1_000; /** * Budget for saveState()'s per-tab storage read. Longer than the probe: a tab - * that misses it loses its localStorage/sessionStorage across a context - * recreation or handoff, so only a tab that is really stuck is skipped. + * that misses it falls back to the last storage it answered with, which may + * be stale, so only a tab that is really stuck takes that path. */ const TAB_STORAGE_READ_TIMEOUT_MS = 5_000; -type TabReadKind = 'probe' | 'title'; +type TabStorage = { localStorage: Record; sessionStorage: Record }; +type TabReadKind = 'probe' | 'title' | 'storage'; + +function urlOrigin(url: string): string { + return URL.canParse(url) ? new URL(url).origin : ''; +} /** * Resolve with `work`'s value, or with undefined once `ms` passes. The losing @@ -333,7 +338,7 @@ export interface BrowserState { pages: Array<{ url: string; isActive: boolean; - storage: { localStorage: Record; sessionStorage: Record } | null; + storage: TabStorage | null; /** * HTML content loaded via load-html (setContent), replayed after context recreation. * In-memory only — never persisted to disk (HTML may contain secrets or customer data). @@ -395,9 +400,11 @@ export class BrowserManager { private navigationGuards = new WeakMap>; blocked: string | null }>(); /** One pending read per page and kind (#3247). A read that outlives its * budget stays queued in the renderer until the page frees up, so a stuck - * tab reuses it instead of collecting a new one on every /health poll or - * `tabs` call. */ + * tab reuses it instead of collecting a new one on every /health poll, + * `tabs` call or persist tick. */ private pendingTabReads = new WeakMap>>(); + /** The last storage each page answered with, and the origin it came from. */ + private lastTabStorage = new WeakMap(); // Our own 'disconnected' listeners per browser. Detach only these: removing // every 'disconnected' listener also strips Playwright's internal one that // resolves browser.close(), so each close waited the full race timeout (and @@ -1782,9 +1789,9 @@ export class BrowserManager { // ─── State Save/Restore (shared by recreateContext + handoff) ─ /** * Capture browser state: cookies, localStorage, sessionStorage, URLs, active tab. - * Skips the storage of pages that fail the read (e.g., already closed) or - * whose renderer is stuck past TAB_STORAGE_READ_TIMEOUT_MS, so `state save` - * still works while one tab is busy (#3247). + * Skips the storage of pages that fail the read (e.g., already closed). + * A page whose renderer is stuck past TAB_STORAGE_READ_TIMEOUT_MS does not + * hold up `state save` (#3247); see readTabStorage() for what it gets. */ async saveState(): Promise { if (!this.context) throw new Error('Browser not launched'); @@ -1794,10 +1801,7 @@ export class BrowserManager { for (const [id, page] of this.pages) { const url = page.url(); - const storage = await withinTabBudget(page.evaluate(() => ({ - localStorage: { ...localStorage }, - sessionStorage: { ...sessionStorage }, - })).catch(() => null), TAB_STORAGE_READ_TIMEOUT_MS) ?? null; + const storage = await this.readTabStorage(page, url); // Capture load-html content so a later context recreation (viewport --scale) // can replay it via setTabContent. Never persisted to disk. @@ -1819,6 +1823,28 @@ export class BrowserManager { return { cookies, pages }; } + /** + * One tab's localStorage/sessionStorage for saveState(). A failed read + * (closed page, opaque origin) is null. A read that times out returns the + * last storage this page gave for the same origin: a periodic + * BROWSE_PERSIST_STATE snapshot must not replace good storage on disk with + * null just because the page is busy. + */ + private async readTabStorage(page: Page, url: string): Promise { + const origin = urlOrigin(url); + const read = await withinTabBudget(this.sharedTabRead(page, 'storage', () => page.evaluate(() => ({ + localStorage: { ...localStorage }, + sessionStorage: { ...sessionStorage }, + })).catch(() => null)), TAB_STORAGE_READ_TIMEOUT_MS); + if (read === undefined) { + const last = this.lastTabStorage.get(page); + return last?.origin === origin ? last.storage : null; + } + if (read) this.lastTabStorage.set(page, { origin, storage: read }); + else this.lastTabStorage.delete(page); + return read; + } + /** * Restore browser state into the current context: cookies, pages, storage. * Navigates to saved URLs, restores storage, wires page events. diff --git a/browse/test/busy-active-tab.test.ts b/browse/test/busy-active-tab.test.ts index 0d55dbe2c..04a15861b 100644 --- a/browse/test/busy-active-tab.test.ts +++ b/browse/test/busy-active-tab.test.ts @@ -7,7 +7,7 @@ * * A real Chromium tab is held in a busy loop while a second tab stays idle; * each surface must answer within its bound and name the busy tab, without - * piling up reads on the stuck renderer. + * piling up reads on the stuck renderer or dropping its persisted storage. */ import { afterAll, beforeAll, describe, expect, test } from 'bun:test'; import * as fs from 'fs'; @@ -17,6 +17,7 @@ import type { Page } from 'playwright'; import { BrowserManager, TAB_PROBE_TIMEOUT_MS } from '../src/browser-manager'; import { HEALTH_REQUEST_TIMEOUT_MS } from '../src/cli'; import { handleMetaCommand } from '../src/meta-commands'; +import { persistSessionState } from '../src/session-persist'; import { callRoute, stubRouteContext } from './route-test-harness'; import { startTestServer } from './test-server'; @@ -43,6 +44,9 @@ beforeAll(async () => { idleId = await bm.newTab(`${testServer.url}/basic.html`); bm.switchTab(busyId, { bringToFront: false }); busyPage = bm.getPage(); + // The good snapshot a BROWSE_PERSIST_STATE tick takes before the page hangs. + await busyPage.evaluate(() => localStorage.setItem('probe', 'kept')); + await bm.saveState(); await busyPage.evaluate((ms) => { setTimeout(() => { const end = Date.now() + ms; while (Date.now() < end) { /* busy */ } }, 50); }, BUSY_MS); @@ -100,11 +104,13 @@ describe('busy active tab (#3247)', () => { expect(evaluates).toBeLessThanOrEqual(1); }, 10_000); - test('saveState finishes with every tab, skipping only the busy tab storage', async () => { - const state = await bm.saveState(); - expect(state.pages).toHaveLength(2); - expect(state.pages.find(p => p.isActive)?.storage).toBeNull(); - expect(state.pages.find(p => !p.isActive)?.storage).not.toBeNull(); + test('a persisted snapshot taken while the tab is busy keeps its last good storage', async () => { + const file = path.join(profileDir, 'session-state.json'); + await persistSessionState(bm, file); + const saved = JSON.parse(fs.readFileSync(file, 'utf8')); + expect(saved.pages).toHaveLength(2); + expect(saved.pages.find((p: { isActive: boolean }) => p.isActive).storage.localStorage).toEqual({ probe: 'kept' }); + expect(saved.pages.find((p: { isActive: boolean }) => !p.isActive).storage).not.toBeNull(); }, 15_000); test('the idle tab still answers', async () => {