This is a focused, well-reasoned fix for a real flake. The root cause (two quick GetFreePort calls getting the same port back from the OS because the first listener was already released) is correctly diagnosed, and the per-process deduplication map is a clean way to eliminate it. The test directly pins the guarantee by calling GetFreePort 200 times and asserting all ports are distinct — exactly the right kind of regression test for this class of bug.
A few things I'd flag for conscious consideration before this graduates to a human reviewer:
The "seen" map is never pruned. In a long test run that calls GetFreePort tens of thousands of times the map grows unboundedly. The ephemeral port range on most systems is roughly 28 000–32 000 ports (Linux default), so after enough calls the map could approach or exceed the usable range, turning the 20-attempt loop into a spin that always hits the t.Fatalf. For typical test suites this is unlikely to matter, but it's worth being aware of.
The retry budget (20 attempts) is now shared between two rejection reasons — UDP unavailable and "already seen" — without distinguishing them. If many ports were previously handed out, legitimate "UDP not free" failures silently eat into the same budget as deduplication retries. This is a minor point, but a comment noting that 20 was chosen to cover both cases would help future readers.
One subtle race window remains. Between udpConn.Close() and the caller actually binding the returned port, another process can still grab it. This was true before the PR too, so it's not a regression — just worth noting that the deduplication protects against intra-process re-use, not inter-process races.
None of these are blockers. The fix is correct for the documented failure mode, the deduplication logic is sound and properly locked, and the test is genuinely meaningful. This is ready to hand to a human reviewer.
Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 981,
"verdict": "ready",
"event": "comment",
"summary": "This is a focused, well-reasoned fix for a real flake. The root cause (two quick `GetFreePort` calls getting the same port back from the OS because the first listener was already released) is correctly diagnosed, and the per-process deduplication map is a clean way to eliminate it. The test directly pins the guarantee by calling `GetFreePort` 200 times and asserting all ports are distinct — exactly the right kind of regression test for this class of bug.\n\nA few things I'd flag for conscious consideration before this graduates to a human reviewer:\n\n**The \"seen\" map is never pruned.** In a long test run that calls `GetFreePort` tens of thousands of times the map grows unboundedly. The ephemeral port range on most systems is roughly 28 000–32 000 ports (Linux default), so after enough calls the map could approach or exceed the usable range, turning the 20-attempt loop into a spin that always hits the `t.Fatalf`. For typical test suites this is unlikely to matter, but it's worth being aware of.\n\n**The retry budget (20 attempts) is now shared between two rejection reasons** — UDP unavailable and \"already seen\" — without distinguishing them. If many ports were previously handed out, legitimate \"UDP not free\" failures silently eat into the same budget as deduplication retries. This is a minor point, but a comment noting that 20 was chosen to cover both cases would help future readers.\n\n**One subtle race window remains.** Between `udpConn.Close()` and the caller actually binding the returned port, another process can still grab it. This was true before the PR too, so it's not a regression — just worth noting that the deduplication protects against *intra-process* re-use, not inter-process races.\n\nNone of these are blockers. The fix is correct for the documented failure mode, the deduplication logic is sound and properly locked, and the test is genuinely meaningful. This is ready to hand to a human reviewer.",
"posted_to_pr": true,
"draft": true
}