Repository navigation
Conversation
…port 0
Authority::serve bound UDP to the requested address, then bound TCP to the
port number the kernel had given the UDP socket. UDP and TCP have separate
port spaces, so a port free for UDP says nothing about TCP, and the TCP bind
failed with "binding TCP 127.0.0.1:<port>" whenever any other socket on the
host held that TCP port. The aggregator DNS tests (and every subzone_fixture
test) bind 127.0.0.1:0 in a binary where other tests hold many loopback TCP
sockets, so they failed intermittently, on main too.
Measured with a probe that does the same two binds, SO_REUSEADDR set on the
TCP socket as tokio does: 16 of 2000 binds failed on this shared box with no
ballast, 98 with 1,000 loopback TCP listeners held, 368 with 5,000.
serve now asks the kernel for each protocol's port when the requested port is
0, and returns a Served { udp, tcp }. A fixed port is still both protocols'
port. The tests' ask_dns sends UDP to served.udp and TCP to served.tcp.
Regression test dns_port_zero_is_asked_of_the_kernel_once_per_protocol: 16
port-0 serves, at least one must have different UDP and TCP ports. With TCP's
port copied from UDP's it fails every run; it passed in 75 of 75 with the fix.
With 12,000 loopback TCP listeners held, the two aggregator tests failed 17 of
20 runs on the old bind and 0 of 20 with the fix.
Tested: make test-fleet-coordinator (75 passed), make clippy
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 33 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddbedc5a3c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let tcp = tokio::net::TcpListener::bind(SocketAddr::new( | ||
| listen.ip(), |
There was a problem hiding this comment.
When --dns-listen specifies a scoped IPv6 link-local address, reconstructing it with SocketAddr::new(listen.ip(), ...) discards its scope ID. UDP binds successfully, but TCP then fails with EINVAL, causing coordinator startup to abort even for a fixed port that previously worked. I reproduced the bind sequence against a local link-local interface: preserving the UDP socket address succeeds, while rebuilding it without the scope fails. Bind TCP directly to listen, which already preserves both the requested port-zero behavior and the IPv6 scope.
Useful? React with 👍 / 👎.
Mechanism
Authority::serve(src/fleet/coord/authority.rs) bound UDP to the requested address, then bound TCP to the port number the kernel had picked for UDP. UDP and TCP have separate port spaces, so a port that is free for UDP says nothing about TCP. The TCP bind failed withbinding TCP 127.0.0.1:<port>whenever any other socket on the host held that TCP port.subzone_fixture_with(used by the two aggregator DNS tests and the other subzone tests) serves on127.0.0.1:0, in a binary where other tests hold loopback TCP sockets.Measured with a probe doing the same two binds (SO_REUSEADDR on the TCP socket, as tokio does): 16/2000 binds failed on this shared box with no ballast, 98/2000 with 1,000 loopback TCP listeners held, 368/2000 with 5,000.
Change
With port 0,
serveasks the kernel for each protocol's port and returnsServed { udp, tcp }. A fixed port is still both protocols' port.ask_dnsin the tests sends UDP toserved.udpand TCP toserved.tcp. The production caller only logs the result.Evidence
dns_port_zero_is_asked_of_the_kernel_once_per_protocol: 16 port-0 serves, at least one pair must differ. With TCP's port copied from UDP's (the old behavior, restored for the check) it fails every run; with the fix it passes (75/75 inmake test-fleet-coordinator).a_host_aggregator_is_named_in_the_subzone_and_answers,the_ec2_aggregator_is_named_in_the_subzone_and_answers) failed 17/20 runs on the old bind and 0/20 with the fix.make clippyclean.