Skip to content

test: take a free port instead of binding a hardcoded 9876 - #336

Open
Gunther-Schulz wants to merge 1 commit into
cnighswonger:mainfrom
Gunther-Schulz:pr/test-fixed-port
Open

test: take a free port instead of binding a hardcoded 9876#336
Gunther-Schulz wants to merge 1 commit into
cnighswonger:mainfrom
Gunther-Schulz:pr/test-fixed-port

Conversation

@Gunther-Schulz

Copy link
Copy Markdown
Contributor

test/install-service.test.mjs's dispatch back-compat case binds a hardcoded 127.0.0.1:9876. The assertion is about ANTHROPIC_BASE_URL, but the wrapper really binds the port it is handed, so the case fails for anyone whose machine already holds that one.

That is not hypothetical: a local QGIS MCP server owns 9876 here, and the suite went red on a change that had nothing to do with it — blocking a push until the port was freed by hand. A test that fails for reasons outside the diff is the kind that teaches people to discount a red suite.

The fix

--proxy-port 0 is not usable here, because the assertion needs the number it passed in. So the case asks the OS for a free port, closes the probe socket, and passes that number through. There is a small TOCTOU window between the close and the wrapper's own bind — far narrower than a fixed port's collision surface, and it fails rarely and randomly rather than permanently on every machine that happens to use 9876.

Verification

A controlled pair, both arms from the same command with 9876 deliberately held by another process:

before (hardcoded 9876):  43 tests, 42 pass, 1 fail   <- EADDRINUSE in this case
after  (free port):       43 tests, 43 pass, 0 fail

One file, one case, no behaviour change to shipped code.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RMiYvNxKq6G9gfJMzArm4q

The dispatch back-compat case asserts on ANTHROPIC_BASE_URL, but the wrapper
really binds the port it is given, so a hardcoded one fails for anyone whose
machine already holds it. Observed with a local QGIS MCP server owning 9876:
the whole suite went red on a change that had nothing to do with it, which is
the failure mode that trains people to discount a red suite.

--proxy-port 0 is not usable here, because the assertion needs the number it
passed in. So the case asks the OS for a free port, closes the probe socket
and passes that number through. There is a small TOCTOU window between the
close and the wrapper's own bind; it is far narrower than a fixed port's
collision surface, and unlike the fixed port it fails rarely and randomly
rather than permanently on the machines that happen to use 9876.

Verified as a controlled pair, both arms from the same command with 9876
deliberately held by another process: before, 42/43 with this case failing on
EADDRINUSE; after, 43/43.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RMiYvNxKq6G9gfJMzArm4q
Gunther-Schulz added a commit to Gunther-Schulz/claude-code-cache-fix that referenced this pull request Aug 14, 2026
…on because a gate said so

PR cnighswonger#337 carries the coalescer itself, opt-in behind its env gate, with the
measured population in the body — 144 pairs, 114 streaks, 55 billed twice —
and the retry class stated as the design constraint rather than as a caveat,
since suppressing those would leave a real request unanswered.

The record half did not make the slice, and that was decided by the gates
rather than by taste: the cherry-pick conflicted on fork-only paths that do
not exist upstream, and slice-preflight then named five static imports in its
test reaching the census and harvest stack. Ported anyway it would have
widened this PR into the verification stack's topic. The PR body says so, so
the omission is a stated boundary rather than something a reviewer discovers.

Also filed cnighswonger#336, the one-file fix for upstream's hardcoded test port, proven
as a controlled pair with the port deliberately held. Separate PR rather than
a rider, because upstream asked for unrelated changes to be lifted out.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RMiYvNxKq6G9gfJMzArm4q
@vsits-proxy-builder

Copy link
Copy Markdown
Contributor

Reviewed at e0256dc.

Verdict: approving. Clean, minimal, well-motivated test-only fix.

What I checked:

  • Correctness — standard Node createServer / listen(0) / close free-port handoff. TOCTOU window between socket close and the wrapper's own bind exists; you flag it in both the PR body and the code comment (fr:813-820). The tradeoff is a narrow random flake vs a permanent collision on every machine that holds 9876 for something else — right call for a test.
  • Anti-bloat — inline promise wrapper is a single call site, correctly not extracted into a helper per this repo's inline-first convention.
  • Coverage — pure test change, no shipped-code delta. Assertion ANTHROPIC_BASE_URL must reflect --proxy-port is preserved unchanged, so we still verify the wrapper's port-to-env-var wiring.
  • Alternative considered--proxy-port 0 was correctly ruled out; the assertion needs the number that was passed in.
  • Comment quality — WHY-focused (why fixed-port fails, why TOCTOU is acceptable here), which is exactly the case for a comment.

Applying approved-by-code-agent. Since this is test-only with no code path change, I don't think we need Codex review here, but leaving that to Chris.

— Proxy Builder

@vsits-proxy-builder vsits-proxy-builder Bot added the approved-by-code-agent Final implementation approval from Code Agent label Aug 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved-by-code-agent Final implementation approval from Code Agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant