fix(relay): remove the bind race in the relay test fixture

[iamquang95]

Fix #590

Relay startup is split into bind then serve:

• bind_relay() binds every listener (monitoring, ENR HTTP, libp2p) and waits for libp2p to report the addresses it bound, before anything is served. BoundRelay::serve() then runs them as select! arms, so exiting drops the listeners.
• Bind failures are now typed errors that fail the relay instead of a warning.
• Tests use port 0 everywhere and read back what was bound, so the race is unrepresentable. Retry/backoff polling is gone (backon dropped from cli dev-deps).

Two follow-ons from kernel-assigned ports: external addrs are now derived from the bound addresses (external_multiaddrs(cfg, listen_addrs) + Node::set_advertised_addrs) rather than the configured ones, which would advertise an undialable port 0; and private-address filtering moved from ingest to render time, since startup waits on every listener and a loopback-only node must still start.

[iamquang95]

@claude review this PR

[claude]

Claude finished @iamquang95's task in 7m 9s —— View job

---

Review — fix(relay): remove the bind race in the relay test fixture

• Read the PR diff
• Review relay-server changes (error.rs, p2p.rs, web.rs)
• Review p2p utils changes
• Review test fixture changes in relay.rs
• Cross-check Charon parity (charon/cmd/relay/relay.go)
• Post review

---

Verdict

Solid, well-reasoned change. The diagnosis is right — retry_get was hiding a permanently-dead relay behind ConnectionRefused retries — and the fix attacks the cause rather than the symptom. The three structural pieces all check out:

• Charon parity confirmed. In charon/cmd/relay/relay.go:93-106, the monitoring goroutine's server.ListenAndServe() result is pushed onto the same buffered serverErr channel as the ENR server, and Run returns on the first value (relay.go:141-148). So promoting the monitoring failure from warn! to fatal is correct parity, and mpsc::channel(3) matches Go's make(chan error, 3).
• Binding monitoring before spawning the ENR server (crates/relay-server/src/p2p.rs:85-97) is a deliberate divergence from Charon (which starts monitoring after), and a good one — it makes "ENR serves" imply "all listeners bound" and removes the leaked-listener path. Well documented in the comment.
• break instead of return Err in the select loop (p2p.rs:130-136, p2p.rs:188-191) so cancel + joins still run: correct, and it's what makes fixture_leaves_no_listener_bound_when_startup_fails meaningful.

Typed io::Error in the bind variants + Error::source() walking instead of string matching is the right call, and the is_addr_in_use_detects_bind_races_through_error_chain unit test pins it. CliError::RelayP2PError(#[from] ..) → RelayP2PError::FailedToBindHttpListener{ #[source] .. } → io::Error gives thiserror a complete chain, so the walk works.

I couldn't run cargo check/cargo test (sandbox blocks cargo), so everything below is from reading. Comments are ordered by severity; nothing here is a blocker.

---

1. The readiness probe can be satisfied by someone else's relay — the race isn't fully closed

crates/cli/src/commands/relay.rs:706-724 — start_relay races the relay task's exit against wait_until_serving. But wait_until_serving only asks "does something answer 200 on this port", and the port is exactly the one that may have been lost to another concurrently-running test in the same binary.

Sequence (all tests in this file run concurrently under cargo test):

1. Test A calls free_tcp_addr() → port P, socket closed.
2. Test B's free_tcp_addr() also returns P (see §3), B's relay binds P first and starts serving /enr.
3. A's relay reaches its own TcpListener::bind much later (after Node::new_server, listen_on, monitoring bind) — fails with AddrInUse.
4. Meanwhile A's wait_until_serving has been polling every 20 ms and gets a 200 from B's relay, so the select! resolves on the serving arm before handle becomes ready.
5. A proceeds, test_fn runs against B's relay, and the final join at relay.rs:639 returns Err(FailedToBindHttpListener) — surfaced as a hard failure with no retry, because the retry loop only covers start_relay.

The probability is low (needs the port collision and the probe to win the poll ordering), but it's the same failure class the PR is closing, and it degrades into a confusing error rather than a retry.

Cheapest fix that catches the common ordering — after wait_until_serving returns Ok, check the task hasn't already died:

if handle.is_finished() {
    // Something else is serving this port; our relay is gone.
    return match handle.await { /* same arms as the select! branch */ };
}

That routes it back through is_addr_in_use and the retry loop. A stronger version verifies identity — GET / and require the returned multiaddrs to carry the relay's own PeerId — but is_finished() is probably enough for a fixture. Fix this →

2. timeout(2s, handle) without abort() can leave the HTTP listener bound after run returns Ok

crates/relay-server/src/p2p.rs:162-186 — on the Err(_) (timeout) arm the JoinHandle is dropped, which detaches the task rather than aborting it. So axum::serve can still be inside graceful shutdown, still holding the TCP listener, while run_relay_p2p_node returns Ok(node) and the whole relay reports a clean exit.

This is pre-existing, but this PR makes it load-bearing: fixture_stops_relay_and_releases_http_port (relay.rs:948-967) asserts the port is rebindable the instant the fixture returns. Axum's graceful shutdown waits for open connections, and reqwest's pooled keep-alive sockets are closed lazily on Client drop — so a 2 s overrun isn't impossible on a loaded CI box, and it would show up as a flaky AddrInUse in the very test meant to prove there is no flake.

handle.abort() (or abort_handle captured up front) on the timeout arm for both the ENR and monitoring joins would make the guarantee real. Fix this →

3. Two back-to-back free_tcp_addr() calls can hand out the same port

crates/cli/src/commands/relay.rs:659 and :666 — free_tcp_addr binds, reads local_addr, and drops the socket before returning. The monitoring address is allocated, released, and then the HTTP address is allocated. Nothing stops the kernel from handing back the port it just reclaimed, so a monitoring-enabled test can collide with itself. The retry loop recovers, but it burns an attempt on noise this fixture is specifically trying to eliminate.

squat_tcp_addr already returns the listener, so this is a two-line fix — hold both, then drop both:

let (mon_l, monitoring_addr) = squat_tcp_addr().await;
let (http_l, http_address) = squat_tcp_addr().await;
drop((mon_l, http_l));

That also removes the need for free_tcp_addr as a separate helper. Fix this →

4. serve_addr_enr_ext_ip now advertises tcp/0 / udp/0

With P2P_LISTEN_ADDR = "127.0.0.1:0", external_tcp_multiaddrs (crates/p2p/src/utils.rs:31-59) derives its ports from the config, so serve_addr_enr_ext_ip now produces /ip4/222.222.222.222/tcp/0 and the ENR carries tcp=0, udp=0. The assertion only checks the IP so it passes, but the test silently stopped covering the listen-port → advertised-port mapping.

The PR handles this honestly — the tradeoff is documented at relay.rs:552-566 and external_multiaddrs_keep_the_listen_ports covers the mapping in isolation. Two optional improvements:

• Assert enr.tcp() == Some(0) in serve_addr_enr_ext_ip so the port-0 consequence is visible rather than implied by a comment.
• Or let that one test opt back into concrete ports via config_fn (args.p2p.tcp_addrs = ...) — it's the only test that cares about external addrs, and it would now be covered by the retry loop anyway.

5. Smaller notes

• relay.rs:600-603 — the doc says "Startup and shutdown errors are returned to the caller", but start_relay panic!s on the wait_until_serving timeout (:729) and on Ok(Ok(())) before serving (:716). Fine for a fixture; the doc just overstates it slightly.
• relay.rs:1022-1026 — http_get has no per-request timeout, where wait_until_serving uses 1 s. A relay that accepts but never answers now hangs the test until the harness timeout instead of failing fast. Cheap to add .timeout(..).
• relay.rs:636-638 — if test_fn panics, ct.cancel() never runs and the relay outlives the fixture (and dir is deleted from under it during unwind). Harmless under #[tokio::test] since the runtime is dropped, but a Drop impl on ServingRelay that cancels would make every failure path leak-free.
• crates/relay-server/src/error.rs:24 vs :41 — FailedToBindHttpListener.addr is String while FailedToBindMonitoringListener.addr is SocketAddr. Defensible (Config::http_addr is a String that's never parsed), but worth a word in the doc comment so the asymmetry doesn't read as an oversight.
• config.rs:36 — debug_addr is parsed into Config but never served, so Charon's third serverErr producer (the pprof server, relay.go:108-125) has no counterpart yet. Out of scope; just flagging that when it lands it should follow the new bind-then-serve shape.
• MetricsExporter::bind/MetricsServer::{start, local_addr} and the MetricsServer<'static> lifetime are the one thing I couldn't verify locally (cargo is blocked in this sandbox) — CI will settle it.

Nice touches worth calling out: resume_relay_panic preserving the original panic instead of a JoinError, exhaust_retries_on_taken_port asserting which listener reported the failure, and fixture_leaves_no_listener_bound_when_startup_fails being a genuine regression test for the reorder rather than a restatement of it.
· branch feat/fix-relay-flank

[emlautarom1]

As part of the refinement phase, we should try to get a proper solution in place, and retrying here seems like a hack. The original issue describes the problem as something that happens on test-code only code, in particular in CI.

Ideally, we should have something like wiremock, where we can start configure the relay server using a builder, then start it and let it live until the end of the scope (the test). The with_relay_server abstraction is introducing more complications that benefits at this point.

[emlautarom1]

For example, tests should ideally look something like:

#[tokio::test]
async fn serve_addr_enr_ext_ip() {
    let relay = test_relay_server_with(|args| args.p2p.external_ip = Some("222.222.222.222".into())).await.unwrap();
    let addr = relay.http_addr.unwrap();

    let response = http_get(&format!("http://{addr}/enr")).await.unwrap();
    let enr = Record::try_from(response.text().await.unwrap().as_str()).unwrap();

    assert_eq!(enr.ip(), Some(Ipv4Addr::new(222, 222, 222, 222)));
}

#[tokio::test]
async fn run_bootnode_auto_p2p() {
    let missing_key = test_relay_server_with(|args| args.relay.auto_p2p_key = false).await;
    assert!(matches!(
        missing_key,
        Err(super::CliError::RelayP2PError(
            pluto_relay_server::RelayP2PError::FailedToLoadPrivateKey(..)
        ))
    ));

    let _relay = test_relay_server_with(|args| { }).await; // starts with an auto-generated key
}

#[tokio::test]
async fn taken_monitoring_port_fails_the_relay() {
    let taken = net::TcpListener::bind(ANY_ADDR).await.unwrap();
    let addr = taken.local_addr().unwrap().to_string();

    let err = test_relay_server(|args| args.debug_monitoring.monitor_addr = Some(addr))
        .await
        .expect_err("relay must not start while its monitoring port is taken");

    assert!(matches!(
        err,
        super::CliError::RelayP2PError(
            pluto_relay_server::RelayP2PError::FailedToBindMonitoringListener { .. }
        )
    ));
}

The tricky part is how we can retrieve the actually bounded addresses. I think this requires some refactoring to the internals so we can split binding from serving: I explored the work a bit in fix/relay-bind-before-serve if you want to take a look.

[emlautarom1]

Test look a lot better without the retry dance. There are a few things to improve but overall LGTM, the PR solves the original problem. The PR body is stale so it should be updated with the final design.