Repository navigation
Conversation
…them The metrics_endpoint control-plane counter test and the dead_endpoint helpers in four other test binaries bound 127.0.0.1:0, read the port, dropped the listener and then dialed it. Under nextest's parallel run another test can bind that freed port in between, so the "closed port" GETs got HTTP responses: calls read 6 instead of 3 in the merge queue. A new tests/support/refused_addr.rs keeps a TCP socket bound to a loopback port for the life of the process without ever listening on it, with SO_REUSEADDR and SO_REUSEPORT both cleared. A connect to it is refused at once, and no other socket can bind the port while it lives. The failing test keeps its exact calls: 3 assertion. Fixes: #3468 Changelog: none Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
|
@claude-fleet review |
|
Review task queued. Task: 91f9d87b-4717-419a-b6fc-9d621930c23b |
There was a problem hiding this comment.
Reviewed the diff against origin/main (parent f609a54), read all six files, and checked the mechanism claims against tokio's source and the counter code the test asserts on.
The fix is sound. Holding a bound-but-never-listening socket is the right shape for this: the port stays in the bind table, so another process binding 127.0.0.1:0 cannot be handed it, and a SYN to it gets an RST rather than being accepted. Clearing SO_REUSEADDR is load-bearing, not decoration — a duplicate bind to the same addr:port is permitted only when both sockets set it and the holder is not in LISTEN, which is exactly the state this socket sits in. The module comment says that correctly.
Approve. Two nits below, neither blocking.
Findings:
- nit, services/ravel-server/tests/support/refused_addr.rs:1 — helper lives where the eight acknowledged follow-up sites cannot reach it
- nit, services/ravel-server/tests/support/refused_addr.rs:21 — the identical
callsexposure inbucket_config/tests.rsis deferred with no issue number
Verified:
record_control_plane_call()runs only after a successful send, so a refused connect leavescallsunmoved — crates/ravel-object-store/src/s3/bucket_config.rs:1493-1498- the failing test keeps its exact assertion (
requests: 6, calls: 3, response_bytes: 568) and the rendered-family check — services/ravel-server/tests/metrics_endpoint.rs:618-645 - the removed
drop(closed)was the only bind/drop site inmetrics_endpoint.rs; the other listener at :540 is kept alive byaxum::serve— services/ravel-server/tests/metrics_endpoint.rs:540-547 - all five call sites converted, none left
.awaiting the now-syncdead_endpoint()— alerting_e2e.rs:1461-1478, analytics_endpoint.rs:821-871, federation_e2e.rs:379-501,727, federation_handler_warnings.rs:132-177 TcpSocketis{ inner: socket2::Socket }and neithernew_v4norbindtouches the reactor, so a process-widestatic OnceLock<(TcpSocket, SocketAddr)>is sound and needs no runtime — tokio-1.53.3/src/net/tcp/socket.rs:91, 163-177, 805-807tests/support/is a subdirectory, so the new file is not auto-discovered as its own test target; same placement as the existingstrict_flush.rs/fragment_tls.rs
Checks run:
cargo fmt --all --check→ exit 0, no diffgrep -rn "dead_endpoint\|refused_addr" services/ravel-server/tests/→ 5 binaries, all converted, no stale.await
Smaller things:
set_reuseportis cfg-gated to unix minus solaris/illumos/cygwin/nuttx (tokio socket.rs:288-303). Fine for this repo's targets; it would stop the test tree compiling on Windows, which nothing here claims to support.- Sharing one address across every test in a binary is a behaviour change under
cargo test(one process, many tests). I looked for process-global state keyed on endpoint in the federation path and found none — each test builds its ownFederationviaArc::new(dead_federation(...))— so the tests stay independent. - Five
dead_endpoint()wrappers now exist that each delegate one line to the same helper, each with its own doc comment restating the same fact. Callingrefused_addr::refused_addr().to_string()at the four use sites would drop four shims and four copies of the comment.
Not checked:
- No build or test run. A cold
ravel-servertest build is not something I can usefully repeat here, and the PR states PR CI is the gate for this change; the compile-level claims above come from reading tokio's source, not from compiling. - Issue #3468 and whether the follow-up list has been filed anywhere.
- Six of the eight out-of-scope sites named in the description (I opened only
crates/ravel-object-store/src/s3/bucket_config/tests.rs).
Not checked:
- No build, clippy or test run: a cold ravel-server test build is impractical here and the PR states PR CI is the gate. Compile-level claims come from reading tokio 1.53.3 source, not from compiling.
- Issue #3468 itself, and whether any follow-up issue has been filed for the eight listed sites.
- Six of the eight out-of-scope sites named in the description; only crates/ravel-object-store/src/s3/bucket_config/tests.rs was opened.
Task 91f9d87b-4717-419a-b6fc-9d621930c23b on pmoust-mbp, model claude-opus-5, effort medium.
| @@ -0,0 +1,34 @@ | |||
| //! A loopback address whose connections are refused for the life of the test | |||
There was a problem hiding this comment.
nit: The helper is a #[path]-included module under services/ravel-server/tests/support/, so only ravel-server's own test tree can use it. The description lists eight further sites with this exact bug, and five of them are in other crates (ravel-object-store, ravel-sql, ravel-promql-difftest, ravel-bench, ravel-tracing-export), which means each follow-up has to copy this file rather than call it.
Failure: no runtime failure; the cost lands on the follow-ups, which either duplicate the module or get skipped.
Evidence: crates/ravel-test-support already exists as the workspace's dev-only shared test scaffolding (crates/ravel-test-support/src/lib.rs:1-11), is publish = false, and services/ravel-server/Cargo.toml:342 already dev-depends on it. Putting refused_addr() there turns every follow-up into a dev-dependency line. It would need tokio (or socket2) added to that crate's [dependencies], which today is parking_lot only.
| static REFUSED: OnceLock<(TcpSocket, SocketAddr)> = OnceLock::new(); | ||
|
|
||
| /// The process-wide refused address. | ||
| pub fn refused_addr() -> SocketAddr { |
There was a problem hiding this comment.
nit: The sweep for other copies of this bug found a site with the same assertion shape that stays unfixed, and the description defers it as a "follow-up to file" with no issue number, so there is nothing tracking it.
Failure: crates/ravel-object-store/src/s3/bucket_config/tests.rs:3548-3552 binds 127.0.0.1:0, reads the address, drops the listener, probes it, and then asserts calls: 0 at :3561. A parallel test binding the freed port and answering makes calls nonzero, which is the same merge-queue failure as #3468 with a tighter assertion.
Evidence: lines 3548-3561 of that file; compare metrics_endpoint.rs:613 before this change.
Fix: file the follow-up and put its number in the description, or fold that one site in here if the helper moves to ravel-test-support.
metrics_render_get_unverified_and_control_plane_countersfailed in the merge queue for #3461 with control-planecallsreading 6 where the test expects 3. Its second half binds127.0.0.1:0, reads the port, drops the listener and dials the address as a closed port.callscounts only GETs that got an HTTP response:record_control_plane_call()runs after the send succeeds, incrates/ravel-object-store/src/s3/bucket_config.rs. Three extra calls therefore means another test running in parallel under nextest had bound the freed port in between.The fix.
tests/support/refused_addr.rskeeps a TCP socket bound to a loopback port for the life of the test process.SO_REUSEADDRandSO_REUSEPORTcleared, so a connect to it is refused at once and no other socket can bind the port while it lives.dead_endpoint()helpers in four other test binaries (federation_e2e,federation_handler_warnings,alerting_e2e,analytics_endpoint) use it.calls: 3assertion.This is the first build of the change: the executor's box was short of disk for the
ravel-servergate, so PR CI is the gate.rustfmt --checkand the authorship guard pass.Out of scope, same pattern, tracked in #3506, which also proposes moving the helper into
ravel-test-support. Each of these binds a port, drops the listener, then dials the port as closed:services/ravel-server/src/alerting.rsdead_sink_url;services/ravel-server/src/service/tests.rsdead_federation;crates/ravel-object-store/src/s3/bucket_config/tests.rs, which has the samecallsassertion exposure;crates/ravel-sql/tests/flight_distributed.rsrefused_port;crates/ravel-promql-difftest, two sites;crates/ravel-bench/tests/sql_latency_flight_smoke.rs;crates/ravel-tracing-export/src/tests.rs, two sites.Fixes: #3468