From fda168119dea074b8d76007cc369f5a07110c559 Mon Sep 17 00:00:00 2001 From: os-litant Date: Sat, 29 Aug 2026 04:44:11 +0000 Subject: [PATCH 1/2] fix(cli): serve publishes the port it BOUND, on all three channels (#13062) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `os serve` announced the port it was ASKED for on the `objectstack:listening` IPC message, the ready banner's `API:` row and `runtime..json`. Requested and bound coincide for every port but one, so this stayed invisible; `--port 0` is the value where they cannot coincide (`MIN_PORT = 0` is legal on purpose — `listen(0)` binds a kernel-assigned port), and all three announced an address nothing was listening on, with nothing erroring. The three now read one number, resolved once off the transport's own `IHttpServer.getPort()` — the contract member that already promises the real bound port, in particular for `listen(0)`. The comment above the publish block asserted this invariant while the code did not hold it; it now says what the code does. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd --- .changeset/serve-publishes-bound-port.md | 29 ++ .../serve-bound-port-publication.test.ts | 239 +++++++++++ packages/cli/src/commands/serve.ts | 112 ++++- packages/cli/test/helpers/serve-process.ts | 32 +- .../cli/test/serve-port-readback.e2e.test.ts | 26 +- .../serve-publishes-bound-port.e2e.test.ts | 400 ++++++++++++++++++ 6 files changed, 816 insertions(+), 22 deletions(-) create mode 100644 .changeset/serve-publishes-bound-port.md create mode 100644 packages/cli/src/commands/serve-bound-port-publication.test.ts create mode 100644 packages/cli/test/serve-publishes-bound-port.e2e.test.ts diff --git a/.changeset/serve-publishes-bound-port.md b/.changeset/serve-publishes-bound-port.md new file mode 100644 index 0000000000..ebba2c0a98 --- /dev/null +++ b/.changeset/serve-publishes-bound-port.md @@ -0,0 +1,29 @@ +--- +"@objectstack/cli": patch +--- + +fix(cli): `os serve` publishes the port it BOUND, not the one it was asked for (#13062) + +The three channels `serve` announces an address on — the `objectstack:listening` +IPC message, the ready banner's `API:` row and `runtime..json` — +were three outputs of one number, and that number was the port the operator +requested. For every port but one the requested and the bound value coincide, so +this stayed invisible; `0` is the value where they cannot coincide. +`MIN_PORT = 0` is legal on purpose (`utils/port-contract.ts`: 0 is "a REQUEST, +not an error" — `listen(0)` binds a kernel-assigned port). + +FROM (`os serve --port 0`): IPC `{ port: 0, url: 'http://localhost:0' }`, banner +`API: http://localhost:0/`, `runtime.env_local.json` `"port": 0` — three +channels naming an address nothing was listening on, with nothing erroring. + +TO: all three name the port the HTTP server actually bound, read off the +transport's own `IHttpServer.getPort()` (the contract member that already +promises "the real bound port — in particular when `listen(0)` requested an +ephemeral port"). The same repair covers a bind that walked past a port taken +between `serve`'s probe and the transport's `listen()`. + +Unchanged for every other port: when the requested port is the bound one — which +is every ordinary boot, including one that dev-auto-shifted off a busy port — +all three channels publish exactly what they published before. A boot with no +HTTP server, or a transport that does not implement the optional member, also +falls back to the previous value. diff --git a/packages/cli/src/commands/serve-bound-port-publication.test.ts b/packages/cli/src/commands/serve-bound-port-publication.test.ts new file mode 100644 index 0000000000..80017990bd --- /dev/null +++ b/packages/cli/src/commands/serve-bound-port-publication.test.ts @@ -0,0 +1,239 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #13062 — `os serve` publishes the port it BOUND, on all three of the channels + * that announce one. + * + * ## The defect, and why it hid + * + * The IPC message (`objectstack:listening`), the ready banner's `API:` row and + * `runtime..json` were three outputs of ONE number, and that + * number was the port the operator ASKED for. For every value but one the + * requested and the bound port coincide, so the three agreed with each other + * AND with the socket, and nothing ever disagreed. For `0` they cannot + * coincide: `utils/port-contract.ts` declares `MIN_PORT = 0` from its own + * measurement and states that 0 is "a REQUEST, not an error" — `listen(0)` + * binds a kernel-assigned port — so `os serve --port 0` announced + * `{ port: 0 }`, printed `API: http://localhost:0/` and wrote `"port": 0`. + * Three channels naming an address nothing listens on, with nothing erroring. + * + * ## Two halves, and the second is the one that rots + * + * The BEHAVIOUR half is {@link resolveBoundPort}, driven below against a fake + * kernel — a unit, so the asymmetry that matters (`0` in, a real port out) is + * exercised without a boot. + * + * The WIRING half cannot be reached that way at all: `run()` is one ~3000-line + * method that needs a whole kernel to enter, so nothing in-process can observe + * which variable its three publish sites read. That is exactly the half the + * card is about — ⛔ "fix one channel and two go on lying, harder to find than + * before" — so it is pinned by reading the source, with comments MASKED so a + * sentence about the bound port can never answer for code that publishes the + * requested one. (`test/serve-publishes-bound-port.e2e.test.ts` drives all + * three through a real boot; this is the cheap half that fails in 40ms and + * names which channel regressed.) + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +// The repo's ONE code/prose separator, typed by the hand-written `.d.mts` +// beside it — the same import `utils/port-contract-single-source.test.ts` uses, +// and for the same reason: this file asks "does the CODE publish the bound +// port", and a comment claiming it does is precisely what was there before. +import { maskComments } from '../../../../scripts/js-comment-mask.mjs'; + +import { resolveBoundPort } from './serve.js'; +import { MAX_PORT } from '../utils/port-contract.js'; + +/** …/packages/cli/src/commands — seeded from `import.meta.url`. */ +const HERE = resolve(fileURLToPath(import.meta.url), '..'); + +/** `serve.ts`'s CODE, with every comment span blanked. */ +const SERVE = maskComments(readFileSync(resolve(HERE, 'serve.ts'), 'utf8')); + +/** `serve.ts` verbatim — only for asserting that the mask actually masked. */ +const SERVE_RAW = readFileSync(resolve(HERE, 'serve.ts'), 'utf8'); + +/** + * A kernel whose transport reports `reported` from `getPort()`. + * + * ⚠️ The miss path THROWS rather than returning `undefined`, because that is + * what `ObjectKernel.getService` really does (`packages/core/src/kernel.ts` — + * a miss is a composition fault it refuses to answer silently). A fake that + * returned `undefined` would leave the production `try` untested and green. + */ +function kernelReporting( + reported: unknown, + opts: { under?: string; getPort?: unknown } = {}, +): { kernel: { getService: (name: string) => unknown }; asked: string[] } { + const under = opts.under ?? 'http.server'; + const asked: string[] = []; + return { + asked, + kernel: { + getService(name: string) { + asked.push(name); + if (name !== under) throw new Error(`Service '${name}' not found`); + return 'getPort' in opts ? { getPort: opts.getPort } : { getPort: () => reported }; + }, + }, + }; +} + +describe('#13062 resolveBoundPort — the transport answers, not the request', () => { + it('answers with the BOUND port when the request was 0', () => { + // ⭐ The whole card in one line: `--port 0` is the one request that can + // never equal its answer, and it is the case every channel got wrong. + const { kernel } = kernelReporting(44321); + expect(resolveBoundPort(kernel, 0)).toBe(44321); + }); + + it('answers with the BOUND port when a non-zero request drifted', () => { + // The second way the two part company on this command, and it needs no + // `--port 0`: `HonoHttpServer.listen()` walks past EADDRINUSE on its own, + // so a port taken between this command's probe and the transport's + // `listen()` is bound one higher than the number serve resolved. + const { kernel } = kernelReporting(41235); + expect(resolveBoundPort(kernel, 41234)).toBe(41235); + }); + + it('is a no-op for the case that was always right — request === bound', () => { + // ⛔ The half most easily broken on the way past: every ordinary boot must + // publish exactly what it published before. + const { kernel } = kernelReporting(41234); + expect(resolveBoundPort(kernel, 41234)).toBe(41234); + }); + + it('asks for the CANONICAL service name, never the deprecated alias', () => { + const { kernel, asked } = kernelReporting(44321); + resolveBoundPort(kernel, 0); + expect(asked).toEqual(['http.server']); + // `http-server` is the same instance under a deprecated second name + // (#4251). Reading it here would be new code taking a retiring dependency. + expect(asked).not.toContain('http-server'); + }); + + describe('the fallback is the OLD behaviour, and it may not narrow what boots', () => { + it('falls back when nothing registered a transport (`--server=false`)', () => { + const { kernel } = kernelReporting(0, { under: 'nothing-registers-this' }); + expect(resolveBoundPort(kernel, 3000)).toBe(3000); + }); + + it('falls back when the transport does not implement the optional member', () => { + const { kernel } = kernelReporting(0, { getPort: undefined }); + expect(resolveBoundPort(kernel, 3000)).toBe(3000); + }); + + it('falls back when `getPort()` itself throws', () => { + const { kernel } = kernelReporting(0, { + getPort: () => { throw new Error('transport is mid-restart'); }, + }); + expect(resolveBoundPort(kernel, 3000)).toBe(3000); + }); + + it('falls back for a kernel that has no `getService` at all', () => { + expect(resolveBoundPort(undefined, 3000)).toBe(3000); + expect(resolveBoundPort({}, 3000)).toBe(3000); + }); + }); + + describe('what may not be published, whatever the transport says', () => { + it('⛔ refuses 0 as an ANSWER, though 0 is a legal REQUEST', () => { + // No socket is bound to port 0. A transport reporting it has not listened + // yet — `HonoHttpServer.getPort()` returns its constructor argument until + // the listening callback fires — and republishing it IS the defect. + const { kernel } = kernelReporting(0); + expect(resolveBoundPort(kernel, 41234)).toBe(41234); + }); + + it('refuses anything that cannot be a bound port', () => { + for (const bad of [-1, 1.5, Number.NaN, Number.POSITIVE_INFINITY, MAX_PORT + 1]) { + const { kernel } = kernelReporting(bad); + expect(resolveBoundPort(kernel, 41234), `accepted ${String(bad)}`).toBe(41234); + } + for (const bad of ['44321', null, undefined, {}]) { + const { kernel } = kernelReporting(bad); + expect(resolveBoundPort(kernel, 41234), `accepted ${JSON.stringify(bad)}`).toBe(41234); + } + }); + + it('accepts the ceiling itself, read from the ONE port contract', () => { + // ⛔ Never written as a literal here — `utils/port-contract.ts` is the one + // place either bound is declared, and `port-contract-single-source.test.ts` + // fails on a second copy. + const { kernel } = kernelReporting(MAX_PORT); + expect(resolveBoundPort(kernel, 41234)).toBe(MAX_PORT); + }); + }); +}); + +describe('#13062 all THREE channels publish that one number — read off the code', () => { + it('the `objectstack:listening` IPC message', () => { + expect( + SERVE, + 'the IPC message no longer publishes `boundPort` — `os dev` reads this channel to learn ' + + 'where its child ended up, and #13061 records that `os start` could read it too', + ).toContain("process.send({ type: 'objectstack:listening', port: boundPort, url: runtimeUrl });"); + }); + + it('`runtime..json`', () => { + expect( + SERVE, + 'the runtime state file no longer publishes `boundPort` — external supervisors and health ' + + 'checks read it for the address to poll', + ).toMatch(/pid: process\.pid,\s*\n\s*port: boundPort,/); + }); + + it('the ready banner, through the runtime\'s own base-URL chain', () => { + expect( + SERVE, + 'the banner no longer resolves its origin from `boundPort`', + ).toContain('externalBaseOrigin: resolveAuthBaseUrl(boundPort).baseOrigin'); + }); + + it('the URL those channels carry is composed from the same number', () => { + expect(SERVE).toContain('const runtimeUrl = `http://localhost:${boundPort}`;'); + }); + + it('⛔ and NONE of the three has drifted back onto the requested port', () => { + // The card's own instruction: three outputs of one defect, and repairing + // one leaves two lying in a place nobody thinks to look next time. + expect(SERVE).not.toContain('port: Number(port)'); + expect(SERVE).not.toContain('externalBaseOrigin: resolveAuthBaseUrl(port)'); + expect(SERVE).not.toContain('const runtimeUrl = `http://localhost:${port}`'); + }); + + it('resolves it ONCE, from the transport, after the boot', () => { + expect( + SERVE, + 'the bound port is no longer read off the transport — three call sites resolving it ' + + 'separately is how they earn the right to disagree', + ).toContain('const boundPort = resolveBoundPort(kernel, port);'); + expect(SERVE.match(/const boundPort =/g) ?? []).toHaveLength(1); + }); + + describe('ANTI-VACUITY: the scan reads CODE, and it read something', () => { + it('the mask blanked comments rather than returning the file unchanged', () => { + // Without this, every `not.toContain` above passes on an empty string. + expect(SERVE.length).toBe(SERVE_RAW.length); + expect(SERVE).not.toBe(SERVE_RAW); + // A sentence that exists ONLY in a comment must be invisible to the scan… + const proseOnly = 'republishing it is the defect itself'; + expect(SERVE_RAW, 'the control sentence was reworded — pick another').toContain(proseOnly); + expect(SERVE).not.toContain(proseOnly); + // …while the code around it is still there. + expect(SERVE).toContain('export function resolveBoundPort('); + }); + + it('the requested port is still what the transport is CONSTRUCTED with', () => { + // The positive control for the negatives above: `port` has not been + // globally renamed, so `not.toContain('port: Number(port)')` is a + // measurement rather than a consequence of the variable disappearing. + expect(SERVE).toContain('new HonoServerPlugin({ port })'); + expect(SERVE).toContain('port = await getAvailablePort(requestedPort)'); + }); + }); +}); diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index b4077e2f0b..d138ff34cf 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -58,6 +58,7 @@ import { parseRequestedPort, formatInvalidPortNotice, portTextReadNotice, + MAX_PORT, type PortInputSource, } from '../utils/port-contract.js'; import { BootLogCapture, isVerboseBootLevel } from '../utils/boot-log-capture.js'; @@ -369,6 +370,69 @@ export function formatExhaustedPortSearchNotice(requestedPort: number, cause: un ); } +/** + * The port the HTTP server ACTUALLY bound, read off the transport that bound it + * (#13062). + * + * ## The defect this closes + * + * `serve` used to publish the port it was ASKED for on all three of the + * channels that ANNOUNCE an address — the `objectstack:listening` IPC message, + * the ready banner's `API:` row (through {@link resolveAuthBaseUrl}), and + * `runtime..json`. For every port but one the requested and the + * bound value coincide, which is why it stayed invisible; for `0` they CANNOT + * coincide. `MIN_PORT = 0` is legal on purpose — `utils/port-contract.ts` says + * so in its own words, from its own measurement, that 0 is "a REQUEST, not an + * error", and `listen(0)` binds a kernel-assigned port. So `os serve --port 0` + * announced `{ port: 0 }`, printed `API: http://localhost:0/` and wrote + * `"port": 0`: three channels naming an address nothing is listening on, and + * nothing anywhere erroring. + * + * ## Where the answer comes from, and why it is not a second measurement + * + * `IHttpServer.getPort?()` is a DECLARED contract member + * (`@objectstack/spec/contracts`): *"after `listen()` resolves, implementations + * that provide this member MUST return the real bound port — in particular when + * `listen(0)` requested an ephemeral port"*. The Hono transport fills it from + * `@hono/node-server`'s own listening callback (`info.port`, the socket's + * `address().port`). So this asks the one component that KNOWS instead of + * re-deriving the number from the request — the same rule + * {@link formatExhaustedPortSearchNotice} follows just above, where every number + * printed is the failing walk's own record rather than a recomputation. + * + * ## The fallback IS today's behaviour, deliberately + * + * A boot with no transport (`--server=false`), a host that mounts a transport + * not implementing the optional member, or an answer that cannot be a bound + * port, all return `requestedPort` — exactly what every channel published + * before this function existed. This repair may not narrow what boots. + * + * ⛔ `0` is NOT accepted as an ANSWER here, though it is a legal REQUEST: no + * socket is ever bound to port 0, so a transport reporting it has not listened + * yet (`HonoHttpServer.getPort()` falls back to its constructor argument until + * `listen()` resolves), and republishing it is the defect itself. + */ +export function resolveBoundPort( + kernel: { getService?: (name: string) => unknown } | undefined, + requestedPort: number, +): number { + let reported: unknown; + try { + // `http.server` is the CANONICAL service name; `http-server` is a + // deprecated alias for the same instance (#4251) that new code must not + // read. ⚠️ `getService` THROWS when nothing registered the name — it does + // not return undefined — hence the `try`, not a `?.` chain alone. + const server = kernel?.getService?.('http.server') as { getPort?: () => unknown } | undefined; + if (typeof server?.getPort !== 'function') return requestedPort; + reported = server.getPort(); + } catch { + return requestedPort; + } + if (typeof reported !== 'number' || !Number.isInteger(reported)) return requestedPort; + if (reported <= 0 || reported > MAX_PORT) return requestedPort; + return reported; +} + /** * The IDENTITIES a capability provider registers under: full `plugin.name` ids * (`com.objectstack.mcp`) and/or exported class names (`MCPServerPlugin`). @@ -4208,6 +4272,20 @@ export default class Serve extends Command { // available to this process at all. if (multiNodeVerdict) emitMultiNodeCapTelemetry(kernel, multiNodeVerdict); + // ── The port this process ACTUALLY bound (#13062) ───────────── + // Read ONCE, here, and handed to every channel that announces an address: + // the ready banner below, the `objectstack:listening` IPC message and + // `runtime..json`. Those three were three outputs of ONE + // number, and that number was the port that had been REQUESTED — equal to + // the bound one for every value except the one where it can never be + // (`--port 0`), which is how all three came to announce `localhost:0` + // with nothing erroring. + // + // ⭐ One read, not three. Three call sites would be free to disagree, and + // what this repairs is exactly a set of channels that agreed with each + // other while disagreeing with the socket. + const boundPort = resolveBoundPort(kernel, port); + // ── Clean startup summary ────────────────────────────────────── // #8978 — the Config:/Artifact: row must name what actually booted, // never `relativeConfig` unconditionally (see resolveBannerConfigRow). @@ -4228,12 +4306,15 @@ export default class Serve extends Command { // reads only `process.env` and the port, so calling it here changes // nothing about what is bound or advertised — this is printed text. // - // `port` is the port the server ACTUALLY bound (past any dev auto-shift), - // so the `http://localhost:` tail of the chain still names the - // right address in the local dev loop. `baseOrigin` is `null` when the - // chain produced something unparseable; the banner then prints paths - // with no origin rather than a confident wrong URL. - externalBaseOrigin: resolveAuthBaseUrl(port).baseOrigin, + // `boundPort` is the port the server ACTUALLY bound — read back off the + // transport that bound it ({@link resolveBoundPort}), not the number + // this process was asked for. So the `http://localhost:` tail of + // the chain still names the right address in the local dev loop, + // including under `--port 0`, where the requested value can never be + // the bound one. `baseOrigin` is `null` when the chain produced + // something unparseable; the banner then prints paths with no origin + // rather than a confident wrong URL. + externalBaseOrigin: resolveAuthBaseUrl(boundPort).baseOrigin, ...resolveBannerConfigRow({ relativeConfig, useArtifactFallback, pinnedArtifact }), isDev, pluginCount: loadedPlugins.length, @@ -4272,17 +4353,24 @@ export default class Serve extends Command { }); // ── Publish the actually-bound port ──────────────────────────── - // `port` here is the port the HTTP server actually bound — already - // resolved past any dev auto-shift (busy 3000 → 3001). Publish it so - // supervisors and the `os dev` parent never have to guess: + // `boundPort` here is the port the HTTP server actually bound, read back + // off the transport ({@link resolveBoundPort}) — ⛔ NOT `port`, the number + // this process was ASKED for. This comment used to claim `port` was the + // bound one, and it was not: the two part company whenever `listen()` + // CHOSE the port rather than accepting it — under `--port 0` always, and + // whenever the transport's own bind walked past a port taken between this + // command's probe and that `listen()`. (Past a DEV auto-shift `port` is + // already right, because `getAvailablePort()` reassigned it; that is the + // reading under which the old sentence looked true.) Publish the bound + // one so supervisors and the `os dev` parent never have to guess: // • IPC: when spawned with an 'ipc' channel (as `os dev` does), the // parent learns the real port without polling. // • runtime.json: a small state file under OS_HOME for external // supervisors / health checks (pid + port + url). - const runtimeUrl = `http://localhost:${port}`; + const runtimeUrl = `http://localhost:${boundPort}`; try { if (typeof process.send === 'function') { - process.send({ type: 'objectstack:listening', port: Number(port), url: runtimeUrl }); + process.send({ type: 'objectstack:listening', port: boundPort, url: runtimeUrl }); } } catch { /* IPC channel closed — best-effort */ } try { @@ -4291,7 +4379,7 @@ export default class Serve extends Command { fs.mkdirSync(path.dirname(runtimeFile), { recursive: true }); fs.writeFileSync(runtimeFile, JSON.stringify({ pid: process.pid, - port: Number(port), + port: boundPort, url: runtimeUrl, environmentId, startedAt: new Date().toISOString(), diff --git a/packages/cli/test/helpers/serve-process.ts b/packages/cli/test/helpers/serve-process.ts index 3147b0cb60..49b5d9ec05 100644 --- a/packages/cli/test/helpers/serve-process.ts +++ b/packages/cli/test/helpers/serve-process.ts @@ -383,8 +383,10 @@ function stripAnsi(text: string): string { * * ⚠️ It carries the bound PORT only conditionally, which is why the read-back * below has an `unreadable` state rather than a boolean. `serve.ts` builds the - * row from `resolveAuthBaseUrl(port).baseOrigin`, whose chain is `OS_AUTH_URL` - * → `BETTER_AUTH_URL` → `OS_BASE_URL` → `http://localhost:`. So a child + * row from `resolveAuthBaseUrl(boundPort).baseOrigin`, whose chain is + * `OS_AUTH_URL` → `BETTER_AUTH_URL` → `OS_BASE_URL` → `http://localhost:` + * — and `boundPort` is the port the transport reports it BOUND (#13062), which + * is the requested one for every value but `0`. So a child * carrying any of those three prints an origin that is NOT what it bound, and * an unparseable one prints paths with no origin at all. Measured on this tree * (`f28f00fbd`): nothing under `packages/cli/test` and nothing in the runner @@ -538,7 +540,7 @@ export function portDriftError( + 'does not name a `http://localhost:` address, so this harness cannot tell whether ' + `the child bound the ${requested} it was asked for.\n` + ` API row: ${readback.apiRow}\n` - + 'That row is `resolveAuthBaseUrl(port).baseOrigin` (`serve.ts`), so a child carrying ' + + 'That row is `resolveAuthBaseUrl(boundPort).baseOrigin` (`serve.ts`), so a child carrying ' + 'OS_AUTH_URL, BETTER_AUTH_URL or OS_BASE_URL prints THAT origin instead of the address it ' + 'bound — and this read-back channel goes with it.\n' + '⛔ Reported rather than skipped on purpose (#12525): a silent skip here is the same ' @@ -547,6 +549,30 @@ export function portDriftError( ); } + // ⭐ `--port 0` is a REQUEST for a kernel-assigned port, not an expectation + // about WHICH one — `utils/port-contract.ts` declares `MIN_PORT = 0` from its + // own measurement and states that 0 is "a REQUEST, not an error". A child + // asked for 0 therefore binds something else BY DESIGN, and reading that as + // drift would reject every healthy `--port 0` boot. + // + // ⛔ Not the silent skip this file's header forbids, either: there is exactly + // one answer such a boot can get wrong, and it is announcing the REQUEST back + // (#13062 — the banner, the IPC message and `runtime..json` all printed + // `localhost:0`, an address nothing was listening on). That one is reported; + // beyond it there is genuinely no comparison this harness can make. + if (requested === 0) { + if (readback.port !== 0) return null; + return new Error( + `ANNOUNCED PORT 0 on \`${what}\`: the child was asked for port 0 — a request for a ` + + 'kernel-assigned port — and its ready banner names `http://localhost:0`, which is not ' + + 'an address anything can listen on.\n' + + 'The banner is built from the port `serve.ts` PUBLISHES, so this is the #13062 defect: ' + + 'the requested port announced in place of the bound one. The same wrong number reaches ' + + 'the `objectstack:listening` IPC message and `runtime..json`.\n' + + `--- child output ---\n${output}`, + ); + } + if (readback.port === requested) return null; return new Error( diff --git a/packages/cli/test/serve-port-readback.e2e.test.ts b/packages/cli/test/serve-port-readback.e2e.test.ts index a8eac5ac9c..cab1a3ab11 100644 --- a/packages/cli/test/serve-port-readback.e2e.test.ts +++ b/packages/cli/test/serve-port-readback.e2e.test.ts @@ -226,18 +226,30 @@ describe('#12525: the child\'s REAL port is read back out of its own banner', () }); it('serve.ts still builds that row from the BOUND port, not the requested one', () => { - // ⭐ The load-bearing premise of the whole read-back. `port` is the - // variable `getAvailablePort()` may have shifted; `requestedPort` is what - // the harness asked for. If the banner were ever built from the latter, - // this check could never disagree with the harness and would be a - // phantom — green on every run, including the drifted ones. + // ⭐ The load-bearing premise of the whole read-back. `requestedPort` is + // what the harness asked for; `boundPort` is what the transport reports + // it BOUND. If the banner were ever built from the former, this check + // could never disagree with the harness and would be a phantom — green + // on every run, including the drifted ones. + // + // ⚠️ It used to read `resolveAuthBaseUrl(port)`, and `port` is only + // ALMOST the bound one: `getAvailablePort()` reassigns it past a dev + // auto-shift, which is the case this file drives, but it is still the + // number that was REQUESTED and it stays 0 under `--port 0` (#13062). + // The banner now reads the transport's own answer, so the premise below + // is the stronger one it was always meant to be. const serveSource = readFileSync(resolve(HERE, '../src/commands/serve.ts'), 'utf8'); expect(serveSource).toContain('port = await getAvailablePort(requestedPort)'); expect( serveSource, - 'the ready banner no longer derives its API row from `resolveAuthBaseUrl(port)` — ' + 'the ready banner no longer derives its API row from `resolveAuthBaseUrl(boundPort)` — ' + 'if it now uses the REQUESTED port, the #12525 read-back is vacuous by construction', - ).toContain('externalBaseOrigin: resolveAuthBaseUrl(port).baseOrigin'); + ).toContain('externalBaseOrigin: resolveAuthBaseUrl(boundPort).baseOrigin'); + expect( + serveSource, + 'the bound port is no longer resolved off the transport — `boundPort` is what the ' + + 'banner, the IPC message and runtime..json all publish (#13062)', + ).toContain('const boundPort = resolveBoundPort(kernel, port)'); }); }); diff --git a/packages/cli/test/serve-publishes-bound-port.e2e.test.ts b/packages/cli/test/serve-publishes-bound-port.e2e.test.ts new file mode 100644 index 0000000000..037059ea28 --- /dev/null +++ b/packages/cli/test/serve-publishes-bound-port.e2e.test.ts @@ -0,0 +1,400 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #13062 — the three channels `os serve` announces an address on all name the + * port it BOUND, driven through a real boot. + * + * ## What is under test, and why one file covers all three + * + * The IPC message (`objectstack:listening`), the ready banner's `API:` row and + * `runtime..json` were three outputs of ONE number, and that + * number was the port the operator ASKED for. ⛔ Repairing one of them and + * leaving two is the failure the card names explicitly — the survivors are + * harder to find afterwards, because the repaired one reads as the whole fix. + * So all three are read out of the SAME boot here, and compared to each other + * as well as to the socket. + * + * `--port 0` is the case where requested and bound can never coincide + * (`utils/port-contract.ts`: `MIN_PORT = 0`, "a REQUEST, not an error"), so it + * is the arm that goes red without the fix. The non-zero arms are the other + * half of the criterion and the easier one to break in passing: every ordinary + * boot must publish exactly what it published before. + * + * ## ⚠️ THE INSTRUMENT, AND ITS PROOF — the card demanded both + * + * The report this card was filed from could NOT confirm a bound port by + * observing sockets: `ss` sees no sockets at all in this fleet's container, + * verified there against a control server on a known port. ⇒ that instrument is + * VOID here, which is a different thing from it answering "no". A conclusion + * drawn from a void instrument is not a measurement. + * + * So this file uses a different one — a real client CONNECT — and proves it can + * answer in both directions before it is believed, in `describe('the + * instrument…')` below: it must reach a server this file starts on a port it + * learned from `address()`, and it must be REFUSED on a port that was probed + * free and left unbound. An instrument with only its positive arm shown is a + * shape that always succeeds. + * + * ## Cost, stated rather than hidden + * + * THREE real boots, each a `tsx` spawn (this package's measured floor is ~6s) + * plus a kernel boot. They buy the three arms of the acceptance criterion — + * `--port 0`, a free non-zero port, and a port taken out from under the boot — + * and none of them substitutes for another. The cheap half of the same + * criterion (which variable each publish site reads, and the resolver's whole + * fallback table) is `src/commands/serve-bound-port-publication.test.ts`, which + * answers in milliseconds. + * + * ## ⚠️ Process-group cleanup, and why it is not decoration + * + * A neighbouring card's e2e round killed only the direct child and left 13 + * orphaned `serve` processes behind, which took the container to 14.4GB of + * 16GB — and the run that followed failed on a TIMEOUT rather than an + * assertion, i.e. a false red about the code under test. Every child here is + * spawned `detached` and torn down by PROCESS GROUP, and each teardown is + * verified by the same connect probe going back to REFUSED. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { spawn, type ChildProcess } from 'node:child_process'; +import { createServer, connect, type Server } from 'node:net'; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { + CLI, + TSX, + boundPortFromBanner, + childEnv, + E2E_SECRET_KEY, + holdPort, + randomPort, + reservePort, +} from './helpers/serve-process.js'; + +/** The platform with no application — the cheapest fixture that reaches a ready banner. */ +const BARE_CONFIG = ` +export default {}; +`; + +/** + * `serve.ts`'s OWN default for the environment id (`OS_ENVIRONMENT_ID ?? + * 'env_local'`), which is what names the runtime state file. The variable is + * UNSET for every child below rather than pinned to a value of this file's + * own, so the name asserted here is the one a plain `os serve` writes. + */ +const RUNTIME_FILE = 'runtime.env_local.json'; + +/** How long a connect probe waits before calling a port unreachable. */ +const CONNECT_TIMEOUT_MS = 2_000; + +/** + * ⭐ THE INSTRUMENT: can a client actually reach `port`? + * + * A TCP connect, not a socket table — see this file's header for why the + * obvious instrument is void in this container. Resolves `true` on `connect`, + * `false` on any error or timeout, and never throws, so a caller reads one + * boolean rather than classifying errno. + */ +function reachable(port: number): Promise { + return new Promise((resolveProbe) => { + let settled = false; + const done = (answer: boolean) => { + if (settled) return; + settled = true; + socket.destroy(); + resolveProbe(answer); + }; + const socket = connect({ port, host: '127.0.0.1' }); + socket.setTimeout(CONNECT_TIMEOUT_MS); + socket.on('connect', () => done(true)); + socket.on('timeout', () => done(false)); + socket.on('error', () => done(false)); + }); +} + +interface Booted { + /** The `objectstack:listening` message, or `null` if the child never sent one. */ + ipc: { type?: string; port?: unknown; url?: unknown } | null; + stdout: string; + stderr: string; + /** Everything written under this child's `OS_HOME`. */ + home: string; + stop: () => Promise; +} + +/** + * Boot a real `os serve` with an IPC channel open, wait until it has BOTH + * printed its banner tail and sent its listening message, and hand back all + * three channels plus a teardown. + * + * The child is `detached` so it leads its own process group, and `stop()` kills + * that GROUP: `tsx` runs the CLI in a grandchild, so signalling the direct + * child alone is what leaves orphans behind (see the header). + */ +function bootServe(cwd: string, args: string[], home: string): Promise { + return new Promise((resolveBoot, rejectBoot) => { + const child: ChildProcess = spawn( + TSX, + [CLI, 'serve', 'objectstack.config.ts', ...args], + { + cwd, + detached: true, + // fd 3 is the IPC channel `os dev` opens on this same child, and the + // only way to read the `objectstack:listening` message at all. + stdio: ['ignore', 'pipe', 'pipe', 'ipc'], + // `childEnv`, never a bare `...process.env` — the runner's own + // `TEST` / `VITEST` variables reach into a spawned boot's auth and + // crypto posture otherwise (see the helper's header). + env: childEnv({ + NO_COLOR: '1', + OS_DATABASE_URL: ':memory:', + OS_LOG_LEVEL: '', + OS_DISABLE_CONSOLE: '1', + OS_SECRET_KEY: E2E_SECRET_KEY, + // The state file this test reads, kept out of the runner's real + // home directory and out of every other worker's way. + OS_HOME: home, + // UNSET, so the file name is the one a plain `os serve` writes. + OS_ENVIRONMENT_ID: undefined, + // UNSET: any of these replaces the banner's `http://localhost:` + // with an external origin, and the banner channel goes with it. + OS_AUTH_URL: undefined, + BETTER_AUTH_URL: undefined, + OS_BASE_URL: undefined, + }), + }, + ); + + let stdout = ''; + let stderr = ''; + let ipc: Booted['ipc'] = null; + let settled = false; + + const stop = (): Promise => + new Promise((done) => { + if (child.exitCode !== null || child.signalCode !== null) { + done(); + return; + } + child.on('exit', () => done()); + try { + // NEGATIVE pid = the whole process group. `tsx` runs the CLI in a + // grandchild; killing `child.pid` alone leaves it running. + process.kill(-(child.pid as number), 'SIGTERM'); + } catch { + try { child.kill('SIGTERM'); } catch { /* already gone */ } + done(); + } + }); + + const timer = setTimeout(() => { + if (settled) return; + settled = true; + clearTimeout(timer); + void stop().then(() => + rejectBoot(new Error( + 'serve never reported both a banner and a listening message.\n' + + `--- ipc ---\n${JSON.stringify(ipc)}\n--- stdout ---\n${stdout}\n--- stderr ---\n${stderr}`, + )), + ); + }, 220_000); + + const settle = () => { + if (settled) return; + // BOTH, deliberately: the banner tail proves the whole banner is in the + // buffer (writes to one stream are ordered), and the message proves the + // IPC channel spoke. Waiting on one and reading the other is how a green + // gets returned for a channel that never answered. + if (ipc === null || !/Press Ctrl\+C to stop/.test(stdout + stderr)) return; + settled = true; + clearTimeout(timer); + resolveBoot({ ipc, stdout, stderr, home, stop }); + }; + + child.stdout?.on('data', (d) => { stdout += String(d); settle(); }); + child.stderr?.on('data', (d) => { stderr += String(d); settle(); }); + child.on('message', (msg: any) => { + if (msg?.type === 'objectstack:listening') { ipc = msg; settle(); } + }); + child.on('error', (err) => { + if (settled) return; + settled = true; + clearTimeout(timer); + rejectBoot(err); + }); + child.on('exit', () => { + if (settled) return; + settled = true; + clearTimeout(timer); + rejectBoot(new Error( + `serve exited before announcing a port.\n--- stdout ---\n${stdout}\n--- stderr ---\n${stderr}`, + )); + }); + }); +} + +/** The three channels, read out of one boot. */ +function channelsOf(booted: Booted): { ipc: unknown; banner: unknown; runtimeFile: unknown } { + const banner = boundPortFromBanner(booted.stdout + booted.stderr); + const state = JSON.parse(readFileSync(join(booted.home, RUNTIME_FILE), 'utf8')); + return { + ipc: booted.ipc?.port, + banner: banner.state === 'bound' ? banner.port : banner, + runtimeFile: state.port, + }; +} + +let bareDir: string; +const homes: string[] = []; + +const newHome = (): string => { + const home = mkdtempSync(join(tmpdir(), 'os-bound-port-home-')); + homes.push(home); + return home; +}; + +beforeAll(() => { + bareDir = mkdtempSync(join(tmpdir(), 'os-bound-port-')); + writeFileSync(join(bareDir, 'objectstack.config.ts'), BARE_CONFIG, 'utf8'); +}); + +afterAll(() => { + if (bareDir) rmSync(bareDir, { recursive: true, force: true }); + for (const home of homes) rmSync(home, { recursive: true, force: true }); +}); + +describe('the instrument, before anything is concluded with it', () => { + let control: Server; + let controlPort: number; + + beforeAll(async () => { + control = createServer(); + await new Promise((ready) => control.listen(0, '127.0.0.1', () => ready())); + const address = control.address(); + if (address === null || typeof address === 'string') throw new Error('listen(0) gave no numeric address'); + // ⭐ `server.address()` in-process: the same reading `serve` now publishes, + // demonstrated here on a server this file owns. + controlPort = address.port; + }); + + afterAll(async () => { + await new Promise((closed) => control.close(() => closed())); + }); + + it('POSITIVE ARM: reaches a server this file started, on the port `address()` reported', async () => { + expect(controlPort).toBeGreaterThan(0); + expect(await reachable(controlPort)).toBe(true); + }); + + it('NEGATIVE ARM: is REFUSED on a port that was probed free and left unbound', async () => { + // Without this, `reachable()` could be a function that always says yes and + // every conclusion below would be vacuous. + expect(await reachable(reservePort())).toBe(false); + }); + + it('and it reports the control port UNREACHABLE once the control server closes', async () => { + const doomed = createServer(); + await new Promise((ready) => doomed.listen(0, '127.0.0.1', () => ready())); + const port = (doomed.address() as { port: number }).port; + expect(await reachable(port)).toBe(true); + await new Promise((closed) => doomed.close(() => closed())); + expect(await reachable(port)).toBe(false); + }); +}); + +describe('#13062 `os serve --port 0` — the request that can never be the answer', () => { + it( + 'announces the BOUND port on all three channels, and it is really listening', + async () => { + const booted = await bootServe(bareDir, ['--port', '0'], newHome()); + let announced = -1; + try { + const { ipc, banner, runtimeFile } = channelsOf(booted); + + // ⭐ The defect, stated as the three readings it produced: + // `{ port: 0 }`, `API: http://localhost:0/`, `"port": 0`. + expect(ipc, 'the IPC message still announces the REQUESTED port').not.toBe(0); + expect(banner, 'the ready banner still names http://localhost:0').not.toBe(0); + expect(runtimeFile, 'runtime.env_local.json still records port 0').not.toBe(0); + + // One number, not three that happen to be non-zero. + expect(banner).toBe(ipc); + expect(runtimeFile).toBe(ipc); + expect(Number.isInteger(ipc)).toBe(true); + + // …and the URL the IPC message carries agrees with its own port field, + // since that string is what `os dev` prints and hands to an AI client. + expect(booted.ipc?.url).toBe(`http://localhost:${ipc}`); + + // ⭐ THE MEASUREMENT the card asked for: the announced port is one a + // client can actually reach. The instrument above proved it can answer + // NO before this was allowed to mean YES. + expect(await reachable(ipc as number)).toBe(true); + announced = ipc as number; + } finally { + await booted.stop(); + } + + // ⚠️ Teardown really tore down — the same probe, the other way. This is + // the orphan check too: `tsx` runs the CLI in a GRANDCHILD, so a teardown + // that signalled only the direct child would leave a server still + // answering here (13 such orphans took this container to 14.4GB once). + expect(await reachable(announced)).toBe(false); + }, + 240_000, + ); +}); + +describe('#13062 the non-zero half — nothing an ordinary boot publishes may move', () => { + it( + 'publishes exactly the port it was asked for when that port is free', + async () => { + const asked = Number(randomPort()); + const booted = await bootServe(bareDir, ['--port', String(asked)], newHome()); + try { + const { ipc, banner, runtimeFile } = channelsOf(booted); + // ⛔ Byte for byte what these channels published before this change: + // requested and bound coincide here, and that is the whole population + // of ordinary boots. + expect(ipc).toBe(asked); + expect(banner).toBe(asked); + expect(runtimeFile).toBe(asked); + expect(booted.ipc?.url).toBe(`http://localhost:${asked}`); + expect(await reachable(asked)).toBe(true); + } finally { + await booted.stop(); + } + // The child is gone, so the port it held is gone with it. This is the + // orphan check as well: a surviving grandchild would still be listening. + expect(await reachable(asked)).toBe(false); + }, + 240_000, + ); + + it( + 'follows the DEV AUTO-SHIFT onto the port it really took, on all three channels', + async () => { + // The second way requested and bound part company on this command, and + // the one that is reachable without `--port 0`: the port is genuinely + // held, so `serve`'s own `getAvailablePort()` walks off it. ⛔ Nothing is + // simulated — this is the drift, produced. + const held = await holdPort(); + let booted: Booted | undefined; + try { + booted = await bootServe(bareDir, ['--port', String(held.port)], newHome()); + const { ipc, banner, runtimeFile } = channelsOf(booted); + + expect(ipc, 'the boot announced the port it could not have bound').not.toBe(held.port); + expect(banner).toBe(ipc); + expect(runtimeFile).toBe(ipc); + expect(await reachable(ipc as number)).toBe(true); + } finally { + if (booted) await booted.stop(); + await held.release(); + } + }, + 240_000, + ); +}); From d96701691572d3ff193e56be0d23fa4e0f96dac9 Mon Sep 17 00:00:00 2001 From: os-litant Date: Sat, 29 Aug 2026 05:12:18 +0000 Subject: [PATCH 2/2] test(cli): name the kernel by package, not by repo-relative path `check:cross-package-test-inputs` is a source scan and cannot tell a path in prose from one a test really opens, so a docblock spelling `packages/core/src/kernel.ts` read as an undeclared cross-package input. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd --- .../src/commands/serve-bound-port-publication.test.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/commands/serve-bound-port-publication.test.ts b/packages/cli/src/commands/serve-bound-port-publication.test.ts index 80017990bd..c413015981 100644 --- a/packages/cli/src/commands/serve-bound-port-publication.test.ts +++ b/packages/cli/src/commands/serve-bound-port-publication.test.ts @@ -61,9 +61,14 @@ const SERVE_RAW = readFileSync(resolve(HERE, 'serve.ts'), 'utf8'); * A kernel whose transport reports `reported` from `getPort()`. * * ⚠️ The miss path THROWS rather than returning `undefined`, because that is - * what `ObjectKernel.getService` really does (`packages/core/src/kernel.ts` — - * a miss is a composition fault it refuses to answer silently). A fake that - * returned `undefined` would leave the production `try` untested and green. + * what `ObjectKernel.getService` really does — a miss is a composition fault + * that `@objectstack/core` refuses to answer silently. A fake that returned + * `undefined` would leave the production `try` untested and green. + * + * ⛔ That kernel is named by PACKAGE, never as a repo-relative path: this file + * does not read it, and `check:cross-package-test-inputs` is a source scan that + * cannot tell a path in prose from one this test really opens (measured — the + * first draft of this comment failed that gate). */ function kernelReporting( reported: unknown,