Skip to content

test(cli): bind-probe the e2e serve port and name a lost race - #12523

Merged
os-litant merged 4 commits into
mainfrom
claude/issue-12441-e2e-port-bind-probe
Aug 26, 2026
Merged

test(cli): bind-probe the e2e serve port and name a lost race#12523
os-litant merged 4 commits into
mainfrom
claude/issue-12441-e2e-port-bind-probe

Conversation

@os-litant

Copy link
Copy Markdown
Collaborator

Fixes#12441

Three packages/cli/test sites drew the os serve 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 in this fleet, and a measured full-suite run went 1 failed / 2101 passed on ✗ Port 49402 is already in use, clean on an isolated re-run. Test-only change.

What landed

① One draw, and it is a real bind probe.reservePort() in test/helpers/serve-process.ts listens on 0.0.0.0:0, reads the port the kernel assigned, closes, and hands it over. The two overlapping inline ranges are gone — serve-node-env-production-default (41000-60000) and serve-app-anchored-optional-import (40000-60000) could collide with each other under --maxWorkers > 1, so three independent draws was itself half the defect. randomPort() survives as String(reservePort()) for the 8 other files in this directory that pass a port straight into an argv list; renaming it would be pure churn across files this card does not own.

② The docblock states the residual race instead of denying it. It says what the mechanism buys — ports already held for a neighbour's whole lifetime are off the table, and the window narrows from "the whole run" to "one close-to-spawn gap" — and that it is still TOCTOU. It also states the one property that is worse than the old range (the kernel draws from the ephemeral range, 32768-60999 here, shared with outbound source ports) rather than hiding it.

③ ⭐ A lost race now fails legibly.portContentionError() turns serve exited 1 before "Server is ready" into a failure naming port contention and the port, wired into all three spawn sites. The port is read out of the child's own diagnostic, not out of what the harness thought it reserved. This is the half that pays: the measured cost was never the lost run, it was an unreproducible red suite on a file the reader has no reason to connect to a port.

④ Unchanged, deliberately.os serve's production no-auto-select rule (#11113) is correct and untouched; no port is pinned.

The probe is an instrument — proved, both directions

test/serve-port-bind-probe.test.ts (new) is the pin, 9 tests, 1.29s. It spawns nothing but the probe's own node -e child.

  • Negative arm (load-bearing). Hold a port open in-process; portIsFree(held) must answer false; after release, true. A probe that reports "free" for a port you are holding is not an instrument, and every claim the docblock makes rests on this.
  • Ablation, and it reds. Mutating probeBind to s.listen(0, …) — i.e. a probe that ignores the port it was asked about — was confirmed on disk (anchor occurrences 1 → 0, injected 1, blob 35e0e2aad97a2123) under a restore trap, and the run went 1 failed / 7 passed with exactly AssertionError: the bind probe called held port 35307 FREE — it is not an instrument. Restore verified by blob equality to HEAD and an empty git diff HEAD.
  • Prevents the shape it claims to. 12 ports held open, 12 probed draws, zero overlap — with a positive control proving the overlap comparison can detect a held port at all.
  • Anti-vacuity on the legibility half. The measured stderr is a transcript, and a transcript cannot notice that serve.ts was reworded. One test reads the live Port ${requestedPort} is already in use template out of src/commands/serve.ts and asserts the detector fires on it, so the day that wording changes this reds instead of going quietly illegible.

Verification

Union re-run at the final commit 8f8c4654f, all exit 0:

check:nul-bytes · check:cli-test-child-env · check:cross-package-test-inputs · check:test-source-alias · check:type-check-coverage · check:published-files · check:slot-lookup · check:page-declaration-shape · check:type-source-resolution · check:objectql-double-limit · check:engine-double-contract · check:where-matcher · check:query-options-erasure · check-ci-filter-parity · check-comment-mask-adoption · check-plugin-teardown-shape · docs-audit/check-affected-docs

Gate list derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack from the real change set (4 paths), not from a hand-written diff.

check:cli-test-child-envcaught a real finding mid-work: the probe's first draft passed { ...process.env, NODE_OPTIONS: '' } to execFileSync, a bulk copy into a spawn (0 → 1 over this file's ceiling). Repaired at the source — the probe child now gets env: {}, since a bare net bind reads no variable at all. Measured side effect: ~74.7 ms/draw → ~38 ms/draw.

pnpm lint narrowed, and the narrowing is declared with its three pieces of evidence. ① Population read from ESLint's own config (not from a guess about which files count): all 4 changed paths came back linted, none ignored. ② File count read from --format json: 4 files, 0 errors, 0 warnings. ③ Invariance for untouched files: this repo runs one eslint.config.mjs which never enables type-aware linting for any file (no parserOptions.project, no typed @typescript-eslint rules — stated and positive-controlled in the config itself at line 328), so no edit in this diff can move any untouched file's verdict. The repo-wide scan is CI's run.

⚠️pnpm --filter @objectstack/cli typecheck is green and says NOTHING about this PR.packages/cli/tsconfig.json is include: ["src"], so test/** is in no tsc program — measured, 0 of the 4 edited files appear in tsc --listFiles. That layer is ledgered (TEST_DEBT['@objectstack/cli'] = 146) and re-measured by check:type-check-debt --re-measure, which needs the whole workspace closure built. Measured the delta directly instead, same compilerOptions, HEAD vs merge base e57744536: 0 errors on these 4 paths in both trees. The first attempt at that measurement was vacuous — tsc bailed with TS5112 and compiled nothing while reporting a clean-looking 0; caught by a positive control (planting const x: number = portIsFree(1) produced no finding). Re-run with --ignoreConfig, the control produces TS2322 as it must, and the 0 above is from that instrument.

Not run locally, named rather than implied:check:i18n / check:i18n-coverage (need packages/cli/dist, prerequisite not met), check:type-check-debt --re-measure (needs the full built closure), and the two heavy e2e files this touches — serve-node-env-production-default and serve-app-anchored-optional-import are among the 20 real-CLI spawners that are 56.1% of this package's file wall. The shared verify lock was held continuously by two sibling cards for ~37 minutes across three attempts (exit 99, NOT MEASURED, holder named each time); the pin itself was narrowed out of the lock on measurement — 906 ms of test body, single worker, no build — and that narrowing is declared here rather than left silent.

For the seat, not folded in

  • packages/cli/vitest.config.ts is claimed concurrently by packages/cli/vitest.config.ts's suite-cost section is a 2026-08-20 measurement whose population changed under it — and it carries two inconsistent file/test vintages in adjacent sections #12499 and is untouched. Checked whether this change makes anything in its cost header stale: it does not. Its measurements are dated (2026-08-20) rather than live claims, and ~14 draws × ~38 ms is inside the noise of a 495.8 s wall.
  • Three more spawn sites keep the illegible failure and are outside this card's ruled file surface: serve-mcp-stdio-answers.e2e.test.ts, serve-mcp-capability-collision.e2e.test.ts, serve-stdio-stdout-purity.e2e.test.ts. They get the bind probe (they call randomPort()), but they spawn bin/run.js directly with NODE_ENV unset — production posture, hard exit 1 on a taken port — without routing through runServe(), so portContentionError() never sees their output.
  • A second, quieter contention shape exists in the runServe() path and is not fixed here: bin/run-dev.js pins NODE_ENV=development, so serve.ts takes portAutoShiftAllowed and getAvailablePort() drifts the child to a different port with no diagnostic at all. runServe() never learns the child's real port, so a test that then talks to the port it asked for reaches whatever else holds it.

Generated by Claude Code

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
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
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

8 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 23 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 14b114553c3e7c61672e3d25a1d15036f7561e27packageMentionDocs.

Which tree this was computed on

This run read content/docs from 06ea8bcf997e0d5a2c221644be1cb6895e6b62e0 — the merge of head 8f8c4654fee0ee136d550358e739fc4dc48b294f into base 14b114553c3e7c61672e3d25a1d15036f7561e27, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 06ea8bcf997e0d5a2c221644be1cb6895e6b62e0 && git checkout 06ea8bcf997e0d5a2c221644be1cb6895e6b62e0
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 14b114553c3e7c61672e3d25a1d15036f7561e27 8f8c4654fee0ee136d550358e739fc4dc48b294f && git checkout -B drift-repro 14b114553c3e7c61672e3d25a1d15036f7561e27 && git merge --no-ff 8f8c4654fee0ee136d550358e739fc4dc48b294f
node scripts/docs-audit/affected-docs.mjs --json 14b114553c3e7c61672e3d25a1d15036f7561e27

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@os-litantos-litant added skip-changeset PR has no user-facing published change; bypasses the changeset gate and removed tests labels Aug 26, 2026 — with Claude
@os-litant
os-litant marked this pull request as ready for review August 26, 2026 08:39
@os-litant
os-litant added this pull request to the merge queueAug 26, 2026
Merged via the queue into main with commit f28f00fAug 26, 2026
40 checks passed
@os-litant
os-litant deleted the claude/issue-12441-e2e-port-bind-probe branch August 26, 2026 09:00
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/lskip-changesetPR has no user-facing published change; bypasses the changeset gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

packages/cli e2e tests pick a serve port by blind Math.random() with no bind probe — the comment claims it "never contends", and it did

2 participants

@os-litant@claude