diff --git a/packages/cli/test/serve-app-anchored-optional-import.e2e.test.ts b/packages/cli/test/serve-app-anchored-optional-import.e2e.test.ts index 958befb06a..db52a0442a 100644 --- a/packages/cli/test/serve-app-anchored-optional-import.e2e.test.ts +++ b/packages/cli/test/serve-app-anchored-optional-import.e2e.test.ts @@ -38,9 +38,38 @@ * to the app, which is the one shape this file must not use. What IS borrowed * from it is everything orthogonal to the directory the child starts in: * `childEnv()` (#11267) — this file boots the real stack, better-auth included, - * which reads `TEST` directly — plus `randomPort()` and `portContentionError()` - * (#12441), because a port draw is not a property of the CWD either and this - * file used to carry its own second, overlapping one. + * which reads `TEST` directly — plus `randomPort()`, `portContentionError()` + * (#12441) and `portDriftError()` (#12525, wired here by #12548), because a port + * draw is not a property of the CWD either and this file used to carry its own + * second, overlapping one. + * + * ── ⚠️ The port apparatus here is INERT, and that is measured, not assumed ── + * + * #12548 wired the drift read-back into this file as the sibling of + * `serve-process-child-env.e2e.test.ts`. On this tree it is SILENT for every run + * this file makes, and saying so is the point of this paragraph — a reader who + * takes it for a live instrument would be reading a green that was never a + * measurement. + * + * Measured on `52a982388`: the fixture driver below only PRINTS its marker, so + * `serve` walks on to `Cluster driver "redis" is not registered` and the child + * exits 1 — 5.6s in, having never called `listen()`. The undeclared leg dies + * earlier still. So no child spawned by this file ever prints a ready banner, + * `boundPortFromBanner()` answers `no-banner` on all three runs, and + * `portDriftError()` returns `null`. + * + * ⇒ this file's exposure is not merely LOWER than the security file's, it is + * nil in both directions: it issues no HTTP request, and its children never bind + * a port there is anything to drift off. The same measurement retires + * `portContentionError()` above to insurance — these children spawn through + * `bin/run-dev.js`, so `serve`'s auto-shift branch is open and a taken port + * never produces the bind failure that helper reads for. + * + * ⛔ Do not "make the read-back live" by gating resolution on the banner the way + * `serve-process-child-env.e2e.test.ts` does: no child here prints one and the + * gate would hang out its timeout. Both calls stay as they are because the day + * this fixture registers a real driver and boots to a banner, a drift must not + * be silent — not because either has been observed firing. * * ── The anti-vacuity floor ─────────────────────────────────────────────── * @@ -59,7 +88,7 @@ import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'nod import { tmpdir } from 'node:os'; import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { childEnv, portContentionError, randomPort } from './helpers/serve-process.js'; +import { childEnv, portContentionError, portDriftError, randomPort } from './helpers/serve-process.js'; const HERE = dirname(fileURLToPath(import.meta.url)); @@ -80,6 +109,9 @@ export default { }; `; +/** How either refusal names this file's child — it spawns directly, so no helper can. */ +const WHAT = 'os serve (bin/run-dev.js \u21d2 NODE_ENV=development)'; + /** The gate package `serve` loads first when OS_CLUSTER_DRIVER is set. */ const CLUSTER = '@objectstack/service-cluster'; /** The driver it loads next, named from the env var. */ @@ -152,12 +184,16 @@ interface Run { stdout: string; stderr: string; both: string } * An early exit resolves rather than rejects: a boot that DIES still has to have * said why, and the refusal case below reads exactly that. * - * ⭐ ONE exception (#12441): a boot that died because it could not BIND rejects, - * naming the port. Resolving it would hand the caller an output buffer with no - * marker in it, and the assertions would then fail with "the cluster gate was - * not loaded from the app" — a sentence about resolution bases, for a failure - * that is entirely about a port. That mis-signalling is the whole cost the card - * measured, and it is more expensive than the lost run. + * ⭐ TWO exceptions, both about the port and both about the same mis-signalling. + * A boot that DIED because it could not BIND rejects, naming the port (#12441): + * resolving it would hand the caller an output buffer with no marker in it, and + * the assertions would then fail with "the cluster gate was not loaded from the + * app" — a sentence about resolution bases, for a failure that is entirely about + * a port. That is the whole cost the card measured, and it is more expensive + * than the lost run. A boot that SUCCEEDED on a port other than the one it was + * asked for rejects too (#12525, wired by #12548) — ⚠️ silent on this tree for + * the measured reason in this file's header, and kept for the tree where it is + * not. */ function runServeFrom( cwd: string, @@ -195,15 +231,19 @@ function runServeFrom( settled = true; clearTimeout(timer); try { child.kill('SIGTERM'); } catch { /* already gone */ } - const contended = portContentionError( - stdout + stderr, - 'os serve (bin/run-dev.js ⇒ NODE_ENV=development)', - port, - ); + const contended = portContentionError(stdout + stderr, WHAT, port); if (contended) { rejectRun(contended); return; } + // ⭐ The read-back half (#12548). ⚠️ Structurally silent here — see this + // file's header for the measurement, and ⛔ do not read a green run as + // evidence that it looked. + const drifted = portDriftError(stdout + stderr, WHAT, port); + if (drifted) { + rejectRun(drifted); + return; + } resolveRun({ stdout, stderr, both: stdout + stderr }); }; const timer = setTimeout(finish, timeoutMs); diff --git a/packages/cli/test/serve-process-child-env.e2e.test.ts b/packages/cli/test/serve-process-child-env.e2e.test.ts index f874146432..c0fe962b68 100644 --- a/packages/cli/test/serve-process-child-env.e2e.test.ts +++ b/packages/cli/test/serve-process-child-env.e2e.test.ts @@ -67,8 +67,42 @@ * reading `TEST`, that leg goes red, and the answer is to re-read this header * and re-measure, not to silence it. * - * Cost: two real `os serve` boots, ~18s together when measured on this - * container. Both children are killed in `afterAll` regardless of outcome. + * ## ⚠️ The port this file talks to is READ BACK, never assumed (#12548) + * + * The probe below is an HTTP request to `localhost:`, and what it asserts + * on the answer is a SECURITY posture. Until #12548 that request went to the + * port this file ASKED for. Children here spawn through `bin/run-dev.js`, which + * pins `NODE_ENV=development` before argv is parsed, so `serve.ts`'s + * `portAutoShiftAllowed` is TRUE for every one of them: a taken port is not an + * error there, it is a hop to the next free one, and the boot then SUCCEEDS. + * The harness got a healthy child, a green run, and a request addressed to + * whatever still held the port it asked for. On the shared container this fleet + * develops in that is plausibly a neighbouring agent's dev server — measured + * answering another harness's own request with + * `{"iAm":"A NEIGHBOURING AGENT DEV SERVER, not os serve"}`. + * + * ⇒ a foreign process could supply the answer to the one assertion in this + * directory where being wrong matters most. So `readyVerdict()` below settles + * only once the child has SAID which port it took, and `portDriftError()` + * (`helpers/serve-process.ts`, #12525) REFUSES a mismatch rather than probing. + * + * ⛔ The gate keys on the banner's TAIL, never on `Server is ready`. That line + * is the banner's HEAD, printed one `console.error` before the `API:` row that + * carries the port — so a head-keyed gate can settle while the port is still + * unknown, and the comparison then compares nothing. Whether it did would + * depend on how the pipe happened to chunk, which is the worst shape a check + * can have: blind at random, and its silence reads as a pass. + * + * ⛔ And this is not an argument to make `os serve` stricter. Auto-shifting in + * development is correct and deliberate (#11113 pins the production half for + * its own reasons). The defect was that this harness could not tell. + * + * Cost: THREE real `os serve` boots — the two legs of the comparison above + * (~18s together when measured on this container), plus the forced-drift arm + * that proves the refusal can actually fire. That third boot is bought + * deliberately: a read-back never observed failing is decoration, and on this + * file it would be decoration over a security assertion. Every child is killed + * in `afterAll` regardless of outcome. */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; @@ -77,7 +111,17 @@ import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import type { Readable } from 'node:stream'; -import { CLI, TSX, E2E_SECRET_KEY, childEnv, randomPort, VITEST_WORKER_ENV_KEYS } from './helpers/serve-process.js'; +import { + CLI, + TSX, + E2E_SECRET_KEY, + boundPortFromBanner, + childEnv, + holdPort, + portDriftError, + randomPort, + VITEST_WORKER_ENV_KEYS, +} from './helpers/serve-process.js'; /** What `spawn(…, { stdio: ['ignore', 'pipe', 'pipe'] })` actually returns — no `stdin`. */ type ProbeChild = ChildProcessByStdio; @@ -149,15 +193,77 @@ async function stop(child: ProbeChild): Promise { }); } +/** How a refusal names this file's child — it spawns directly, so no helper can. */ +const WHAT = 'os serve (bin/run-dev.js \u21d2 NODE_ENV=development, spawned directly by this file)'; + +/** What this file's ready gate makes of the child's output SO FAR. */ +type ReadyVerdict = + /** The child has not announced a bound port yet — keep waiting. */ + | { settled: false } + /** It announced the port it was asked for, and this is that number. */ + | { settled: true; port: number } + /** It announced a DIFFERENT port, or a banner that cannot say which. */ + | { settled: true; refusal: Error }; + /** - * Boot the real `os serve` through the same entrypoint `runServe()` uses, probe - * the origin check while it is UP, then stop it. `runServe()` itself cannot - * stand in here: it kills the child the moment `waitFor` matches, so there is - * no window in which to send a request. + * ⭐ The ready gate (#12548): settle on having SEEN the port, never on a ready + * marker. + * + * `boundPortFromBanner()` keys on the banner's LAST line, so `no-banner` means + * "the child has not finished saying which port it took" — and returning + * `{ settled: false }` for it is what makes the comparison below have something + * to compare. ⛔ Do not settle this on `Server is ready`: that is the banner's + * HEAD, one `console.error` ahead of the `API:` row, and a gate keyed there + * resolves with the port still unknown. `portDriftError()` would then answer + * `null` — a silent pass — and the whole check would be decorative. + * + * Every other state is a settled verdict, and the two failing ones are ERRORS + * rather than skips for the reason `portDriftError()`'s own docblock gives: an + * instrument that cannot answer has to say it could not answer. */ -async function probeOrigin(env: Record): Promise<{ status: number; code: unknown }> { - const port = randomPort(); - const child = spawn(TSX, [CLI, 'serve', 'objectstack.config.ts', '--port', port], { +function readyVerdict(output: string, requestedPort: string): ReadyVerdict { + const readback = boundPortFromBanner(output); + if (readback.state === 'no-banner') return { settled: false }; + + const refusal = portDriftError(output, WHAT, requestedPort); + if (refusal) return { settled: true, refusal }; + + if (readback.state !== 'bound') { + // Unreachable while `portDriftError()` answers for every state but + // `no-banner`. Written as a refusal rather than assumed away, so that + // widening its silence some day fails loudly HERE instead of quietly + // handing this file a port no one verified. + return { + settled: true, + refusal: new Error( + `portDriftError() stayed silent on a ${readback.state} banner, so this harness has no ` + + `verified port to probe.\n--- child output ---\n${output}`, + ), + }; + } + + return { settled: true, port: readback.port }; +} + +/** + * Boot the real `os serve` through the same entrypoint `runServe()` uses, read + * the port the child ACTUALLY bound back out of its own banner, probe the + * origin check on THAT port while it is UP, then stop it. + * + * `runServe()` cannot stand in here, twice over: it kills the child the moment + * `waitFor` matches, so there is no window in which to send a request — and + * because this file therefore spawns directly, #12525's read-back inside + * `runServe()` never reaches it. That is why the gate is written out here. + * + * `requestedPort` is a parameter rather than a local so the forced-drift arm + * below can hand it a port that is genuinely HELD. ⛔ Nothing else should pass + * one: the default draw is bind-probed, and a hand-picked number is not. + */ +async function probeOrigin( + env: Record, + requestedPort: string = randomPort(), +): Promise<{ status: number; code: unknown }> { + const child = spawn(TSX, [CLI, 'serve', 'objectstack.config.ts', '--port', requestedPort], { cwd: dir, stdio: ['ignore', 'pipe', 'pipe'], env, @@ -166,27 +272,38 @@ async function probeOrigin(env: Record): Promise<{ s let out = ''; let err = ''; - await new Promise((ready, fail) => { - const timer = setTimeout( - () => fail(new Error(`serve never became ready\n--- stdout ---\n${out}\n--- stderr ---\n${err}`)), - 150_000, - ); - const onData = () => { - if (/Press Ctrl\+C to stop|Server is ready/.test(out + err)) { + try { + const bound = await new Promise((ready, fail) => { + const timer = setTimeout( + () => fail(new Error( + 'serve never printed a COMPLETE ready banner, so it never said which port it bound' + + `\n--- stdout ---\n${out}\n--- stderr ---\n${err}`, + )), + 150_000, + ); + const onData = () => { + const verdict = readyVerdict(out + err, requestedPort); + if (!verdict.settled) return; clearTimeout(timer); - ready(); - } - }; - child.stdout.on('data', (d) => { out += String(d); onData(); }); - child.stderr.on('data', (d) => { err += String(d); onData(); }); - child.on('exit', (code) => { - clearTimeout(timer); - fail(new Error(`serve exited ${code} before it was ready\n--- stdout ---\n${out}\n--- stderr ---\n${err}`)); + if ('refusal' in verdict) fail(verdict.refusal); + else ready(verdict.port); + }; + child.stdout.on('data', (d) => { out += String(d); onData(); }); + child.stderr.on('data', (d) => { err += String(d); onData(); }); + child.on('exit', (code) => { + clearTimeout(timer); + fail(new Error( + `serve exited ${code} before it announced a bound port` + + `\n--- stdout ---\n${out}\n--- stderr ---\n${err}`, + )); + }); }); - }); - try { - const res = await fetch(`http://localhost:${port}/api/v1/auth/sign-in/email`, { + // ⭐ `bound`, not `requestedPort`. They are provably the same number on this + // line — a mismatch was REFUSED above rather than reaching it — and + // addressing the child's own announced port is what keeps that true if the + // gate is ever loosened. + const res = await fetch(`http://localhost:${bound}/api/v1/auth/sign-in/email`, { method: 'POST', headers: { 'content-type': 'application/json', @@ -229,6 +346,16 @@ describe('#11267: childEnv() keeps the vitest worker out of a spawned os serve', expect(env.OS_LOG_LEVEL).toBe('warn'); }); + it('the ready gate does not settle until the child has SAID which port it took', () => { + // ⭐ #12548. The banner's HEAD alone is exactly what the gate this file used + // to have settled on, and at that instant the `API:` row naming the port has + // not been printed yet — so a drift comparison made there compares nothing + // and `portDriftError()` answers `null`, a silent pass. This is the cheap + // half of the check; the forced-drift boot below is the load-bearing one. + expect(readyVerdict('', '41234')).toEqual({ settled: false }); + expect(readyVerdict('\n \u2713 Server is ready\n', '41234')).toEqual({ settled: false }); + }); + it('an `undefined` override survives as an own key, so spawn() unsets it', () => { const env = childEnv({ NODE_ENV: undefined }); expect(Object.hasOwn(env, 'NODE_ENV')).toBe(true); @@ -269,5 +396,29 @@ describe('#11267: childEnv() keeps the vitest worker out of a spawned os serve', }, 180_000, ); + + it( + 'THE LOAD-BEARING ARM: a child that DRIFTS off a held port is REFUSED, not probed', + async () => { + // ⛔ Nothing is simulated here: the port is really held, so `serve`'s own + // `isPortAvailable()` says no and its dev-mode `getAvailablePort()` walks + // off it. This is the lost race, produced. + // + // ⭐ What it pins is not `portDriftError()` — `serve-port-readback.e2e. + // test.ts` owns that — but THIS file's wiring: that the gate above + // reaches the refusal at all. A read-back called at a point that cannot + // observe the port is silent forever, and its silence reads as a pass on + // the security assertion this file exists to make. + const held = await holdPort(); + try { + await expect(probeOrigin(childEnv(OVERRIDES), String(held.port))).rejects.toThrow( + /PORT DRIFT/, + ); + } finally { + await held.release(); + } + }, + 240_000, + ); }); });