fix(cli): serialize port-availability probes (#309)

packages/cli/src/server/portUtils.test.ts

@@ -0,0 +1,95 @@
+import { afterEach, describe, expect, it, vi } from "vitest";
+import { createServer, type Server } from "node:net";
+import { PORT_PROBE_HOSTS, testPortOnAllHosts } from "./portUtils.js";
+
+// High-ephemeral range with runway so parallel test shards don't collide.
+const BASE = 45_000;
+
+const openServers: Server[] = [];
+
+function allocFreePort(): number {
+  return BASE + Math.floor(Math.random() * 1_000);
+}
+
+afterEach(async () => {
+  await Promise.all(
+    openServers.splice(0).map(
+      (s) =>
+        new Promise<void>((resolve) => {
+          s.close(() => resolve());
+        }),
+    ),
+  );
+  vi.restoreAllMocks();
+});
+
+describe("testPortOnAllHosts — real-socket behaviour (OS-dependent)", () => {
+  // These exercise the real network stack. On Linux the buggy parallel
+  // implementation reliably fails the first test (issue #309 repro); on
+  // macOS the race is not deterministic so both old and new code pass
+  // here. The sequential-contract test below is the platform-agnostic
+  // regression gate.
+
+  it("returns true for a genuinely free port (regression: #309)", async () => {
+    const port = allocFreePort();
+    const result = await testPortOnAllHosts(port);
+    expect(result).toBe(true);
+  });
+
+  it("returns false when the port is occupied on 0.0.0.0", async () => {
+    const port = allocFreePort();
+    const blocker = createServer();
+    openServers.push(blocker);
+    await new Promise<void>((resolve, reject) => {
+      blocker.once("error", reject);
+      blocker.listen({ port, host: "0.0.0.0" }, () => resolve());
+    });
+    const result = await testPortOnAllHosts(port);
+    expect(result).toBe(false);
+  });
+});
+
+describe("testPortOnAllHosts — sequential contract (platform-agnostic)", () => {
+  /**
+   * Load-bearing regression test. Injects a recording fake probe that
+   * holds each call open for a few ms and tracks how many are in flight.
+   * The parallel (buggy) implementation would drive overlap to 4; the
+   * sequential fix keeps it at 1. Deterministic on every OS.
+   */
+  it("runs host probes sequentially — never more than one concurrent", async () => {
+    let inFlight = 0;
+    let peakConcurrency = 0;
+    const hostsProbed: string[] = [];
+
+    const fakeProbe = async (_port: number, host: string): Promise<boolean> => {
+      inFlight++;
+      if (inFlight > peakConcurrency) peakConcurrency = inFlight;
+      hostsProbed.push(host);
+      // Hold so any parallel overlap from a regression would be visible
+      // here regardless of OS scheduling.
+      await new Promise((r) => setTimeout(r, 20));
+      inFlight--;
+      return true;
+    };
+
+    const result = await testPortOnAllHosts(7777, fakeProbe);
+
+    expect(result).toBe(true);
+    expect(peakConcurrency).toBe(1);
+    expect(hostsProbed).toEqual([...PORT_PROBE_HOSTS]);
+  });
+
+  it("short-circuits on the first unavailable host", async () => {
+    const hostsProbed: string[] = [];
+    const fakeProbe = async (_port: number, host: string): Promise<boolean> => {
+      hostsProbed.push(host);
+      // Second host reports in-use; verify we never probe hosts three and four.
+      return host === "127.0.0.1";
+    };
+
+    const result = await testPortOnAllHosts(7777, fakeProbe);
+
+    expect(result).toBe(false);
+    expect(hostsProbed).toEqual(["127.0.0.1", "0.0.0.0"]);
+  });
+});

packages/cli/src/server/portUtils.ts

@@ -49,15 +49,37 @@ function isPortAvailableOnHost(port: number, host: string): Promise<boolean> {
   });
 }
 
+export const PORT_PROBE_HOSTS = ["127.0.0.1", "0.0.0.0", "::1", "::"] as const;
+
 /**
- * Test a port across IPv4 and IPv6 interfaces in parallel. A port is only
- * unavailable if ANY host reports EADDRINUSE. This catches the devbox bug
- * where a port is free on localhost but occupied on 0.0.0.0 via SSH forwarding.
+ * Test a port across IPv4 and IPv6 interfaces. A port is only available if
+ * EVERY host binds and releases cleanly — that catches the devbox class of
+ * bug where a port is free on `127.0.0.1` but held on `0.0.0.0` via SSH
+ * forwarding.
+ *
+ * **Must be sequential, not Promise.all.** Binding `127.0.0.1` holds the
+ * socket open until `server.close()` resolves on the next event-loop tick.
+ * In parallel, the wildcard `0.0.0.0` / `::` tests race that still-open
+ * socket and return spurious `EADDRINUSE` — which makes every port in the
+ * scan range look occupied and the preview server refuse to start. Repro
+ * on Linux (Crostini on ChromeOS in the reporting environment, issue #309)
+ * is deterministic; on macOS/Windows the behaviour is less consistent but
+ * the race is there all the same. Serializing each bind past its close
+ * callback eliminates the window entirely.
+ *
+ * `probe` is injectable for deterministic testing of the sequential
+ * contract — callers in production pass nothing and get the real socket
+ * probe. Tests can pass a recording fake that tracks in-flight probes.
  */
-export async function testPortOnAllHosts(port: number): Promise<boolean> {
-  const hosts = ["127.0.0.1", "0.0.0.0", "::1", "::"];
-  const results = await Promise.all(hosts.map((h) => isPortAvailableOnHost(port, h)));
-  return results.every(Boolean);
+export async function testPortOnAllHosts(
+  port: number,
+  probe: (port: number, host: string) => Promise<boolean> = isPortAvailableOnHost,
+): Promise<boolean> {
+  for (const host of PORT_PROBE_HOSTS) {
+    const available = await probe(port, host);
+    if (!available) return false;
+  }
+  return true;
 }
 
 // ── Existing instance detection ────────────────────────────────────────────