mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-10 22:20:14 +00:00
Closes #309. Full credit to @gigadeniga for the diagnosis — the root cause + proposed fix in that issue are exactly what landed here. ## The bug \`npx hyperframes preview\` failed deterministically on Crostini (ChromeOS Linux) with \`Ports 3002–3101 are all in use\`, even when nothing was actually listening on any of them. ## Why \`testPortOnAllHosts\` ran four probes in parallel: \`\`\`ts const hosts = ["127.0.0.1", "0.0.0.0", "::1", "::"]; const results = await Promise.all(hosts.map((h) => isPortAvailableOnHost(port, h))); \`\`\` Each probe binds a socket and then calls \`server.close()\`. Close is async — the socket stays open until its callback fires on the next event-loop tick. While it's open, the wildcard binds (\`0.0.0.0\`, \`::\`) that include the loopback address race the still-open loopback socket and return \`EADDRINUSE\` spuriously. On Crostini this happens 100% of the time; other Linux configs hit it intermittently; macOS is less predictable. Net effect: every port in the 100-port scan range appears busy and the preview refuses to start. Reproduces on any Linux box with the standalone snippet from the issue: \`\`\` 127.0.0.1: OK 0.0.0.0: EADDRINUSE ← false positive ::1: OK ::: EADDRINUSE ← false positive \`\`\` ## Fix Serialize the probes. Each socket is fully closed before the next opens, eliminating the race window entirely. \`\`\`ts for (const host of hosts) { const available = await isPortAvailableOnHost(port, host); if (!available) return false; } return true; \`\`\` Kept the four-host check rather than collapsing to just \`0.0.0.0\` + \`::\` — the multi-host coverage is load-bearing for the devbox / SSH-forwarding case where a port is free on loopback but held on the wildcard. Sequentializing is the smaller, less-behaviourally-affecting fix. ## Regression tests \`packages/cli/src/server/portUtils.test.ts\` — three cases binding real sockets, no mocks: - **Returns true for a genuinely free port** — directly reproduces the Crostini bug; would fail on Linux against the parallel implementation. - **Returns false when the port is occupied on \`0.0.0.0\`** — confirms the multi-host check still catches the devbox scenario. - **Releases each probe socket before the next run** — two back-to-back calls for the same free port both return true, pinning the sequential contract against future refactors that might try to reparallelize for perf. ## Test plan - [x] \`bunx vitest run packages/cli/src/server/portUtils.test.ts\` — 3/3 pass - [x] Full CLI suite — 109/109 pass - [x] \`tsc --noEmit\` clean ## Notes - Independent of any version bump; ship whenever. - Probing 4 hosts serially adds at most ~tens of milliseconds per port on the scan (binds are very fast on loopback). The worst-case cost shows up when the first port in the range is free — previously 1 parallel round-trip, now 4 sequential — and it's imperceptible (\`preview\` bind is a one-time startup cost, not a hot path).
This commit is contained in:
@@ -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"]);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user