Uh oh!
There was an error while loading. Please reload this page.
fix(devx): reserve the port smoke_pick_free_port hands out, instead of probing and letting go - #10263
Merged
Merged
Conversation
…f probing and letting go `smoke_pick_free_port` bound a probe socket, closed it, and only then reported the port free; `objectstack dev` bound it afterwards. The scan walks `base` upward deterministically, so concurrent callers were handed the same port — the first one, every time. Ports are now CLAIMED before they are probed, in a host-shared registry, and the claim outlives the function. Ports the same design PR #10217 landed for `sdui_pick_free_port` (#10167), keeping this script's wildcard probe, which matches serve.ts isPortAvailable(). Part of #10212 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt
…oke-pick-free-port-toctou
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. |
os-zhuang
marked this pull request as ready for review
August 20, 2026 15:14
os-zhuang
enabled auto-merge
August 20, 2026 15:14
Uh oh!
There was an error while loading. Please reload this page.
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#10212
What was wrong, re-derived rather than taken from the card
smoke_pick_free_portbound a probe socket, closed it, and only then reported theport free;
objectstack devbound it afterwards. The scan walksbaseupwarddeterministically, so concurrent callers did not diverge — they were handed the same
port, and the first one every time. Check-then-use, with the "use" in another process.
Both of the PM's mechanism assumptions were measured and both hold: the probe closes
before reporting, and the collision test draws from base
3210exactly four times(
FIRST,SECOND, and the twoREQUESTEDs).Measured on this tree before the change, eight concurrent callers from base
3210:After the change, the same construction:
The distinct-count moved between rounds on the unfixed script (1 of 8, then 2 of 8)
while
BIND_ERR_COUNTstayed at 6 — the collision is by construction, its exact shapevaries with load. That is the property that let it survive: it is never absent, only
sometimes cheaper.
The first repro harness computed
DISTINCT_PORTSbycat-ing the eight result filestogether. The picker prints without a trailing newline, so all eight numbers landed
on one line and the count read
1 of 8— for the fixed script too. The instrument, notthe picker, was reporting. It is fixed above and in the test (each file read separately),
and the same trap is commented at the assertion.
The fix
Ported from #10217 (#10167), which landed on
mainat318f96aewhile this card was inflight. I read the merged source, not the card's summary — and to show the port is
faithful rather than approximate, here is a diff of the two
nodeprograms with thesdui/smokenaming normalised away:Everything else —
O_EXCLclaim, release-on-probe-failure, the sweep with its floor andceiling, pid liveness,
flockaround the sweep only — is byte-identical. The threedifferences are deliberate:
card: it is the spelling
serve.ts's ownisPortAvailable()uses, so this scriptsees a busy port exactly when the CLI would. Copying the sdui helper's
'127.0.0.1'would have been the silent-divergence failure, not the faithful port.
sdui-prefixed. Explained below.So a port is claimed before it is probed, in a registry every caller on the host
shares (
${TMPDIR:-/tmp}/objectstack-port-reservations, overridable viaSMOKE_PORT_RESERVATION_DIR), and the claim outlives the function — released whenthe claiming process dies, not when the function returns. Verified that the claim file
records the caller's pid and not the
nodechild's:CLAIM_OWNER=16154/MY_PID=16154.The one design decision the card asked to be made deliberately: one registry or two
The card asked whether the two helpers should share a registry. They now run the same
protocol against two directories, and this PR chose not to converge them here:
[3210, 3410)vs[5180, 5380)— so they cannot handeach other the same port. Sharing buys nothing measurable today.
a parameter, not a constant. That is a refactor, not a rename.
scripts/gen-sdui-manifest.shand its collision test — bothoutside this card's declared file surface, hours after fix(devx): reserve the port sdui_pick_free_port hands out, instead of probing and letting go #10217 landed there.
This is not a second incompatible registry: the on-disk format is identical (filename
is the port, contents are the owner pid), and the directory name was chosen neutral
precisely so convergence is a pure rename on the sdui side with no migration. Recorded as
#10261 with the design sketch.
#10261 is not addressed here.Tests
publish-smoke-port-collision.test.tsgains the race as executed assertions:CONCURRENT_TOTAL/CONCURRENT_DISTINCT— eight subshells, one base, at once.CONCURRENT_TOTALis the vacuity guard: without it a picker that failed outrightwould score a perfect distinct-count of zero.
TRIPLE_DISTINCT— three picks in one run, nothing bound in between, must differ.STEAL_HELD/STEAL_PICK/STEAL_CLAIM_RELEASED, withSTEAL_CLAIM_ON_PICKasits positive control.
⭐ The blind-spot check the card asked for — and it is a different blind spot
The card asked whether this file's collision test carries the hole that made #10167's
PICKED_WITH_BUSYstay green on the broken picker. It does not, and that wasmeasured rather than assumed:
picks a per-run port and skips one that is already heldalready curls its occupier (
HOLDER_REACHABLE), which is exactly theBUSY_HELDprecondition guard #10217 had to add. Nothing needed adding there.
But it is still blind to this card's defect, for an unrelated reason: it draws its two
ports sequentially, and a TOCTOU race is about callers that overlap in time. It stayed
green on the broken picker in both ablation rounds — correctly, because a second pick
taken after the first port is already held really does skip it. Sound, and silent on the
defect. That is why the new assertions use concurrency instead of strengthening it. Both
findings are written into the test file's header so the next reader does not have to
re-derive them.
Ablation (fix reverted on disk, tests kept), re-run at the merged head
No rebuild leg is involved, and that is a property of the tests rather than an
assumption:
const SCRIPT = path.resolve(..., 'scripts', 'publish-smoke.sh')— theharness reads the shell script from the worktree at run time, so there is no
dist/thatcan go stale. Mutation confirmed on disk before reading any result, anchored on the text
each side owns:
smoke_scan_and_reserve_port=0,It reserves nothing=1. Restorationconfirmed the same way (
=3/=0, cleangit statusagainst HEAD).STEAL_CLAIM_RELEASEDis vacuously green on the unfixed script — measured, notargued. The assertion at line 331 that fails is
STEAL_CLAIM_ON_PICK, the positivecontrol;
STEAL_CLAIM_RELEASEDon line 332 never even runs. Driving the same harnessbody directly against the unfixed script prints why:
Note also that
STEAL_PICK != STEAL_BASEpasses on the unfixed script. It is aregression guard, not a defect assertion, and is labelled as such.
Green, at
0983862aAll four port-drawing test files together at
--maxWorkers=4, so the contention betweenthe two registries is real and not staged:
The collision test alone was also run three times back to back —
Tests 8 passed (8)each time — because a concurrency assertion that passes once proves less than most.
One thing worth flagging that was NOT mine. Before merging
main, running those fourfiles together reproduced #10167 live on this branch:
gen-sdui-manifest-collision.test.tsdied with
Error: Command failed: bash /tmp/sdui-collision-*/harness.sh/Error: listen EADDRINUSE: address already in use 127.0.0.1:5180underrunHarness—the merge-queue signature verbatim — and
gen-sdui-manifest-write-target.test.tslost5180 beside it. Confirmed pre-existing by running the three sdui files without this
PR's test file in the run (same failures), and gone after merging #10217.
Gates at
0983862aUnion derived by
node scripts/pm/dispatch-gates.mjswith no path args, re-derived afterthe merge (2 paths, merge base
318f96ae). Exit codes captured before any pipe; verdictsquoted from what each gate printed, not from
$?. All green:check:cross-package-test-inputs·check-cross-package-test-inputs.mjs·spec check:empty-state·spec check:liveness·check:merge-driver·check:slot-lookup·spec check:strictness-ledger·check:type-source-resolution·spec check:variant-docs·docs-audit/check-affected-docs.mjs·check:query-options-erasure·check:type-check-coverage·check:engine-double-contract·check:where-matcher·check:nul-bytesDeclared narrowing — two gates NOT MEASURED, and that is not a green.
check-dev-prereqs.mjsand thecheck:type-check-debt --re-measureratchet both refuseon an unbuilt worktree;
check-dev-prereqsreports67 of 67 workspace packagesmissingtheir
dist/entry point, a property of the worktree, not of a diff whose two files are aroot shell script and one test file importing nothing from the workspace. Building the
full closure would have held the container's shared verify lock against the other agents
in this round — the same shared-resource harm this card is about. The risk the ratchet
covers for this diff is that the new TS does not typecheck or grows the debt, and that was
measured directly instead:
263 is unchanged from what #10217 measured on the same ledger, so the new test code added
none. CI runs the whole farm regardless; this is the cheap half, not a substitute for it.
Release fence
Untouched, and deliberately so. This PR changes only how a port is picked; it changes
nothing about what the smoke publishes. No
changeset publish, nopnpm run release,no tag push, no publish-capable
workflow_dispatch. The diff is one shell function plusits test.
No changeset
Root
scripts/plus onepackages/spec/scripts/*.test.ts. Nothing published changesbehaviour, so this carries
skip-changeset.Generated by Claude Code