mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-01 06:24:41 +01:00
fix(browse): one pending probe per stuck tab, not one per poll (#3247)
Review on #3250: the 1s race stops waiting but leaves page.evaluate() queued in the stuck renderer, so every /health poll (the extension polls every 10s), `status` and `tabs` call added another pending read, all drained at once when the page recovered. Playwright cannot cancel an in-flight evaluate, so the active-tab probe and the `tabs` title read now join the page's pending read of the same kind instead of starting a new one. busy-active-tab.test.ts counts page.evaluate calls across three probes of the stuck tab: 3 before this change, at most 1 after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wb6Wntntmwau19fSuSbuaL
This commit is contained in:
1 parent
3dbb256796
commit
19fc9b9709
2 files changed
+45
-5
No files matched your search
@@ -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<Page, { pending: Set<Promise<void>>; 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<Page, Map<TabReadKind, Promise<unknown>>>();
|
||||
// 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<T>(page: Page, kind: TabReadKind, read: () => Promise<T>): Promise<T> {
|
||||
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<T>;
|
||||
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<Array<{ id: number; url: string; title: string; active: boolean; busy: boolean }>> {
|
||||
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(),
|
||||
|
||||
@@ -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<typeof startTestServer>;
|
||||
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<Page['evaluate']>) => { 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);
|
||||
|
||||
Reference in new issue
Block a user