From cce2ff731ae043a74f32dc7b748f3f7f9e44f98e Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Wed, 12 Aug 2026 23:47:34 +0700 Subject: [PATCH] fix(server): order an echo reply's timestamp pair --- crates/irtt-server/AGENTS.md | 7 ++++ crates/irtt-server/src/clock.rs | 22 +++++++++++ crates/irtt-server/src/core.rs | 6 +++ crates/irtt-server/src/tests/echo.rs | 59 ++++++++++++++++++++++++++++ 4 files changed, 94 insertions(+) diff --git a/crates/irtt-server/AGENTS.md b/crates/irtt-server/AGENTS.md index 9fce93b..333311a 100644 --- a/crates/irtt-server/AGENTS.md +++ b/crates/irtt-server/AGENTS.md @@ -131,6 +131,13 @@ effective parameters must be safe for the server to run. classified as an echo and the send instant just before the reply is built, so they bracket the server's own handling. The monotonic origin belongs to the clock source and is stable for its life. +- A reply's receive instant is held back to its send instant where the wall + clock stepped backwards between the two readings, so the required ordering + holds. That correction is per reply and stateless on purpose: latching a + pre-step wall value across packets would keep reporting a time the host has + corrected away, which is the smoothing the specification forbids, and + anchoring the wall clock to the monotonic origin would drift and never pick up + a legitimate correction. The server reports its own wall clock honestly. - For a single-clock midpoint, `irtt-rs` emits the one negotiated field. It does not reproduce upstream 0.9.1's dual-field midpoint; `irtt-proto` still decodes that form from a peer. diff --git a/crates/irtt-server/src/clock.rs b/crates/irtt-server/src/clock.rs index 9f08468..8a5b424 100644 --- a/crates/irtt-server/src/clock.rs +++ b/crates/irtt-server/src/clock.rs @@ -44,6 +44,28 @@ impl ClockSample { mono_ns: mean_ns(self.mono_ns, other.mono_ns), } } + + /// This sample, held back in any domain where it runs ahead of `later`. + /// + /// A reply's receive instant must not be later than its send instant. The + /// monotonic domain gets that from its source, which only moves forward. + /// The wall clock does not: it can be stepped backwards between two + /// readings by NTP, a hypervisor or an administrator, which would otherwise + /// invert the pair. + /// + /// The *earlier* reading is the one that moves, so the pair settles on the + /// clock as it now stands rather than on a value the clock has already + /// disowned. Nothing is remembered between calls: a latch that carried a + /// pre-step value forward would keep reporting a wall time the host has + /// corrected away, which is the smoothing across packets the specification + /// forbids — and it would make one-way delays wrong for as long as the + /// latch held, rather than for one reply. + pub(crate) fn not_after(self, later: Self) -> Self { + Self { + wall_ns: self.wall_ns.min(later.wall_ns), + mono_ns: self.mono_ns.min(later.mono_ns), + } + } } /// A source of clock samples. diff --git a/crates/irtt-server/src/core.rs b/crates/irtt-server/src/core.rs index 8dddc11..3f5977f 100644 --- a/crates/irtt-server/src/core.rs +++ b/crates/irtt-server/src/core.rs @@ -421,7 +421,13 @@ struct EchoRequest { /// [`Clock::Unspecified`] selects no domain and so produces no field. It cannot /// be reached with a non-none `StampAt` on an acknowledged session, because /// that combination is refused at open; nothing here needs to repair it. +/// +/// The pair is ordered before anything is built from it, because a reply's +/// receive instant must not be later than its send instant and the wall clock +/// can be stepped backwards between the two readings. See +/// [`ClockSample::not_after`]; the correction is confined to this one reply. fn timestamp_fields(params: &Params, received: ClockSample, sent: ClockSample) -> TimestampFields { + let received = received.not_after(sent); let per_clock = |sample: ClockSample| { ( params.clock.has_wall().then_some(sample.wall_ns), diff --git a/crates/irtt-server/src/tests/echo.rs b/crates/irtt-server/src/tests/echo.rs index be40da7..26e2faa 100644 --- a/crates/irtt-server/src/tests/echo.rs +++ b/crates/irtt-server/src/tests/echo.rs @@ -594,6 +594,65 @@ fn only_the_negotiated_timestamps_are_emitted() { } } +#[test] +fn a_backward_wall_step_never_reports_receive_after_send() { + // A reply's receive instant must not be later than its send instant, and + // the wall clock can be stepped backwards between the two readings by NTP, + // a hypervisor or an administrator. The pair settles on the later reading, + // and only the wall domain moves: the monotonic one was already ordered. + let mut core = core_with_sources( + ServerConfig::default(), + ScriptedTokens::new([TOKEN_A]), + ScriptedClock::new([sample(1_000, 10_000), sample(990, 10_300)]), + ); + let params = echo_params(ReceivedStats::None, StampAt::Both, Clock::Both, 0); + let (token, negotiated) = open_negotiated(&mut core, peer(), ¶ms, None); + + let packet = core + .handle_datagram(peer(), &echo_request(token, 0, &negotiated, &[], None)) + .unwrap() + .expect("a backward wall step must not stop the echo being answered"); + let reply = expect_echo_reply(&packet, &negotiated, None); + + assert_eq!( + reply.timestamps, + TimestampFields { + recv_wall: Some(990), + recv_mono: Some(10_000), + send_wall: Some(990), + send_mono: Some(10_300), + ..TimestampFields::default() + }, + "the receive wall value is held back to the send reading, and only it" + ); + assert!(reply.timestamps.recv_wall <= reply.timestamps.send_wall); + assert!(reply.timestamps.recv_mono <= reply.timestamps.send_mono); + + // A midpoint drawn from the same stepped pair is ordered for the same + // reason: it is the mean of two values that are no longer inverted. + let mut core = core_with_sources( + ServerConfig::default(), + ScriptedTokens::new([TOKEN_A]), + ScriptedClock::new([sample(1_000, 10_000), sample(990, 10_300)]), + ); + let params = echo_params(ReceivedStats::None, StampAt::Midpoint, Clock::Wall, 0); + let (token, negotiated) = open_negotiated(&mut core, peer(), ¶ms, None); + + let packet = core + .handle_datagram(peer(), &echo_request(token, 0, &negotiated, &[], None)) + .unwrap() + .expect("a backward wall step must not stop the echo being answered"); + let reply = expect_echo_reply(&packet, &negotiated, None); + + assert_eq!( + reply.timestamps, + TimestampFields { + midpoint_wall: Some(990), + ..TimestampFields::default() + } + ); +} + #[test] fn a_single_clock_midpoint_emits_exactly_one_field() { // A guard against reproducing upstream 0.9.1's dual-field midpoint, which