Use a never-listened port for the refused-connection fixtures - #633
Conversation
huangminghuang
left a comment
There was a problem hiding this comment.
I found two determinism issues in the new port-reservation fixture:
-
libraries/libfc/test/network/test_http_client.cpp:441: this still asks the kernel for port0, bypassing the sharedBindConfigProviderregistry. The kernel can select a port already claimed in/tmp/wire-platform-bind-configby a cluster process that has not bound yet, or one in the reserved Agave band. Keeping the socket open fixes the local close/reuse race but not this cross-process registry race. Please originate both the fresh reservation and stale-server reclaim from a registry-issued port. -
libraries/libfc/test/network/test_http_client.cpp:449: the explicit reclaim path leavesSO_REUSEADDRenabled, so the reservation is not exclusive. On Linux I reproduced a normal Boost.Asiotcp::acceptorbinding/listening on that exact endpoint whilereserved_portremained alive, after which a client connected successfully; Windows documents similarly indeterminate takeover semantics. Please make the post-reclaim reservation exclusive on every supported platform, or inject a deterministic connect failure, and add a regression asserting that a competing listener cannot bind.
The http transport tests opened an acceptor on an ephemeral loopback port, closed it, and connected expecting ECONNREFUSED. Listening is what breaks that: on a host whose listener teardown is asynchronous -- WSL2 mirrored networking among them -- the port keeps accepting for several milliseconds after close and then resets, which the transport correctly classifies as an io failure rather than a connect failure. Bind a plain socket to obtain a free port, never listen on it, and release it. The port is unconnectable for the same reason it was before, without ever having entered the listening state that opens that window. Holding a bound-but-unlistening socket open instead would also close the local port-reuse race, but a bound-unlistening socket does not yield a connect failure on macOS, so it is not portable. stale_metadata_reconnect_failure_cleans_up_safely is unchanged: it needs a real server for its first request, so its port genuinely listens and no choice of port helps it.
4c0c325 to
2971d86
Compare
|
Reworked in 2971d86 — and first, the approach you reviewed was broken on macOS, which CI caught before either of your findings mattered.
The fix now keeps the port released, exactly as the original fixture did, and changes one thing: the socket never listens. Listening is what opens the window — a port that has listened keeps accepting briefly after close on hosts with asynchronous listener teardown, while a port that never listened is refused immediately. Verified 20/20 refused on WSL2 mirrored networking, and the full suite passes under
On the SO_REUSEADDR finding — moot now, since no reservation is held. You were right that it was not exclusive, though the reservation approach turned out to be unsound for the unrelated macOS reason above, so it is gone entirely. The rejected approach is recorded in the source comment so it is not retried. On the registry finding — I could not find the infrastructure it refers to in this repo. If you were thinking of wire-platform or the cluster tooling, agreed that it applies there — but let me know if there is a wire-sysio-side registry I have missed, and I will wire these through it. |
There was a problem hiding this comment.
Correction to my previous review: the bind-registry requirement I cited comes from the workspace-root AGENTS.md, which is a symlink to wire-platform-manifest/CLAUDE.md. Its current “EVERY port” wording overstates a wire-tools-ts-specific cluster-tooling rule as a requirement for every sibling repository. BindConfigProvider and /tmp/wire-platform-bind-config are not implemented or exposed in wire-sysio, so that documentation issue is not a blocker for this PR.
I suggest replacing that paragraph in wire-platform-manifest/CLAUDE.md with:
wire-tools-tscluster ports come from the bind registry. Inwire-tools-tscluster tooling and its tests, any port that a managed cluster process binds or dials, or that is pinned in cluster configuration, must be obtained fromBindConfigProvider(findAvailable,findAvailableRange, orresolve). Do not use fixed bind-port literals,listen(0), or another allocation path outside that registry. This rule does not apply to standalone code or unit tests in sibling repositories unless they explicitly participate in the shared cluster registry. See.claude/rules/bind-available-ports-not-fixed.md.
The close-before-connect race noted previously already existed in these fixtures; this PR does not introduce it. It replaces the listened-and-closed socket with a never-listened socket to address the observed WSL2 behavior. With the current required checks green, I found no remaining issue and am approving.
huangminghuang
left a comment
There was a problem hiding this comment.
Re-reviewed current head 2971d86c. The manifest-scoping concern is not a wire-sysio blocker, the close-before-connect exposure is pre-existing, the updated fixture addresses the WSL2 listener-teardown failure, and all required Linux, sanitizer, package, and Apple Silicon checks are green. No remaining issues found.
Four http transport test cases opened an acceptor on an ephemeral loopback port, closed it, and then connected expecting
ECONNREFUSED.Listening is what breaks that. On a host whose listener teardown is asynchronous, the port keeps accepting for a few milliseconds after
close()and then resets. WSL2networkingMode=mirroredis one such host — loopback is bridged with the Windows side rather than using the kernel's loopback path:close()The connect therefore succeeds and the peer resets during the header read, which the transport correctly classifies as
failure_kind::iorather thanfailure_kind::connect. The same probe underunshare -rnrefuses 8/8, confirming it is the mirrored network path rather than the kernel — which is why CI on real Linux is green.The fix
Bind a plain socket to obtain a free port, never listen on it, and release it. The port is unconnectable for exactly the reason it was before, without ever entering the listening state that opens the window. Verified 20/20 refused on mirrored networking, and the full suite passes under
unshare -rn.stale_metadata_reconnect_failure_cleans_up_safelyis deliberately unchanged. It needs a real server for its first request, so its port genuinely listens and no choice of port helps it; it still fails on WSL2 and passes everywhere else, as before.What did not work
Holding a bound-but-unlistening socket open for the test's duration also refuses reliably on Linux and WSL2, and additionally closes a port-reuse race — the released port can in principle be claimed by another process before the connect. But a bound-unlistening socket does not produce a connect failure on macOS: CI failed there with
dns_cache_refresh_policy_is_preservedreporting1 != 2andidempotent_retry_exhaustion_is_boundedlosing itsretry_exhausted, both consistent with the connection attempt never being classified as a connect failure. Portability wins over closing that race, and the rejected approach is recorded in the source comment so it is not retried.Semantics are unchanged
dns_cache_refresh_policy_is_preservedis the proof. It asserts resolution counts of 2/2/1/2, which only come out right when a genuinefailure_kind::connectfires and triggers the DNS-cache refresh — an io failure yields 1, which is how it was failing. It passes now, so the transport really is producing connect failures rather than the tests being loosened.idempotent_retry_exhaustion_is_boundedpassed before this change (it only assertsretry_exhausted, which both failure kinds produce) and is converted too, for consistency with the other three.