From c5ff1d94929f28c891e5c8112ef48ba4ebb94190 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 14:40:41 +0000 Subject: [PATCH] fix(devx): reserve the port smoke_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 `smoke_pick_free_port` bound a probe socket, closed it, and only then reported the port free; `objectstack dev` bound it afterwards. The scan walks `base` upward deterministically, so concurrent callers were handed the same port — the first one, every time. Ports are now CLAIMED before they are probed, in a host-shared registry, and the claim outlives the function. Ports the same design PR #10217 landed for `sdui_pick_free_port` (#10167), keeping this script's wildcard probe, which matches serve.ts isPortAvailable(). Part of #10212 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt --- .../publish-smoke-port-collision.test.ts | 94 ++++++++++ scripts/publish-smoke.sh | 176 +++++++++++++++++- 2 files changed, 262 insertions(+), 8 deletions(-) diff --git a/packages/spec/scripts/publish-smoke-port-collision.test.ts b/packages/spec/scripts/publish-smoke-port-collision.test.ts index f2b9d21a25..a09c0ae549 100644 --- a/packages/spec/scripts/publish-smoke-port-collision.test.ts +++ b/packages/spec/scripts/publish-smoke-port-collision.test.ts @@ -53,6 +53,25 @@ // server. Without those a green "retargeted" could mean nothing was listening // and nothing was turned down. // +// ## What the "skips one that is already held" test can and cannot see (#10212) +// +// The sibling collision test lost a round to a vacuity hole: its `PICKED_WITH_BUSY` +// assertion stayed green on the broken picker because its occupier could lose its +// own bind and die, making the next pick return the occupied number legitimately. +// That hole is NOT present here — `HOLDER_REACHABLE` curls the occupier and is the +// precondition guard the sibling was missing, and it was re-measured against the +// pre-fix script rather than assumed. +// +// The blind spot here is a different one, and worth naming because a green run is +// what hid it: that test draws its two ports SEQUENTIALLY, and the TOCTOU defect +// is about CONCURRENT callers. It is sound but silent on the defect — measured +// green on the unfixed picker, exactly as designed, because a second pick taken +// after the first port is already held really does skip it. Detecting a +// check-then-use race needs callers that overlap in time, which is what +// `CONCURRENT_DISTINCT` below adds. `TRIPLE_DISTINCT` covers the same defect from +// the cheap direction: before the fix, two picks in one run with nothing bound in +// between both returned the base. +// // No `objectstack dev` boot and no scaffold: the contract under test belongs to // the shell script, 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 @@ -149,6 +168,9 @@ describe.skipIf(!RUNNABLE)('[#9647] publish-smoke.sh smoke-tests its OWN dev ser expect(r.SCAFFOLDED).toBe('no'); }); + // Sequential by construction, and therefore blind to the TOCTOU race by + // construction — see the header. Kept because "does it skip a busy port" is a + // real property worth pinning; the concurrent cases below are what pin #10212. it('picks a per-run port and skips one that is already held', () => { const r = runHarness([ 'FIRST="$(smoke_pick_free_port 3210)"', @@ -237,4 +259,76 @@ describe.skipIf(!RUNNABLE)('[#9647] publish-smoke.sh smoke-tests its OWN dev ser expect(r.WAIT_RC).not.toBe('0'); expect(r.WAIT_SAID_STATE_FILE).toBe('1'); }); + + // ── #10212: the port is RESERVED, not merely probed ────────────────────── + // + // These deliberately do NOT override SMOKE_PORT_RESERVATION_DIR: sharing one + // host registry is the fix, so the cases that measure contention have to run + // against the registry real callers use. Only the protocol test below, which + // needs to plant a specific registry state, points somewhere scratch. + + it('hands concurrent callers scanning one base distinct ports', () => { + const r = runHarness([ + // Eight at once from a single base. Before the fix every one of them was + // handed 3210 — not usually, but by construction, because the scan is + // deterministic and the probe socket was closed before the port was + // reported free. + 'for i in 1 2 3 4 5 6 7 8; do ( smoke_pick_free_port 3210 > "$SMOKE_ROOT/pick.$i" ) & done', + 'wait', + // The picker prints with NO trailing newline, so each file is read on its + // own; `cat`-ing them together concatenates eight numbers into one line + // and any distinct-count taken from that reads 1 whatever happened. + 'for f in "$SMOKE_ROOT"/pick.*; do printf "%s\\n" "$(cat "$f")"; done > "$SMOKE_ROOT/picks.txt"', + 'echo "CONCURRENT_TOTAL=$(grep -c "[0-9]" "$SMOKE_ROOT/picks.txt")"', + 'echo "CONCURRENT_DISTINCT=$(sort -u "$SMOKE_ROOT/picks.txt" | grep -c "[0-9]")"', + ]); + // Vacuity guard: all eight callers actually produced a port. Without this a + // picker that failed outright would score a perfect distinct-count of 0. + expect(r.CONCURRENT_TOTAL).toBe('8'); + expect(r.CONCURRENT_DISTINCT).toBe('8'); + }); + + it('gives each pick within one run its own port', () => { + const r = runHarness([ + // Nothing binds anything here: the only thing that can keep these apart is + // the claim outliving the call. Before the fix all three returned the base. + 'A="$(smoke_pick_free_port 3210)"', + 'B="$(smoke_pick_free_port 3210)"', + 'C="$(smoke_pick_free_port 3210)"', + 'echo "TRIPLE=$A,$B,$C"', + 'echo "TRIPLE_DISTINCT=$(printf "%s\\n" "$A" "$B" "$C" | sort -u | grep -c "[0-9]")"', + ]); + expect(r.TRIPLE).toMatch(/^\d+,\d+,\d+$/); + expect(r.TRIPLE_DISTINCT).toBe('3'); + }); + + it('skips a port held from outside the registry and hands the claim back', () => { + const r = runHarness([ + 'export SMOKE_PORT_RESERVATION_DIR="$SMOKE_ROOT/reg"', + // Take a port, then forget the claim — now the port is held by something + // the registry knows nothing about, which is the one case a claim cannot + // cover and the probe still has to. + 'STEAL="$(smoke_pick_free_port 3210)"', + 'rm -f "$SMOKE_PORT_RESERVATION_DIR/$STEAL"', + 'STUB_PORT="$STEAL" STUB_NAME=THIEF node -e "$STUB" >/dev/null 2>&1 & THIEF=$!', + 'sleep 1', + 'echo "STEAL_BASE=$STEAL"', + 'echo "STEAL_HELD=$(curl -sS "http://localhost:$STEAL/" | jq -r .iam)"', + 'PICK="$(smoke_pick_free_port $STEAL)"', + 'echo "STEAL_PICK=$PICK"', + 'echo "STEAL_CLAIM_RELEASED=$([ -e "$SMOKE_PORT_RESERVATION_DIR/$STEAL" ] && echo no || echo yes)"', + 'echo "STEAL_CLAIM_ON_PICK=$([ -e "$SMOKE_PORT_RESERVATION_DIR/$PICK" ] && echo yes || echo no)"', + 'kill "$THIEF" 2>/dev/null', + ]); + // Vacuity guard: the thief really held the port. + expect(r.STEAL_HELD).toBe('THIEF'); + expect(r.STEAL_PICK).not.toBe(r.STEAL_BASE); + // POSITIVE CONTROL, and it is not optional. "No claim file for the stolen + // port" is also what a registry that does not exist looks like, so + // STEAL_CLAIM_RELEASED is vacuously green on the unfixed script — measured, + // not assumed. STEAL_CLAIM_ON_PICK is what fails there, and it is what makes + // the release assertion mean anything. + expect(r.STEAL_CLAIM_ON_PICK).toBe('yes'); + expect(r.STEAL_CLAIM_RELEASED).toBe('yes'); + }); }); diff --git a/scripts/publish-smoke.sh b/scripts/publish-smoke.sh index a071b895c5..0897beb0d5 100644 --- a/scripts/publish-smoke.sh +++ b/scripts/publish-smoke.sh @@ -175,22 +175,173 @@ cleanup() { # app-specific assertion available is the authenticated CRUD probe in section 3, # which lands after the auth probes have already run against the wrong app. -# Print a TCP port that is free right now, searching upward from $1. +# Print a TCP port that is free right now AND reserved for this caller, +# searching upward from $1. # -# ADVISORY ONLY. It reserves nothing, and `objectstack dev` binds a moment later, -# so a concurrent run can still take the port in between. Closing that race is -# not this helper's job — smoke_wait_for_own_server turns losing it into a -# warning and a correctly retargeted BASE_URL instead of a silent wrong answer. -# This only has to make losing it rare. +# 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 `objectstack dev`'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, colliding by +# construction rather than by bad luck. Measured on this tree with the +# reservation removed, eight concurrent callers scanning from 3210: +# +# DISTINCT_PORTS=1 of 8 # every one of them was handed 3210 +# BIND_OK=1 BIND_ERR=7 # seven lost the follow-up bind: +# # Error: listen EADDRINUSE :::3210 +# +# The identical shape in scripts/gen-sdui-manifest.sh dequeued a PR from the +# merge queue (#10167, fixed in #10217 — this is the port of that fix). The +# contention here is not hypothetical either: the collision test beside this +# script draws from 3210 four times and the real path below draws once more, in +# a container several agents share. +# +# 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, and that half is deliberately unchanged: nothing +# here stops a process outside the registry from taking the port between this +# function and `objectstack dev`'s bind. smoke_wait_for_own_server is what turns +# losing that race into a warning and a correctly retargeted BASE_URL instead of +# a silent wrong answer — see the collision-safety block above, which is why +# this script never needed the loud-failure half the sdui fix relies on. What +# the claim removes is the collision this script's own callers cause each other, +# which is every collision anyone has actually measured here. # # Probes the wildcard address with no host argument, the same spelling # serve.ts's own isPortAvailable() uses, so this sees a busy port exactly when -# the CLI would. +# the CLI would. (The sibling helper probes 127.0.0.1 because vite binds +# loopback; that difference is load-bearing in both directions and is why these +# two functions are ported rather than merged — see the registry note below.) smoke_pick_free_port() { - node - "${1:-3210}" "${2:-200}" <<'SMOKE_PICK_FREE_PORT' + local base="${1:-3210}" span="${2:-200}" + local dir="${SMOKE_PORT_RESERVATION_DIR:-${TMPDIR:-/tmp}/objectstack-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 script runs on developer machines too. + # + # The descriptor is opened by a subshell that exits before the dev server is + # spawned, so nothing this script starts can inherit it. That matters more here + # than it looks: `objectstack dev` leaves a `serve` child that outlives its + # parent (see kill_tree above), and an inherited lock fd would keep the + # registry locked by that orphan for the whole run. + if command -v flock >/dev/null 2>&1; then + ( + flock -w 30 9 2>/dev/null || true + smoke_scan_and_reserve_port "$base" "$span" + ) 9>"${dir}.lock" + else + smoke_scan_and_reserve_port "$base" "$span" + fi +} + +# The scan itself: claim, then probe, then hand the port over. Split out from +# `smoke_pick_free_port` only so the lock above wraps one named thing. +smoke_scan_and_reserve_port() { + local base="$1" span="$2" + node - "$base" "$span" "$$" <<'SMOKE_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 (it spawns `objectstack dev`), and its death is what makes the claim +// collectable. `$$` survives the command substitution this helper is called in, +// so it names the shell running the smoke, not a transient subshell. +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. +// +// The name is deliberately NOT script-specific. scripts/gen-sdui-manifest.sh +// runs the same protocol against its own directory today; the two draw from +// disjoint bases (3210 here, 5180 there), so they cannot collide with each +// other and nothing is lost by that split right now. Converging both onto this +// neutral default — and then onto one shared helper — is the follow-up, and it +// is a pure rename on that side because the on-disk format is identical: +// filename is the port, contents are the owner pid. +const dir = + process.env.SMOKE_PORT_RESERVATION_DIR || + path.join(process.env.TMPDIR || os.tmpdir(), 'objectstack-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 */ + } +}; + +// No host argument: the wildcard bind is the spelling serve.ts's own +// isPortAvailable() uses, so a port this rejects is a port the CLI would too. const isFree = (port) => new Promise((resolve) => { const probe = net.createServer(); @@ -198,12 +349,21 @@ const isFree = (port) => probe.once('listening', () => probe.close(() => resolve(true))); probe.listen(port); }); + (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;