Uh oh!
There was an error while loading. Please reload this page.
fix(cli): os serve writes the runtime state file before it announces the bound port - #13209
Conversation
…d port os serve announced its address on the ready banner and the objectstack:listening IPC message BEFORE it wrote runtime.<environment>.json, so every consumer that reacts to an announcement raced a file that did not exist yet. Publish through one ordered seam instead: publishBoundPort() drives the state file first, then IPC, then the banner. The channels are injected so the ORDER is observable, and a new deterministic test records the sequence and goes red if it is ever reversed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TvqBFLRzXdSPcbusDoED9k
📓 Docs Drift CheckThis PR changes 1 package(s): 17 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: ⛔ 4 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails. What this run could not see
Coarse fallback — 23 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin fa43e1cebe593501bd1fe13eeea9704d7e39b675 && git checkout fa43e1cebe593501bd1fe13eeea9704d7e39b675
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 96d8b20ee4f4894b4281a552dd5f7eee1c8bb0c5 35574c9dbb7aff8559f055961f96b44471be8b63 && git checkout -B drift-repro 96d8b20ee4f4894b4281a552dd5f7eee1c8bb0c5 && git merge --no-ff 35574c9dbb7aff8559f055961f96b44471be8b63
node scripts/docs-audit/affected-docs.mjs --json 96d8b20ee4f4894b4281a552dd5f7eee1c8bb0c5
|
The three pins that read serve.ts source text for the IPC call, the state-file literal and const runtimeUrl went red when #13193 folded those three publish sites into publishBoundPort(). The behaviour they guarded is intact, so they are rewritten UPWARD rather than relaxed: two channels are now driven through the seam and observed, the state file is read off disk (pid included), and the IPC leg is observed reaching process.send. The wiring half stays a source pin because run() is still un-enterable in process - but it is now one call site instead of three publish sites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TvqBFLRzXdSPcbusDoED9k
Uh oh!
There was an error while loading. Please reload this page.
…fix so PR CI runs on a clean preview)
Fixes#13193
Fixes#13158
Authored in Claude Code session
session_01TvqBFLRzXdSPcbusDoED9k(durable copy kept in the body, since editing a PR body rewrites the footer link).The ordering grep came first, and it decided the shape of the fix
The anchor's triage set one step ahead of any repair: establish whether the announcements precede the state-file write. Measured on
origin/main33184fd,packages/cli/src/commands/serve.ts:const boundPort = resolveBoundPort(kernel, port);printServerReady({ ... })— ready banner, ANNOUNCE #1process.send({ type: 'objectstack:listening', ... })— IPC, ANNOUNCE #2fs.writeFileSync(runtimeFile, ...)— the state file, WRITTEN LASTBoth announcements fire strictly before the file exists, so the race is structural, not environmental. That rules out the reader-side repair: a poll-with-deadline in the test would have hidden a live product defect, because a real supervisor that opens
runtime.env_local.jsonwhen the banner says "ready", and anos devparent that reacts to the IPC message, lose exactly the same race. Load never created it — a busy machine only widens it by descheduling the child between the announcement and the write.The corollary that explains the reported symptom:
bootServeresolves readiness on the banner tail AND the IPC message (test/serve-publishes-bound-port.e2e.test.ts:210), both of which precede the write. The e2e is an ordinary consumer, not a badly written test — which is why the only two arms that hit it are the two where bound port is not the requested port, i.e. the two for which the state file is the sole carrier of the truth.The change
serve.tspublishes through one ordered seam:The banner call became a thunk so the seam owns all three; nothing inside that literal changed and nothing in it is async, so deferring it is a pure move. Each leg keeps its own
try, so a state-file write that fails still cannot take the announcements down with it — a boot does not die because a supervision file could not be written.Reverse verification — the part that matters for a race
A race repair whose test passes once is not evidence, so the ordering is observed directly rather than raced for.
publishBoundPorttakes its channels as arguments;test/serve-bound-port-publish-order.test.tsrecords the sequence they are driven in and measuresexistsSyncat the instant each announcement fires — the exact predicate that was false in production.Ablation, with the fix committed first so the restore had a real reference point: the order inside
publishBoundPortwas reversed to the pre-fix banner/IPC/file sequence. Mutation proven on disk by blob hash (f77ec4e8to196993e3) and by marker counts, not by an editor exit code; the script carried atrapwith absolute paths.3 of 4 red, deterministically, with no load, no sleep and no retry —
expected false to be trueis the merge queue'sENOENTreproduced directly. The one test that stayed green is the payload-agreement one, which does not assert order. Restore proven bygit diff HEADempty plus a blob hash equal toHEAD— not by an exit code. Mutating source alone flipped the result, which also proves the pin resolvessrc/through its relative import, so nodist/is involved.Rework: #13062's publish pins, rewritten upward
The first push turned
Test Core (1/6)red — honestly, and on this PR's own diff.packages/cli/src/commands/serve-bound-port-publication.test.tscarries #13062's sibling pins, and three of them assert the source text ofserve.ts("read off the code", as the suite says). Folding the three publish sites into the seam removed the literal text they grepped for. Reproduced locally before touching anything: 3 failed, 16 passed, exactly the three reported.Neither escape was taken: nothing was skipped, deleted or weakened, and the old text was not reintroduced to satisfy a grep — that would have undone the ordering repair. The pins were rewritten against the new structure, which the refactor makes better assertable, because the seam takes its channels as arguments:
process.send(...)call; now drivesruntimeBoundPortChannels().announceListeningwithprocess.sendswapped for a recorder and asserts the message that really arrives. (forkspoolprocess.sendis the runner's own control channel. The swap is synchronous, one call, restored infinally.) A new pin also asserts the leg stays silent rather than throwing when no IPC channel is open.OS_HOMEand reads the file off disk, assertingport,url,environmentIdandpid(the supervisor contract that regex was really guarding).const runtimeUrl— that variable is gone. Replaced by the pin that still cannot be driven in-process: the one wiring site hands the seamboundPort, neverport. This is strictly stronger than before — there used to be three publish sites to get wrong, and there is now one, asserted by both a positivetoContainand apublishBoundPort(count of exactly 2 (declaration plus that single call).not.toContain('const runtimeUrl = ...${port}')had gone vacuous (the variable no longer exists, so it passed for the wrong reason). It is replaced by the live spelling of the same regression:not.toContain('publishBoundPort(port,').Suite now 21 passed, up from 19 — the rewrite adds coverage rather than trading it away.
Each rewritten pin proven able to fail
A pin that cannot fail is worse than the source-text pin it replaced, so all four were ablated against
serve.ts, each mutation proven on disk by blob hash and marker counts, each restored and the restore proven bygit diff HEADempty plus a blob equal toHEAD:publishBoundPort(port, ...)writeRuntimeState({ port: boundPort + 1, ... })pid: process.pid,deletedprocess.sendRecorded rather than hidden: the fourth ablation's first attempt matched zero anchors and was a no-op, so it produced no reading at all. It was re-run with a corrected anchor; the row above is from the run that actually mutated the file.
The ordering property stays in
test/serve-bound-port-publish-order.test.tsand the #13062 properties stay here — kept in separate files deliberately, so the two fail for different reasons and keep naming them.Gates
Re-derived for the enlarged 4-path change set with
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstackat head35574c9d. (It first refused — the recreated worktree was shallow, so it declined to fall back to a two-dot range that would have attributed other PRs' files to this branch; deepened, then re-derived.)Green, each read from the gate's own verdict line with the exit code captured before any pipe:
check:type-check-coverage,check:type-check-debt(31 ledger entries re-measured, 1570 raw errors, none above its recorded number, "surplus: none"),check:test-source-alias,check:cli-test-child-env,check:cross-package-test-inputs,check:comment-mask-adoption,check:route-envelope,check:published-files,check:nul-bytes,check:empty-changeset,check:objectui-changeset,check:page-declaration-shape,check:slot-lookup,check:logger-receiver-detach,check:objectql-double-limit,check:type-source-resolution,check:changeset-gate-self-tests,check:i18n,check:i18n-coverage,check:pm-half-states,check:engine-double-contract,check:where-matcher,check:query-options-erasure,check-keyed-text-bounds,check-undeclared-dep-imports,check-ci-filter-parity,check-plugin-teardown-shape,check-adr-0087-registration,check-changeset-no-major, pluspnpm --filter @objectstack/cli typecheck.check:type-check-debtfirst exited 1 — a refusal, not a ratchet failure: one workspace dependency had no builtdist, and the gate declines to "silently measure a DIFFERENT WORLD". That closure was built and the gate then returned the green above.check:i18n/check:i18n-coveragehit the same class of prerequisite earlier and were likewise cleared before being read.Declared narrowing, one gate:
check:dual-build-cjs-loadsis NOT MEASURED — it needs a full-workspacepnpm build, and the missingdistdirectories it names are unrelated packages. This diff adds no package and noexportsentry, so it cannot move that gate, and CI builds the whole workspace regardless.Tests at head
35574c9d:serve-bound-port-publication.test.ts21 passed ·serve-bound-port-publish-order.test.ts4 passed ·serve-publishes-bound-port.e2e.test.ts6 passed (the suite that was ejecting PRs) — 31 across the three. Regression sweep over the wholeserve-*source-test family plus the port-contract and bind-probe pins, since many tests readserve.ts: 32 files, 375 tests, all passing. Recorded honestly: the clitypecheckprogram excludes test files (tsc --listFilesreturns 0 hits for them), so that green says nothing about the test files; their type coverage rests oncheck:type-check-debt.Out of scope, not touched: #12884 remains open and is a different defect in the same family.