From 6a2e589cbd5f8bf3161e3268f649f7569c329ad4 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sat, 15 Aug 2026 09:50:19 -0700 Subject: [PATCH] fix(browse): SIGKILL abandoned Chromium on close-race timeout (suite wedge) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit close()'s launched-mode path raced browser.close() against 5s and on timeout ABANDONED the child: this.browser nulled, process handle lost, Chromium alive holding keep-alive connections into test servers whose stop() then waits forever. Reproduced twice as an intermittent (~50%) whole-suite wedge — a 44min 0.1%-CPU hang pinned by a leaked LISTEN socket, and a 400s hang with commands.test.ts teardown in flight. The child handle is now captured BEFORE the race and SIGKILLed on race-timeout (launched mode only; headed keeps context.close). Race timers are unref'd so a successful close stops pinning the caller's event loop for the window. The four browse test servers force-close keep-alives (stop(true)) as belt-and-braces. Co-Authored-By: Claude Fable 5 --- browse/src/browser-manager.ts | 27 +++++++++++++++++++++------ browse/test/batch.test.ts | 2 +- browse/test/commands.test.ts | 2 +- browse/test/handoff.test.ts | 2 +- browse/test/snapshot.test.ts | 2 +- 5 files changed, 25 insertions(+), 10 deletions(-) diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts index 255f6b583..c93fa5971 100644 --- a/browse/src/browser-manager.ts +++ b/browse/src/browser-manager.ts @@ -727,6 +727,12 @@ export class BrowserManager { } async close() { + // unref'd race timer: without unref, every successful close still pins + // the caller's event loop for the full window. + const raceTimeout = (ms: number) => new Promise((resolve) => { + const t = setTimeout(() => resolve(false), ms); + (t as { unref?: () => void }).unref?.(); + }); if (this.browser || (this.connectionMode === 'headed' && this.context)) { if (this.connectionMode === 'headed') { // Headed/persistent context mode: close the context (which closes the browser) @@ -734,15 +740,24 @@ export class BrowserManager { if (this.browser) this.browser.removeAllListeners('disconnected'); await Promise.race([ this.context ? this.context.close() : Promise.resolve(), - new Promise(resolve => setTimeout(resolve, 5000)), + raceTimeout(5000), ]).catch(() => {}); } else { - // Launched mode: close the browser we spawned + // Launched mode: close the browser we spawned. this.browser.removeAllListeners('disconnected'); - await Promise.race([ - this.browser.close(), - new Promise(resolve => setTimeout(resolve, 5000)), - ]).catch(() => {}); + // 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) + // open forever — the intermittent whole-suite wedge. If graceful close + // doesn't finish in time, the child gets SIGKILL, not freedom. + const child = this.browser.process?.(); + const closed = await Promise.race([ + this.browser.close().then(() => true as const), + raceTimeout(5000), + ]).catch(() => false as const); + if (closed === false && child && child.exitCode === null && !child.killed) { + try { child.kill('SIGKILL'); } catch { /* already gone */ } + } } this.browser = null; } diff --git a/browse/test/batch.test.ts b/browse/test/batch.test.ts index a6ee8a2b0..452f60e08 100644 --- a/browse/test/batch.test.ts +++ b/browse/test/batch.test.ts @@ -43,7 +43,7 @@ beforeAll(async () => { }); afterAll(async () => { - try { testServer.server.stop(); } catch {} + try { testServer.server.stop(true); } catch {} // force-close keep-alives — a lingering Chromium connection otherwise blocks stop() forever // Close only this file's own browser — never process.exit(): bun test runs // all files in one process, so a delayed exit kills the whole suite // (see test/no-suicide-exit.test.ts). close() can hang when the browser diff --git a/browse/test/commands.test.ts b/browse/test/commands.test.ts index 8b33fdb75..caf1464df 100644 --- a/browse/test/commands.test.ts +++ b/browse/test/commands.test.ts @@ -95,7 +95,7 @@ beforeAll(async () => { }); afterAll(async () => { - try { testServer.server.stop(); } catch {} + try { testServer.server.stop(true); } catch {} // force-close keep-alives — a lingering Chromium connection otherwise blocks stop() forever // Close only this file's own browser — never process.exit(): bun test runs // all files in one process, so a delayed exit kills the whole suite // (see test/no-suicide-exit.test.ts). close() can hang when the browser diff --git a/browse/test/handoff.test.ts b/browse/test/handoff.test.ts index a395ab51d..22d87b3af 100644 --- a/browse/test/handoff.test.ts +++ b/browse/test/handoff.test.ts @@ -27,7 +27,7 @@ beforeAll(async () => { }); afterAll(async () => { - try { testServer.server.stop(); } catch {} + try { testServer.server.stop(true); } catch {} // force-close keep-alives — a lingering Chromium connection otherwise blocks stop() forever // Close only this file's own browser — never process.exit(): bun test runs // all files in one process, so a delayed exit kills the whole suite // (see test/no-suicide-exit.test.ts). close() can hang when the browser diff --git a/browse/test/snapshot.test.ts b/browse/test/snapshot.test.ts index d3c012eaf..51d2b24f1 100644 --- a/browse/test/snapshot.test.ts +++ b/browse/test/snapshot.test.ts @@ -32,7 +32,7 @@ beforeAll(async () => { }); afterAll(async () => { - try { testServer.server.stop(); } catch {} + try { testServer.server.stop(true); } catch {} // force-close keep-alives — a lingering Chromium connection otherwise blocks stop() forever // Close only this file's own browser — never process.exit(): bun test runs // all files in one process, so a delayed exit kills the whole suite // (see test/no-suicide-exit.test.ts). close() can hang when the browser