Uh oh!
There was an error while loading. Please reload this page.
test(cli): catch the silent port drift in three serve e2e spawners - #12552
Merged
Conversation
The three spawners pass `--dev`, and serve.ts reads `portAutoShiftAllowed = flags.dev || NODE_ENV === 'development'` — flags.dev alone opens the auto-select branch. So a port taken between randomPort()'s bind probe and the spawn does not fail the boot: getAvailablePort() hops the child onto another port and it reports itself READY, after which each file's own requests reach whatever else holds the port it reserved. Read the bound port back out of the child's OWN output and refuse to hand back a boot that drifted. Also correct the classification in serve-node-env-production-default's docblock, which called these three "this file's own shape" — that sentence is what put them in the loud column. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd
…rt-contention-legibility
Contributor
📓 Docs Drift CheckNothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs. What this run could not see
Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
This was referenced Aug 26, 2026
os-litant
marked this pull request as ready for review
August 26, 2026 10:35
This was referenced Aug 26, 2026
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#12526
These three are in the SILENT-DRIFT column, not the loud one
The card originally asked for
portContentionError()at these three sites, on the premise that they run in production posture where a taken port is a hardexit 1. Measured onorigin/main, that premise does not hold, and the PM withdrew it (ruling 1's action and ruling 3's classification are withdrawn; this PR implements the re-aimed fix).All three spawn
serve -p PORT --dev.packages/cli/src/commands/serve.tsreads:flags.devalone opens the auto-select branch, whateverNODE_ENVis — andserve.tsalso assignsNODE_ENV=developmentin-process for--dev. So a port taken betweenrandomPort()'s bind probe and the spawn does not fail the boot:getAvailablePort()hops the child onto the next free port and it reports itself READY.portContentionError()lives in the child'sexithandler, which never runs, so adding it here would have been dead code — a fix that passes every gate and changes nothing.That is the strictly worse direction: a green boot on the wrong port. Every request each file makes afterwards goes to the port it reserved and no longer owns, so it measures whatever else took it.
Only
serve-node-env-production-default.e2e.test.ts— which spawns without--dev, so unsetNODE_ENVdefaults to production — is in the loud column whereportContentionError()has something to read.Not a regression from #11707 / #12459
The
--devresponsible has been on all three spawn lines since83e6016fa, long predating #11707 / #12459, which changedNODE_ENVand never touched it. This was never read, not newly introduced. Same for the classification error itself, which is inherited rather than new.What this changes
Each of the three files now reads the bound port back out of the child's own output — never out of what the harness reserved, which is #12441 ruling 2's principle applied to the failure that actually occurs — and refuses to hand back a boot that drifted. Two patterns, because either alone can be absent: the structured
HTTP server started successfullylog obeysOS_LOG_LEVEL, and the banner'sAPI:line is what survives when it does not.Resolution is now gated on having seen that port. That is part of the check, not an optimisation: resolving on
waitForalone would let a boot through before the child had said which port it took, and the drift check would then be a no-op on an already-settled promise.Also corrected: the
THEY REACH IT NOWparagraph inserve-node-env-production-default.e2e.test.ts, which called these three "this file's own shape". That sentence is the seed of the misclassification — leaving it standing guarantees the next reader re-derives the same wrong column from the tree.packages/cli/test/helpers/serve-process.tsis deliberately untouched (held by #12525), andos serve's auto-shift rule is unchanged — auto-shifting in development is correct, and #11113 pins the production half.Proof the check fires
Held the port the file reserves with an HTTP neighbour, then ran the real suite. The child auto-shifted and the harness refused it:
Ablation discipline: the mutation was proven on disk before the run (
git hash-objectbefore6a845fa6/ afterafd25a47, injected marker count 1,port = randomPort();count 0), restored under anEXIT/INT/TERMtrap pinned to an absolute path, and the restore proven by an emptygit diff HEADplus the marker's absence. Without the fix this run is a green boot on port 37168 while the assertions talk to 37167.Verification — all on the final commit
36b472381pnpm --filter '@objectstack/cli...' build(package and closure) —VERDICT command-exit 0lint.ymldoes:check-type-check-coverage --re-measure: OK — 32 ledger entr(ies) re-measured in 223.8s, 1843 raw tsc error(s) total, none above its recorded numberscripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack(24 families):check:cli-test-child-env,check:test-source-alias,check:type-check-coverage,check:type-check-debt,check:cross-package-test-inputs,check-comment-mask-adoption,check-empty-changeset,check-ci-filter-parity,check-plugin-teardown-shape— all exit 0, each captured before any pipeTwo declared readings rather than silent claims:
pnpm --filter @objectstack/cli typecheckdoes NOT cover these files.packages/cli/tsconfig.jsonis"include": ["src"], and--listFilesfinds 0 of 4 edited files in that program. Its green says nothing about this diff. Real coverage came from compiling the four files directly under the same strict options: 4/4 in program, 0 errors, confirmed with--listFiles.eslint --no-inline-config --format json, 4 files linted, 0 errors, 0 warnings) rather than the repo-widepnpm lint. Type-aware linting is not enabled, so this diff cannot move the verdict on any untouched file; the full-repo sweep is CI's run.Test-only diff (four files under
packages/cli/test/), so no changeset —skip-changeset.Generated by Claude Code