fix(browser): address post-impl code review findings

Fixes 4 issues identified in the 2026-04-07 post-implementation code
review. See .tlc/tracks/adopt-lightpanda/reviews/ for the full report.

C-1 (critical, blocker) — src/browser/resolver.js:409
  resolveProbeOnly crashed with 'Cannot read properties of null' when
  stepLocalProbe returned null for a downloadable entry (lightpanda)
  with no local install. Triggered on clean systems with
  BROWSER_CHANNEL=lightpanda via the back-compat shim path.

  Fix: null-guard + return {} (matches pre-subsystem behavior — no
  local install, caller falls through to bundled chromium).

  Regression test added in resolver.chain.test.js covering both
  'lightpanda' and 'panda' (alias).

I-1 (important) — src/browser/downloader.js:239
  Empty-body error path called ws.close() (graceful flush) instead of
  ws.destroy() (immediate), and was missing fsp.unlink(partialPath)
  for the orphan cleanup that the pipeline-failure path at 306 already
  does. Made the two error paths consistent.

I-2 (important) — src/server.js:107 + new ContextPool.replaceBrowser()
  Direct mutation of pool._browser during lightpanda restart was
  reaching into private state and had an undocumented race window
  against in-flight _allocate() calls.

  Added ContextPool.replaceBrowser(browser) with explicit race
  contract in the JSDoc: in-flight calls holding the old reference
  will error, _allocate's catch block releases the slot, client gets
  a retryable error. This is the correct outcome — the old browser is
  dead; no safe way to recover in-flight calls.

  server.js now calls pool.replaceBrowser() instead of mutating the
  private field directly.

I-3 (important) — src/browser/providers/github.js:108
  Asset name matching used .includes(expected) which would
  false-positive on variants like 'lightpanda-aarch64-macos-debug'
  if present before the main asset in upload order.

  Fix: prefer exact match, fall back to extension-suffix match
  (expected + '.') for future archived formats (.tar.gz, .zip).
  Maintains compatibility with current bare-binary releases.

Tests: 943 passing (+1 regression test), 0 failures.

Track: adopt-lightpanda
Review: .tlc/tracks/adopt-lightpanda/reviews/2026-04-07-post-impl-code-review.md

Author: jadb

src/browser/downloader.js

@@ -237,7 +237,13 @@ export async function download(
   // Node's fetch() exposes a web ReadableStream on body; convert to a node stream.
   const body = res.body;
   if (!body) {
-    ws.close();
+    // destroy (not close) to release fd immediately; unlink orphan .partial
+    ws.destroy();
+    try {
+      await fsp.unlink(partialPath);
+    } catch {
+      // ignore — orphan cleanup is best-effort
+    }
     throw new Error(`download: empty response body for ${assetUrl}`);
   }
 

src/browser/providers/github.js

@@ -105,7 +105,17 @@ function findBinaryAsset(release, channelEntry, platform, arch) {
   }
   const expected = substituteAssetPattern(pattern, platform, arch);
   const assets = release.assets || [];
-  const match = assets.find((a) => a.name && a.name.includes(expected));
+
+  // Prefer exact match to avoid false-positives on variants like
+  // `lightpanda-aarch64-macos-debug` or `...-signed`. Fall back to an
+  // extension-suffix match (`<expected>.tar.gz`, `.zip`, etc.) so the
+  // resolver still works if upstream starts shipping archived binaries.
+  const exact = assets.find((a) => a.name === expected);
+  const extSuffix = exact
+    ? null
+    : assets.find((a) => a.name && a.name.startsWith(expected + '.'));
+  const match = exact ?? extSuffix;
+
   if (!match) {
     const names = assets.map((a) => a.name).join(', ') || '(none)';
     throw new Error(

src/browser/resolver.js

@@ -407,6 +407,11 @@ export function resolveProbeOnly(channelRaw) {
   if (!channelRaw) return {};
   const channelId = canonicalizeChannel(channelRaw);
   const record = stepLocalProbe(channelId);
+  // stepLocalProbe returns null for downloadable entries (e.g. lightpanda)
+  // on probe miss — the shim treats that as "not locally installed" and
+  // returns the legacy empty shape so callers fall through to bundled
+  // chromium, matching pre-subsystem behavior.
+  if (!record) return {};
   const out = {};
   if (record.channel) out.channel = record.channel;
   if (record.executablePath) out.executablePath = record.executablePath;

src/server.js

@@ -102,9 +102,11 @@ function attachDisconnectHandler(browserConfig) {
         browserHandle = newHandle;
         browser = newHandle.browser;
         if (pool) {
-          // ContextPool stores the browser as `_browser`; mutate it so new
-          // checkouts see the fresh browser without re-creating the pool.
-          pool._browser = browser;
+          // Swap the browser reference. In-flight _allocate() calls against
+          // the dead browser will throw; ContextPool catches, decrements
+          // active, and the client gets a retryable error. See
+          // ContextPool#replaceBrowser for the race contract.
+          pool.replaceBrowser(browser);
         }
         attachDisconnectHandler(browserConfig);
         return;

src/server/ContextPool.js

@@ -125,6 +125,26 @@ export class ContextPool {
     logger.debug('ContextPool drained', { active: this._active });
   }
 
+  /**
+   * Swap the underlying browser after a lifecycle event (e.g. lightpanda
+   * crash + restart). Future checkouts and future _allocate() calls see
+   * the new browser immediately.
+   *
+   * **Race contract (intentional):** any in-flight _allocate() that has
+   * already read the previous `_browser` reference will complete its
+   * newContext() call against the dead browser and throw. _allocate()'s
+   * catch block decrements `_active` on failure, so the slot is released
+   * and the client receives a retryable error. This is the correct
+   * outcome — in-flight calls were made against a browser that no longer
+   * exists; retrying is the only safe action.
+   *
+   * @param {import('playwright').Browser} browser - fresh browser instance
+   */
+  replaceBrowser(browser) {
+    this._browser = browser;
+    logger.debug('ContextPool: browser replaced', { active: this._active });
+  }
+
   get activeCount() { return this._active; }
   get queueDepth() { return this._queue.length; }
 

test/unit/browser/resolver.chain.test.js

@@ -141,6 +141,19 @@ describe('resolveProbeOnly', () => {
       existsSpy.mockRestore();
     }
   });
+
+  // Regression for post-impl review finding C-1: resolveProbeOnly used to
+  // crash with "Cannot read properties of null" when stepLocalProbe returned
+  // null for a downloadable entry (lightpanda) with no local install.
+  it('returns {} for downloadable channel with no local probe hit', () => {
+    const existsSpy = vi.spyOn(fs, 'existsSync').mockReturnValue(false);
+    try {
+      expect(resolveProbeOnly('lightpanda')).toEqual({});
+      expect(resolveProbeOnly('panda')).toEqual({}); // alias
+    } finally {
+      existsSpy.mockRestore();
+    }
+  });
 });
 
 // ── emitResolved event shape ─────────────────────────────────────────────────