Skip to content

Commit f28f00f

Browse files
os-litantclaude
andauthored
test(cli): bind-probe the e2e serve port and name a lost race (#12523)
* wip: bind-probe port draw + legible contention (#12441) * test(cli): bind-probe the e2e serve port and name a lost race (#12441) The three draw sites in packages/cli/test drew a port blind with Math.random() under a docblock claiming a run "never contends with another agent's dev server on this host". Several agents share one container here and a measured full-suite run went 1 failed / 2101 passed on `Port 49402 is already in use`, clean on an isolated re-run. Unify on one draw in test/helpers/serve-process.ts, make it a real bind probe (listen 0.0.0.0:0, read the assigned port, close, spawn), and say in the docblock what that does and does not guarantee -- it stops drawing ports already HELD for a neighbour's lifetime and narrows the window to the close-to-spawn gap; it is still TOCTOU. Make the residual loss legible: portContentionError() turns `serve exited 1 before "Server is ready"` into a failure that names port contention and the port, at all three spawn sites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd * test(cli): hold the contention detector against serve.ts's live wording A transcript of the measured stderr cannot notice that the source it was copied from has been reworded, so the pin reads the live template out of src/commands/serve.ts and asserts the detector fires on it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent ae60d02 commit f28f00f

4 files changed

Lines changed: 539 additions & 20 deletions

File tree

‎packages/cli/test/helpers/serve-process.ts‎

Lines changed: 241 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
* rather than re-implements it.
1111
*/
1212

13-
import { spawn } from 'node:child_process';
13+
import { execFileSync, spawn } from 'node:child_process';
1414
import { resolve } from 'node:path';
1515
import { fileURLToPath } from 'node:url';
1616

@@ -20,9 +20,208 @@ const HERE = resolve(fileURLToPath(import.meta.url), '..');
2020
export const CLI = resolve(HERE, '../../bin/run-dev.js');
2121
export const TSX = resolve(HERE, '../../../../node_modules/.bin/tsx');
2222

23-
/** A random high port, so a run never contends with a dev server on this host. */
23+
/**
24+
* The bind probe, run in a throwaway Node process: bind `0.0.0.0:<want>`, print
25+
* the port the kernel actually assigned, close. `want = 0` asks the kernel to
26+
* choose. Returns `null` when the bind failed — which for a specific `want`
27+
* means "that port is taken", and for `0` means the probe itself malfunctioned.
28+
*
29+
* ## Why a SUBPROCESS rather than an in-process `net.createServer()`
30+
*
31+
* `net.Server#listen()` reports its assigned port ASYNCHRONOUSLY. Measured on
32+
* this container (node 22.22.2): `server.address()` is `null` on the very next
33+
* line after `listen(0, '0.0.0.0')`, so an in-process probe can only be
34+
* `async`. `randomPort()` below is called from ~14 sites across 8 other files
35+
* in this directory, every one of them passing it straight into a `spawn()`
36+
* argument list; turning it async would edit all of them for no behavioural
37+
* gain. A `node -e` child does the same bind and is synchronous from this
38+
* process's point of view.
39+
*
40+
* The price, measured on the container this suite runs in: ~38 ms per draw
41+
* (5-draw mean, empty child env — an inherited environment measured 74.7 ms,
42+
* so the strip below roughly halves it too). Against this package's own
43+
* measured per-spawn floor — 2.9 s for `node bin/run.js --version`, 6.5 s for
44+
* the tsx source entry — one draw is ~1.3% of the cheapest thing it precedes,
45+
* and ~14 draws are ~0.5 s against a suite whose wall was 495.8 s when
46+
* `vitest.config.ts` last measured it (~0.1%).
47+
*
48+
* ⛔ The probe child gets an environment built from NOTHING — not
49+
* `{ ...process.env, … }`, not even `childEnv()`. It is a bare `net` bind that
50+
* reads no variable at all, so the whole runner environment is surplus, and a
51+
* bulk copy into a spawn is the class `pnpm check:cli-test-child-env` closes in
52+
* this directory (a spread here is a real finding under that gate, not a false
53+
* positive — measured: it takes this file from 0 to 1 over its ceiling). Not
54+
* inheriting `NODE_OPTIONS` is the concrete win: an `--import` hook meant for
55+
* the suite would otherwise load inside every port draw.
56+
*/
57+
function probeBind(want: number): number | null {
58+
const src = [
59+
"const net = require('node:net');",
60+
'const want = Number(process.argv[1] || 0);',
61+
'const s = net.createServer();',
62+
"s.on('error', () => process.exit(3));",
63+
"s.listen(want, '0.0.0.0', () => {",
64+
' const p = s.address().port;',
65+
' s.close(() => process.stdout.write(String(p)));',
66+
'});',
67+
].join('\n');
68+
try {
69+
const out = execFileSync(process.execPath, ['-e', src, String(want)], {
70+
encoding: 'utf8',
71+
stdio: ['ignore', 'pipe', 'pipe'],
72+
timeout: 30_000,
73+
env: {},
74+
});
75+
const port = Number(out.trim());
76+
return Number.isInteger(port) && port > 0 ? port : null;
77+
} catch {
78+
return null;
79+
}
80+
}
81+
82+
/**
83+
* The ONE port draw for every e2e spawn in this directory — a real bind probe,
84+
* not a blind `Math.random()` (#12441).
85+
*
86+
* Listen on `0.0.0.0:0`, read the port the kernel assigned, close the listener,
87+
* return the port for the caller to hand to `os serve`.
88+
*
89+
* ## ⚠️ What this guarantees, and what it does NOT — both halves, deliberately
90+
*
91+
* It is **still TOCTOU**. The listener is closed before `serve` binds, so the
92+
* port is unheld across the close-to-spawn gap and this can still lose a race.
93+
* What it actually buys:
94+
*
95+
* • It never draws a port that is **already held**. A blind draw picks a
96+
* number out of a range without asking anyone, so a neighbouring agent's
97+
* dev server — bound for that process's entire lifetime, minutes or hours —
98+
* is a live target on every single draw. The kernel does not assign a port
99+
* that is currently bound, so that whole population is off the table.
100+
* • It narrows the window from "the whole run" to "one close-to-spawn gap"
101+
* (milliseconds). Only something that binds *inside* that gap can take it.
102+
* • It removes this directory's second collision source. There used to be
103+
* three independent draws over two overlapping ranges (41000-60000 here,
104+
* 40000-60000 in `serve-app-anchored-optional-import.e2e.test.ts`), which
105+
* could collide with EACH OTHER under `--maxWorkers > 1`, not only with a
106+
* neighbour. There is one draw now and it asks the kernel.
107+
*
108+
* ⛔ The comment this replaced claimed "a run never contends with another
109+
* agent's dev server on this host". That unqualified negative is the reason
110+
* nobody re-examined the draw until a real run in this fleet went red on
111+
* `✗ Port 49402 is already in use`. **Do not write another one here.** The
112+
* residual race is real and unclosed; what pays for it is
113+
* `portContentionError()` below, which makes the residual failure SAY it is a
114+
* port race instead of `serve exited 1 before "Server is ready"`.
115+
*
116+
* ⚠️ One property that is worse than the old range and is stated rather than
117+
* hidden: the kernel assigns from its ephemeral range
118+
* (`/proc/sys/net/ipv4/ip_local_port_range`, 32768-60999 on this container),
119+
* which is also where it draws source ports for OUTBOUND connections. The old
120+
* 40000-60000 range overlapped that anyway, and the probe's win — never
121+
* handing out a port some listener already holds — is the larger term. Nothing
122+
* here makes the port immune once it is handed over.
123+
*/
124+
export function reservePort(): number {
125+
// Retried, because a `null` for `want = 0` is a malfunctioning probe (the
126+
// kernel cannot answer "busy" to a request for any free port), and turning a
127+
// transient subprocess hiccup into a hard failure would replace one flake
128+
// class with another.
129+
for (let attempt = 0; attempt < 3; attempt++) {
130+
const port = probeBind(0);
131+
if (port !== null) return port;
132+
}
133+
throw new Error(
134+
'bind probe failed: could not obtain a free TCP port from the kernel after 3 attempts '
135+
+ '(listen on 0.0.0.0:0 in a `node -e` child). This is a host problem, not a verdict '
136+
+ 'about the code under test.',
137+
);
138+
}
139+
140+
/**
141+
* Is `port` bindable RIGHT NOW? The **negative arm** of the same probe.
142+
*
143+
* Exported so `serve-port-bind-probe.test.ts` can prove the instrument is able
144+
* to answer NO: a probe that reports "free" for a port the test is holding open
145+
* is not an instrument, and every claim `reservePort()` makes rests on this
146+
* being a real bind rather than a shape that always succeeds.
147+
*/
148+
export function portIsFree(port: number | string): boolean {
149+
return probeBind(Number(port)) !== null;
150+
}
151+
152+
/**
153+
* `reservePort()` as a string, for the call sites that pass a port straight
154+
* into a `spawn()` argument list.
155+
*
156+
* ⚠️ The name is kept — and is now a slight misnomer, which is cheaper than the
157+
* alternative. It is `String(reservePort())` and nothing else; the draw, the
158+
* guarantees and the residual race are all documented on `reservePort()` above
159+
* and there is no second mechanism hiding behind this name. Renaming it would
160+
* be a rename-only edit across the 8 other files in this directory that call
161+
* it, which is churn this change deliberately does not spend.
162+
*/
24163
export function randomPort(): string {
25-
return String(40000 + Math.floor(Math.random() * 20000));
164+
return String(reservePort());
165+
}
166+
167+
/** How a boot says the port was taken — `serve.ts`'s own diagnostic, then the raw kernel error. */
168+
const PORT_TAKEN_PATTERNS = [
169+
/Port (\d+) is already in use/,
170+
/EADDRINUSE[^\n]*?:(\d+)/,
171+
];
172+
173+
/**
174+
* ⭐ Turn a boot that died on a taken port into a failure that SAYS SO (#12441
175+
* ruling ④).
176+
*
177+
* Returns an `Error` when `output` shows the child could not bind, else `null`.
178+
*
179+
* ## Why this exists, and why it is the half that pays
180+
*
181+
* The measured cost of a lost port race here is not the lost run. It is *"a red
182+
* suite that is not reproducible, on a test file the reader has no reason to
183+
* connect to a port"* — the failure surfaced as `serve exited 1 before "Server
184+
* is ready"` inside a file about `NODE_ENV` defaulting, and it cost an agent a
185+
* round to decide whether the failure belonged to the change under test. A fix
186+
* that only lowers the probability leaves that cost exactly where it was, just
187+
* rarer and therefore even more surprising when it lands.
188+
*
189+
* So the port number is read out of the CHILD's own diagnostic rather than
190+
* passed in: whatever the harness thought it reserved, the number the child
191+
* printed is the one that was contended. `probedPort` is threaded in only to
192+
* say, in the message, that the port WAS probed free moments earlier — which is
193+
* what tells the reader this is the residual TOCTOU gap and not a harness that
194+
* never looked.
195+
*/
196+
export function portContentionError(
197+
output: string,
198+
what: string,
199+
probedPort?: number | string,
200+
): Error | null {
201+
let port: string | undefined;
202+
for (const pattern of PORT_TAKEN_PATTERNS) {
203+
const match = pattern.exec(output);
204+
if (match) {
205+
port = match[1];
206+
break;
207+
}
208+
}
209+
if (port === undefined) return null;
210+
const probed = probedPort === undefined
211+
? ''
212+
: `This harness bind-probed ${probedPort} and the kernel reported it FREE moments earlier `
213+
+ '(`reservePort()` in `test/helpers/serve-process.ts`), so this is the residual '
214+
+ 'close-to-spawn gap that probe narrows but does not close.\n';
215+
return new Error(
216+
`PORT CONTENTION on port ${port}: \`${what}\` could not bind it.\n`
217+
+ probed
218+
+ 'Several agents share one container in this fleet, so another process took the port '
219+
+ 'between the probe and the spawn.\n'
220+
+ '⛔ This is a HOST race, not a verdict about the code under test. Do not spend a round '
221+
+ 'deciding whether your change caused it — re-run this file in isolation. If it '
222+
+ 'reproduces there, the port is genuinely held and the message above names it.\n'
223+
+ `--- child output ---\n${output}`,
224+
);
26225
}
27226

28227
/**
@@ -214,13 +413,38 @@ export interface ServeRun {
214413
stderr: string;
215414
}
216415

416+
/**
417+
* The port a caller asked for, read back out of its own `args`.
418+
*
419+
* `runServe` takes the port as an opaque argv element rather than a parameter,
420+
* so this is how it learns which port it probed — for the message only, never
421+
* for the verdict (`portContentionError` reads the contended port out of the
422+
* child's own diagnostic). `undefined` when the caller passed no `--port`,
423+
* which just drops one sentence from the message.
424+
*/
425+
function portOf(args: string[]): string | undefined {
426+
for (const flag of ['--port', '-p']) {
427+
const at = args.indexOf(flag);
428+
if (at !== -1 && at + 1 < args.length) return args[at + 1];
429+
}
430+
return undefined;
431+
}
432+
217433
/**
218434
* Boot `os serve` in `cwd`, collect its output until `waitFor` matches (or the
219435
* process exits), then stop it. Never leaves the child running.
220436
*
221437
* A boot that DIES still has to have said why, so an early exit resolves rather
222438
* than rejects — the caller's assertions read what it printed on the way down.
223439
*
440+
* ⭐ ONE narrow exception to that, and it is deliberate (#12441): a boot that
441+
* died because it could not BIND rejects, with `portContentionError()`'s
442+
* message. That death says nothing about the code under test, and letting it
443+
* resolve hands the caller an output buffer whose assertions then fail on
444+
* whatever marker is missing — which is the illegible shape the card measured.
445+
* No test in this directory drives a deliberately-busy port, so nothing is
446+
* asserting on the resolved form of it.
447+
*
224448
* `waitFor` is matched against **stdout and stderr together** (#7915). `serve`
225449
* writes every human line — banner, boot progress, kernel logs — to stderr now,
226450
* because its stdout belongs to the MCP stdio transport when one is mounted;
@@ -271,8 +495,20 @@ export function runServe(
271495
} catch {
272496
/* already gone */
273497
}
274-
if (err) rejectRun(err);
275-
else resolveRun({ stdout, stderr });
498+
if (err) {
499+
rejectRun(err);
500+
return;
501+
}
502+
const contended = portContentionError(
503+
stdout + stderr,
504+
'os serve (bin/run-dev.js, via runServe)',
505+
portOf(args),
506+
);
507+
if (contended) {
508+
rejectRun(contended);
509+
return;
510+
}
511+
resolveRun({ stdout, stderr });
276512
};
277513

278514
const timer = setTimeout(

‎packages/cli/test/serve-app-anchored-optional-import.e2e.test.ts‎

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,12 @@
3535
*
3636
* The spawn is written out here rather than taken from `test/helpers/
3737
* serve-process.ts` on purpose: that helper always runs the child WITH `cwd` set
38-
* to the app, which is the one shape this file must not use. Only its
39-
* `childEnv()` choke point is borrowed (#11267) — what the child INHERITS is
40-
* orthogonal to which directory it is started in, and this file boots the real
41-
* stack, better-auth included, which reads `TEST` directly.
38+
* to the app, which is the one shape this file must not use. What IS borrowed
39+
* from it is everything orthogonal to the directory the child starts in:
40+
* `childEnv()` (#11267) — this file boots the real stack, better-auth included,
41+
* which reads `TEST` directly — plus `randomPort()` and `portContentionError()`
42+
* (#12441), because a port draw is not a property of the CWD either and this
43+
* file used to carry its own second, overlapping one.
4244
*
4345
* ── The anti-vacuity floor ───────────────────────────────────────────────
4446
*
@@ -57,7 +59,7 @@ import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'nod
5759
import { tmpdir } from 'node:os';
5860
import { dirname, join, resolve } from 'node:path';
5961
import { fileURLToPath } from 'node:url';
60-
import { childEnv } from './helpers/serve-process.js';
62+
import { childEnv, portContentionError, randomPort } from './helpers/serve-process.js';
6163

6264
const HERE = dirname(fileURLToPath(import.meta.url));
6365

@@ -149,15 +151,29 @@ interface Run { stdout: string; stderr: string; both: string }
149151
*
150152
* An early exit resolves rather than rejects: a boot that DIES still has to have
151153
* said why, and the refusal case below reads exactly that.
154+
*
155+
* ⭐ ONE exception (#12441): a boot that died because it could not BIND rejects,
156+
* naming the port. Resolving it would hand the caller an output buffer with no
157+
* marker in it, and the assertions would then fail with "the cluster gate was
158+
* not loaded from the app" — a sentence about resolution bases, for a failure
159+
* that is entirely about a port. That mis-signalling is the whole cost the card
160+
* measured, and it is more expensive than the lost run.
152161
*/
153162
function runServeFrom(
154163
cwd: string,
155164
configArg: string,
156165
waitFor: RegExp,
157166
timeoutMs = 240_000,
158167
): Promise<Run> {
159-
return new Promise((resolveRun) => {
160-
const port = String(40000 + Math.floor(Math.random() * 20000));
168+
return new Promise((resolveRun, rejectRun) => {
169+
// ⛔ Was an inline `String(40000 + Math.random() * 20000)`, a SECOND blind
170+
// draw whose range overlapped the one in
171+
// `serve-node-env-production-default.e2e.test.ts` (41000-60000) — so under
172+
// `--maxWorkers > 1` the two files could collide with EACH OTHER, not only
173+
// with a neighbouring agent's dev server. One bind-probed draw now, in the
174+
// helper; its docblock is the authority on what that does and does not
175+
// guarantee.
176+
const port = randomPort();
161177
const child = spawn(TSX, [CLI, 'serve', configArg, '--port', port], {
162178
cwd,
163179
env: childEnv({
@@ -179,6 +195,15 @@ function runServeFrom(
179195
settled = true;
180196
clearTimeout(timer);
181197
try { child.kill('SIGTERM'); } catch { /* already gone */ }
198+
const contended = portContentionError(
199+
stdout + stderr,
200+
'os serve (bin/run-dev.js ⇒ NODE_ENV=development)',
201+
port,
202+
);
203+
if (contended) {
204+
rejectRun(contended);
205+
return;
206+
}
182207
resolveRun({ stdout, stderr, both: stdout + stderr });
183208
};
184209
const timer = setTimeout(finish, timeoutMs);

0 commit comments

Comments
 (0)