From d010d7b243a7c9039dfb06469d66a193e7e37556 Mon Sep 17 00:00:00 2001 From: Andrei Vershinin Date: Sat, 17 Oct 2026 14:07:44 +0000 Subject: [PATCH] fix(browse): bound per-tab reads in `tabs` and `state save` (#3247) A renderer stuck in a long script never answers page.title() or page.evaluate(). `tabs` awaited each title in turn and `state save` read each tab's storage unbounded, so both hung on one busy tab even though neither needs it. - getTabListWithTitles() reads titles in parallel, each bounded by TAB_PROBE_TIMEOUT_MS; a tab that misses it is listed as "(not responding)". - saveState() skips the storage of a tab that does not answer within 5s, the same outcome as a tab whose read fails. busy-active-tab.test.ts holds a real Chromium tab in a busy loop next to an idle one and checks /health, tabs, status, saveState and closetab against it. On main, /health takes 2004 ms and says unhealthy, `tabs` hangs past the test timeout and `status` never names the tab. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Wb6Wntntmwau19fSuSbuaL --- browse/src/browser-manager.ts | 43 +++++++---- browse/src/meta-commands.ts | 2 +- browse/test/busy-active-tab.test.ts | 109 ++++++++++++++++++++++++++++ 3 files changed, 137 insertions(+), 17 deletions(-) create mode 100644 browse/test/busy-active-tab.test.ts diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts index 419299859..1d57b3040 100644 --- a/browse/src/browser-manager.ts +++ b/browse/src/browser-manager.ts @@ -300,6 +300,13 @@ export async function handleChromiumDisconnect(browser: Browser | null): Promise */ 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. + */ +const TAB_STORAGE_READ_TIMEOUT_MS = 5_000; + /** * Resolve with `work`'s value, or with undefined once `ms` passes. The losing * read settles on its own when the renderer frees up, so callers pass a promise @@ -1333,17 +1340,22 @@ export class BrowserManager { return released; } - async getTabListWithTitles(): Promise> { - const tabs: Array<{ id: number; url: string; title: string; active: boolean }> = []; - for (const [id, page] of this.pages) { - tabs.push({ + /** + * Every tab with its title. Titles are read in parallel, each bounded by + * TAB_PROBE_TIMEOUT_MS: a tab whose renderer is stuck comes back with + * `busy: true` and an empty title instead of hanging the list (#3247). + */ + async getTabListWithTitles(): Promise> { + return Promise.all([...this.pages].map(async ([id, page]) => { + const title = await withinTabBudget(page.title().catch(() => ''), TAB_PROBE_TIMEOUT_MS); + return { id, url: page.url(), - title: await page.title().catch(() => ''), + title: title ?? '', active: id === this.activeTabId, - }); - } - return tabs; + busy: title === undefined, + }; + })); } // ─── Session Access ──────────────────────────────────────── @@ -1747,7 +1759,9 @@ export class BrowserManager { // ─── State Save/Restore (shared by recreateContext + handoff) ─ /** * Capture browser state: cookies, localStorage, sessionStorage, URLs, active tab. - * Skips pages that fail storage reads (e.g., already closed). + * 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). */ async saveState(): Promise { if (!this.context) throw new Error('Browser not launched'); @@ -1757,13 +1771,10 @@ export class BrowserManager { for (const [id, page] of this.pages) { const url = page.url(); - let storage = null; - try { - storage = await page.evaluate(() => ({ - localStorage: { ...localStorage }, - sessionStorage: { ...sessionStorage }, - })); - } catch {} + const storage = await withinTabBudget(page.evaluate(() => ({ + localStorage: { ...localStorage }, + sessionStorage: { ...sessionStorage }, + })).catch(() => null), TAB_STORAGE_READ_TIMEOUT_MS) ?? null; // Capture load-html content so a later context recreation (viewport --scale) // can replay it via setTabContent. Never persisted to disk. diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index d79ee91b4..af49910a9 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -265,7 +265,7 @@ export async function handleMetaCommand( case 'tabs': { const tabs = await bm.getTabListWithTitles(); return tabs.map(t => - `${t.active ? '→ ' : ' '}[${t.id}] ${t.title || '(untitled)'} — ${t.url}` + `${t.active ? '→ ' : ' '}[${t.id}] ${t.busy ? '(not responding)' : t.title || '(untitled)'} — ${t.url}` ).join('\n'); } diff --git a/browse/test/busy-active-tab.test.ts b/browse/test/busy-active-tab.test.ts new file mode 100644 index 000000000..3d26b3f82 --- /dev/null +++ b/browse/test/busy-active-tab.test.ts @@ -0,0 +1,109 @@ +/** + * #3247: one busy tab must not make the daemon look dead. A renderer stuck in + * a long script never answers page.evaluate() or page.title(). /health folded + * that into its `status`, so the CLI refused every command (status, tabs, + * tab, newtab, stop) with "Daemon busy" until --force-restart threw the + * session away, and `tabs` and `state save` hung on the tab as well. + * + * 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. + */ +import { afterAll, beforeAll, describe, expect, test } from 'bun:test'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +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 { callRoute, stubRouteContext } from './route-test-harness'; +import { startTestServer } from './test-server'; + +// Outlasts every check below; closing the busy tab at the end stops it sooner. +const BUSY_MS = 30_000; + +let testServer: ReturnType; +let bm: BrowserManager; +let busyId = 0; +let idleId = 0; +let profileDir = ''; +const originalProfile = process.env.CHROMIUM_PROFILE; + +beforeAll(async () => { + // Per-file profile, set in a hook (see commands.test.ts for why). + profileDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-busy-tab-profile-')); + process.env.CHROMIUM_PROFILE = profileDir; + testServer = startTestServer(0); + bm = new BrowserManager(); + await bm.launch(); + busyId = bm.getActiveTabId(); + await bm.getPage().goto(`${testServer.url}/basic.html`); + idleId = await bm.newTab(`${testServer.url}/basic.html`); + bm.switchTab(busyId, { bringToFront: false }); + await bm.getPage().evaluate((ms) => { + setTimeout(() => { const end = Date.now() + ms; while (Date.now() < end) { /* busy */ } }, 50); + }, BUSY_MS); + await Bun.sleep(300); // let the loop start +}, 30_000); + +afterAll(async () => { + try { testServer.server.stop(true); } catch {} + try { await Promise.race([bm?.close(), Bun.sleep(3000)]); } catch {} + if (originalProfile === undefined) delete process.env.CHROMIUM_PROFILE; + else process.env.CHROMIUM_PROFILE = originalProfile; + if (profileDir) fs.rmSync(profileDir, { recursive: true, force: true }); +}); + +describe('busy active tab (#3247)', () => { + test('/health says the daemon is alive and names the busy tab before the CLI gives up', async () => { + // Equal budgets were the bug: the CLI aborted just before the reply landed. + expect(TAB_PROBE_TIMEOUT_MS).toBeLessThan(HEALTH_REQUEST_TIMEOUT_MS); + const ctx = stubRouteContext({ + browserManager: bm, + terminal: { readPort: () => null, grantToken: async () => true, restartSession: async () => true }, + }); + const started = Date.now(); + const body = await (await callRoute('GET', '/health', ctx)).json(); + expect(Date.now() - started).toBeLessThan(HEALTH_REQUEST_TIMEOUT_MS); + expect(body.status).toBe('healthy'); + expect(body.activeTab).toEqual({ id: busyId, responsive: false }); + }); + + test('tabs lists every tab without waiting on the busy one', async () => { + const started = Date.now(); + const out = await handleMetaCommand('tabs', [], bm, async () => {}); + expect(Date.now() - started).toBeLessThan(HEALTH_REQUEST_TIMEOUT_MS); + expect(out).toContain(`[${busyId}] (not responding)`); + expect(out).toContain(`[${idleId}] Test Page - Basic`); + }); + + test('status stays healthy and names the busy tab with a way out', async () => { + const out = await handleMetaCommand('status', [], bm, async () => {}); + expect(out).toContain('Status: healthy'); + expect(out).toContain(`Active tab ${busyId}`); + expect(out).toContain(`closetab ${busyId}`); + }); + + 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(); + }, 15_000); + + test('the idle tab still answers', async () => { + bm.switchTab(idleId, { bringToFront: false }); + try { + expect(await bm.probeActiveTab()).toEqual({ id: idleId, responsive: true }); + expect(await bm.getPage().title()).toBe('Test Page - Basic'); + } finally { + bm.switchTab(busyId, { bringToFront: false }); + } + }); + + test('closetab closes the busy tab and the session carries on', async () => { + const out = await handleMetaCommand('closetab', [String(busyId)], bm, async () => {}); + expect(out).toContain(`Closed tab ${busyId}`); + expect(bm.getTabCount()).toBe(1); + expect(await bm.probeActiveTab()).toEqual({ id: idleId, responsive: true }); + }, 15_000); +});