mirror of
https://relay.ngit.dev/npub15qydau2hjma6ngxkl2cyar74wzyjshvl65za5k5rl69264ar2exs5cyejr/ngit-grasp.git
synced 2026-10-05 15:08:24 +00:00
The naive find_free_port pattern — bind 127.0.0.1:0, read the port, drop
the listener, return — has a TOCTOU window where another caller in the
process can be handed the same port back before the consumer's real bind.
The hazard is especially sharp where a port is pre-allocated to embed in
an announcement event, then used minutes later to spawn the relay that
will host it (purgatory_sync, sync/historic_sync, sync/metrics,
archive_grasp_services, sync_helpers::run_sync_test_*).
Introduce tests/common/port.rs with PortReservation + reserve_port():
the listener stays bound for the lifetime of the reservation, so no
other reserve_port call in this process can be handed the same number.
TestRelay grows reservation-based constructors and routes every
existing public start* method through the same reservation path
internally; the bare find_free_port / start_on_port_* /
start_with_archive_and_sync / start_with_all_options API is removed
and all callers migrated.
Defense-in-depth retry on subprocess early-exit (detected via try_wait
during the readiness probe so we fail fast rather than waiting the
full 5s timeout) covers the residual microsecond-scale TOCTOU window
between PortReservation::release and ngit-grasp's own bind. Has not
been observed to fire in local stress runs; kept as belt-and-braces
for CI / loaded hardware.
Mirrors ngit commit 5883494 ("test(harness): port reservation + retry
to harden against :0 bind race").
139 lines
5.7 KiB
Rust
139 lines
5.7 KiB
Rust
//! Race-free port reservation for test fixtures.
|
|
//!
|
|
//! ## The race
|
|
//!
|
|
//! The naive pattern — bind `127.0.0.1:0`, read the kernel-assigned port,
|
|
//! drop the listener, hand the bare `u16` to whoever wants it — has a
|
|
//! TOCTOU window between drop and the consumer's actual `bind`. During
|
|
//! that window, anything else in the process (or, more rarely, another
|
|
//! process) can be handed the same port by the kernel.
|
|
//!
|
|
//! The race is rare on lightly-loaded hardware but has been observed in
|
|
//! CI and during development, and the failure mode
|
|
//! (`Address already in use (os error 98)`) is a hard test fail with no
|
|
//! useful information for the next debugger. The hazard is sharply worse
|
|
//! in patterns that reserve a port well in advance of binding (e.g.
|
|
//! pre-allocating a port to embed in an announcement event before
|
|
//! starting the relay that will host it).
|
|
//!
|
|
//! Our in-process fixtures ([`MockRelay`], [`SmartGitServer`]) avoid the
|
|
//! race entirely by **keeping the listener bound** and handing it
|
|
//! straight to their tokio accept loop. That trick doesn't work for the
|
|
//! [`TestRelay`] subprocess — `ngit-grasp` binds itself from
|
|
//! `NGIT_BIND_ADDRESS`, and inheriting the pre-bound fd would require
|
|
//! Unix-specific `pre_exec` plumbing we'd rather not own in the test
|
|
//! harness.
|
|
//!
|
|
//! ## The reservation pattern
|
|
//!
|
|
//! Instead, [`reserve_port`] returns a [`PortReservation`] that **holds the
|
|
//! bound `TcpListener`** until the caller is about to start the real
|
|
//! service. While any reservation is live, no other call to
|
|
//! `reserve_port` in this process can be handed the same port — the
|
|
//! kernel won't reissue a port that is currently bound.
|
|
//!
|
|
//! The caller drops the reservation immediately before the real bind,
|
|
//! shrinking the TOCTOU window from "however long the fixture takes to
|
|
//! spawn" (or, worse, "however long the test takes to build the
|
|
//! announcement event") to "a few microseconds inside the start
|
|
//! function". The retry loop in [`crate::common::relay::TestRelay`]
|
|
//! covers that residual window — defense-in-depth that has never been
|
|
//! observed to fire in local stress runs.
|
|
//!
|
|
//! [`MockRelay`]: crate::common::mock_relay::MockRelay
|
|
//! [`SmartGitServer`]: crate::common::git_server::SmartGitServer
|
|
//! [`TestRelay`]: crate::common::relay::TestRelay
|
|
|
|
use std::net::TcpListener;
|
|
|
|
/// A port that the kernel has assigned to us via `:0` bind, held open by
|
|
/// a live `TcpListener` so that no other [`reserve_port`] call in this
|
|
/// process can be handed the same number.
|
|
///
|
|
/// The reservation is released by:
|
|
///
|
|
/// - calling [`PortReservation::release`] to consume the reservation and
|
|
/// return the port number (preferred — makes the release explicit at the
|
|
/// call site), or
|
|
/// - simply dropping the value (also fine, but the release point is then
|
|
/// tied to lexical scope).
|
|
///
|
|
/// The caller should release **immediately** before the consuming service
|
|
/// performs its own `bind` so that the TOCTOU window between
|
|
/// reservation-release and service-bind is as small as possible.
|
|
#[derive(Debug)]
|
|
pub struct PortReservation {
|
|
port: u16,
|
|
/// The listener whose binding holds the port. Dropped on
|
|
/// [`Self::release`] or when the reservation goes out of scope.
|
|
_listener: TcpListener,
|
|
}
|
|
|
|
impl PortReservation {
|
|
/// The kernel-assigned loopback port number held by this reservation.
|
|
pub fn port(&self) -> u16 {
|
|
self.port
|
|
}
|
|
|
|
/// Consume the reservation, dropping the underlying listener and
|
|
/// returning the port number. The port is now free for the caller's
|
|
/// real service to bind. Prefer this over relying on lexical drop —
|
|
/// it makes the release point explicit at the call site.
|
|
pub fn release(self) -> u16 {
|
|
let port = self.port;
|
|
// `self` is consumed; the listener inside is dropped here.
|
|
drop(self);
|
|
port
|
|
}
|
|
}
|
|
|
|
/// Bind `127.0.0.1:0`, capture the assigned port, and **keep the listener
|
|
/// bound** inside the returned [`PortReservation`] until the caller
|
|
/// releases it.
|
|
///
|
|
/// While the reservation is live, no other `reserve_port` call in this
|
|
/// process will be handed the same port. See module docs for why this
|
|
/// matters.
|
|
pub fn reserve_port() -> PortReservation {
|
|
let listener =
|
|
TcpListener::bind("127.0.0.1:0").expect("Failed to bind 127.0.0.1:0 for port reservation");
|
|
let port = listener
|
|
.local_addr()
|
|
.expect("Failed to read local_addr from bound listener")
|
|
.port();
|
|
PortReservation {
|
|
port,
|
|
_listener: listener,
|
|
}
|
|
}
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use super::*;
|
|
|
|
/// Two reservations held simultaneously must return distinct ports.
|
|
/// This is the core same-process guarantee the reservation pattern
|
|
/// provides — and exactly what the naive "bind, drop, return" pattern
|
|
/// fails to give under parallel load.
|
|
#[test]
|
|
fn parallel_reservations_get_distinct_ports() {
|
|
let a = reserve_port();
|
|
let b = reserve_port();
|
|
let c = reserve_port();
|
|
assert_ne!(a.port(), b.port());
|
|
assert_ne!(b.port(), c.port());
|
|
assert_ne!(a.port(), c.port());
|
|
}
|
|
|
|
// Note: there is intentionally no "released port is immediately
|
|
// bindable" unit test. Once released, the port re-enters the
|
|
// kernel's free pool, and under heavy parallel test load (where
|
|
// dozens of `reserve_port` / `TcpListener::bind("127.0.0.1:0")`
|
|
// calls are racing each other) another test can be handed that
|
|
// port number before this one rebinds. That race is exactly what
|
|
// `reserve_port` exists to suppress for the held-reservation
|
|
// window; once released, it is by design out of scope. The
|
|
// "bindable after release" property is implicitly exercised by
|
|
// every passing `TestRelay::start*` integration test.
|
|
}
|