test(spec): let the sdui collision harness OWN the port it calls busy (#10456)

`BUSY_HELD=no` / `BUSY_PORT=5180` / `PICKED_WITH_BUSY=5181` reddened three
unrelated PRs in one afternoon. The picker was right every time; the harness
could not guarantee its own precondition.

The occupier took its port from `sdui_pick_free_port` and bound it afterwards.
A reservation is not a bind, so the port stays takeable in that gap — and
`port_held`, spawned milliseconds later, binds and closes that very port.
Measured over 80 trials on an idle container: the probe won the bind 39 times,
and once the occupier's bind landed inside the probe's hold window and the
occupier exited. No external holder of 5180 is required, which is why none was
ever identified.

Both occupiers (case 2 and case 7) now bind `:0` and report back the port the
kernel gave them, so the port is held before it is named.

Second, independent half: sourcing `gen-sdui-manifest.sh` (`set -euo pipefail`
at its top) turned errexit back ON in the harness, overriding its own
`set -uo pipefail`. The failing cleanup `kill` of a dead occupier then aborted
the harness mid-measurement, `execFileSync` threw in the `describe` body, and
vitest reported `0 test` against a bare "Command failed". `set +e` after the
source and a final `exit 0` make a lost precondition fail as an assertion, with
the whole `seen` map printed beside it.

`expect(seen.BUSY_HELD).toBe('yes')` is untouched, both probes still address
127.0.0.1, and case 7 still covers a holder foreign to the registry. A third
sequential pick (RPORT) replaces BUSY_PORT in the "distinct picks in one run"
set, which would otherwise compare an ephemeral port against the 5180 band and
pass for free.


Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-zhuang

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