Fix port-selection TOCTOU race in e2e tests - #6142
Merged
Merged
Conversation
networking.FindOrUsePort probes a port by binding then immediately releasing it, so anything else on the machine can steal the number before the real bind happens later. Under sharded CI this is lost often enough to flake (observed on PR #6088 as "OIDC mock server error: listen tcp :49278: bind: address already in use"). Two fixes depending on where the real bind happens: - In-process mock servers (OIDCMockServer, LLMGatewayMock) now open their own listener at construction time instead of accepting a pre-selected port, closing the window entirely. - Subprocess-launched proxies (thv serve, thv llm proxy start) get a shared RetryOnPortConflict helper that retries once on a detected bind conflict, generalizing the pattern already used for Docker workload port races in the vMCP dual-era test helpers. thv proxy needed a different approach: it silently substitutes a different port on conflict for a nonzero request (no error to retry on) and its real bind can lag port selection by a full OAuth exchange, so those tests instead pass --port 0 and read the real bound port back from the subprocess's own stdout. Production callers with the same root cause (Docker port bindings, in-process OAuth/proxy binders) are tracked separately in #6141. Generated with Claude Code (https://claude.com/claude-code)
jhrozek
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
aponcedeleonch,
rdimitrov and
reyortiz3
as code owners
July 29, 2026 12:18
JAORMX
approved these changes
Jul 29, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6142 +/- ##
==========================================
+ Coverage 72.34% 72.51% +0.17%
==========================================
Files 731 734 +3
Lines 75748 75939 +191
==========================================
+ Hits 54799 55070 +271
+ Misses 17047 16974 -73
+ Partials 3902 3895 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 free
to 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.
Summary
networking.FindOrUsePort/FindAvailablepick a port by binding a probe listener, reading the port, and immediately closing it — the real bind happens later, so anything else on the machine can steal the number in between. Under sharded CI this is lost often enough to flake, observed on PR #6088 asOIDC mock server error: listen tcp :49278: bind: address already in use.OIDCMockServerandLLMGatewayMock(test/e2e/oidc_mock.go,test/e2e/llm_gateway_mock.go) now open their own listener at construction time instead of accepting a pre-selected port, closing the race window entirely for the 6 e2e call sites that use them.test/e2e/api_helpers.gogets a sharedRetryOnPortConflicthelper, applied toNewServer(thv serve) and tothv llm proxy startcall sites incli_llm_all_clients_test.go— both hard-fail on a real bind conflict, so a retry-once-with-a-fresh-port is correct and sufficient there. This generalizes the pattern already used for Docker workload port races intest/e2e/vmcp_dual_era_helpers_test.go.thv proxy(test/e2e/proxy_oauth_test.go) needed a different fix: for a nonzero requested port it silently substitutes a different one on conflict with no error to retry on, and its real bind can trail port selection by a full OAuth exchange. Those tests now pass--port 0and read the actual bound port back from the subprocess's own stdout instead of pre-selecting.Production callers sharing the same root cause (Docker port bindings in
pkg/container/docker, in-process OAuth/proxy binders inpkg/authandcmd/thv/app/proxy.go) are deliberately out of scope here and tracked in #6141.Type of change
Test plan
task lint-fix)Ran
task buildandtask lint-fixclean throughout. Ran targeted, label-filteredginkgopasses covering the touched specs (llm,oauth,infralabels) plus the ~14 other e2e files sharing the modifiedNewServer/RetryOnPortConflicthelper (apilabel). Every failure surfaced traced to a cause unrelated to this diff:TOOLHIVE_SKIP_DESKTOP_CHECKnot being set for a local ToolHive Desktop conflict, Docker workload-readiness timeouts under local resource contention, and a pre-existing macOS-only gap in aSSL_CERT_FILEtrust mechanism one test class relies on — confirmed by reverting the two files involved to cleanmainand reproducing the identical failure there. No failure was attributable to the code in this diff.Changes
test/e2e/oidc_mock.goNewOIDCMockServeropens its own listener at construction; addsPort()test/e2e/llm_gateway_mock.goNewLLMGatewayMockopens its own listener at construction; addsPort()test/e2e/api_helpers.goRetryOnPortConflict;NewServeruses ittest/e2e/cli_llm_all_clients_test.gothv llm proxy startsites inRetryOnPortConflictvia a newstartLLMProxyhelpertest/e2e/cli_llm_setup_test.gotest/e2e/proxy_oauth_test.go--port 0and discovers the real bound port from subprocess output (discoverProxyPort,startProxyAndDiscoverPort); thread-safe output capture via a newsyncBuffertest/e2e/vmcp_infra_features_test.goDoes this introduce a user-facing change?
No.
Special notes for reviewers
startLLMProxy's TCP-dial readiness check double-checks the subprocess's own exit signal before trusting a successful dial, to avoid a false positive if the dial happens to reach a different process that won the same port-bind race.Generated with Claude Code