Skip to content

fix(devx): reserve the port sdui_pick_free_port hands out, instead of probing and letting go - #10217

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-10167-pick-free-port-toctou
Aug 20, 2026
Merged

fix(devx): reserve the port sdui_pick_free_port hands out, instead of probing and letting go#10217
os-zhuang merged 2 commits into
mainfrom
claude/issue-10167-pick-free-port-toctou

Conversation

@os-zhuang

@os-zhuangos-zhuang commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Fixes#10167

What was wrong, re-derived rather than taken from the card

sdui_pick_free_port bound a probe socket, closed it, and only then reported the
port free; the caller bound it afterwards. The scan walks base upward
deterministically, 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.

Measured on this tree before the change, eight concurrent callers from base 5180:

== phase 1: 8 concurrent callers scanning from base 5180 ==
8 5180
DISTINCT_PORTS=1 of 8
== phase 2: 8 concurrent callers pick-then-BIND ==
BIND_OK_COUNT=2
BIND_ERR_COUNT=6 # Error: listen EADDRINUSE 127.0.0.1:5180

After the change, the same construction:

 DISTINCT_PORTS=8 of 8
BIND_OK_COUNT=8
BIND_ERR_COUNT=0

One correction to the card's reading. The card explains the CI trace as the picker
handing two callers 5180. The trace also carried EADDRINUSE 127.0.0.1:5180 from an
unhandlederror event, and reproducing it here pins a more specific chain: the
collision test's own occupier lost its bind to a concurrent scanner and died, so the
following pick returned the occupied number legitimately — the port really was free
by then. The read-out (BUSY_PORT == PICKED_WITH_BUSY) then accused the picker of not
skipping a busy port, which is not what happened. Same root cause, different victim:
the harness was one of the TOCTOU's casualties, not its witness. Both halves are fixed
below.

The fix

A port is claimed before it is probed, in a registry every caller on the host
shares (${TMPDIR:-/tmp}/sdui-port-reservations, overridable via
SDUI_PORT_RESERVATION_DIR), and the claim outlives the function — it is released
when the claiming process dies, not when the function returns.

  • O_EXCL file creation is the mutual exclusion: exactly one caller wins a port, and
    the losers are told so there instead of discovering it at bind time in another
    process.
  • Claims whose owner process is gone are swept, with a floor (never collect a claim
    written moments ago) and a ceiling (a backstop for a dead owner whose pid number was
    reused).
  • flock wraps the scan where available. It is not what makes a claim exclusive —
    O_EXCL already is — it makes the sweep safe, the one step that unlinks a file
    another scanner may be creating. Missing flock degrades to "sound, minus the
    sweep's tie-break", never to a hard failure.

When the port is stolen anyway

Named plainly, because the reservation binds only callers that share the registry:

  1. Stolen by a cooperating caller — cannot happen any more. That is the whole fix,
    and it is every collision anyone has actually measured here.
  2. Stolen by a process outside the registry, before the probe — the probe fails,
    the picker hands the claim back and advances. This retry is asserted, not
    argued (STEAL_* below), because a hoarded claim would cost a port on every later
    run in the container. Measured: foreign listener on 5180PICKED=5181,
    LEFTOVER_CLAIM_ON_5180=no, CLAIM_ON_PICKED=yes.
  3. Stolen between the pick and the caller's own bind — nothing here can prevent
    that, and this PR does not change what happens: --strictPort plus
    sdui_wait_for_own_server still turn it into a loud failure whose remedy is to pin
    a free port with SDUI_DUMP_PORT. Converting that refusal into a vite respawn is
    a change to a contract the collision test deliberately pins, and belongs to its own
    decision rather than to this one.

Tests

gen-sdui-manifest-collision.test.ts gains the vacuity guard it was missing, plus the
race as an executed assertion:

  • BUSY_HELD — the occupier must really hold the port before "the picker skipped it"
    means anything. Its absence is why this file accused the wrong function.
  • CONCURRENT_TOTAL / CONCURRENT_DISTINCT — eight subshells, one base, at once.
  • BUSY_PORT / NPORT / OPORT distinct — two picks in one run cannot return one port.
  • STEAL_HELD / STEAL_PICK / STEAL_CLAIM_RELEASED, with STEAL_CLAIM_ON_PICK as
    its positive control — "no claim file for that port" is also what a registry that
    does not exist looks like, and the release assertion alone was measured green
    against the pre-fix script it was written to fail.

Both raw listeners in the harness also stopped throwing on a lost bind. An unhandled
error event killed the harness mid-measurement and buried the result under a stack
trace — which is exactly what the CI report was.

Ablation (fix reverted on disk, tests kept)

Mutation confirmed on disk before running, by grepping the text it removes and the text
it restores: sdui_scan_and_reserve_port=0, ADVISORY ONLY. It reserves nothing=1.
No rebuild is involved — these tests read scripts/gen-sdui-manifest.sh from the
worktree at run time, so there is no dist/ to go stale. Restoration confirmed the same
way (=3 / =0, clean git status against HEAD).

× hands concurrent callers scanning one base distinct ports CONCURRENT_DISTINCT: expected '2' to be '8'
× gives each pick within one run its own port 5180,5180,5180: expected 1 to be 3
× skips a port held from outside the registry ... STEAL_CLAIM_ON_PICK: expected 'no' to be 'yes'
× creates its output directory instead of dying on it (write-target's REAL path — see below)
Test Files 2 failed | 1 passed (3)
Tests 4 failed | 10 passed (14)

The fourth red was not planted. gen-sdui-manifest-write-target.test.ts drives the real
script, and on the unfixed picker it lost 5180 to the collision test running beside
it — the cross-file contention the card predicted, reproduced live rather than argued.
An earlier ablation round, before the harness was hardened, reproduced the merge-queue
signature verbatim: Error: Command failed: bash /tmp/sdui-collision-*/harness.sh /
Error: listen EADDRINUSE: address already in use 127.0.0.1:5180 under runHarness.

Note that PICKED_WITH_BUSY came back 5181 in every ablation round — the original
assertion stayed green on the broken picker. That is the card's point about a green run
not being the acceptance criterion, measured.

Green, at 7d865c1e

All three files together (--maxWorkers=3, so the contention is real and not staged):

 Test Files 3 passed (3)
Tests 14 passed (14)
os-verify-lock: VERDICT command-exit 0

Gates at 7d865c1e

Union derived by node scripts/pm/dispatch-gates.mjs with no path args (2 paths, merge
base da891e0e). 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:engine-double-contract · check:where-matcher ·
check:type-check-coverage · check:nul-bytes

Declared narrowing — two gates NOT MEASURED, and that is not a green.
check-dev-prereqs.mjs and the check:type-check-debt --re-measure ratchet both refuse
on an unbuilt worktree, and check-dev-prereqs here reports 67 of 67 workspace packages
missing their dist/ entry point — a property of the worktree, not of a diff whose two
files are a root shell script and one test file that imports nothing from the workspace.
Building the full closure would have held the container's shared verify lock against
three concurrently active agents (queue waits were already 5m21s during this run), which
is 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:

check:test-typecheck: OK — @objectstack/spec's test layer compiles under
packages/spec/tsconfig.test.json; 55 file(s) / 263 error(s) held in
test-typecheck-debt.json (shrink-only)

CI runs the whole farm regardless; this is the cheap half, not a substitute for it.

No changeset

Root scripts/ plus one packages/spec/scripts/*.test.ts. Nothing published changes
behaviour, so this carries skip-changeset.

Not addressed here

smoke_pick_free_port in scripts/publish-smoke.sh is the same defect, byte for byte
in shape, drawing from base 3210. It sits outside this card's declared file surface
(as does its own test file), so it is filed separately rather than fixed as a rider.
Filed as issue 10212 (referenced without a closing keyword on purpose).


Generated by Claude Code

… probing and letting go
`sdui_pick_free_port` bound a probe socket, closed it, and only then reported
the port free; the caller bound it afterwards. The scan is deterministic from
`base` upward, so concurrent callers did not diverge — they were handed the
same port, and the first one every time. Measured before this change, eight
concurrent callers scanning from 5180: DISTINCT_PORTS=1 of 8, and six of eight
lost the follow-up bind with `EADDRINUSE 127.0.0.1:5180` — the signature that
dequeued a PR from the merge queue.
A port is now claimed in a host-shared registry (`O_EXCL`, swept of claims
whose owner process is gone) before it is probed, and the claim outlives the
function. Two cooperating callers can no longer be handed one port. The probe
stays for processes outside the registry, and a lost probe hands the claim back
rather than hoarding it.
The collision test gains the vacuity guard it was missing — BUSY_HELD, so an
occupier that lost its own bind can no longer read as a picker that ignores
busy ports — plus the concurrency and foreign-steal assertions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt
…dying on it
Both raw listeners in the harness bound with no `error` handler, so a lost bind
killed node with an unhandled `error` event and took the harness's exit code
with it. That is why the merge-queue report was a stack trace
(`EADDRINUSE 127.0.0.1:5180` under `runHarness`) rather than a named
precondition — reproduced here on the pre-fix script before this change.
They now exit, so the vacuity guards (BUSY_HELD, STEAL_HELD) get to speak.
STEAL_CLAIM_ON_PICK joins STEAL_CLAIM_RELEASED as its positive control:
"no claim file for that port" is also what a registry that does not exist looks
like, so the release assertion alone was measured green against the pre-fix
script it was written to fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing 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-zhuangos-zhuang added skip-changeset PR has no user-facing published change; bypasses the changeset gate and removed size/m tests tooling labels Aug 20, 2026 — with Claude
@os-zhuang
os-zhuang marked this pull request as ready for review August 20, 2026 14:23
@os-zhuang
os-zhuang added this pull request to the merge queueAug 20, 2026
Merged via the queue into main with commit 318f96aAug 20, 2026
41 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-10167-pick-free-port-toctou branch August 20, 2026 14:47
os-zhuang pushed a commit that referenced this pull request Aug 20, 2026
…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
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changesetPR has no user-facing published change; bypasses the changeset gate

Projects

None yet

2 participants

@os-zhuang@claude