test(spec): let the sdui collision harness own the port it calls busy

packages/spec/scripts/gen-sdui-manifest-collision.test.ts

@@ -69,6 +69,50 @@
 // BUSY_HELD is asserted before PICKED_WITH_BUSY is believed: a precondition
 // that can evaporate silently is a test that accuses the wrong function.
 //
+// ## The precondition needed an OWNED port, not a picked one — measured
+//
+// That paragraph was still only half of it, and the other half reddened three
+// unrelated PRs in one afternoon, byte-identical every time:
+//
+// BUSY_HELD=no
+// BUSY_PORT=5180
+// PICKED_WITH_BUSY=5181
+//
+// `PICKED_WITH_BUSY=5181` is the picker answering CORRECTLY — asked to avoid
+// 5180 it returned 5181. `BUSY_HELD=no` alone is the failure, and the thief was
+// this file. The occupier used to bind the port `sdui_pick_free_port` had just
+// handed it, and a RESERVATION IS NOT A BIND: the port stays takeable in the gap
+// between the pick returning and the occupier binding. `port_held` below binds
+// and closes that exact port, and it is spawned within milliseconds of the
+// occupier. Measured, 80 trials on an idle container:
+//
+// probe won the bind 39/80 # the two genuinely race
+// occupier died 1/80 # its bind landed inside the probe's hold
+//
+// Nothing outside this file has to hold 5180 for that to fire, which is why no
+// one could name the process that did. So the occupier binds `:0` and reports
+// back the port the kernel gave it: it is already holding that port when it
+// names it, which is the property BUSY_HELD asserts, and no gap is left.
+//
+// ## Why `set +e` follows the `source`, and why the harness ends `exit 0`
+//
+// `scripts/gen-sdui-manifest.sh` is `set -euo pipefail` at its top, so SOURCING
+// it turns errexit back ON here, overriding the `set -uo pipefail` written one
+// line earlier. Measured rather than read: `case "$-" in *e*)` after the source
+// reports `e` set. That is what truncated the three captures above. When the
+// occupier had already exited, the cleanup `kill` returned 1, errexit aborted
+// the harness on that line, `execFileSync` threw in the `describe` body, and
+// vitest reported `0 test` and a bare `Error: Command failed: bash
+// /tmp/sdui-collision-*/harness.sh` — every assertion that would have named the
+// problem pre-empted, the diagnosis surviving only because stdout rides along
+// on the serialized error. Reproduced here: forcing the occupier to lose its
+// bind stopped the harness dead after `PICKED_WITH_BUSY`, exit 1, exactly the
+// captured shape. So errexit is turned off again AFTER the source, and the
+// harness exits 0 unconditionally: its exit status is not a measurement. Every
+// measurement is a printed KEY=VALUE line and the assertions below grade those,
+// so a precondition that fails now fails AS AN ASSERTION, printing the whole
+// `seen` map with it.
+//
 // 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.
@@ -103,24 +147,31 @@ const HTTP_STUB = [
  '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.
+ // problem. See OWNED_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.
+ * A bare TCP listener that binds an EPHEMERAL port and prints the one it got.
+ *
+ * Bind first, name second. Asking the picker for a port and binding it
+ * afterwards leaves a window in which anything at all — including `port_held`
+ * below, measured — can take it, and losing there evaporates the precondition
+ * the BUSY/STEAL assertions rest on. Port 0 closes the window by construction:
+ * the number is written from inside the `listening` callback, so by the time
+ * the harness can read it the socket is already held.
  *
- * 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".
+ * The error guard stays. It is near-unreachable on `:0`, but the unguarded form
+ * of this line is what made the merge-queue failure this file pins so hard to
+ * read: 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 OWNED_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");',
+ 's.listen(0, "127.0.0.1", () => console.log(s.address().port));',
 ].join('');
 
 function runHarness(): Record<string, string> {
@@ -137,6 +188,11 @@ function runHarness(): Record<string, string> {
  // Sourcing runs no generation — the script returns right after defining
  // its helpers, so this exercises the REAL functions.
  `source ${JSON.stringify(SCRIPT)}`,
+ // …but the file just sourced opens with `set -euo pipefail`, so the source
+ // turns errexit back ON here and silently overrides the line above. That
+ // is what turned a failed precondition into a harness crash and a `0 test`
+ // report; see the header. Off again, after the source, where it sticks.
+ 'set +e',
  `DIR=${JSON.stringify(dir)}`,
  'NODE_BIN="$(command -v node)"',
  // "is $1 held right now?" — the same probe shape the script uses, so a
@@ -150,22 +206,42 @@ function runHarness(): Record<string, string> {
  'printf "DEV_ARGV=%s\\n" "${ARGV[*]}"',
  '',
  '# ── 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 ${JSON.stringify(RAW_LISTENER)} &`,
+ // The busy port is the one the occupier BOUND, not one the picker handed
+ // it: a reservation is not a bind, and the gap between the two is what
+ // `port_held` walked through. See the header for the 80-trial count.
+ 'BUSY_FILE="$DIR/busy.port"',
+ `"$NODE_BIN" -e ${JSON.stringify(OWNED_LISTENER)} > "$BUSY_FILE" &`,
  '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 [ "$(port_held "$BUSY")" = yes ] && break; sleep 0.25; done',
+ // The port appears only once the socket is bound, so waiting for the
+ // number IS waiting for the hold — there is no second thing to wait for.
+ 'BUSY=""',
+ 'for _ in $(seq 1 40); do BUSY="$(tr -d "[:space:]" < "$BUSY_FILE" 2>/dev/null || true)"; [ -n "$BUSY" ] && 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.
+ // green/red says nothing about the picker. Nothing can lose the port to a
+ // racer any more; the guard stays because it is what would make the next
+ // way of losing it legible instead of an accusation of the picker.
  'printf "BUSY_HELD=%s\\n" "$(port_held "$BUSY")"',
  'printf "BUSY_PORT=%s\\n" "$BUSY"',
+ // An ephemeral base does not risk scanning off the end of the port space:
+ // the first candidate is the port we hold, the second is free, and the
+ // scan returns there rather than walking its 200-port span upward.
  'printf "PICKED_WITH_BUSY=%s\\n" "$(sdui_pick_free_port "$BUSY")"',
  'kill -KILL "$BUSY_PID" 2>/dev/null',
  '',
+ '# ── 2b. a third sequential pick from the shared base ────────────────',
+ // Case 2 used to contribute one of these as a by-product, back when its
+ // occupier took its port from the picker. It owns an ephemeral port now,
+ // so the third pick is taken explicitly: without it the "each pick within
+ // one run its own port" assertion compares two picked ports against one
+ // ephemeral one, and those differ whatever the registry does — a guard
+ // that would go green for a reason unrelated to what it guards.
+ 'RPORT="$(sdui_pick_free_port 5180)"',
+ 'printf "RPORT=%s\\n" "$RPORT"',
+ '',
  '# ── 3. a NEIGHBOUR answering on our port is refused, not accepted ───',
  'NPORT="$(sdui_pick_free_port 5180)"',
  'printf "NPORT=%s\\n" "$NPORT"',
@@ -232,15 +308,21 @@ function runHarness(): Record<string, string> {
  // 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)} &`,
+ // Bound, then named — the same precondition case 2 needs, and the same
+ // way of guaranteeing it. STEAL_HELD is not a weaker assertion than
+ // BUSY_HELD and had no business resting on a weaker mechanism.
+ 'STEAL_FILE="$DIR/steal.port"',
+ `"$NODE_BIN" -e ${JSON.stringify(OWNED_LISTENER)} > "$STEAL_FILE" &`,
  'STEAL_PID=$!',
  'disown "$STEAL_PID" 2>/dev/null || true',
- 'for _ in $(seq 1 40); do [ "$(port_held "$SPORT")" = yes ] && break; sleep 0.25; done',
+ 'SPORT=""',
+ 'for _ in $(seq 1 40); do SPORT="$(tr -d "[:space:]" < "$STEAL_FILE" 2>/dev/null || true)"; [ -n "$SPORT" ] && 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.
+ // A registry of its own, so the claim files the two lines below read are
+ // written by this leg and nothing else. The holder is foreign to that
+ // registry either way, and more plainly than before: a port bound
+ // directly from the ephemeral range was never claimed in any registry,
+ // which is exactly the non-participant this case exists to cover.
  'RESV="$DIR/resv"',
  'STEAL_PICK="$( (export SDUI_PORT_RESERVATION_DIR="$RESV"; sdui_pick_free_port "$SPORT") )"',
  'printf "STEAL_PORT=%s\\n" "$SPORT"',
@@ -253,6 +335,15 @@ function runHarness(): Record<string, string> {
  // 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',
+ '',
+ // Grading belongs to the assertions, not to whatever the last cleanup
+ // returned. Reaping an occupier that already exited fails, and under the
+ // errexit this file used to inherit that failure ended the harness where
+ // it stood; `execFileSync` then threw in the `describe` body and vitest
+ // reported `0 test` against a bare "Command failed". Exit 0 and let the
+ // KEY=VALUE lines above be the evidence — a red then names its assertion
+ // and prints the whole `seen` map beside it.
+ 'exit 0',
  ].join('\n'),
  { mode: 0o755 },
  );
@@ -311,7 +402,11 @@ describe.skipIf(!RUNNABLE)('gen-sdui-manifest.sh concurrent-run contract', () =>
  });
 
  it('gives each pick within one run its own port', () => {
- const picked = [seen.BUSY_PORT, seen.NPORT, seen.OPORT];
+ // Three ports the PICKER handed out, from one base, in one run. BUSY_PORT
+ // is deliberately not among them any more: the occupier binds an ephemeral
+ // port of its own, and an ephemeral port differs from the 5180 band however
+ // the registry behaves, so counting it here would be free.
+ const picked = [seen.NPORT, seen.OPORT, seen.RPORT];
  expect(picked.every((p) => /^\d+$/.test(p ?? '')), picked.join(',')).toBe(true);
  expect(new Set(picked).size, picked.join(',')).toBe(3);
  });