diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts index 1d57b3040..e89a4a7c6 100644 --- a/browse/src/browser-manager.ts +++ b/browse/src/browser-manager.ts @@ -307,6 +307,8 @@ export const TAB_PROBE_TIMEOUT_MS = 1_000; */ const TAB_STORAGE_READ_TIMEOUT_MS = 5_000; +type TabReadKind = 'probe' | 'title'; + /** * 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 @@ -391,6 +393,11 @@ export class BrowserManager { private extensionContext = false; /** Pending and recorded navigation-guard verdicts per page (D2). */ 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. */ + private pendingTabReads = 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 @@ -1141,8 +1148,23 @@ export class BrowserManager { const id = this.activeTabId; const page = this.pages.get(id); if (!page) return null; - const answered = await withinTabBudget(page.evaluate('1').then(() => true, () => false), timeoutMs); - return { id, responsive: answered === true }; + const probe = this.sharedTabRead(page, 'probe', () => page.evaluate('1').then(() => true, () => false)); + return { id, responsive: (await withinTabBudget(probe, timeoutMs)) === true }; + } + + /** Join the page's pending read of this kind, or start one. `read` must not + * reject. */ + private sharedTabRead(page: Page, kind: TabReadKind, read: () => Promise): Promise { + let reads = this.pendingTabReads.get(page); + if (!reads) { + reads = new Map(); + this.pendingTabReads.set(page, reads); + } + const pending = reads.get(kind); + if (pending) return pending as Promise; + const started = read().finally(() => reads.delete(kind)); + reads.set(kind, started); + return started; } // ─── Tab Management ──────────────────────────────────────── @@ -1347,7 +1369,8 @@ export class BrowserManager { */ async getTabListWithTitles(): Promise> { return Promise.all([...this.pages].map(async ([id, page]) => { - const title = await withinTabBudget(page.title().catch(() => ''), TAB_PROBE_TIMEOUT_MS); + const title = await withinTabBudget( + this.sharedTabRead(page, 'title', () => page.title().catch(() => '')), TAB_PROBE_TIMEOUT_MS); return { id, url: page.url(), diff --git a/browse/test/busy-active-tab.test.ts b/browse/test/busy-active-tab.test.ts index 3d26b3f82..0d55dbe2c 100644 --- a/browse/test/busy-active-tab.test.ts +++ b/browse/test/busy-active-tab.test.ts @@ -6,12 +6,14 @@ * 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. + * each surface must answer within its bound and name the busy tab, without + * piling up reads on the stuck renderer. */ 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 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'; @@ -25,6 +27,7 @@ let testServer: ReturnType; let bm: BrowserManager; let busyId = 0; let idleId = 0; +let busyPage: Page; let profileDir = ''; const originalProfile = process.env.CHROMIUM_PROFILE; @@ -39,7 +42,8 @@ beforeAll(async () => { 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) => { + busyPage = bm.getPage(); + await busyPage.evaluate((ms) => { setTimeout(() => { const end = Date.now() + ms; while (Date.now() < end) { /* busy */ } }, 50); }, BUSY_MS); await Bun.sleep(300); // let the loop start @@ -83,6 +87,19 @@ describe('busy active tab (#3247)', () => { expect(out).toContain(`closetab ${busyId}`); }); + test('a stuck tab gets one pending probe, not one per poll', async () => { + let evaluates = 0; + const evaluate = busyPage.evaluate.bind(busyPage); + Object.assign(busyPage, { evaluate: (...args: Parameters) => { evaluates++; return evaluate(...args); } }); + try { + for (let i = 0; i < 3; i++) expect(await bm.probeActiveTab()).toEqual({ id: busyId, responsive: false }); + } finally { + delete (busyPage as { evaluate?: unknown }).evaluate; // back to the prototype method + } + // The /health check above may already hold the pending probe. + 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);