fix(devx): reserve the port smoke_pick_free_port hands out, instead of probing and letting go

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');
+ });
 });

scripts/publish-smoke.sh

@@ -175,35 +175,195 @@ 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();
  probe.once('error', () => resolve(false));
  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;