fix(localscale): reserve distinct test ports by holding listeners together

[Kiran01bm]

The localscale proxy port tests now reserve their free TCP ports by holding all reservation listeners open until every port is captured, so two sequential reservations can never return the same port.

Why

The port-picking helper listened on 127.0.0.1:0, read the assigned port, and closed the listener before the next call. The kernel is free to hand the just-released ephemeral port straight back to the next :0 bind, so two sequential calls could return the same port and the tests failed their port1 != port2 precondition intermittently in CI.

What

• freeTCPPort becomes freeTCPPorts(t, n): all n listeners are opened before any is closed, guaranteeing n distinct ports; they are released together for the test to bind, and the helper asserts its own distinctness contract so a refactor to per-iteration release fails loudly.
• TestNewBranchProxyWithRetrySkipsOccupiedPort derives the occupied port from the listener that stays bound for the whole test instead of close-then-rebinding it. In this test that also removes the window where another process could steal the port between release and re-listen; the reserve-then-bind test keeps that inherent window since it must bind the reserved ports itself.
• TestPortAllocatorIgnoresDuplicateRelease uses plain constants — the allocator is pure bookkeeping and never binds, so it needs no relationship with the kernel at all.
• The now-structural require.NotEqual(port1, port2) preconditions are dropped.

Before / after

Before: listen-close-listen per port
┌──────────────────────┐   close   ┌──────────────────────┐
│ listen :0 → port A   │──────────▶│ listen :0 → port A   │  kernel reuses
└──────────────────────┘           └──────────────────────┘  the freed port
        port1 = A                          port2 = A         ✗ NotEqual fails

After: hold all listeners, then release together
┌──────────────────────┐  ┌──────────────────────┐
│ listen :0 → port A   │  │ listen :0 → port B   │   both bound at once,
│ (held)               │  │ (held)               │   A ≠ B guaranteed
└──────────┬───────────┘  └──────────┬───────────┘
           └────────── close together ┘
        port1 = A                  port2 = B                 ✓

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[Copilot]

Pull request overview

Improves LocalScale proxy port-selection tests by making test port reservations deterministic and race-resistant, preventing CI flakes caused by ephemeral port reuse between sequential :0 binds.

Changes:

• Replace single-port reservation helper (freeTCPPort) with freeTCPPorts(t, n) that holds multiple listeners open simultaneously to guarantee distinct ports.
• Update affected tests to consume reserved port slices and remove now-unnecessary require.NotEqual preconditions.
• In the occupied-port retry test, keep the “occupied” port bound via a held listener and derive its port from the active listener.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by their agent. Reviewed at head 5fc1f4c3, in a worktree, with the three touched tests run 200× under -race and the ephemeral-port reuse behavior measured directly.

Verdict: clean — nothing blocks. This is the right shape of flake fix: no timeout bump, no retry, no Eventually — it makes the failure impossible by construction, because a port that is still bound cannot be handed to the next :0 bind. The SkipsOccupiedPort change is the best part of the diff and it's the one the title undersells: deriving the occupied port from the listener that stays bound removes a second hazard (something else grabbing the port between release and re-listen), not just the duplicate-port one. Three things, none of them in the mechanism.

#	Finding	Severity
1	One of the three tests never binds a port, so it never needed real ports at all	simplification
2	The same helper with the same defect exists in e2e/k8s, with a wider window	correctness (pre-existing)
3	The distinctness invariant went from an assertion to a comment	robustness

---

1. TestPortAllocatorIgnoresDuplicateRelease doesn't touch the network

portAllocator is pure bookkeeping — acquire pops from a slice and marks a map, release checks the map and pushes back. Neither ever binds. And the test only exercises that: acquire, double-release, then assert the FIFO order coming back out.

ports := freeTCPPorts(t, 2)
port1, port2 := ports[0], ports[1]
alloc := newTestPortAllocator(port1, port2)

So this test opens two real loopback sockets to obtain two distinct integers it then puts in a slice. newTestPortAllocator(1, 2) tests exactly the same behavior, and it's the only version that's structurally incapable of flaking — not "guaranteed distinct by a helper" but "has no relationship with the kernel at all". It also reads better: the assertions become acquire returns 1, then 2, then 1 again, which states the FIFO contract, instead of comparing two opaque ephemeral numbers.

That's the part I'd push on hardest, because it's the difference between hardening the flake surface and removing this test from it. A flake fix is a good moment to ask which tests needed to be near the hazard in the first place, and this one is the answer. TestTrackProxyDoesNotReleaseOldPortBeforeCloseCompletes is the opposite case and correctly keeps real ports — newTestBranchProxyAtAddr actually listens on them, so it needs ports that bind.

2. e2e/k8s has the same helper, and its window is much wider

freeLocalPort in e2e/k8s/kubectl_test.go is freeTCPPort under another name — bind :0, read the port, defer close, hand the number to a caller that binds it later:

func freeLocalPort(t *testing.T) int {
	listener, err := listenConfig.Listen(t.Context(), "tcp", "127.0.0.1:0")
	require.NoError(t, err)
	defer utils.CloseAndLog(listener)
	…
	return addr.Port
}

Three callers — startControlPlanePortForward, startDataPlanePodGRPCPortForward, startDataPlaneServiceGRPCPortForward — and the rebinder is an external kubectl port-forward process started after cmd.Start(), so the gap between release and rebind is a process spawn rather than a few instructions. The control-plane forward is suite-owned and outlives the test that created it, so two of these are genuinely live at once and the sequential-reservation collision this PR is about applies to them directly.

The failure mode is worse there, too. Here a collision trips a NotEqual precondition and names itself. There it surfaces as a port-forward that dies or serves the wrong backend, which reads as a cluster or deploy problem and costs someone a real triage session. Different package and out of scope for a one-file fix — but it's the same defect and this PR is the moment the pattern got a name, so worth a follow-up rather than leaving the next person to rediscover it from a confusing e2e failure.

3. The invariant is now documented rather than checked

The three require.NotEqual(t, port1, port2) lines are gone as "now-structural", and they are structural — while freeTCPPorts holds every listener. That guarantee lives entirely in the helper's comment, and the comment is doing real work: it's the only thing telling a future reader that the defer closing all listeners together is load-bearing rather than tidy cleanup. Someone refactoring it into the obvious-looking per-iteration defer inside the loop reintroduces the exact bug, and every caller stays green.

One line inside the helper keeps that from being silent:

require.Len(t, slices.Compact(slices.Sorted(slices.Values(ports))), n, "reserved ports must be distinct")

Same check the three call sites used to make, in one place, protecting callers that don't exist yet — which is the argument for removing it from the call sites in the first place, just followed one step further.

Also

(nit) TestTrackProxyDoesNotReleaseOldPortBeforeCloseCompletes still has the steal window the third test just closed: freeTCPPorts releases both ports and the proxies bind them a few statements later. It's inherent to reserve-then-bind and can't be fixed the same way, since this test has to bind those exact ports itself. Worth knowing the PR closes the distinctness hole everywhere and the steal hole in one of two places — the body's "which also removes the window" reads like it applies more broadly than it does.

(nit) freeTCPPorts(t, 1)[0] is a slightly awkward way to ask for one port. Not worth a second helper; just the one call site that reads oddly.

---

Action items

1. (Finding 1) Give TestPortAllocatorIgnoresDuplicateRelease two constants instead of two reserved ports — it never binds either one.
2. (Finding 2) Follow up on e2e/k8s's freeLocalPort, which has the same defect across three concurrent port-forwards.
3. (optional, Finding 3) Assert distinctness inside freeTCPPorts so the helper enforces its own contract.

---

Verified — tried to break, couldn't

The guarantee is real and it's the only thing that matters here. Two listeners bound to distinct 127.0.0.1:0 sockets cannot share a port — Go sets no SO_REUSEPORT, and SO_REUSEADDR on a listening socket doesn't permit a second bind to a live address — so holding all n before releasing any makes the collision impossible rather than unlikely. That's the correct class of fix for a flake, and it's what the repo's rules ask for over deadline tuning.

200 runs of all three tests under -race: green in 2.8s. That proves no regression; it deliberately isn't offered as evidence the flake is gone. I measured the underlying behavior separately — 20,000 close-then-rebind pairs on this machine produced 0 collisions. So the old pattern is unreproducible on an idle macOS box, which fits a Linux-under-CI-load flake and means local reruns could never have confirmed either the diagnosis or the fix. The structural argument is the evidence, and it holds.

portAllocator really is network-free, which is what makes Finding 1 safe: acquire returns free[0] and marks inUse, release no-ops on an unknown port. The require.Error in TestTrackProxy… after two acquisitions is pool exhaustion, not a bind failure — so nothing in either allocator test depends on the ports being bindable.

Nothing else in the repo has this shape. I checked every 127.0.0.1:0 site across pkg/, integration/ and e2e/: the rest bind-and-serve on the listener they opened, which is the safe pattern and needs no change. e2e/k8s's freeLocalPort (Finding 2) is the single exception.

The helper is exception-safe. The listener slice is closed by one deferred loop, so a require.NoError failure partway through the reservation loop — which t.Helper() turns into a Goexit — still runs the defer and leaks nothing.

The retained require usage keeps the import live and the package builds clean; the three deleted assertions are the only test lines removed, and each is replaced by a stronger structural guarantee rather than dropped.

Copilot left an overview only, no inline comments — nothing to re-raise.

Ran locally at head: go build ./... and ./pkg/localscale/... green under -race. CI 34/34. Leak check on the body and diff clean, terminology clean.

This review was generated by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/1162, follow-up commit

#	Concern	Status
2	e2e/k8s's freeLocalPort has the same reserve-then-release defect with a wider window (external kubectl port-forward rebinds after a process spawn; suite-owned control-plane forward makes two forwards concurrently live)	deferred — pending your OK to spin up a follow-up PR (separate package, out of scope for this one-file fix, per the reviewer)
3	The distinctness invariant went from three call-site assertions to a helper comment — a per-iteration defer refactor silently reintroduces the bug	fixed — freeTCPPorts now asserts require.Len(slices.Compact(slices.Sorted(...)), n) so the helper enforces its own contract for callers that don't exist yet
1	TestPortAllocatorIgnoresDuplicateRelease never binds a port, so it never needed real ports — constants are the only structurally flake-free version	fixed — the test uses const port1, port2 = 1, 2 with a comment noting the allocator is pure bookkeeping
nit	PR body's "which also removes the window" overstates — the steal window is closed in the occupied-port test only; reserve-then-bind tests keep it inherently	fixed — body rewording scopes the claim to SkipsOccupiedPort and names the inherent window in the reserve-then-bind test.
nit	freeTCPPorts(t, 1)[0] reads awkwardly	reply — agreed with the reviewer's own conclusion: not worth a second helper for one call site; left as is