From 17cd11d4653657f63bd26f3ed995a09eaa87a73d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Fri, 17 Apr 2026 23:59:35 +0200 Subject: [PATCH 1/2] fix(cli): serialize port-availability probes (#309) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit testPortOnAllHosts ran the four host probes (127.0.0.1 / 0.0.0.0 / ::1 / ::) in parallel via Promise.all. The first bind on 127.0.0.1 held its socket open until server.close() resolved on the next event-loop tick, and while it was open the wildcard 0.0.0.0 / :: probes raced it and got EADDRINUSE. Result: every port in the 3002–3101 scan range looked occupied and the preview server refused to start — deterministic on Linux (Crostini on ChromeOS in the reporting environment, gigadeniga's issue #309). Fix is to test the hosts sequentially so each probe's socket is fully closed before the next opens. The multi-host check itself stays — it's load-bearing for devbox / SSH-forwarding layouts where a port is free on loopback but held on the wildcard. Regression tests bind real sockets (no mocks) and would have failed against the parallel implementation on any Linux shard. --- packages/cli/src/server/portUtils.test.ts | 62 +++++++++++++++++++++++ packages/cli/src/server/portUtils.ts | 24 +++++++-- 2 files changed, 81 insertions(+), 5 deletions(-) create mode 100644 packages/cli/src/server/portUtils.test.ts diff --git a/packages/cli/src/server/portUtils.test.ts b/packages/cli/src/server/portUtils.test.ts new file mode 100644 index 000000000..8f64704dd --- /dev/null +++ b/packages/cli/src/server/portUtils.test.ts @@ -0,0 +1,62 @@ +import { afterEach, describe, expect, it } from "vitest"; +import { createServer, type Server } from "node:net"; +import { testPortOnAllHosts } from "./portUtils.js"; + +// A genuinely-free port on every OS we care about. Picked from the high +// IANA-ephemeral range with enough runway that random collisions are rare; +// if a test still flakes on a loaded CI shard we bump `BASE`. +const BASE = 45_000; + +const openServers: Server[] = []; + +function allocFreePort(): number { + // Each test picks its own port out of a counter to avoid cross-test reuse. + return BASE + Math.floor(Math.random() * 1_000); +} + +afterEach(async () => { + await Promise.all( + openServers.splice(0).map( + (s) => + new Promise((resolve) => { + s.close(() => resolve()); + }), + ), + ); +}); + +describe("testPortOnAllHosts", () => { + it("returns true for a genuinely free port (regression: #309)", async () => { + // The bug: running the four probes in parallel caused the `0.0.0.0` bind + // to collide with the still-open `127.0.0.1` socket, so `every port is + // occupied` on Linux. This test binds NOTHING and asserts the scanner + // sees the port as free — which only works when the internal probes are + // sequential and each socket is fully closed before the next opens. + 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((resolve, reject) => { + blocker.once("error", reject); + blocker.listen({ port, host: "0.0.0.0" }, () => resolve()); + }); + const result = await testPortOnAllHosts(port); + expect(result).toBe(false); + }); + + it("releases each probe socket before starting the next (no wildcard-vs-loopback race)", async () => { + // Sanity-check the shape of the bug itself: two back-to-back calls for + // the same free port must both return true. If the first call left a + // socket lingering, the second would see EADDRINUSE. + const port = allocFreePort(); + const first = await testPortOnAllHosts(port); + const second = await testPortOnAllHosts(port); + expect(first).toBe(true); + expect(second).toBe(true); + }); +}); diff --git a/packages/cli/src/server/portUtils.ts b/packages/cli/src/server/portUtils.ts index d66a74e7d..d681e12e5 100644 --- a/packages/cli/src/server/portUtils.ts +++ b/packages/cli/src/server/portUtils.ts @@ -50,14 +50,28 @@ function isPortAvailableOnHost(port: number, host: string): Promise { } /** - * 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. */ export async function testPortOnAllHosts(port: number): Promise { 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); + for (const host of hosts) { + const available = await isPortAvailableOnHost(port, host); + if (!available) return false; + } + return true; } // ── Existing instance detection ──────────────────────────────────────────── From 3bbe78b63c84877afedf623330547a4131867d13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Sat, 18 Apr 2026 00:10:24 +0200 Subject: [PATCH 2/2] fix(cli): platform-agnostic regression test for port probe race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Strengthens the regression gate from issue #309. The original tests binding real sockets only caught the bug on Linux (Crostini and likely Ubuntu), not macOS where wildcard-vs-loopback parallel binds happen to succeed. That left CI as the only gate and only if the CI runner's kernel reproduces the race. testPortOnAllHosts now takes an optional injectable probe. The contract test passes a recording fake that holds each call for 20ms and tracks in-flight count. Buggy parallel code drives it to 4; the sequential fix keeps it at 1. Verified by reverting the fix in place and confirming the contract test fails with expected=1 / received=4 — then restored and all 4 tests pass. Production callers pass no probe and get the real socket check, unchanged behaviour. --- packages/cli/src/server/portUtils.test.ts | 75 ++++++++++++++++------- packages/cli/src/server/portUtils.ts | 16 +++-- 2 files changed, 66 insertions(+), 25 deletions(-) diff --git a/packages/cli/src/server/portUtils.test.ts b/packages/cli/src/server/portUtils.test.ts index 8f64704dd..b48d0f7a5 100644 --- a/packages/cli/src/server/portUtils.test.ts +++ b/packages/cli/src/server/portUtils.test.ts @@ -1,16 +1,13 @@ -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { createServer, type Server } from "node:net"; -import { testPortOnAllHosts } from "./portUtils.js"; +import { PORT_PROBE_HOSTS, testPortOnAllHosts } from "./portUtils.js"; -// A genuinely-free port on every OS we care about. Picked from the high -// IANA-ephemeral range with enough runway that random collisions are rare; -// if a test still flakes on a loaded CI shard we bump `BASE`. +// High-ephemeral range with runway so parallel test shards don't collide. const BASE = 45_000; const openServers: Server[] = []; function allocFreePort(): number { - // Each test picks its own port out of a counter to avoid cross-test reuse. return BASE + Math.floor(Math.random() * 1_000); } @@ -23,15 +20,17 @@ afterEach(async () => { }), ), ); + vi.restoreAllMocks(); }); -describe("testPortOnAllHosts", () => { +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 () => { - // The bug: running the four probes in parallel caused the `0.0.0.0` bind - // to collide with the still-open `127.0.0.1` socket, so `every port is - // occupied` on Linux. This test binds NOTHING and asserts the scanner - // sees the port as free — which only works when the internal probes are - // sequential and each socket is fully closed before the next opens. const port = allocFreePort(); const result = await testPortOnAllHosts(port); expect(result).toBe(true); @@ -48,15 +47,49 @@ describe("testPortOnAllHosts", () => { const result = await testPortOnAllHosts(port); expect(result).toBe(false); }); +}); - it("releases each probe socket before starting the next (no wildcard-vs-loopback race)", async () => { - // Sanity-check the shape of the bug itself: two back-to-back calls for - // the same free port must both return true. If the first call left a - // socket lingering, the second would see EADDRINUSE. - const port = allocFreePort(); - const first = await testPortOnAllHosts(port); - const second = await testPortOnAllHosts(port); - expect(first).toBe(true); - expect(second).toBe(true); +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 => { + 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 => { + 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"]); }); }); diff --git a/packages/cli/src/server/portUtils.ts b/packages/cli/src/server/portUtils.ts index d681e12e5..dec508cc6 100644 --- a/packages/cli/src/server/portUtils.ts +++ b/packages/cli/src/server/portUtils.ts @@ -49,6 +49,8 @@ function isPortAvailableOnHost(port: number, host: string): Promise { }); } +export const PORT_PROBE_HOSTS = ["127.0.0.1", "0.0.0.0", "::1", "::"] as const; + /** * 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 @@ -64,11 +66,17 @@ function isPortAvailableOnHost(port: number, host: string): Promise { * 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 { - const hosts = ["127.0.0.1", "0.0.0.0", "::1", "::"]; - for (const host of hosts) { - const available = await isPortAvailableOnHost(port, host); +export async function testPortOnAllHosts( + port: number, + probe: (port: number, host: string) => Promise = isPortAvailableOnHost, +): Promise { + for (const host of PORT_PROBE_HOSTS) { + const available = await probe(port, host); if (!available) return false; } return true;