From ec154bcb83c553e85c6b22aa965519efb9ea9e69 Mon Sep 17 00:00:00 2001
From: Garry Tan <garrytan@gmail.com>
Date: Sat, 15 Aug 2026 16:49:40 -0700
Subject: [PATCH] test(browse): unit coverage for the close() SIGKILL fallback
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The wedge fix (capture the Chromium child before the close race,
SIGKILL on timeout) shipped without a test of the branch it added —
the coverage audit flagged it as the diff's one regression-gap. The
5s race window becomes an injectable closeRaceMs field, and four unit
tests pin: SIGKILL on hang, no SIGKILL on clean close, no SIGKILL on
an already-exited child, SIGKILL on a rejecting close.
---
 browse/src/browser-manager.ts            | 10 +++-
 browse/test/browser-manager-unit.test.ts | 66 ++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 2 deletions(-)

diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts
index 39bed7712..a18bb92bd 100644
--- a/browse/src/browser-manager.ts
+++ b/browse/src/browser-manager.ts
@@ -797,6 +797,12 @@ export class BrowserManager {
     this.consecutiveFailures = 0;
   }
 
+  // How long close() waits for a graceful shutdown before falling back to
+  // SIGKILL (launched mode) or abandoning the context close (headed mode).
+  // A field, not a literal, so the SIGKILL fallback is unit-testable without
+  // a 5-second wait.
+  private closeRaceMs = 5000;
+
   async close() {
     // unref'd race timer: without unref, every successful close still pins
     // the caller's event loop for the full window.
@@ -811,7 +817,7 @@ export class BrowserManager {
         if (this.browser) this.browser.removeAllListeners('disconnected');
         await Promise.race([
           this.context ? this.context.close() : Promise.resolve(),
-          raceTimeout(5000),
+          raceTimeout(this.closeRaceMs),
         ]).catch(() => {});
       } else {
         // Launched mode: close the browser we spawned.
@@ -824,7 +830,7 @@ export class BrowserManager {
         const child = this.browser.process?.();
         const closed = await Promise.race([
           this.browser.close().then(() => true as const),
-          raceTimeout(5000),
+          raceTimeout(this.closeRaceMs),
         ]).catch(() => false as const);
         if (closed === false && child && child.exitCode === null && !child.killed) {
           try { child.kill('SIGKILL'); } catch { /* already gone */ }
diff --git a/browse/test/browser-manager-unit.test.ts b/browse/test/browser-manager-unit.test.ts
index d0ef8d7a6..11e8b822d 100644
--- a/browse/test/browser-manager-unit.test.ts
+++ b/browse/test/browser-manager-unit.test.ts
@@ -303,3 +303,69 @@ describe('stealth injected on every context-creation path', () => {
     expect(sites.length).toBeGreaterThanOrEqual(2);
   });
 });
+
+describe('close() launched-mode SIGKILL fallback', () => {
+  // The wedge this guards against: browser.close() hangs, the race times
+  // out, and pre-fix code nulled this.browser — ABANDONING a live Chromium
+  // whose sockets pinned the caller's event loop forever. The child must be
+  // captured before the race and SIGKILLed when graceful close loses.
+  type FakeChild = { exitCode: number | null; killed: boolean; kill: (sig: string) => void };
+  const makeCloseFakes = (closeBehavior: () => Promise<void>, child?: Partial<FakeChild>) => {
+    const kills: string[] = [];
+    const fakeChild: FakeChild = {
+      exitCode: null,
+      killed: false,
+      kill: (sig: string) => { kills.push(sig); },
+      ...child,
+    };
+    const fakeBrowser = {
+      removeAllListeners: () => fakeBrowser,
+      process: () => fakeChild,
+      close: closeBehavior,
+    };
+    return { kills, fakeBrowser };
+  };
+
+  const managerWith = async (fakeBrowser: unknown) => {
+    const { BrowserManager } = await import('../src/browser-manager');
+    const bm = new BrowserManager();
+    const raw = bm as unknown as { browser: unknown; connectionMode: string; closeRaceMs: number };
+    raw.browser = fakeBrowser;
+    raw.connectionMode = 'launched';
+    raw.closeRaceMs = 20;
+    return { bm, raw };
+  };
+
+  it('SIGKILLs a live child when graceful close exceeds the race window', async () => {
+    const { kills, fakeBrowser } = makeCloseFakes(() => new Promise<void>(() => {}));
+    const { bm, raw } = await managerWith(fakeBrowser);
+    await bm.close();
+    expect(kills).toEqual(['SIGKILL']);
+    expect(raw.browser).toBeNull();
+  });
+
+  it('does not SIGKILL when graceful close finishes in time', async () => {
+    const { kills, fakeBrowser } = makeCloseFakes(async () => {});
+    const { bm, raw } = await managerWith(fakeBrowser);
+    await bm.close();
+    expect(kills).toEqual([]);
+    expect(raw.browser).toBeNull();
+  });
+
+  it('does not SIGKILL a child that already exited', async () => {
+    const { kills, fakeBrowser } = makeCloseFakes(
+      () => new Promise<void>(() => {}),
+      { exitCode: 0 },
+    );
+    const { bm } = await managerWith(fakeBrowser);
+    await bm.close();
+    expect(kills).toEqual([]);
+  });
+
+  it('survives a rejecting close() and still SIGKILLs the live child', async () => {
+    const { kills, fakeBrowser } = makeCloseFakes(() => Promise.reject(new Error('target closed')));
+    const { bm } = await managerWith(fakeBrowser);
+    await bm.close();
+    expect(kills).toEqual(['SIGKILL']);
+  });
+});