test(netgw): Make the forward tests survive the shared CI Mac

[ananos]

Two internal/netgw tests failed the unit job on #115 across two attempts, on a package that PR does not touch. This PR fixes what made them fail; they pass on a third attempt and are flakes, but flakes that cost a re-run each time.

• TestExposeLeavesAnOverlappingAddressToTheKernel bound a port the probe said was free and found it held outside this gateway. The probe listens on 127.0.0.1 and the kernel's free-port search is exact-address for a SO_REUSEADDR socket, so a port another process holds on 0.0.0.0 is handed out anyway; the other packages' tests run beside this one and take ports as they go. The tests that publish on a fresh port now draw again on that one refusal (exposeFresh), and fail on any other.
• TestICMPEchoIsAnsweredNotForwarded timed out after the three-second reply wait, under the race detector on the shared Mac. Every reply is computed in-process, so the wait is a liveness bound; it is now ten seconds, named (replyWait), and used for every bound of the same kind in the package: the forwarder's dial wait, the poll for a stack counter, the member's pipe write deadline, and the two WatchHosts waits. expectNoDial keeps its 500ms, being a negative check.

Testing

go test -race ./internal/netgw/ passes, and each failing test passes repeated in isolation (-count=5 and -count=3 under -race). Three claude-fleet rounds are folded in; the last asked that exposeFresh say its forwards must differ in protocol, which it now says and checks.

One thing I ran into that this does not fix: repeating the whole package in one process (-count=3 -race) fails the three TestEgress*CarriesRealBytes tests with SYN-ACK timeouts, on unmodified main just the same. The tests pass alone, so it looks like accumulation across tests (networks are never closed). CI runs the suite once and does not hit it; tracked in #118.

🤖 Generated with Claude Code

[ananos]

@claude-fleet review

[claude-fleet]

Review task queued.

Task: 332430ec-d5d8-4a6c-8236-1351b394f005
Commit: 04c7b16
Model: claude-opus-5 (effort medium)
Executor: ravel-clickbench-ubuntu-2
Status: done. Review posted.

[claude-fleet]

Request changes: one should_fix, two nits. The two fixes themselves are sound — exposeFresh retries on exactly the error class it claims, and replyWait is a real liveness bound — but the PR leaves one test publishing on a fresh port without the retry, so the described class of failure is not closed.

Verified:

• Expose's "held outside this gateway" error wraps the kernel error with %w, so errors.Is(err, syscall.EADDRINUSE) in exposeFresh matches — internal/netgw/forward.go:267-268. The TCP path returns tcpproxy.Proxy.Start's raw *net.OpError (tcpproxy.go:195-199) and the UDP path returns net.ListenUDP's, so the errno is reachable through Unwrap on both protocols.
• A failed Expose leaves nothing behind, so redrawing is safe: s.set[f.key()] is written only after the bind succeeds (forward.go:274), gvisor registers its proxy only after Start returns nil (ports.go:241-253), and tcpproxy.Start calls p.Close() on its own error.
• The other ErrForwardExists branch — the one for a port this gateway holds on another address (forward.go:263-266) — does not wrap EADDRINUSE and would stop the retry, but it is unreachable from all six exposeFresh call sites: each is the first Expose on a network from forwardNetwork, so holderOfPort finds nothing.
• exposeFresh(t, n, "", "", ...) builds ":"+port, which is what TestExposeReadsAnEmptyHostAsAllInterfaces needs — forward_test.go:189.
• replyWait is shared with the forwarder's dial wait — forwarder_test.go:72. All six await call sites are positive assertions (egress_e2e_test.go:147, forwarder_test.go:273/282/321, network_test.go:204, dns_test.go:208), so widening the bound cannot turn a failing test into a passing one; expectNoDial carries the negative case and keeps its 500ms.

Checks run:

• go test -race -run 'TestExpose|TestICMPEcho|TestUnexpose|TestStartupForwards|TestIPv6NeverEgresses' ./internal/netgw/ → ok, 3.076s
• No revert check: this is a test-only change with no production fix to revert.

Smaller things:

• exposeFresh's doc says it publishes "on a port nothing else holds". It publishes on a port the bind accepted, which is weaker and is the point of the helper — the comment below it already says this correctly.
• Widening await to 10s also applies to tcpClient.receive (egress_e2e_test.go:147), so the -count=3 TestEgress*CarriesRealBytes failures the description mentions now take about three times as long to time out. CI runs the suite once, so this costs nothing there.

Not checked:

• The CI Mac's actual load profile and whether 10s is wide enough there — the widening is justified by the comment's reasoning, not by a reproduction.
• The -count=3 TestEgress*CarriesRealBytes accumulation failure the description reports as pre-existing on main; it is explicitly out of scope for this PR and I did not reproduce it.
• Whether another process holding a port on 0.0.0.0 really causes the kernel to hand the same port out for a 127.0.0.1 ephemeral bind on darwin; I read the claim and the error-wrapping path it relies on, but did not test it on macOS.

Findings outside the diff:

• internal/netgw/forward_test.go:307 (should_fix): TestStartupForwardsAreListedLikeAnyOther still uses the raw probe-then-bind pattern this PR replaces everywhere else, so it keeps the flake the PR exists to remove.

Failure: another process takes the probed port between freePort at line 307 and the bind inside New, and the test fails with New: a forward already listens there: 127.0.0.1:P is held outside this gateway — the same message and the same race as TestExposeLeavesAnOverlappingAddressToTheKernel, with no retry. The description says "the tests that publish on a fresh port now draw again on that one refusal", which is not true of this one.

Evidence: forward_test.go:307-308 passes the port straight to forwardNetwork, which fatals on any New error (forward_test.go:84-86); New installs startup forwards through the same set.expose (network.go:310-311) and so returns the identical EADDRINUSE-wrapping error from forward.go:267.

Fix: wrap the freePort + forwardNetwork pair in the same ten-attempt errors.Is(err, syscall.EADDRINUSE) loop, by giving forwardNetwork a variant that returns the error instead of fatalling.

Task 332430ec-d5d8-4a6c-8236-1351b394f005 on ravel-clickbench-ubuntu-2, model claude-opus-5, effort medium.

[claude-fleet]

nit: The second Expose here is not covered by exposeFresh's retry, and it binds a protocol the probe never tested.

Failure: freePort probes TCP, so the UDP port it returns is unverified; if another process holds UDP 127.0.0.1:P or 0.0.0.0:P, this line fails with udp beside tcp on one address: ... is held outside this gateway. Narrower than the TCP case — it needs a UDP holder on the one port TCP happened to hand out — but it is the same failure mode and the test cannot draw again, because the whole point is reusing the port from line 290.

Evidence: forward_test.go:290-292; exposeFresh only guards the call it makes itself (forward_test.go:105-131).

Fix: loop the pair together — draw a port, try both binds, redraw if either returns EADDRINUSE.

[claude-fleet]

nit: replyWait's reasoning applies to two other in-process liveness bounds in this package that stay at 2s, below the three seconds the comment says was crossed on the CI Mac.

Failure: on the same loaded Mac under -race, TestIPv6NeverEgresses fails at the stack never counted an unsupported 34525 frame after 2s (forwarder_test.go:248), or any test fails at send frame: i/o timeout from the 2s write deadline (network_test.go:132), for the same reason the echo reply was late. Both are answers computed in-process, which is exactly the class replyWait is defined for.

Evidence: forwarder_test.go:248, network_test.go:132; replyWait's own comment at network_test.go:139-147.

Fix: use replyWait at both, or say in the comment why a counter poll and a socket write get a tighter bound than a frame reply.

[ananos]

@claude-fleet review

[claude-fleet]

Review task queued.

Task: 068f68a2-56ed-4375-bc10-b88ddf096b00
Commit: 926bcf7
Model: claude-opus-5 (effort medium)
Executor: rp2
Status: done. Review posted.

[ananos]

@claude-fleet review

[claude-fleet]

Review task queued.

Task: 4898a990-afd1-4d41-ad78-adcc93007ff6
Commit: efa9729
Model: claude-opus-5 (effort medium)
Executor: ravel-clickbench-ubuntu-2
Status: done. Review posted.

[claude-fleet]

Reviewed 926bcf7. PR head is now efa9729.

Request changes: one should_fix finding.

The port-redraw work itself is sound, and I verified the retry guard actually fires rather than silently never matching. The problem is the description: its second bullet promises a change that is not in this PR at all.

Findings:

• should_fix, internal/netgw/network_test.go:141 — the described replyWait / 10s ICMP fix is absent from the diff; that flake is still live
• nit, internal/netgw/forward_test.go:259 — an outside port holder makes the overlap assertion pass vacuously
• nit, internal/netgw/forward_test.go:160 — each EADDRINUSE retry abandons a half-built Network

Verified:

• The retry guard reaches the errno. forward.go:269 wraps syscall.EADDRINUSE with a second %w beside ErrForwardExists; forwardHostVM returns the error unchanged (network.go:311-312) and New wraps it with %w (network.go:126). So errors.Is(err, syscall.EADDRINUSE) matches through both Expose and New.
• The self-holder branch deliberately does not wrap the errno — forward.go:266-267 has a single %w for ErrForwardExists only. A port this gateway already holds on another address therefore breaks out of the retry on the first attempt instead of burning 10 redraws on a message that would misname the holder. This is the right split and it is easy to get wrong.
• Outside-holder collisions really do surface as EADDRINUSE for both protocols, which is what makes the redraw reachable: TestExposeReportsAPortHeldOutsideTheGatewayAsPublished (forward_test.go:277) and TestExposeNamesTheForwardHoldingAPortItCannotBind (forward_test.go:291) can only reach ErrForwardExists through the EADDRINUSE branch at forward.go:260, and both pass.
• The reported failure was on order[0]'s Expose in TestExposeLeavesAnOverlappingAddressToTheKernel; that call is now routed through exposeFresh (forward_test.go:257). The remaining bind at line 258 already tolerates refusal.
• Sweep of freePort callers: every one that leads to a bind now goes through a redrawing helper. The single direct use left, "localhost:" + freePort(t) at forward_test.go:307, is rejected by validate (forward.go:145) before any socket is opened, so leaving it alone is correct.
• The earlier nit about the TCP/UDP pair is addressed: both binds go through one exposeFresh call (forward_test.go:338-340), and a failure on either unwinds the other and redraws (forward_test.go:135-144).
• f := f at forward_test.go:157 and the range copy at forward_test.go:127 keep f.Local from accumulating :port across retries.

Checks run:

• go test -run 'TestExpose|TestUnexpose|TestStartupForwards|TestParseForward' ./internal/netgw/ → ok
• go test -count=5 -run 'TestExpose|TestUnexpose|TestStartupForwards' ./internal/netgw/ → ok
• go test -count=3 -run TestICMPEchoIsAnsweredNotForwarded ./internal/netgw/ → ok, 1.5s for three runs, so roughly 0.5s against the unchanged 3s bound on an idle host
• -race is unavailable on this host: FATAL: ThreadSanitizer: unsupported VMA range. The -race results in your Testing section are unverified here.
• No revert check: the change touches only test scaffolding and its trigger is an environmental port collision, so there is no test that fails without it. That is inherent to the fix, not a gap in it.

Smaller things:

• The "probe is TCP, so a UDP holder is never seen" point is now stated in exposeFresh's comment (forward_test.go:112-113) and again at the call site (forward_test.go:335-337). The call-site copy earns its place by adding the "no way to draw again from here" reason, but it is a second copy to keep in sync.

Not checked:

• The darwin SO_REUSEADDR behavior both the description and the comment at forward_test.go:246-250 rest on. This host is Linux, so I confirmed only that the claim is consistent with the existing in-repo comment at forward.go:256-258.
• The TestEgress*CarriesRealBytes accumulation under -count=3, which the description defers to an issue.

Not checked:

• darwin SO_REUSEADDR behavior underlying the description and the comment at forward_test.go:246-250; this host is Linux, so the claim was only cross-checked against the existing comment at forward.go:256-258
• the author's -race results: the race detector aborts on this arm64 host with 'FATAL: ThreadSanitizer: unsupported VMA range'
• the TestEgress*CarriesRealBytes accumulation under -count=3, which the description defers to a separate issue
• no revert check was possible: the change is test scaffolding whose trigger is an environmental port collision, so no test fails without it

Task 068f68a2-56ed-4375-bc10-b88ddf096b00 on rp2, model claude-opus-5, effort medium.

Findings outside the diff:

• internal/netgw/forward_test.go:160: nit: On an EADDRINUSE redraw, the *Network that New half-built is abandoned rather than torn down, and this is the first code to call New in a loop, so a collision now multiplies the cost.

Failure: New returns nil on error, but everything built before the failing step stays alive — the tap endpoint and switch (network.go:113-119), the gvisor stack (network.go:121), and the DNS and DHCP servers with their ActivateAndServe goroutines (network.go:245-250, 270-277). The forward bind is the last step (network.go:251, forwardHostVM) and the only one that can return EADDRINUSE, so every redraw leaks one full stack plus its goroutines. On the contended Mac this adds up to nine of them in-process under -race, which is scheduler noise added to the test that was already timing out on scheduler noise.

Evidence: forward_test.go:160-172 drops n and continues; network.go:101-138 has no cleanup on any error return. The leaked listeners are on the virtual stack via gonet (network.go:256-263), not host sockets, so no host port is held.

I note your description defers the "networks are never closed" topic to an issue, and I am not reopening that here — this is only about the new retry loop making it per-attempt. A Close on the failed network before redrawing, if one is reachable, keeps the retry from paying for itself.

Findings outside the diff:

• internal/netgw/network_test.go:141: should_fix: The description's second bullet claims the ICMP reply wait "is now ten seconds, named (replyWait), and shared with the forwarder's dial wait", but no such change is in this PR, so the second of the two flakes it says it fixes is still live.

Failure: on the next loaded CI Mac run under -race, TestICMPEchoIsAnsweredNotForwarded times out at the same three-second bound and costs another re-run. A reader who trusts the PR body believes this was already raised to ten seconds and goes looking for a different cause, and the commit record now says a fix landed that did not.

Evidence: grep -rn replyWait . --include=*.go returns nothing across the whole tree. member.await still uses deadline := time.After(3 * time.Second) at network_test.go:141, and that is the wait the ICMP test blocks on (forwarder_test.go:321). The forwarder's dial wait is likewise still 3 * time.Second at forwarder_test.go:72. The diff is one commit (926bcf7, parent 159772e) touching only internal/netgw/forward_test.go.

Fix: either re-add the dropped replyWait commit, or drop that bullet from the body and the "Make the forward tests survive the shared CI Mac" framing from the title, and file the ICMP timeout separately so it is not recorded as fixed.

• internal/netgw/forward_test.go:259: nit: The second Expose accepts ErrForwardExists from any cause, including an outside process holding the port, so an outside holder turns this case into a pass that tests nothing. This is the one path left where the condition the PR is fixing still changes the test's meaning.

Failure: on the row {"127.0.0.1", "0.0.0.0"}, exposeFresh probes and binds 127.0.0.1:P successfully while another process holds 0.0.0.0:P, which the TCP probe never sees. The 0.0.0.0:P bind then fails with ErrForwardExists: ... is held outside this gateway, line 259 accepts it, and the test passes without ever observing the kernel's answer on the overlap. A regression where darwin starts refusing the 0.0.0.0/127.0.0.1 TCP pair would be invisible on exactly the runs where ports are contended.

Evidence: forward_test.go:258-262 asserts only err == nil || errors.Is(err, ErrForwardExists); forward.go:269 returns ErrForwardExists for the outside-holder case, and forward.go:266 returns it for the gateway's own. The two are distinguishable only by message text.

Fix: assert the refusal is not the outsider's, the way TestExposeNamesTheForwardHoldingAPortItCannotBind already does at forward_test.go:295 — on a non-nil error, fail if err.Error() contains "outside this gateway".

[claude-fleet]

Reviewed both commits (926bcf7, efa9729) against 159772e.

Approve. The two flakes are correctly diagnosed and the fixes match the diagnosis. Both nits from the previous round are addressed: the UDP bind in TestExposeAllowsOnePortOnTwoSpecificAddresses now goes through exposeFresh as a pair (forward_test.go:338-340), and the two 2s bounds at forwarder_test.go:248 and network_test.go:134 now use replyWait. One nit remains, below.

Verified:

• The retry predicate fires on the failure the PR describes. forwards.expose enters the outside-holder branch only under errors.Is(err, syscall.EADDRINUSE) and re-wraps that error with a second %w, so errors.Is holds at the call site — forward.go:255-269. The in-gateway-holder branch drops the syscall error, but no exposeFresh call site can reach it: the only multi-forward call uses two different protocols, and holderOfPort filters by protocol (forward.go:280-296, forward_test.go:338-340).
• startupNetworkFresh's claim that New "refuses the same way": New → addServices → forwardHostVM calls the same set.expose and returns its error through two %w wraps, so errors.Is(err, syscall.EADDRINUSE) survives — network.go:125-128, network.go:308-315.
• Every port that gets bound now comes from a retrying helper. The one remaining bare freePort is forward_test.go:307, where the address is rejected by ParseLocal before any bind, so no port is taken.
• send's new comment is accurate: m.conn is a net.Pipe end (network_test.go:62), so the write completes when the gateway's reader takes the frame.
• Everything replyWait covers is in-process: the forwarder dial is the test's own fake dialer that records and fails (forwarder_test.go:52-58), awaitUnknownL3 polls a stack counter (forwarder_test.go:250), await reads from the member goroutine's channel.
• Retry bookkeeping is correct: f is a per-iteration copy so f.Local += ":" + port does not accumulate across draws, and partially installed forwards are withdrawn before the next draw (forward_test.go:124-140).

Checks run:

• go test -race -count=2 -run 'TestExpose|TestUnexpose|TestStartup|TestNewRefuses|TestICMPEchoIsAnsweredNotForwarded|TestIPv6NeverEgresses' ./internal/netgw/ → ok, 5.1s
• go vet ./internal/netgw/ → clean

No revert check: this is a test-only change with no production line to revert.

Smaller things:

• startupNetworkFresh leaks a whole *Network per failed draw — the stack, the DNS/DHCP goroutines and their gonet listeners are all up by the time forwardHostVM fails (network.go:121-128), and there is no Network.Close in the package to call. That is the accumulation your description blames for the -count=3 TestEgress*CarriesRealBytes failures, now reachable one more way. Rare enough not to matter in CI, and not fixable without the close path that issue would add; worth a sentence in it.
• network_test.go:144 wraps mid-clause ("so this is a" / "liveness bound") — the rest of the comment block is filled to width.

Not checked:

• Whether the two tests still flake on the real CI Mac under its actual load -- the package was run here on Linux only.
• The PR's claim that -count=3 -race fails the three TestEgress*CarriesRealBytes tests on unmodified main; not reproduced, and out of scope by the author's own statement.

Task 4898a990-afd1-4d41-ad78-adcc93007ff6 on ravel-clickbench-ubuntu-2, model claude-opus-5, effort medium.

[claude-fleet]

nit: replyWait's argument covers two 3s bounds in egress_test.go that this round left behind, at exactly the value the comment says was crossed on the CI Mac.

Failure: on the same loaded Mac under -race, TestWatchHostsResolvesBeforeTheFirstTick fails at "WatchHosts did not resolve before its first tick" after 3s, or TestWatchHostsStopsWithTheContext at "WatchHosts outlived its context" after 3s. Both wait on work computed in-process -- a fake rotating resolver and a context cancellation -- which is the class replyWait is defined for, and three seconds is the bound your own comment records as crossed.

Evidence: egress_test.go:399 and egress_test.go:418 still hold 3 * time.Second; replyWait's comment at network_test.go:140-150; the same package, so the constant is already in scope there.

Fix: use replyWait at both, the way forwarder_test.go:248 and network_test.go:134 now do.

[ananos]

@claude-fleet review

[claude-fleet]

Review task queued.

Task: d13251ff-7a17-474f-98de-55fa5d6e7f75
Commit: 9c30479
Model: claude-opus-5 (effort medium)
Executor: rp2
Status: done. Review posted.

[claude-fleet]

Restored the workspace: git status --porcelain shows only the untracked node_modules.

---

Approve. The retry and the widened bound both do what the description says; one nit about a latent trap in the new helper's variadic contract.

Verified:

• Expose's refusal for a port held outside the gateway wraps both ErrForwardExists and the underlying EADDRINUSE (forward.go:269-270, multi-%w), so exposeFresh's errors.Is(err, syscall.EADDRINUSE) test at forward_test.go:141 actually matches the failure the CI run hit. The same holds through New: addServices returns the expose error unaltered (network.go:311-312) and New wraps it with %w (network.go:127), so startupNetworkFresh:172 matches too.
• The second Expose in each test — the one exposeFresh deliberately does not cover — stays correct with an outside holder present. forward_test.go:221, :240 and :260 accept ErrForwardExists, which is what an outside holder produces; forward_test.go:292 additionally requires the message to name 0.0.0.0:P and not say "outside this gateway", and holderOfPort is consulted before that branch (forward.go:265-270) and always finds the gateway's own udp/0.0.0.0:P, so the assertion does not depend on who else holds the port.
• The prior round's two nits are addressed: forwarder_test.go:248 and network_test.go:134 now use replyWait, as do egress_test.go:399 and :418. After this diff no in-process liveness bound in the package is left at 2s or 3s.
• member.send's new comment holds: the member is attached over net.Pipe() (network_test.go:62), which is synchronous, so the write completes when the gateway's read side takes the frame — the same in-process work await waits on.
• replyWait's comment claim that the forwarder dial is computed in-process: the test dialer pushes to dials and returns an error without touching the host (forwarder_test.go:47-60).
• Sweep for other copies of the probe-then-rebind pattern: net.Listen("tcp", ...:0) appears only at forward_test.go:97, forward_test.go:272 (a deliberate holder) and egress_e2e_test.go:64 (a real server that keeps its listener). Nothing else in the repo draws a port and rebinds it later.

Checks run:

• go test -count=1 ./internal/netgw/ → ok (5.5s).
• go test -count=5 -run 'TestExposeLeavesAnOverlappingAddressToTheKernel|TestICMPEchoIsAnsweredNotForwarded' ./internal/netgw/ → ok.
• Retry path proven, not just read: I temporarily made both helpers bind a real listener on the port drawn on attempt 0, forcing the exact collision the PR describes on every call. go test -run 'TestExpose|TestStartupForwards|TestUnexpose' still passes, so the redraw (and the withdrawal of the partially-installed set) carries the tests through a taken port. Before this PR that same collision is the held outside this gateway failure asserted at forward_test.go:271-283. File restored.
• go test -race is unavailable here: the sandbox is arm64 with a 47-bit VMA, and the race detector aborts with FATAL: ThreadSanitizer: unsupported VMA range.

Smaller things:

• The description covers exposeFresh and the echo-reply wait, but the diff also widens awaitUnknownL3 (forwarder_test.go:248), the member write deadline (network_test.go:134) and the two WatchHosts waits (egress_test.go:399, :418). Those came out of the review round; the text still reads as if only the dial wait was shared.
• expectNoDial keeps 500ms (forwarder_test.go:83). That is the right direction for a negative assertion — widening it only costs wall-clock — but on the loaded machine this PR is written for it can pass before the forwarder would have dialed at all. Pre-existing, no change asked.

Not checked: the -race claims in the Testing section (no working race detector here), and the -count=3 TestEgress*CarriesRealBytes failure the description attributes to unmodified main.

Not checked:

• The -race runs claimed in the Testing section: this machine is arm64 with a 47-bit VMA and ThreadSanitizer aborts with unsupported VMA range, so every run above was without the race detector.
• The -count=3 SYN-ACK failures in TestEgress*CarriesRealBytes that the description attributes to unmodified main; not reproduced and out of this diff's scope.

Task d13251ff-7a17-474f-98de-55fa5d6e7f75 on rp2, model claude-opus-5, effort medium.

[claude-fleet]

nit: exposeFresh retries only on a refusal that carries EADDRINUSE, but the doc invites a caller whose refusal will not carry it. The comment at forward_test.go:107 says each Local is "a host with no port -- "127.0.0.1", "0.0.0.0", or """, so a later caller may pass two same-protocol forwards on different hosts.

Failure: exposeFresh(t, n, Forward{Local: "0.0.0.0", ...}, Forward{Local: "127.0.0.1", ...}) on Linux. The first bind takes the wildcard; the second refuses with EADDRINUSE, holderOfPort finds the gateway's own tcp/0.0.0.0:P, and forward.go:266-267 returns an ErrForwardExists that does not wrap the syscall error. errors.Is(err, syscall.EADDRINUSE) is false, the loop breaks on the first attempt, and the test dies with Expose on a fresh port: ... -- a message that blames the port draw when the real answer is that the two addresses cannot share a port on this kernel, which is a fact about the gateway rather than a flake.

Evidence: forward.go:265-270 (the own-holder branch returns ErrForwardExists alone, the outside-holder branch at :269 wraps err as well); forward_test.go:141. The two current call sites are unaffected -- the only multi-forward call pairs tcp with udp (forward_test.go:340-342), and holderOfPort filters by protocol (forward.go:292-294).

Fix: say in the helper's doc that the forwards must differ in protocol, or treat ErrForwardExists from a bind the way EADDRINUSE is treated and let the caller's own assertion rule on it.

[ananos]

@claude-fleet review

[claude-fleet]

Review task queued.

Task: 668ea609-9da1-4c59-9d5b-d41f08f06974
Commit: 7c0325b
Model: claude-opus-5 (effort medium)
Executor: rp2
Status: done. Review posted.

[claude-fleet]

Verified the package, ran the tests, and read the code behind the PR's claims.

Approve. One nit; nothing blocks.

The diff does what the description says. exposeFresh and startupNetworkFresh redraw only on a refusal carrying EADDRINUSE, which is the refusal that comes from an outside holder, so a real gateway bug still fails the test instead of looping. replyWait is applied to every in-process liveness bound the package has; the only remaining literal bounds are expectNoDial's negative 500ms and 10ms poll/tick intervals, which is correct.

Verified:

• Expose's outside-holder refusal wraps the errno, so errors.Is(err, syscall.EADDRINUSE) in the helpers matches — forward.go:269-270; the own-holder branch at forward.go:265-267 does not wrap, which is what the helper's protocol rule is there to keep out of reach.
• New's error chain preserves the errno through %w at every hop, so startupNetworkFresh's redraw condition fires: network.go:125-127 → network.go:251 → network.go:311.
• holderOfPort filters by protocol (forward.go:291-294), so the tcp+udp pair in TestExposeAllowsOnePortOnTwoSpecificAddresses takes the outside-holder branch on a collision and is redrawable — forward_test.go:348-351.
• The retry withdraws what it installed before drawing again, and expose writes its record only after the bind succeeds, so there is nothing to withdraw for the forward that failed — forward_test.go:145-150, forward.go:259-275.
• The replyWait sweep is complete: egress_test.go:399,418, forwarder_test.go:72,248, network_test.go:134,155. The two earlier review nits asking for forwarder_test.go:248, network_test.go:134 and the two egress waits are all addressed.
• The earlier nit about the uncovered second Expose at the old forward_test.go:290-292 is addressed: both binds now go through the helper together (forward_test.go:345-351).
• expectNoDial keeps 500ms as a negative check — forwarder_test.go:83.

Checks run:

• go test -count=1 ./internal/netgw/ → ok (5.5s)
• go test -count=3 -run 'TestExpose|TestStartup|TestUnexpose' ./internal/netgw/ → ok
• go test -race ./internal/netgw/ → could not run here: FATAL: ThreadSanitizer: unsupported VMA range on this arm64 host. The race-detector run is CI's.

No revert check: the PR adds no regression test for a product fix, and the helper's redraw path only runs when an outside process holds the drawn port, which is not reproducible without writing probe code.

Smaller things:

• The redraw branches themselves (forward_test.go:145-154, 180-183) run only on a collision, so CI never exercises them; a mistake in the withdrawal would surface as a new flake rather than a failure.
• freePort can hand back the same number on consecutive draws, so a port held on 0.0.0.0 could in principle burn several of the 10 attempts. Ephemeral allocation rotates, so this is unlikely to exhaust them.
• "a SO_REUSEADDR socket, which every Go listener is" (forward_test.go:114-115) holds for stream listeners; Go does not set SO_REUSEADDR on a unicast ListenPacket. The sentence's conclusion is unaffected because the probe is TCP, as the next clause says.

Not checked:

• The race-detector run: go test -race ./internal/netgw/ aborts on this host with FATAL: ThreadSanitizer: unsupported VMA range, so the -race and -count=5 results in the Testing section are the author's and CI's, not reproduced here.
• The behaviour of the redraw branches under a real port collision (forward_test.go:145-154 and 180-183); no outside holder occurred during the runs.
• Issue #118 and the TestEgress*CarriesRealBytes accumulation it tracks; out of this diff.

Task 668ea609-9da1-4c59-9d5b-d41f08f06974 on rp2, model claude-opus-5, effort medium.

[claude-fleet]

nit: The guard compares the raw Protocol field, so it misses the pair it exists to catch when one of the two is the empty default.

Failure: exposeFresh(t, n, Forward{Local: "127.0.0.1", Remote: ...}, Forward{Protocol: "tcp", Local: "0.0.0.0", Remote: ...}) passes the guard with keys "" and "tcp", and both forwards are installed as tcp. On Linux the second bind refuses with EADDRINUSE, holderOfPort finds the gateway's own tcp/127.0.0.1:P, and forward.go:265-267 returns an ErrForwardExists with no errno wrapped, so errors.Is(err, syscall.EADDRINUSE) is false, the loop breaks on the first attempt, and the test dies with Expose on a fresh port: ... -- the port-draw message for a question about address sharing, which is exactly what the guard is meant to prevent. Protocol: "TCP" beside "tcp" slips through the same way.

Evidence: forward_test.go:124-130 keys on f.Protocol as passed; validate normalizes "" to "tcp" and lowercases only inside expose, on its own copy (forward.go:82-87, forward.go:161-171); forward.go:265-267 is the unwrapped refusal. No current caller hits it -- the only multi-forward call sets both protocols explicitly (forward_test.go:348-351).

Fix: key the map on parseProtocol(f.Protocol)'s result rather than the raw field.