Skip to content

test(spec): let the publish-smoke THIEF own the port it calls held - #10684

Merged
os-elon merged 1 commit into
mainfrom
claude/issue-10457-publish-smoke-thief-bind
Aug 21, 2026
Merged

test(spec): let the publish-smoke THIEF own the port it calls held#10684
os-elon merged 1 commit into
mainfrom
claude/issue-10457-publish-smoke-thief-bind

Conversation

@os-elon

Copy link
Copy Markdown
Collaborator

Fixes#10457

Ports the shape PR #10456 landed for the sdui sibling. That PR is the settled direction for this defect class; this is the same change one file over, with the one deliberate divergence noted under Fences.

What changed

The "skips a port held from outside the registry" case staged its holder by picking a port, deleting the claim, then binding it:

STEAL="$(smoke_pick_free_port 3210)"
rm -f "$SMOKE_PORT_RESERVATION_DIR/$STEAL"# claim released ON PURPOSE
STUB_PORT="$STEAL" STUB_NAME=THIEF node -e "$STUB">/dev/null 2>&1& THIEF=$!
sleep 1

Between the rm -f and the stub's listen the port is claimed by nobody, so any concurrent smoke_pick_free_port caller scanning from 3210 may legitimately take it. Losing there does not report as "the precondition evaporated" — it reports as STEAL_HELD != THIEF, which reads as an accusation of the picker on a PR that never touched it.

Now the stub binds :0 and reports back the port the kernel gave it, printed from inside the listening callback, so by the time the harness can read the number the socket is already held. The pick-then-rm dance is gone, and the fixed sleep 1 with it — waiting for the number is waiting for the hold, so there is no second thing to wait for and no fixed sleep to be wrong about on a loaded container.

The registry property gets plainer, not weaker: a port bound straight from the ephemeral range was never claimed in any registry, so there is no claim to remove. Being outside the registry is now a property of how the thief got the port rather than something staged by deleting a claim.

Fences, each checked

  • The probe-address asymmetry is preserved, not converged.OWNED_HTTP_STUB takes no host argument, unlike the sdui sibling's s.listen(0, "127.0.0.1", …). smoke_pick_free_port's own probe binds the wildcard address — scripts/publish-smoke.sh says so in place: "No host argument: the wildcard bind is the spelling serve.ts's own isPortAvailable() uses" — so an occupier here must hold the wildcard address for that probe to see it at all. The only 127.0.0.1 this diff adds is the word inside the docblock that explains why it is absent. Two port-reservation registries now run the same protocol in different directories — converge sdui_pick_free_port and smoke_pick_free_port onto one helper #10261 is untouched, and stays open.
  • The set +e +o pipefail half was not touched. It is present at what is now :184, byte for byte, with its original comment.
  • H17: neither scripts/publish-smoke.sh nor scripts/gen-sdui-manifest.sh is in this diff.git diff --name-only origin/main...HEAD returns exactly one path. Both shell scripts were read only.
  • The existing assertions are untouched, byte for byteSTEAL_HELD, STEAL_PICK, STEAL_CLAIM_ON_PICK, STEAL_CLAIM_RELEASED. The change makes the precondition satisfiable, never optional.

STEAL_CLAIM_RELEASED still exercises the path it always did, and for a better reason. The picker claims the base it is asked to scan, probes it, finds it busy, and hands the claim back; previously the rm -f was what made that claim attempt succeed, and now nothing ever claimed the ephemeral port in the first place.

One assertion added, and why it is not ceremony

expect(r.STEAL_BASE).toMatch(/^\d+$/);

The base is now reported by the thief rather than returned by the picker, so "is it a number at all" is a question this case did not used to have. It closes a vacuity hole the change would otherwise open: with STEAL empty, smoke_pick_free_port falls back to its own 3210 default (local base="${1:-3210}") and STEAL_PICK would then differ from STEAL_BASEfor free.

No analogue of the sibling's RPORT repair is needed here. That was required because sdui's gives each pick within one run its own port counted BUSY_PORT among three picker outputs, and an ephemeral port differs from the 5180 band however the registry behaves. This file's equivalent test draws A/B/C from base 3210 and never referenced the thief's port, so it is untouched and stays at full strength.

Evidence

The case runs green, and so does the package. Full suite at this PR's HEAD a57228046:

pnpm --filter @objectstack/spec exec vitest run scripts/publish-smoke-port-collision.test.ts
Test Files 1 passed (1) · Tests 8 passed (8)
VERDICT command-exit 0
pnpm --filter @objectstack/spec test -> VERDICT command-exit 0
Test Files 416 passed (416) · Tests 11097 passed (11097)
pnpm --filter @objectstack/spec run typecheck -> VERDICT command-exit 0
check:test-typecheck: OK — 55 file(s) / 263 error(s) held in test-typecheck-debt.json (unchanged)

Positive control on the vacuity guard, after the change. Ablation: make the thief bind, close, then report — naming a port it does not hold. The guard fires, and fires first:

AssertionError: expected '' to be 'THIEF' // Object.is equality
❯ scripts/publish-smoke-port-collision.test.ts:389:26
389| expect(r.STEAL_HELD).toBe('THIEF');
Test Files 1 failed (1) · Tests 1 failed | 7 passed (8)

Firing first is the point. With the port released, smoke_pick_free_port returns it and STEAL_PICK === STEAL_BASE — which without the guard reads as "the picker hands back busy ports", the exact misdiagnosis this file's header warns about. The guard names the real problem instead.

No rebuild is involved in either leg, and this was checked rather than inherited from the sibling PR. The file's complete import list is vitest plus node:child_process/node:fs/node:os/node:path/node:url — no @objectstack/* specifier, so nothing resolves through a package exports/dist, and the script under test is sourced by absolute path. pnpm --filter '@objectstack/spec^...' build reports No projects matched the filters, corroborated by packages/spec declaring no workspace dependency at all.

Both ablation legs were confirmed on disk by grepping for the injected and the removed spelling, never by the editor's exit code: mutation leg removed=0 injected=1, restore leg removed=1 injected=0, and the restore ran from a trap … EXIT INT TERM so a mid-run kill could not leave a mutated tree behind. The tree is clean at a57228046.

A finding this turned up, filed rather than absorbed

The first ablation attempt failed in an unexpected shapeError: Command failed: bash /tmp/publish-smoke-collision-*/harness.sh at runHarness, with the assertion never reached. Cause: this harness has no trailing exit 0, so its exit status is whatever the last command returned, and every case ends with a cleanup kill that returns 1 against a stub which has already exited.

This partially falsifies the card's "Not in this finding" note. The note is right that the set +e +o pipefail half is already here — but that is only the first half of #10370's reporting fix; PR #10456 landed set +eand a final exit 0, and this file has only the former. Filed as #10671 with the measurement, and left out of this diff: the card fences the reporting machinery off, and the second ablation above isolates the guard from it rather than absorbing a fix for it. #10671 stays open.

After this change the THIEF stub binds :0 and cannot realistically lose its bind, so this case no longer trips that. The file's four other stubs still bind ports handed to them by the picker, so it remains live for them.

Changeset

None owed, re-derived rather than assumed.@objectstack/spec publishes files: ["dist","json-schema","liveness","prompts","llms.txt","README.md","src/**/*.zod.ts","CHANGELOG.md","api-surface","spec-changes.json"]scripts/ is not among them, so this file reaches no published surface. Tests-only in this repo goes through the skip-changeset label rather than an empty changeset (the label exists here as a real mechanism: "PR has no user-facing published change; bypasses the changeset gate"), and it is applied to this PR.

Gates

Re-derived against the real diff with node scripts/pm/dispatch-gates.mjs (it took the change set from the merge base itself, 9dd192d48), all run at a57228046 on a clean tree:

check:nul-bytes 0 · check:merge-driver 0 · check:slot-lookup 0 · check:test-source-alias 0
check:type-source-resolution 0 · check:engine-double-contract 0 · check:where-matcher 0
check:query-options-erasure 0 · check:type-check-coverage 0 · docs-audit/check-affected-docs 0
spec: check:empty-state 0 · check:liveness 0 · check:strictness-ledger 0 · check:variant-docs 0

Verdict lines rather than bare exit codes, e.g. check-test-source-alias OK — 72 packages with tests scanned; check-engine-double-contract: OK — 371 pinned, 133 in the DEBT ledger, 2 exempt; ✓ slot-lookup ratchet holds: 107 unswept site(s) in 25 file(s), none new.

The derivation named check:cross-package-test-inputs in the dispatch but does not derive it for this path; it named five convention-triggered families instead (check:query-options-erasure, check:type-check-coverage, check:type-check-debt, check:engine-double-contract, check:where-matcher), which are run above except as noted below.

Two narrowings, declared:

  1. check:dev-prereqs and check:type-check-debt --re-measure both require a fully built 67-package workspace, which this worktree does not have; CI runs both against a built tree. The ratchet half of the latter is the same ledger check:test-typecheck reported unchanged above.
  2. Repo-wide pnpm lint is CI's run. Narrowed to the changed file, and the narrowing is measured rather than asserted: the file is in ESLint's own population (ESLint#isPathIgnoredfalse, config resolved with 4 rules); --format json reports 1 file linted, 0 errors, 0 warnings, 0 suppressed; and the diff cannot move any untouched file's verdict because this repo never enables type-aware linting — calculateConfigForFile reports no parserOptions.project and no projectService, matching eslint.config.mjs's own measured note that it enables type-aware linting "for ANY file, test or not" nowhere.

Generated by Claude Code

The "held from outside the registry" case staged its holder by picking a
port, deleting the claim, then binding it. Between the `rm -f` and the
stub's `listen` the port was claimed by nobody, so any concurrent
`smoke_pick_free_port` caller scanning from 3210 could legitimately take
it — and losing there fails as `STEAL_HELD != THIEF`, which reads as an
accusation of the picker on a PR that never touched it.
Port the shape PR #10456 landed for the sdui sibling: the stub binds `:0`
and reports back the port the kernel gave it, so the port it holds is one
it provably owns, and the pick-then-`rm` dance goes away entirely. The
registry property gets plainer rather than weaker — a port bound straight
from the ephemeral range was never claimed in any registry, so there is
no claim to remove.
The stub takes NO host argument, unlike the sdui sibling's `127.0.0.1`:
`smoke_pick_free_port`'s probe binds the wildcard address because that is
the spelling serve.ts's own isPortAvailable() uses. That asymmetry is
load-bearing and recorded on #10261; it is preserved here, not converged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B4h3medzvhB9rpfoja9jcw
@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.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/sskip-changesetPR has no user-facing published change; bypasses the changeset gateteststooling

Projects

None yet

2 participants

@os-elon@claude