diff --git a/packages/spec/scripts/gen-sdui-manifest-collision.test.ts b/packages/spec/scripts/gen-sdui-manifest-collision.test.ts new file mode 100644 index 0000000000..615b8a09bf --- /dev/null +++ b/packages/spec/scripts/gen-sdui-manifest-collision.test.ts @@ -0,0 +1,212 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Pins the CONCURRENT-RUN contract of `scripts/gen-sdui-manifest.sh` — the half +// that decides whether the manifest this script writes describes THIS tree. +// +// ## What was measured, and why the obvious fix is only half of one +// +// Agent dispatch containers run several agents against one filesystem and one +// network namespace. The script used to name a fixed port (5180) and a fixed log +// path, so two overlapping runs shared both. Measured against the pinned vite +// (8.2.1) with the requested port already held: +// +// $ vite dev --port 5390 +// Port 5390 is in use, trying another one... +// ➜ Local: http://localhost:5391/ +// +// vite AUTO-INCREMENTS. So run B's server came up on 5391 while run B's wait +// loop and BASE_URL still named 5390 — run B curled run A's server, got 200, +// and dumped A's manifest as its own with exit 0. The output is a real manifest +// and nothing in the run says which tree it describes, which is what makes it +// worse than no manifest: it feeds the ADR-0082 declaration-parity ratchet. +// +// `--strictPort` alone does NOT fix that, and this was measured too rather than +// reasoned: with it, run B's vite exits (`Error: Port 5390 is already in use`) +// into run B's OWN log, while run A keeps answering on the port. A probe that +// asks only "does the port answer?" still gets its 200 and still dumps A's tree. +// So the flag is necessary — it makes "the port we asked for" and "the port we +// got" the same port or no port — but the probe has to ALSO require that the +// server answering is the one this run spawned. Both halves, or neither works. +// +// ## Why these are executed assertions and not greps +// +// A grep for `--strictPort` passes against a file that only mentions the flag in +// a comment, and a grep for "liveness" passes against a check that runs in the +// wrong order. So the flag is asserted through `sdui_dev_server_cmd`, the +// function the script itself builds its argv from, and the refusal is asserted +// by standing up a real neighbouring server on the port and watching the real +// wait function turn it down. +// +// The vacuity guards matter as much as the assertions. `NEIGHBOUR_BODY` proves +// the neighbour was genuinely reachable at the same `http://localhost:/` +// spelling the script probes, and `OUR_LEADER_ALIVE` proves our stand-in server +// was genuinely gone. Without those two lines a green "refused" could mean +// nothing was listening and nothing was checked — the phantom-assertion failure +// the cleanup test's header records paying for once already. +// +// 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. + +import { describe, it, expect } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const HERE = path.dirname(fileURLToPath(import.meta.url)); +const SCRIPT = path.resolve(HERE, '..', '..', '..', 'scripts', 'gen-sdui-manifest.sh'); + +function have(bin: string): boolean { + try { + execFileSync('sh', ['-c', `command -v ${bin}`], { stdio: 'ignore' }); + return true; + } catch { + return false; + } +} + +// Linux-only by construction, like the cleanup test beside it: the failure being +// pinned is an agent-container one, and the helpers read process sessions. +const RUNNABLE = + process.platform === 'linux' && ['setsid', 'pgrep', 'curl', 'node'].every(have); + +/** A tiny HTTP server on $SDUI_TEST_PORT, as a `node -e` program. */ +const HTTP_STUB = [ + 'const http = require("node:http");', + 'http.createServer((_q, r) => { r.writeHead(200); r.end("SERVER"); })', + ' .listen(Number(process.env.SDUI_TEST_PORT), "127.0.0.1");', +].join(''); + +function runHarness(): Record { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'sdui-collision-')); + const harness = path.join(dir, 'harness.sh'); + + fs.writeFileSync( + harness, + [ + '#!/usr/bin/env bash', + // Not `set -e`: several steps below are expected to fail, and their exit + // codes are the measurement. + 'set -uo pipefail', + // Sourcing runs no generation — the script returns right after defining + // its helpers, so this exercises the REAL functions. + `source ${JSON.stringify(SCRIPT)}`, + `DIR=${JSON.stringify(dir)}`, + 'NODE_BIN="$(command -v node)"', + '', + '# ── 1. the argv the script actually spawns ──────────────────────────', + 'readarray -t ARGV < <(sdui_dev_server_cmd 4321)', + '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 'require("node:net").createServer().listen(Number(process.env.SDUI_TEST_PORT), "127.0.0.1")' &`, + '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', + ' "$NODE_BIN" -e \'const n=require("node:net");const s=n.createServer();s.once("error",()=>process.exit(0));s.once("listening",()=>s.close(()=>process.exit(1)));s.listen(Number(process.env.SDUI_TEST_PORT),"127.0.0.1")\' && break', + ' sleep 0.25', + 'done', + 'printf "BUSY_PORT=%s\\n" "$BUSY"', + 'printf "PICKED_WITH_BUSY=%s\\n" "$(sdui_pick_free_port "$BUSY")"', + 'kill -KILL "$BUSY_PID" 2>/dev/null', + '', + '# ── 3. a NEIGHBOUR answering on our port is refused, not accepted ───', + 'NPORT="$(sdui_pick_free_port 5180)"', + 'export SDUI_TEST_PORT="$NPORT"', + `"$NODE_BIN" -e ${JSON.stringify(HTTP_STUB)} &`, + 'NEIGHBOUR_PID=$!', + 'disown "$NEIGHBOUR_PID" 2>/dev/null || true', + 'for _ in $(seq 1 40); do curl -sf "http://localhost:$NPORT/" > /dev/null 2>&1 && break; sleep 0.25; done', + // Vacuity guard: the neighbour must really be answering, at the exact + // spelling the script probes, or "refused" below proves nothing. + 'printf "NEIGHBOUR_BODY=%s\\n" "$(curl -sf "http://localhost:$NPORT/" || echo NONE)"', + // Stands in for `vite --strictPort` losing the race: our own server exits + // immediately, leaving the neighbour holding the port. + 'PF="$DIR/lost.pid"', + 'sdui_spawn_detached "$PF" "$DIR/lost.log" "$NODE_BIN" -e "process.exit(1)"', + 'sleep 1', + // Vacuity guard: ours must really be gone before the refusal means anything. + 'printf "OUR_LEADER_ALIVE=%s\\n" "$([ -n "$(sdui_live_pids "$(cat "$PF")")" ] && echo yes || echo no)"', + 'START=$SECONDS', + 'if sdui_wait_for_own_server "$PF" "$NPORT" 20; then', + ' printf "ACCEPTED_NEIGHBOUR=yes\\n"', + 'else', + ' printf "ACCEPTED_NEIGHBOUR=no\\n"', + 'fi', + 'printf "REFUSAL_SECONDS=%s\\n" "$((SECONDS - START))"', + 'kill -KILL "$NEIGHBOUR_PID" 2>/dev/null', + '', + '# ── 4. our OWN server is accepted (the check is not just "always no") ─', + 'OPORT="$(sdui_pick_free_port 5180)"', + 'export SDUI_TEST_PORT="$OPORT"', + 'PF2="$DIR/own.pid"', + `sdui_spawn_detached "$PF2" "$DIR/own.log" "$NODE_BIN" -e ${JSON.stringify(HTTP_STUB)}`, + 'if sdui_wait_for_own_server "$PF2" "$OPORT" 20; then', + ' printf "ACCEPTED_OWN=yes\\n"', + 'else', + ' printf "ACCEPTED_OWN=no\\n"', + 'fi', + 'sdui_stop_detached "$PF2"', + '', + '# ── 5. no fixed per-run state left in the file ──────────────────────', + // A spelling pin, and labelled as one: the behaviour above is what the + // suite really asserts. This exists so a fixed path cannot be reintroduced + // quietly, since nothing else in the run would notice until two agents + // overlapped again. + `printf "FIXED_LOG_LITERALS=%s\\n" "$(grep -c '/tmp/sdui-dump-dev\\.log' ${JSON.stringify(SCRIPT)} || true)"`, + ].join('\n'), + { mode: 0o755 }, + ); + + const out = execFileSync('bash', [harness], { encoding: 'utf8', timeout: 120_000 }); + const parsed: Record = {}; + for (const line of out.split('\n')) { + const m = /^([A-Z_]+)=(.*)$/.exec(line.trim()); + if (m) parsed[m[1]] = m[2]; + } + fs.rmSync(dir, { recursive: true, force: true }); + return parsed; +} + +describe.skipIf(!RUNNABLE)('gen-sdui-manifest.sh concurrent-run contract', () => { + const seen = RUNNABLE ? runHarness() : ({} as Record); + + it('asks vite for a port it must actually get, or none at all', () => { + // Read off the function the script builds its real argv from, so this cannot + // pass against a file that merely mentions the flag. + expect(seen.DEV_ARGV).toContain('--strictPort'); + expect(seen.DEV_ARGV).toContain('--port 4321'); + }); + + it('picks a per-run port, skipping one that is already taken', () => { + expect(seen.BUSY_PORT).toMatch(/^\d+$/); + expect(seen.PICKED_WITH_BUSY).toMatch(/^\d+$/); + expect(seen.PICKED_WITH_BUSY).not.toBe(seen.BUSY_PORT); + }); + + it("refuses a neighbouring run's server answering on this run's port", () => { + // Both guards first: without them a green here can mean "nothing was + // listening and nothing was checked". + expect(seen.NEIGHBOUR_BODY).toBe('SERVER'); + expect(seen.OUR_LEADER_ALIVE).toBe('no'); + + expect(seen.ACCEPTED_NEIGHBOUR).toBe('no'); + // And it says so at once rather than after the full 90s wait, because the + // answer never depended on waiting longer. + expect(Number(seen.REFUSAL_SECONDS)).toBeLessThan(10); + }); + + it('accepts the server this run started', () => { + expect(seen.ACCEPTED_OWN).toBe('yes'); + }); + + it('keeps no fixed dev-server log path (spelling pin)', () => { + expect(seen.FIXED_LOG_LITERALS).toBe('0'); + }); +}); diff --git a/scripts/gen-sdui-manifest.sh b/scripts/gen-sdui-manifest.sh index b14caa1984..00fc70ce09 100755 --- a/scripts/gen-sdui-manifest.sh +++ b/scripts/gen-sdui-manifest.sh @@ -151,6 +151,124 @@ sdui_stop_detached() { return 0 } +# --------------------------------------------------------------------------- +# Collision safety between CONCURRENT RUNS. +# +# Agent dispatch containers run several agents against one filesystem and one +# network namespace, so a fixed port and a fixed log path are shared mutable +# state. Measured on the pinned vite (8.2.1, objectui's lockfile at the SHA in +# .objectui-sha) with another server already holding the requested port: +# +# $ vite dev --port 5390 +# Port 5390 is in use, trying another one... +# ➜ Local: http://localhost:5391/ +# +# So run B's server comes up on 5391 while run B's probe and BASE_URL still name +# 5390: run B curls run A's server, succeeds, and dumps A's manifest as its own +# with exit 0 and no diagnostic. That output feeds the ADR-0082 declaration +# parity ratchet below, which is why a wrong-but-plausible manifest is worse +# here than no manifest at all. +# +# `--strictPort` is necessary but NOT sufficient, and that was measured too. With +# it, run B's vite exits — `Error: Port 5390 is already in use` — into run B's OWN +# log, while run A keeps answering on 5390. A probe that asks only "does the port +# answer?" still gets 200, and still dumps A's tree. Both halves are required: +# +# 1. `--strictPort`, so the port we ASKED for is the port we GOT, or no server +# of ours exists at all. Without it `--port` is a request vite may silently +# decline, and "where I asked the server to be" stops meaning "where it is". +# 2. a probe that requires the session THIS RUN spawned to still be alive +# before it accepts an answer on that port. Given (1), "our leader is alive" +# leaves no third possibility: what answers on $DUMP_PORT is ours. +# +# That pair is what makes $DUMP_PORT single-valued in the sense that matters. +# Deriving the probe URL and BASE_URL from one variable was ALREADY true when +# this defect was filed and did not prevent it — the divergence was never between +# two literals in this file, it was between the port requested and the port bound. +# --------------------------------------------------------------------------- + +# Print a TCP port on 127.0.0.1 that is free right now, searching upward from $1. +# +# ADVISORY ONLY. It reserves nothing, and between this probe and vite's own bind +# a concurrent run can take the port. Closing that race is not this helper's job: +# `--strictPort` turns losing it into a loud failure instead of a silent redirect +# onto a neighbour's server. +# +# 127.0.0.1 is the interface vite's dev server binds. A wildcard listener on the +# same port collides with a loopback bind too, so this probe sees a neighbour +# whichever way the neighbour bound. +sdui_pick_free_port() { + local base="${1:-5180}" span="${2:-200}" + node - "$base" "$span" << 'SDUI_PICK_FREE_PORT' +const net = require('node:net'); +const base = Number(process.argv[2]); +const span = Number(process.argv[3]); +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, '127.0.0.1'); + }); +(async () => { + for (let port = base; port < base + span; port += 1) { + if (await isFree(port)) { + process.stdout.write(String(port)); + return; + } + } + process.stderr.write(`no free TCP port in [${base}, ${base + span})\n`); + process.exitCode = 1; +})(); +SDUI_PICK_FREE_PORT +} + +# The dev-server argv for port $1, one word per line. +# +# A function rather than an inline command line so the `--strictPort` half of the +# fix is assertable by SOURCING this file, instead of only by grepping it. A grep +# assertion would also pass against a version that merely mentions the flag in a +# comment, which is the failure mode the cleanup test's header warns about. +sdui_dev_server_cmd() { + local port="$1" + printf '%s\n' pnpm --filter @object-ui/console exec vite dev --port "$port" --strictPort +} + +# Wait until the dev server THIS RUN spawned answers on port $2, or fail. +# $1 pidfile written by sdui_spawn_detached $2 port $3 timeout seconds +# +# The liveness check is ordered BEFORE the curl deliberately: a neighbouring run's +# server on the same port answers 200 exactly like ours, so "the port answers" is +# no evidence by itself. Reading our own leader first — and again after a +# successful curl, since ours can exit between the two — is what makes a green +# here mean "our server". It also turns a collision into a fast, explicit failure +# instead of a 90-second wait ending in a confident wrong answer. +# +# `curl`, not a socket table: `ss`/`netstat` are absent from the agent dispatch +# containers, and `ss -ltn | grep :$PORT` prints nothing there whether or not +# anything is listening (docs/qa/platform-checklist/RUNNER.md). +sdui_wait_for_own_server() { + local pidfile="${1:-}" port="${2:-}" timeout="${3:-90}" leader="" _i + for _i in $(seq 1 "$timeout"); do + leader="$(tr -d '[:space:]' < "$pidfile" 2>/dev/null || true)" + case "$leader" in '' | *[!0-9]*) leader="" ;; esac + if [[ -n "$leader" && -z "$(sdui_live_pids "$leader")" ]]; then + echo "✗ the dev server this run started is no longer running." >&2 + return 1 + fi + if curl -sf "http://localhost:${port}/" > /dev/null 2>&1; then + if [[ -n "$leader" && -z "$(sdui_live_pids "$leader")" ]]; then + echo "✗ the dev server this run started exited while its port was being probed." >&2 + return 1 + fi + return 0 + fi + sleep 1 + done + echo "✗ the dev server this run started never answered on port ${port} within ${timeout}s." >&2 + return 1 +} + sdui_on_signal() { local sig="$1" sdui_stop_detached "${DUMP_PID_FILE:-}" @@ -193,10 +311,35 @@ fi echo "→ Generating SDUI public-tier manifest (ADR-0080) from objectui@${PINNED_SHA:0:12}..." pushd "$BUILD_ROOT" > /dev/null -DUMP_PORT=5180 -DUMP_DEV_LOG="/tmp/sdui-dump-dev.log" +# Per-run port. `SDUI_DUMP_PORT` pins one explicitly and is honoured exactly — +# no search — because an explicit request that quietly lands somewhere else is +# the defect this block exists to remove; `--strictPort` makes a busy pinned port +# fail loudly, which is what an explicit request should do. Unset, the search +# starts at 5180 so an uncontended run still lands where the docs say it does. +if [[ -n "${SDUI_DUMP_PORT:-}" ]]; then + DUMP_PORT="$SDUI_DUMP_PORT" +else + DUMP_PORT="$(sdui_pick_free_port 5180 || true)" +fi +# Never let an empty or non-numeric value through: the dump consumer defaults to +# `http://localhost:5180` when BASE_URL is absent, and that default is precisely +# the shared port this run is trying not to use. +case "$DUMP_PORT" in + '' | *[!0-9]*) + echo "✗ could not find a free TCP port for this run's dev server." >&2 + exit 1 + ;; +esac + +# Per-run log. The fixed path this replaced was truncated by whichever run +# started last, so a diagnosing reader could be reading another run's output — +# and the failure branches below point readers straight at it. mktemp, matching +# the pidfile beside it. +DUMP_DEV_LOG="$(mktemp "${TMPDIR:-/tmp}/sdui-dump-dev.XXXXXX.log")" DUMP_PID_FILE="$(mktemp "${TMPDIR:-/tmp}/sdui-dump-pid.XXXXXX")" +echo " dev server: port ${DUMP_PORT}, log ${DUMP_DEV_LOG}" + # Armed BEFORE the spawn: everything from here on must be able to fail without # leaving a server behind. EXIT alone is not enough — bash runs no EXIT trap # when the script is itself signalled, and being signalled is exactly how an @@ -206,13 +349,19 @@ trap 'sdui_on_signal INT' INT trap 'sdui_on_signal TERM' TERM trap 'sdui_on_signal HUP' HUP -sdui_spawn_detached "$DUMP_PID_FILE" "$DUMP_DEV_LOG" \ - pnpm --filter @object-ui/console exec vite dev --port "$DUMP_PORT" +readarray -t DUMP_DEV_ARGV < <(sdui_dev_server_cmd "$DUMP_PORT") +sdui_spawn_detached "$DUMP_PID_FILE" "$DUMP_DEV_LOG" "${DUMP_DEV_ARGV[@]}" -# `curl`, not a socket table: `ss`/`netstat` are absent from the agent dispatch -# containers, and `ss -ltn | grep :$DUMP_PORT` prints nothing there whether or -# not anything is listening (docs/qa/platform-checklist/RUNNER.md). -for _ in $(seq 1 90); do curl -sf "http://localhost:${DUMP_PORT}/" > /dev/null 2>&1 && break; sleep 1; done +# Failing here is a REFUSAL TO GUESS, not an inconvenience: the alternative is +# dumping whatever else happens to answer on this port. See the collision-safety +# block above for what the two halves buy. +if ! sdui_wait_for_own_server "$DUMP_PID_FILE" "$DUMP_PORT" 90; then + echo " Its log for this run is at ${DUMP_DEV_LOG}" + echo " A port taken by a concurrent run reports 'Port ${DUMP_PORT} is already in use' there;" + echo " re-run, or pin a port with SDUI_DUMP_PORT=." + popd > /dev/null + exit 1 +fi if BASE_URL="http://localhost:${DUMP_PORT}" OUT="${TARGET}/sdui.manifest.json" node scripts/dump-public-manifest.mjs; then echo "✓ wrote ${TARGET}/sdui.manifest.json" @@ -223,6 +372,7 @@ else echo " pnpm exec playwright install chromium-headless-shell" echo " Can't install here (e.g. an agent dispatch container)? See docs/releases-maintenance.md" echo " 'If the dispatch container's Playwright browser doesn't match the revision'." + echo " The dev server log for THIS run is at ${DUMP_DEV_LOG}" popd > /dev/null exit "$status" fi