From 9d06e3c9086f4da96ee3cc57c02d18e12445476d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 14:06:15 +0000 Subject: [PATCH] fix(scripts): gen-sdui-manifest picks a per-run port and proves the server it dumps is its own (#9578) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The script named a fixed port (5180) and a fixed log path for per-run state. Agent dispatch containers run several agents against one filesystem and one network namespace, so both were shared between concurrent runs. Measured on the pinned vite (8.2.1) with the requested port already held: vite prints "Port N is in use, trying another one..." and binds N+1. Run B's server came up on the neighbour port while run B's wait loop and BASE_URL still named N, so run B curled run A's server, got 200, and dumped A's manifest as its own with exit 0 — output that then feeds the ADR-0082 declaration-parity ratchet. --strictPort alone does not fix it (measured): run B's vite exits into run B's own log while run A keeps answering, and a probe that only asks "does the port answer?" still accepts it. So both halves ship — the flag, and a probe that requires the session this run spawned to still be alive before it accepts an answer on that port. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja --- .../gen-sdui-manifest-collision.test.ts | 212 ++++++++++++++++++ scripts/gen-sdui-manifest.sh | 166 +++++++++++++- 2 files changed, 370 insertions(+), 8 deletions(-) create mode 100644 packages/spec/scripts/gen-sdui-manifest-collision.test.ts 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