diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts index 1de111a77..bf2b707ff 100644 --- a/browse/src/browser-manager.ts +++ b/browse/src/browser-manager.ts @@ -323,6 +323,21 @@ export class BrowserManager { private extensionContext = false; /** Pending and recorded navigation-guard verdicts per page (D2). */ private navigationGuards = new WeakMap>; blocked: string | null }>(); + // 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 + // Playwright 1.62 has no Browser.process() for the SIGKILL fallback). + private disconnectHandlers = new WeakMap void>>(); + private onBrowserDisconnected(browser: Browser | null | undefined, handler: () => void): void { + if (!browser) return; + browser.on('disconnected', handler); + this.disconnectHandlers.set(browser, [...(this.disconnectHandlers.get(browser) ?? []), handler]); + } + private detachDisconnectHandlers(browser: Browser | null | undefined): void { + if (!browser) return; + for (const handler of this.disconnectHandlers.get(browser) ?? []) browser.off('disconnected', handler); + this.disconnectHandlers.delete(browser); + } // ─── Dialog Handling (global, not per-tab) ────────────────── private dialogAutoAccept: boolean = true; @@ -601,7 +616,7 @@ export class BrowserManager { }), { usesCustomExecutable: Boolean(executablePath) }).catch(nameCustomChromium); this.extensionContext = true; this.browser = this.context.browser(); - this.browser?.on('disconnected', () => { + this.onBrowserDisconnected(this.browser, () => { void handleChromiumDisconnect(this.browser); }); for (const page of this.context.pages()) await page.close().catch(() => {}); @@ -651,7 +666,7 @@ export class BrowserManager { // means "user wanted this, don't restart"; non-zero means "crash, please // bring me back." Without this distinction every Cmd+Q gets treated as // a crash and the user-visible window keeps respawning. - this.browser.on('disconnected', () => { + this.onBrowserDisconnected(this.browser, () => { void handleChromiumDisconnect(this.browser); }); @@ -944,7 +959,7 @@ export class BrowserManager { // terminal agent, save session, clean profile locks + state file) so // crashes don't strand resources either. if (this.browser) { - this.browser.on('disconnected', () => { + this.onBrowserDisconnected(this.browser, () => { if (this.intentionalDisconnect) return; const browserRef = this.browser; void (async () => { @@ -1003,14 +1018,14 @@ export class BrowserManager { if (this.connectionMode === 'headed' || this.handoffPrevious || this.extensionContext) { // Headed/persistent context mode: close the context (which closes the browser) this.intentionalDisconnect = true; - if (this.browser) this.browser.removeAllListeners('disconnected'); + this.detachDisconnectHandlers(this.browser); await Promise.race([ this.context ? this.context.close() : Promise.resolve(), raceTimeout(this.closeRaceMs), ]).catch(() => {}); } else if (this.browser) { // Launched mode: close the browser we spawned. - this.browser.removeAllListeners('disconnected'); + this.detachDisconnectHandlers(this.browser); // Grab the child handle BEFORE the race: nulling this.browser after a // race-timeout used to ABANDON a live Chromium whose sockets kept the // caller's event loop (and keep-alive connections into test servers) @@ -1028,7 +1043,7 @@ export class BrowserManager { this.browser = null; } if (previousBrowser && previousBrowser !== currentBrowser) { - previousBrowser.removeAllListeners('disconnected'); + this.detachDisconnectHandlers(previousBrowser); const child = launchedProcess(previousBrowser); const closed = await Promise.race([ previousBrowser.close().then(() => true), raceTimeout(this.closeRaceMs), @@ -2001,7 +2016,7 @@ export class BrowserManager { if (this.browser) { const browserRef = this.browser; - this.browser.on('disconnected', () => { + this.onBrowserDisconnected(this.browser, () => { if (this.intentionalDisconnect) return; void handleChromiumDisconnect(browserRef); }); @@ -2015,7 +2030,7 @@ export class BrowserManager { catch (err) { console.warn('[browse] Headed promotion callback failed:', err); } // 4. Close old headless browser (fire-and-forget) - previous.browser.removeAllListeners('disconnected'); + this.detachDisconnectHandlers(previous.browser); previous.browser.close().catch(() => {}); return [ diff --git a/browse/src/xvfb.ts b/browse/src/xvfb.ts index 64f8284f6..90d85383e 100644 --- a/browse/src/xvfb.ts +++ b/browse/src/xvfb.ts @@ -234,14 +234,23 @@ export async function spawnXvfb(displayNum: number): Promise { * * Best-effort: never throws. */ +function isZombie(pid: number): boolean { + try { + const stat = fs.readFileSync(`/proc/${pid}/stat`, 'utf8'); + return stat.slice(stat.lastIndexOf(')') + 2).startsWith('Z'); + } catch { return false; } +} + export function cleanupXvfb(state: { pid: number; startTime: string; display: string }): void { if (!state.pid) return; if (!isOurXvfb(state.pid, state.startTime)) return; try { safeKill(state.pid, 'SIGTERM'); } catch { /* swallow */ } // Wait briefly for Xvfb to exit, then SIGKILL if still alive. + // An exited child stays a zombie until our event loop reaps it, and this + // synchronous wait blocks that loop, so a zombie counts as exited here. const deadline = Date.now() + 1000; while (Date.now() < deadline) { - if (!isProcessAlive(state.pid)) break; + if (!isProcessAlive(state.pid) || isZombie(state.pid)) break; Bun.sleepSync(10); } if (isOurXvfb(state.pid, state.startTime)) { diff --git a/browse/test/handoff.test.ts b/browse/test/handoff.test.ts index 9fb3649cf..793a40b45 100644 --- a/browse/test/handoff.test.ts +++ b/browse/test/handoff.test.ts @@ -331,10 +331,12 @@ describe.skipIf(process.platform !== 'linux')('lazy owned display lifecycle', () let root: string; let hbm: BrowserManager; const displays = () => { - const result = Bun.spawnSync(['ps', '--ppid', String(process.pid), '-o', 'comm='], { + // A terminated Xvfb can linger as a zombie until this process reaps it; it is no display. + const result = Bun.spawnSync(['ps', '--ppid', String(process.pid), '-o', 'stat=,comm='], { stdout: 'pipe', stderr: 'pipe', timeout: 2000, }); - return result.stdout.toString().split('\n').filter(line => line.trim() === 'Xvfb').length; + return result.stdout.toString().split('\n').map(line => line.trim().split(/\s+/)) + .filter(([stat, comm]) => comm === 'Xvfb' && !stat?.startsWith('Z')).length; }; beforeEach(async () => { diff --git a/browse/test/headless-launch-options.test.ts b/browse/test/headless-launch-options.test.ts index 586bdcd52..3d569fa95 100644 --- a/browse/test/headless-launch-options.test.ts +++ b/browse/test/headless-launch-options.test.ts @@ -51,6 +51,17 @@ describe.skipIf(process.platform !== 'linux')('headless launch options (Linux, r expect(cmdlines.some(c => c.includes('headless_shell'))).toBe(false); }, 60_000); + test('close returns when Chromium exits, not at the close race timeout', async () => { + // Removing every 'disconnected' listener used to strip Playwright's own, + // so browser.close() never resolved and every close waited 5 s. + bm = new BrowserManager(); + await bm.launch(); + const started = performance.now(); + await bm.close(); + expect(performance.now() - started).toBeLessThan(2_500); + bm = null; + }, 60_000); + test('a broken GSTACK_CHROMIUM_PATH fails with the path named', async () => { const missing = path.join(scratch, 'no-such-chromium'); process.env.GSTACK_CHROMIUM_PATH = missing;