Skip to content

Bind the vMCP test server's port directly - #6044

Merged
JAORMX merged 1 commit into
mainfrom
fix-vmcp-integration-port-race
Jul 27, 2026
Merged

Bind the vMCP test server's port directly#6044
JAORMX merged 1 commit into
mainfrom
fix-vmcp-integration-port-race

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Why: test/integration/vmcp/helpers.getFreePort picked a port by binding 127.0.0.1:0, reading the port back, and closing the listener — then handed that port to the vMCP server to bind later, in Start. Closing the probe listener releases the port, so between that close and the server's own net.Listen the OS could hand the same port to any of the ten t.Parallel() tests in this package (each of which also starts httptest backends). The symptom was an intermittent bind: address already in use failing CI on PRs with unrelated diffs — most recently Echo the request JSON-RPC id in session-not-found 404s #6031.
  • What: Set ServerConfig.Port to 0 so the vMCP server's own listener picks the port and holds it, and read the bound address back from vmcpServer.Address(), which the helper already waits for via Ready(). getFreePort is deleted along with the now-unused net import. This closes the window rather than narrowing it: the port is never unbound between being chosen and being served.

Closes #6034

Type of change

  • Bug fix

Test plan

  • Unit tests (task test — 3 pre-existing environmental failures, see below)
  • Linting (task lint — 0 issues)
  • Manual testing (described below)
  • E2E tests

Verified by code trace, not inferred from the doc comment:

  1. Port: 0 really does reach a port-0 bind. buildServeConfig (pkg/vmcp/server/serve.go:413) passes cfg.Port straight through; Start formats addr from Host/Port (server.go:733) and calls net.Listen("tcp", addr) (:745).
  2. Address() returns the OS-assigned port after Ready(). It returns s.listener.Addr().String() when the listener is non-nil (server.go:882-889), and Start stores the listener under listenerMu (:750-752) before readyOnce.Do(close(s.ready)) (:771). That ordering is what makes the fix correct — it is not incidental.
  3. No test reads a port or builds a URL before Ready(). All 20 call sites in test/integration/vmcp/*_test.go build "http://" + vmcpServer.Address() + … after NewVMCPServer returns, and it only returns after <-vmcpServer.Ready().
  4. net becomes unused and is removed; require and testing are still used elsewhere and stay.

The original failure was reproduced before fixing it. On a clean tree with the old getFreePort intact, amplifying the existing race (64 goroutines churning ephemeral-port allocation, plus a 3s delay widening the already-present window between the probe Close() and the server's bind) failed on the first run:

--- FAIL: TestVMCPServer_StructuredContent (8.13s)
    vmcp_server.go:242: vMCP server error: failed to create listener:
        listen tcp 127.0.0.1:39743: bind: address already in use

A standalone harness modelling only the bind-close-rebind pattern under the same pressure gave 24 collisions / 31,773 rebind attempts (~0.08%), all bind: address already in use.

The fix removes it under the identical amplification that failed above: 3/3 runs green, 0 collisions.

Stress runs on the committed diff (no instrumentation):

Command Result
go vet ./... 0 issues
task lint (golangci-lint + go vet ./...) 0 issues.
go test ./test/integration/vmcp/... -count=1 -race ok 95.6s
go test ./test/integration/vmcp/... -count=10 -race -timeout 60m ok 945.8s, exit 0, 0 bind collisions
task test 3 failures, all pre-existing (below)

go vet ./... was run separately from task test deliberately: task test's formatter can swallow build failures, so a package whose tests don't compile would otherwise read as green.

The three task test failures are unrelated to this diff — TestNewServer_ReadTimeoutConfigured and TestSecurityHeaders (pkg/api, "no available runtime found") and TestCompositeProvider_RealProviders (pkg/secrets/keyring, no keyring). Confirmed pre-existing by checking out origin/main and running the same three there: all three fail identically.

API Compatibility

  • This PR does not break the v1beta1 API.

Test-helper only; no production code touched.

Does this introduce a user-facing change?

No.

Special notes for reviewers

  • The fix depends on the listener-stored-before-ready ordering in (*Server).Start (point 2). If that ordering is ever inverted, Address() falls back to the configured port — 0 — and every test in this package fails loudly rather than flakily, so the regression could not go unnoticed.
  • Out of scope, worth a follow-up: pkg/transport/proxy/streamable has its own getFreePort (streamable_proxy_integration_test.go:25) with the identical bind-close-rebind shape, used by 8 call sites. Separate package, separate flake surface, left untouched to keep this to one logical change. Filed separately.
  • Nothing contradicted Flaky: getFreePort bind-close-rebind race in vmcp integration helpers #6034's diagnosis. One nuance worth recording: the race is narrower than it looks, because Linux's ephemeral-port allocator actively avoids immediate reuse — with 8 competing binders and a 50ms window, 0 collisions in 300 attempts despite 74,630 competing binds; reproduction needed 64 victims and 16 churners. That is consistent with a rare CI flake, and it means a handful of green runs of the old code would not have been evidence it was fine.

Generated with Claude Code

The vmcp integration test helper picked a port by binding 127.0.0.1:0,
reading the port back, and closing the listener — then handed that port to
the vMCP server to bind later. Closing the probe listener releases the port,
so between the close and the server's own net.Listen any of the ten
t.Parallel() tests in this package (each also starting httptest backends)
could be handed the same port by the OS. The result was an intermittent
"bind: address already in use", which most recently failed CI on a PR whose
diff was unrelated.

Set ServerConfig.Port to 0 instead and read the bound address back from
Address() after Ready(), which the helper already awaits. The server's
listener is created and stored before the ready channel closes, so
Address() reports the OS-assigned port to every caller. This removes the
window rather than narrowing it: the port is never unbound between being
chosen and being served.

Closes #6034
@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.24%. Comparing base (e837d69) to head (84c73bf).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6044      +/-   ##
==========================================
+ Coverage   72.16%   72.24%   +0.07%     
==========================================
  Files         721      721              
  Lines       75069    75060       -9     
==========================================
+ Hits        54174    54226      +52     
+ Misses      17033    16958      -75     
- Partials     3862     3876      +14     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JAORMX
JAORMX merged commit 401eec7 into main Jul 27, 2026
45 checks passed
@JAORMX
JAORMX deleted the fix-vmcp-integration-port-race branch July 27, 2026 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky: getFreePort bind-close-rebind race in vmcp integration helpers

2 participants