From 95b6a782481f0b7cea0691134333e94780fb249b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 13:35:18 +0000 Subject: [PATCH 1/2] fix(devx): reserve the port sdui_pick_free_port hands out, instead of probing and letting go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sdui_pick_free_port` bound a probe socket, closed it, and only then reported the port free; the caller bound it afterwards. The scan is deterministic from `base` upward, so concurrent callers did not diverge — they were handed the same port, and the first one every time. Measured before this change, eight concurrent callers scanning from 5180: DISTINCT_PORTS=1 of 8, and six of eight lost the follow-up bind with `EADDRINUSE 127.0.0.1:5180` — the signature that dequeued a PR from the merge queue. A port is now claimed in a host-shared registry (`O_EXCL`, swept of claims whose owner process is gone) before it is probed, and the claim outlives the function. Two cooperating callers can no longer be handed one port. The probe stays for processes outside the registry, and a lost probe hands the claim back rather than hoarding it. The collision test gains the vacuity guard it was missing — BUSY_HELD, so an occupier that lost its own bind can no longer read as a picker that ignores busy ports — plus the concurrency and foreign-steal assertions. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt --- .../gen-sdui-manifest-collision.test.ts | 101 +++++++++++- scripts/gen-sdui-manifest.sh | 155 +++++++++++++++++- 2 files changed, 245 insertions(+), 11 deletions(-) diff --git a/packages/spec/scripts/gen-sdui-manifest-collision.test.ts b/packages/spec/scripts/gen-sdui-manifest-collision.test.ts index 615b8a09bf..fc2d4b2a56 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. @@ -95,6 +120,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)', @@ -108,16 +138,18 @@ function runHarness(): Record { // 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 +176,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 +193,41 @@ 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 'require("node:net").createServer().listen(Number(process.env.SDUI_TEST_PORT), "127.0.0.1")' &`, + '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)"', + 'kill -KILL "$STEAL_PID" 2>/dev/null', ].join('\n'), { mode: 0o755 }, ); @@ -185,6 +253,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 +279,26 @@ 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. + 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; From 7d865c1e89939522ce2a8595a993578a9e12e9e4 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 13:45:31 +0000 Subject: [PATCH 2/2] test(devx): make the collision harness report a lost bind instead of dying on it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both raw listeners in the harness bound with no `error` handler, so a lost bind killed node with an unhandled `error` event and took the harness's exit code with it. That is why the merge-queue report was a stack trace (`EADDRINUSE 127.0.0.1:5180` under `runHarness`) rather than a named precondition — reproduced here on the pre-fix script before this change. They now exit, so the vacuity guards (BUSY_HELD, STEAL_HELD) get to speak. STEAL_CLAIM_ON_PICK joins STEAL_CLAIM_RELEASED as its positive control: "no claim file for that port" is also what a registry that does not exist looks like, so the release assertion alone was measured green against the pre-fix script it was written to fail. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt --- .../gen-sdui-manifest-collision.test.ts | 37 ++++++++++++++++--- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/packages/spec/scripts/gen-sdui-manifest-collision.test.ts b/packages/spec/scripts/gen-sdui-manifest-collision.test.ts index fc2d4b2a56..f49cbb06e3 100644 --- a/packages/spec/scripts/gen-sdui-manifest-collision.test.ts +++ b/packages/spec/scripts/gen-sdui-manifest-collision.test.ts @@ -100,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 { @@ -133,7 +152,7 @@ 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. @@ -215,7 +234,7 @@ function runHarness(): Record { // a port on every future run in this container. 'SPORT="$(sdui_pick_free_port 5180)"', 'export SDUI_TEST_PORT="$SPORT"', - `"$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)} &`, 'STEAL_PID=$!', 'disown "$STEAL_PID" 2>/dev/null || true', 'for _ in $(seq 1 40); do [ "$(port_held "$SPORT")" = yes ] && break; sleep 0.25; done', @@ -227,6 +246,12 @@ function runHarness(): Record { '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 }, @@ -295,7 +320,9 @@ describe.skipIf(!RUNNABLE)('gen-sdui-manifest.sh concurrent-run contract', () => 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. + // 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'); });