stacklok / stacklok/toolhive

Second getFreePort bind-close-rebind race in streamable proxy tests

Open
#6,045 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

needs-triage
Dominant language
Go
Stars
2.2k
Forks
300
Avg merge
1d 15h
Merged PRs (30d)
184

Description

Follow-up to #6034 / #6044. Same bug, second location, found while fixing the first.

Summary

pkg/transport/proxy/streamable/streamable_proxy_integration_test.go:25 has its own getFreePort with the identical bind-close-rebind (TOCTOU) shape that #6034 documents:

// getFreePort returns a free port by binding to port 0 and getting the assigned port
func getFreePort(t *testing.T) int {
	t.Helper()
	listener, err := net.Listen("tcp", "127.0.0.1:0")
	require.NoError(t, err)
	defer listener.Close()                          // <-- releases the port
	return listener.Addr().(*net.TCPAddr).Port      // <-- caller binds it later
}

The port is released before it is used, so anything else in the process can claim it in the gap. 8 call sites in that package, each feeding the port to NewHTTPProxy("localhost", port, …).

#6044 fixes only the test/integration/vmcp copy — deliberately, to keep that PR to one logical change. This tracks the second.

Why it is worth fixing rather than waiting for it to bite

#6044 quantified the mechanism with a standalone harness: 24 collisions in 31,773 rebind attempts (~0.08%) under contention, all bind: address already in use. That is exactly the rate that produces an occasional red check on an unrelated PR and gets waved through as "just CI".

An important nuance from that work, which applies here too: the race is narrower than it looks. Linux's ephemeral-port allocator actively avoids immediate reuse — 8 competing binders over a 50ms window produced 0 collisions in 300 attempts despite 74,630 competing binds; reproduction needed 64 victims plus 16 churners. So a long history of green runs is not evidence this copy is safe — it is evidence the window is usually not hit.

Suggested fix

Whether the clean fix from #6044 transfers depends on the API here, and that needs checking rather than assuming:

  • #6044's approach worked because vmcpserver.ServerConfig.Port accepts 0 meaning "OS-assigned", and the server exposes the bound address via Address() after Ready(). Verify whether NewHTTPProxy / the streamable proxy has an equivalent — a port-0 path and a way to read the actual bound address back after start.
  • If it does, the fix is the same shape: pass 0, read the address back, delete getFreePort.
  • If it does not, the alternative is to keep the probe listener open and hand it to the proxy, or to add a bound-address accessor. That is a slightly larger change to a test-facing constructor.

Either way, please leave a comment at the site explaining why a probe listener must not be reintroduced — #6044 does this, and it is the part that stops the pattern coming back.

Context

Third test-reliability issue from this cluster, alongside #6026 (optimizer search-quality spec — nondeterministic, no obvious fix) and #6040 (group e2e concurrent workload starts — cause still unidentified). This one and #6034 are the two with unambiguous mechanisms and small local fixes.

Generated with Claude Code

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in pkg/transport/proxy/streamable/streamable_proxy_integration_test.go at getFreePort and its eight NewHTTPProxy call sites. Inspect the streamable proxy startup API to determine whether port 0 and a bound-address accessor are available, then run the package integration tests. Done means removing the bind-close-rebind race and documenting why the probe listener must not be reintroduced.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
networking, testing-qa
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.