|
20 | 20 | * half of the criterion and the easier one to break in passing: every ordinary |
21 | 21 | * boot must publish exactly what it published before. |
22 | 22 | * |
| 23 | + * ## ⚠️ THE NON-ZERO ARM ASSERTS THE BOUND PORT, NOT THE REQUESTED ONE |
| 24 | + * |
| 25 | + * It used to assert `published === asked` under the premise "when that port is |
| 26 | + * free". That premise is a RACE, not a fact: `randomPort()` bind-probes a port |
| 27 | + * that is free at THAT INSTANT and then closes the listener, and on this |
| 28 | + * fleet's six-shard shared runner another process can take it during the |
| 29 | + * seconds between that probe and the spawned child's `listen()`. The child then |
| 30 | + * auto-shifts — which is exactly the behaviour the sibling arm below tests ON |
| 31 | + * PURPOSE — and the assertion was reading the RUNNER rather than the CLI. |
| 32 | + * Measured cost: it did not merely fail a pull request, it dropped one out of |
| 33 | + * the merge queue, rebuilt every entry behind it and burned a 24-minute Test |
| 34 | + * Core cycle for the whole batch — on a pull request whose diff was entirely |
| 35 | + * under `scripts/`, so to the seat driving it the drop looked like a real |
| 36 | + * defect. |
| 37 | + * |
| 38 | + * ⛔ The repair is NOT to skip, disable or quarantine this arm. The property it |
| 39 | + * guards is real; what was wrong is the QUANTITY the assertion pointed at. It |
| 40 | + * now asserts what #13062 actually cares about — the published port is the port |
| 41 | + * this server BOUND — through two legs that hold whether or not the request was |
| 42 | + * honoured: |
| 43 | + * |
| 44 | + * LEG 1 a client reaches the published port while the child is up, so |
| 45 | + * something really is listening on the number that was published; |
| 46 | + * LEG 2 that port stops answering once THIS child's process group dies, so |
| 47 | + * the listener WAS this server and not whatever else held the number. |
| 48 | + * |
| 49 | + * Leg 2 is the one that catches the defect in the only population where |
| 50 | + * requested and bound can differ: a boot that published the port it ASKED for |
| 51 | + * while binding another leaves the asked-for port still answering after its own |
| 52 | + * teardown, because something else is what holds it. |
| 53 | + * |
| 54 | + * ⛔ `reserve-then-release` is the same race relocated and is deliberately not |
| 55 | + * used. The requested-versus-bound relation is still asserted — that is the |
| 56 | + * "nothing an ordinary boot publishes may move" half and it is not dropped — |
| 57 | + * but it is read out of the CHILD's own #12543 drift notice, a statement |
| 58 | + * contemporaneous with its own bind, instead of out of this harness's stale |
| 59 | + * free-check. |
| 60 | + * |
23 | 61 | * ## ⚠️ THE INSTRUMENT, AND ITS PROOF — the card demanded both |
24 | 62 | * |
25 | 63 | * The report this card was filed from could NOT confirm a bound port by |
@@ -122,6 +160,46 @@ function reachable(port: number): Promise<boolean> { |
122 | 160 | }); |
123 | 161 | } |
124 | 162 |
|
| 163 | +/** |
| 164 | + * `serve`'s own DEV AUTO-SHIFT notice (#12543) — the one line that holds BOTH |
| 165 | + * the port that was asked for and the port that was taken instead. |
| 166 | + * |
| 167 | + * ⭐ Why this and not another free-check. "Was the request honoured?" cannot be |
| 168 | + * answered by probing `asked` from out here: whatever held it may have let it |
| 169 | + * go by the time this harness looks, and whatever the harness saw a moment |
| 170 | + * earlier is what the header calls a race rather than a fact. The notice is the |
| 171 | + * CHILD's own statement, made at the moment it decided which port to bind, so |
| 172 | + * it is contemporaneous with the bind by construction and no scheduling on this |
| 173 | + * container can move it. |
| 174 | + * |
| 175 | + * It reaches this harness reliably: `serve.ts` prints it through |
| 176 | + * `printDiagnostic` to **stderr** before the boot-quiet window opens, so it |
| 177 | + * cannot be swallowed and it survives a boot that dies later — and `bootServe` |
| 178 | + * below captures stderr from the first chunk. |
| 179 | + * |
| 180 | + * ⚠️ The pattern is the fifth copy of one regex in this package |
| 181 | + * (`serve-port-drift-notice.e2e.test.ts` pins it end to end; three unit files |
| 182 | + * hold it too). It is re-spelled rather than imported because the shared |
| 183 | + * harness does not export it and this card's fence does not reach that file; |
| 184 | + * see the PR's acceptance notes. Colour is not stripped first on purpose: |
| 185 | + * `chalk.yellow()` wraps the whole line, so the escapes sit at its ends and |
| 186 | + * never between these two numbers — and `childEnv` pins `NO_COLOR=1` anyway. |
| 187 | + */ |
| 188 | +const DRIFT_NOTICE = /Port (\d+) is in use — serving on (\d+) instead\./; |
| 189 | + |
| 190 | +/** What the child said about its own bind, or `null` if it said nothing. */ |
| 191 | +interface DriftNotice { |
| 192 | + /** The port it was asked for. */ |
| 193 | + requested: number; |
| 194 | + /** The port it took instead. */ |
| 195 | + bound: number; |
| 196 | +} |
| 197 | + |
| 198 | +function driftNoticeOf(output: string): DriftNotice | null { |
| 199 | + const match = DRIFT_NOTICE.exec(output); |
| 200 | + return match === null ? null : { requested: Number(match[1]), bound: Number(match[2]) }; |
| 201 | +} |
| 202 | + |
125 | 203 | interface Booted { |
126 | 204 | /** The `objectstack:listening` message, or `null` if the child never sent one. */ |
127 | 205 | ipc: { type?: string; port?: unknown; url?: unknown } | null; |
@@ -357,26 +435,64 @@ describe('#13062 `os serve --port 0` — the request that can never be the answe |
357 | 435 |
|
358 | 436 | describe('#13062 the non-zero half — nothing an ordinary boot publishes may move', () => { |
359 | 437 | it( |
360 | | - 'publishes exactly the port it was asked for when that port is free', |
| 438 | + 'publishes the port it BOUND — the one it was asked for, or the one it announced it took instead', |
361 | 439 | async () => { |
362 | 440 | const asked = Number(randomPort()); |
363 | 441 | const booted = await bootServe(bareDir, ['--port', String(asked)], newHome()); |
| 442 | + let published = -1; |
364 | 443 | try { |
365 | 444 | const { ipc, banner, runtimeFile } = channelsOf(booted); |
366 | | - // ⛔ Byte for byte what these channels published before this change: |
367 | | - // requested and bound coincide here, and that is the whole population |
368 | | - // of ordinary boots. |
369 | | - expect(ipc).toBe(asked); |
370 | | - expect(banner).toBe(asked); |
371 | | - expect(runtimeFile).toBe(asked); |
372 | | - expect(booted.ipc?.url).toBe(`http://localhost:${asked}`); |
373 | | - expect(await reachable(asked)).toBe(true); |
| 445 | + |
| 446 | + // ONE number on all three channels — not three that happen to agree |
| 447 | + // with an element of the argv list this harness passed in. |
| 448 | + expect(banner).toBe(ipc); |
| 449 | + expect(runtimeFile).toBe(ipc); |
| 450 | + expect(Number.isInteger(ipc)).toBe(true); |
| 451 | + expect(ipc as number).toBeGreaterThan(0); |
| 452 | + expect(booted.ipc?.url).toBe(`http://localhost:${ipc}`); |
| 453 | + |
| 454 | + // ⭐ LEG 1 of "published == BOUND": a client reaches the published |
| 455 | + // port. The instrument above proved it can answer NO before this was |
| 456 | + // allowed to mean YES. |
| 457 | + expect(await reachable(ipc as number)).toBe(true); |
| 458 | + |
| 459 | + // ⭐ REQUESTED versus BOUND, decided by the CHILD's own statement. |
| 460 | + // This is the "nothing an ordinary boot publishes may move" half: with |
| 461 | + // no drift notice the child bound what it was asked for, so the |
| 462 | + // published number must still be byte for byte what these channels |
| 463 | + // published before #13062 — and with one, the published number must be |
| 464 | + // the port the child itself said it took. |
| 465 | + const shift = driftNoticeOf(booted.stdout + booted.stderr); |
| 466 | + if (shift === null) { |
| 467 | + expect( |
| 468 | + ipc, |
| 469 | + 'the child printed no #12543 auto-shift notice, so it bound the port it was asked ' |
| 470 | + + 'for — and then published a different number', |
| 471 | + ).toBe(asked); |
| 472 | + } else { |
| 473 | + expect( |
| 474 | + shift.requested, |
| 475 | + 'the auto-shift notice names a requested port this test never asked for', |
| 476 | + ).toBe(asked); |
| 477 | + expect( |
| 478 | + ipc, |
| 479 | + 'the child announced it took one port and then published another', |
| 480 | + ).toBe(shift.bound); |
| 481 | + expect(ipc).not.toBe(asked); |
| 482 | + } |
| 483 | + |
| 484 | + published = ipc as number; |
374 | 485 | } finally { |
375 | 486 | await booted.stop(); |
376 | 487 | } |
377 | | - // The child is gone, so the port it held is gone with it. This is the |
378 | | - // orphan check as well: a surviving grandchild would still be listening. |
379 | | - expect(await reachable(asked)).toBe(false); |
| 488 | + |
| 489 | + // ⭐ LEG 2 of "published == BOUND", and the orphan check in one probe: |
| 490 | + // the child is gone, so the port THIS server held is gone with it. A boot |
| 491 | + // that published the port it was ASKED for while binding another would |
| 492 | + // leave that number still answering here, held by whatever took it; and a |
| 493 | + // surviving grandchild (`tsx` runs the CLI one level down) would leave the |
| 494 | + // real one answering. |
| 495 | + expect(await reachable(published)).toBe(false); |
380 | 496 | }, |
381 | 497 | 240_000, |
382 | 498 | ); |
|
0 commit comments