diff --git a/packages/spec/scripts/gen-sdui-manifest-collision.test.ts b/packages/spec/scripts/gen-sdui-manifest-collision.test.ts index f49cbb06e3..cd5a0f65fa 100644 --- a/packages/spec/scripts/gen-sdui-manifest-collision.test.ts +++ b/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 { @@ -137,6 +188,11 @@ function runHarness(): Record { // 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 { '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 { // 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 { // 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); });