diff --git a/packages/spec/scripts/gen-sdui-manifest-collision.test.ts b/packages/spec/scripts/gen-sdui-manifest-collision.test.ts index 615b8a09bf..f49cbb06e3 100644 --- a/packages/spec/scripts/gen-sdui-manifest-collision.test.ts +++ b/packages/spec/scripts/gen-sdui-manifest-collision.test.ts @@ -44,6 +44,31 @@ // nothing was listening and nothing was checked — the phantom-assertion failure // the cleanup test's header records paying for once already. // +// ## The port helper had to become a RESERVATION, and that is measured here too +// +// "picked at run time by the script's own helper" was not enough, and the way +// it failed is worth keeping. The helper used to bind a probe socket, CLOSE it, +// and report the port free; the caller bound it afterwards. The scan is +// deterministic from the base upward, so concurrent callers did not diverge — +// they were handed the SAME port, every time, and the first one every time. +// Measured on this tree with the reservation removed, eight concurrent callers +// from base 5180: +// +// DISTINCT_PORTS=1 of 8 # all eight got 5180 +// BIND_OK=2 BIND_ERR=6 # six lost the follow-up bind: +// # Error: listen EADDRINUSE 127.0.0.1:5180 +// +// That is what dequeued a PR from the merge queue, and it presented HERE — this +// file draws from 5180 three times, and the write-target test beside it drives +// the real path for a fourth, concurrently, in one package's test run. +// +// The subtle half is how it presented. The occupier below lost its OWN bind to +// a concurrent scanner and died, so the following pick returned the occupied +// number quite legitimately — the port really was free by then — and the +// failure read as "the picker does not skip a busy port". It was not that. So +// BUSY_HELD is asserted before PICKED_WITH_BUSY is believed: a precondition +// that can evaporate silently is a test that accuses the wrong function. +// // No vite and no console build: the contract under test is one the shell script // owns, and the ports are picked at run time by the script's own helper so this // test cannot collide with a concurrent agent — which would be a poor look here. @@ -75,8 +100,27 @@ const RUNNABLE = /** A tiny HTTP server on $SDUI_TEST_PORT, as a `node -e` program. */ const HTTP_STUB = [ 'const http = require("node:http");', - 'http.createServer((_q, r) => { r.writeHead(200); r.end("SERVER"); })', - ' .listen(Number(process.env.SDUI_TEST_PORT), "127.0.0.1");', + 'const s = http.createServer((_q, r) => { r.writeHead(200); r.end("SERVER"); });', + // Losing the bind must EXIT, not throw: an unhandled 'error' event kills the + // harness with a stack trace where a vacuity guard would have named the + // problem. See RAW_LISTENER below for the same reasoning. + 's.once("error", () => process.exit(1));', + 's.listen(Number(process.env.SDUI_TEST_PORT), "127.0.0.1");', +].join(''); + +/** + * A bare TCP listener on $SDUI_TEST_PORT that reports a lost bind by exiting. + * + * The unguarded form of this line is what made the merge-queue failure this + * file now pins so hard to read: when a concurrent scanner took the port + * first, node threw `Unhandled 'error' event ... EADDRINUSE 127.0.0.1:5180`, + * the harness died mid-measurement, and the report was a stack trace instead + * of "the port this test meant to occupy was never occupied". + */ +const RAW_LISTENER = [ + 'const s = require("node:net").createServer();', + 's.once("error", () => process.exit(1));', + 's.listen(Number(process.env.SDUI_TEST_PORT), "127.0.0.1");', ].join(''); function runHarness(): Record { @@ -95,6 +139,11 @@ function runHarness(): Record { `source ${JSON.stringify(SCRIPT)}`, `DIR=${JSON.stringify(dir)}`, 'NODE_BIN="$(command -v node)"', + // "is $1 held right now?" — the same probe shape the script uses, so a + // yes here and a skip there are answers to the same question. + 'port_held() {', + ' "$NODE_BIN" -e \'const n=require("node:net");const s=n.createServer();s.once("error",()=>{console.log("yes");process.exit(0)});s.once("listening",()=>s.close(()=>{console.log("no");process.exit(0)}));s.listen(Number(process.argv[1]),"127.0.0.1")\' "$1"', + '}', '', '# ── 1. the argv the script actually spawns ──────────────────────────', 'readarray -t ARGV < <(sdui_dev_server_cmd 4321)', @@ -103,21 +152,23 @@ function runHarness(): Record { '# ── 2. the free-port search skips a port that is taken right now ────', 'BUSY="$(sdui_pick_free_port 5180)"', 'export SDUI_TEST_PORT="$BUSY"', - `"$NODE_BIN" -e 'require("node:net").createServer().listen(Number(process.env.SDUI_TEST_PORT), "127.0.0.1")' &`, + `"$NODE_BIN" -e ${JSON.stringify(RAW_LISTENER)} &`, 'BUSY_PID=$!', // disown: otherwise bash prints its own "Killed" job notice when this is // reaped below, which reads like a test failure in the vitest output. 'disown "$BUSY_PID" 2>/dev/null || true', - 'for _ in $(seq 1 40); do', - ' "$NODE_BIN" -e \'const n=require("node:net");const s=n.createServer();s.once("error",()=>process.exit(0));s.once("listening",()=>s.close(()=>process.exit(1)));s.listen(Number(process.env.SDUI_TEST_PORT),"127.0.0.1")\' && break', - ' sleep 0.25', - 'done', + 'for _ in $(seq 1 40); do [ "$(port_held "$BUSY")" = yes ] && break; sleep 0.25; done', + // Vacuity guard, and the one this file was missing when it ejected a PR: + // if the occupier never took the port, the pick below returns it and the + // green/red says nothing about the picker. + 'printf "BUSY_HELD=%s\\n" "$(port_held "$BUSY")"', 'printf "BUSY_PORT=%s\\n" "$BUSY"', 'printf "PICKED_WITH_BUSY=%s\\n" "$(sdui_pick_free_port "$BUSY")"', 'kill -KILL "$BUSY_PID" 2>/dev/null', '', '# ── 3. a NEIGHBOUR answering on our port is refused, not accepted ───', 'NPORT="$(sdui_pick_free_port 5180)"', + 'printf "NPORT=%s\\n" "$NPORT"', 'export SDUI_TEST_PORT="$NPORT"', `"$NODE_BIN" -e ${JSON.stringify(HTTP_STUB)} &`, 'NEIGHBOUR_PID=$!', @@ -144,6 +195,7 @@ function runHarness(): Record { '', '# ── 4. our OWN server is accepted (the check is not just "always no") ─', 'OPORT="$(sdui_pick_free_port 5180)"', + 'printf "OPORT=%s\\n" "$OPORT"', 'export SDUI_TEST_PORT="$OPORT"', 'PF2="$DIR/own.pid"', `sdui_spawn_detached "$PF2" "$DIR/own.log" "$NODE_BIN" -e ${JSON.stringify(HTTP_STUB)}`, @@ -160,6 +212,47 @@ function runHarness(): Record { // quietly, since nothing else in the run would notice until two agents // overlapped again. `printf "FIXED_LOG_LITERALS=%s\\n" "$(grep -c '/tmp/sdui-dump-dev\\.log' ${JSON.stringify(SCRIPT)} || true)"`, + '', + '# ── 6. concurrent callers from ONE base get DISTINCT ports ──────────', + // The card, as an executed assertion. Eight subshells, one base, at + // once: before the reservation every one of them was handed the same + // number — not sometimes, by construction. Eight makes a regression + // certain to show rather than likely to. + 'mkdir -p "$DIR/picks"', + 'for i in $(seq 1 8); do ( sdui_pick_free_port 5180 > "$DIR/picks/$i" 2>/dev/null; echo >> "$DIR/picks/$i" ) & done', + 'wait', + // grep -c . rather than wc -l: a pick that failed leaves an empty line, + // and counting it would let TOTAL and DISTINCT agree at the wrong value. + 'printf "CONCURRENT_TOTAL=%s\\n" "$(cat "$DIR/picks"/* | grep -c .)"', + 'printf "CONCURRENT_DISTINCT=%s\\n" "$(cat "$DIR/picks"/* | sort -u | grep -c .)"', + '', + '# ── 7. a port held from OUTSIDE the registry is skipped, and the ────', + '# claim taken on it is handed back rather than hoarded ──────────', + // The honest limit of a reservation: it binds only callers that share + // the registry. Against everything else the probe is still the answer, + // and losing there must not leak the claim — a hoarded claim would cost + // a port on every future run in this container. + 'SPORT="$(sdui_pick_free_port 5180)"', + 'export SDUI_TEST_PORT="$SPORT"', + `"$NODE_BIN" -e ${JSON.stringify(RAW_LISTENER)} &`, + 'STEAL_PID=$!', + 'disown "$STEAL_PID" 2>/dev/null || true', + 'for _ in $(seq 1 40); do [ "$(port_held "$SPORT")" = yes ] && break; sleep 0.25; done', + 'printf "STEAL_HELD=%s\\n" "$(port_held "$SPORT")"', + // A registry of its own, so the listener above is genuinely foreign to + // it — the shared registry already knows this port is ours. + 'RESV="$DIR/resv"', + 'STEAL_PICK="$( (export SDUI_PORT_RESERVATION_DIR="$RESV"; sdui_pick_free_port "$SPORT") )"', + 'printf "STEAL_PORT=%s\\n" "$SPORT"', + 'printf "STEAL_PICK=%s\\n" "$STEAL_PICK"', + 'printf "STEAL_CLAIM_RELEASED=%s\\n" "$([ -e "$RESV/$SPORT" ] && echo no || echo yes)"', + // Positive control for the line above. "No claim file for $SPORT" is + // also what a registry that does not exist at all looks like, so on its + // own that line goes green against the version this test was written to + // fail — measured. The claim on the port actually HANDED OUT is the + // half that can only be true when the registry is real. + 'printf "STEAL_CLAIM_ON_PICK=%s\\n" "$([ -e "$RESV/$STEAL_PICK" ] && echo yes || echo no)"', + 'kill -KILL "$STEAL_PID" 2>/dev/null', ].join('\n'), { mode: 0o755 }, ); @@ -185,6 +278,11 @@ describe.skipIf(!RUNNABLE)('gen-sdui-manifest.sh concurrent-run contract', () => }); it('picks a per-run port, skipping one that is already taken', () => { + // Precondition before conclusion: the occupier must really hold the port. + // Without this line an occupier that lost its own bind reads as a picker + // that ignores busy ports, which is how this file accused the wrong + // function while ejecting a PR from the merge queue. + expect(seen.BUSY_HELD, JSON.stringify(seen)).toBe('yes'); expect(seen.BUSY_PORT).toMatch(/^\d+$/); expect(seen.PICKED_WITH_BUSY).toMatch(/^\d+$/); expect(seen.PICKED_WITH_BUSY).not.toBe(seen.BUSY_PORT); @@ -206,6 +304,28 @@ describe.skipIf(!RUNNABLE)('gen-sdui-manifest.sh concurrent-run contract', () => expect(seen.ACCEPTED_OWN).toBe('yes'); }); + it('hands concurrent callers scanning one base distinct ports', () => { + // Vacuity: all eight callers must have produced a port at all. + expect(seen.CONCURRENT_TOTAL, JSON.stringify(seen)).toBe('8'); + expect(seen.CONCURRENT_DISTINCT, JSON.stringify(seen)).toBe('8'); + }); + + it('gives each pick within one run its own port', () => { + const picked = [seen.BUSY_PORT, seen.NPORT, seen.OPORT]; + expect(picked.every((p) => /^\d+$/.test(p ?? '')), picked.join(',')).toBe(true); + expect(new Set(picked).size, picked.join(',')).toBe(3); + }); + + it('skips a port held from outside the registry and releases the claim', () => { + expect(seen.STEAL_HELD, JSON.stringify(seen)).toBe('yes'); + expect(seen.STEAL_PICK).toMatch(/^\d+$/); + expect(seen.STEAL_PICK).not.toBe(seen.STEAL_PORT); + // The claim it took before probing must not survive a lost probe — and + // the positive control first, so "released" cannot mean "never taken". + expect(seen.STEAL_CLAIM_ON_PICK, JSON.stringify(seen)).toBe('yes'); + expect(seen.STEAL_CLAIM_RELEASED, JSON.stringify(seen)).toBe('yes'); + }); + it('keeps no fixed dev-server log path (spelling pin)', () => { expect(seen.FIXED_LOG_LITERALS).toBe('0'); }); diff --git a/scripts/gen-sdui-manifest.sh b/scripts/gen-sdui-manifest.sh index 43fdc1e4fe..53136ce35e 100755 --- a/scripts/gen-sdui-manifest.sh +++ b/scripts/gen-sdui-manifest.sh @@ -187,22 +187,154 @@ sdui_stop_detached() { # two literals in this file, it was between the port requested and the port bound. # --------------------------------------------------------------------------- -# Print a TCP port on 127.0.0.1 that is free right now, searching upward from $1. -# -# ADVISORY ONLY. It reserves nothing, and between this probe and vite's own bind -# a concurrent run can take the port. Closing that race is not this helper's job: -# `--strictPort` turns losing it into a loud failure instead of a silent redirect -# onto a neighbour's server. +# Print a TCP port on 127.0.0.1 that is free right now AND reserved for this +# caller, searching upward from $1. +# +# WHY A RESERVATION AND NOT JUST A PROBE. +# +# The probe below used to be the whole of this helper: bind a socket, CLOSE it, +# and report the port free. Between that close and the caller's own bind the +# port is unowned, and the scan is deterministic from $base upward, so every +# concurrent caller starting at the same base was handed the same number. A +# check-then-use race whose "use" is in another process — and one that collides +# by construction rather than by bad luck. Measured on this tree with the +# reservation removed, eight concurrent callers scanning from 5180: +# +# DISTINCT_PORTS=1 of 8 # every one of them was handed 5180 +# BIND_OK=2 BIND_ERR=6 # six lost the follow-up bind: +# # Error: listen EADDRINUSE 127.0.0.1:5180 +# +# That is the shape that dequeued a PR from the merge queue. The contention is +# not hypothetical: the collision test beside this script draws from 5180 three +# times, the write-target test drives this real path for a fourth, and they run +# concurrently inside one package's test run. +# +# So a port is CLAIMED before it is probed, in a registry every caller on this +# host shares, and the claim outlives this function — it is released when the +# claiming process dies, not when this function returns. Two cooperating callers +# can no longer be handed the same port at all. The probe stays, because a claim +# says nothing about processes that never heard of this registry. +# +# STILL ADVISORY against those. Nothing here can stop a process outside the +# registry from taking the port between this function and the caller's bind; +# `--strictPort` plus `sdui_wait_for_own_server` is what turns losing that race +# into a loud failure instead of a silent redirect onto a neighbour's server. +# What the claim removes is the collision this script's own callers cause each +# other, which is every collision anyone has actually measured here. # # 127.0.0.1 is the interface vite's dev server binds. A wildcard listener on the # same port collides with a loopback bind too, so this probe sees a neighbour # whichever way the neighbour bound. sdui_pick_free_port() { local base="${1:-5180}" span="${2:-200}" - node - "$base" "$span" << 'SDUI_PICK_FREE_PORT' + local dir="${SDUI_PORT_RESERVATION_DIR:-${TMPDIR:-/tmp}/sdui-port-reservations}" + + # `O_EXCL` alone already gives exactly one winner per port, so the lock is not + # what makes a claim exclusive — it makes the SWEEP of abandoned claims safe, + # which is the one step that unlinks a file another scanner may be creating. + # Missing `flock` therefore degrades to "sound, minus the sweep's tie-break", + # never to a hard failure: this helper runs on developer machines too. + # + # The descriptor is opened by a subshell that exits before anything is + # spawned, so nothing this script starts can inherit it — see the cleanup + # contract above, where an inherited lock fd is what closed a container. + if command -v flock > /dev/null 2>&1; then + ( + flock -w 30 9 2> /dev/null || true + sdui_scan_and_reserve_port "$base" "$span" + ) 9> "${dir}.lock" + else + sdui_scan_and_reserve_port "$base" "$span" + fi +} + +# The scan itself: claim, then probe, then hand the port over. Split out from +# `sdui_pick_free_port` only so the lock above wraps one named thing. +sdui_scan_and_reserve_port() { + local base="$1" span="$2" + node - "$base" "$span" "$$" << 'SDUI_PICK_FREE_PORT' const net = require('node:net'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + const base = Number(process.argv[2]); const span = Number(process.argv[3]); +// The CALLER's pid, not this node process's: the caller is what will hold the +// port, and its death is what makes the claim collectable. +const owner = Number(process.argv[4]); + +// One registry per host, shared by every caller that sources this script — +// sharing it IS the fix, so the tests that measure contention deliberately do +// not override this. The override exists for tests of the protocol itself. +const dir = + process.env.SDUI_PORT_RESERVATION_DIR || + path.join(process.env.TMPDIR || os.tmpdir(), 'sdui-port-reservations'); + +// A claim is collectable once its owner is gone. The floor keeps a claim +// written moments ago out of the sweep whatever its pid says; the ceiling is a +// backstop for the case pid liveness cannot see — a dead owner whose pid number +// has since been reused by something unrelated. +const SWEEP_FLOOR_MS = 10_000; +const SWEEP_CEILING_MS = 12 * 60 * 60 * 1000; + +const alive = (pid) => { + try { + process.kill(pid, 0); + return true; + } catch (err) { + return err.code === 'EPERM'; // it exists, it is simply not ours to signal + } +}; + +const sweep = () => { + let names = []; + try { + names = fs.readdirSync(dir); + } catch { + return; + } + const now = Date.now(); + for (const name of names) { + if (!/^\d+$/.test(name)) continue; + const file = path.join(dir, name); + let holder; + let age; + try { + holder = Number(String(fs.readFileSync(file, 'utf8')).trim()); + age = now - fs.statSync(file).mtimeMs; + } catch { + continue; // vanished under us — someone else already collected it + } + if (age < SWEEP_FLOOR_MS) continue; + if (age < SWEEP_CEILING_MS && Number.isInteger(holder) && holder > 0 && alive(holder)) continue; + try { + fs.unlinkSync(file); + } catch { + /* already gone */ + } + } +}; + +// `wx` is O_CREAT|O_EXCL: exactly one creator wins, and the losers are told so +// HERE rather than discovering it at bind time in another process. +const claim = (port) => { + try { + fs.writeFileSync(path.join(dir, String(port)), `${owner}\n`, { flag: 'wx' }); + return true; + } catch { + return false; + } +}; + +const release = (port) => { + try { + fs.unlinkSync(path.join(dir, String(port))); + } catch { + /* already gone */ + } +}; + const isFree = (port) => new Promise((resolve) => { const probe = net.createServer(); @@ -210,12 +342,21 @@ const isFree = (port) => probe.once('listening', () => probe.close(() => resolve(true))); probe.listen(port, '127.0.0.1'); }); + (async () => { + fs.mkdirSync(dir, { recursive: true }); + sweep(); for (let port = base; port < base + span; port += 1) { + // Claimed by a live caller — including by an earlier call from THIS caller, + // which is why two picks in one run cannot return one port. + if (!claim(port)) continue; if (await isFree(port)) { process.stdout.write(String(port)); return; } + // Held by something outside the registry. Hand the claim back rather than + // hoarding a port we never got, and keep scanning. + release(port); } process.stderr.write(`no free TCP port in [${base}, ${base + span})\n`); process.exitCode = 1;