Skip to content

Use a never-listened port for the refused-connection fixtures - #633

Merged
heifner merged 1 commit into
masterfrom
fix/http-test-port-reservation
Sep 18, 2026
Merged

heifner merged 1 commit into
masterfrom
fix/http-test-port-reservation

Conversation

@heifner

@heifner heifner commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

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. WSL2 networkingMode=mirrored is one such host — loopback is bridged with the Windows side rather than using the kernel's loopback path:

delay after close() connects succeeded
0–5 ms 6/6
10 ms 1/6
≥50 ms 0/6

The connect therefore succeeds and the peer resets during the header read, which the transport correctly classifies as failure_kind::io rather than failure_kind::connect. The same probe under unshare -rn refuses 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_safely is 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_preserved reporting 1 != 2 and idempotent_retry_exhaustion_is_bounded losing its retry_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_preserved is the proof. It asserts resolution counts of 2/2/1/2, which only come out right when a genuine failure_kind::connect fires 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_bounded passed before this change (it only asserts retry_exhausted, which both failure kinds produce) and is converted too, for consistency with the other three.

@heifner
heifner marked this pull request as ready for review September 18, 2026 18:05
@heifner
heifner requested review from a team and huangminghuang September 18, 2026 18:05

@huangminghuang huangminghuang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 port 0, bypassing the shared BindConfigProvider registry. The kernel can select a port already claimed in /tmp/wire-platform-bind-config by 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 leaves SO_REUSEADDR enabled, so the reservation is not exclusive. On Linux I reproduced a normal Boost.Asio tcp::acceptor binding/listening on that exact endpoint while reserved_port remained 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.
@heifner
heifner force-pushed the fix/http-test-port-reservation branch from 4c0c325 to 2971d86 Compare September 18, 2026 19:53
@heifner heifner changed the title Reserve the refused port in the http transport tests Use a never-listened port for the refused-connection fixtures Sep 18, 2026
@heifner

heifner commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Reworked in 2971d86 — and first, the approach you reviewed was broken on macOS, which CI caught before either of your findings mattered.

dns_cache_refresh_policy_is_preserved reported 1 != 2 and idempotent_retry_exhaustion_is_bounded lost its retry_exhausted, both consistent with a bound-but-unlistening socket never being classified as a connect failure there. macOS is green on master and on #632, which carries the same file without this change, so the regression was mine. I had only checked Linux and WSL2 and claimed portability I had not tested.

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 unshare -rn. That takes test_fc on WSL2 from four failures to one.

stale_metadata_reconnect_failure_cleans_up_safely is deliberately left alone: it needs a real server for its first request, so its port genuinely listens and no choice of port helps it. It fails on WSL2 only, as it did before.

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. BindConfigProvider and /tmp/wire-platform-bind-config do not appear anywhere in wire-sysio, in C++ or Python. There are already ten port-0 binds across the libfc tests, scripted_http_server among them, so routing through a registry would be a repo-wide change to unit tests that never start a cluster, rather than something this PR introduces.

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.

@huangminghuang huangminghuang left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-ts cluster ports come from the bind registry. In wire-tools-ts cluster tooling and its tests, any port that a managed cluster process binds or dials, or that is pinned in cluster configuration, must be obtained from BindConfigProvider (findAvailable, findAvailableRange, or resolve). 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 huangminghuang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@heifner
heifner merged commit e4a91d9 into master Sep 18, 2026
13 checks passed
@heifner
heifner deleted the fix/http-test-port-reservation branch September 18, 2026 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants