From aac5247f01455412e83e3724c274a0c50b3f273d Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Sat, 15 Aug 2026 19:46:19 +0700 Subject: [PATCH] fix(stats): clamp loss windowing timestamp to its own sent_at timeout_at is pre-send anchored (F-04), so it can precede the same probe's post-send sent_at when probe_timeout is small relative to the send-call duration. Rolling-window eviction assumes non-decreasing event timestamps, so clamp the internal StatsEvent::Loss windowing value without changing the public EchoLoss.timeout_at field. Also correct an AGENTS.md claim that sent_at feeds the downstream one-way delay, which it does not. --- crates/irtt-client/AGENTS.md | 12 +++++--- crates/irtt-stats/src/normalization.rs | 40 +++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/crates/irtt-client/AGENTS.md b/crates/irtt-client/AGENTS.md index 52779fe..f025376 100644 --- a/crates/irtt-client/AGENTS.md +++ b/crates/irtt-client/AGENTS.md @@ -150,10 +150,14 @@ timestamp was observed, the upstream endpoint falls back to `sent_at.wall` maximum-lag bound: unlike the kernel receive side, legitimate client send-to-reply time can exceed a second. -`sent_at` remains the sole input to RTT and the downstream -(`server_to_client`) one-way delay, exactly as before this capability -existed; `TX_SOFTWARE` is still a software kernel timestamp near the driver -handoff, not a NIC hardware transmit time. `irtt-stats`'s `ReplySample` +`sent_at` remains the RTT endpoint and the userspace fallback for the +upstream (`client_to_server`) one-way delay, exactly as before this +capability existed. It is not an input to the downstream +(`server_to_client`) one-way delay, which is derived solely from +`ReceiveMeta::preferred_receive_wall` and the negotiated server send +timestamp — unaffected by this capability or by F-04. `TX_SOFTWARE` is +still a software kernel timestamp near the driver handoff, not a NIC +hardware transmit time. `irtt-stats`'s `ReplySample` copies public `sent_at` straight into `client_send_mono`/`client_send_wall_ns`, which `send_ipdv_ns` compares across probes for the upstream IPDV component — so that component shifts endpoint along with `sent_at` itself (pre-send vs. diff --git a/crates/irtt-stats/src/normalization.rs b/crates/irtt-stats/src/normalization.rs index 5445961..1c6c11a 100644 --- a/crates/irtt-stats/src/normalization.rs +++ b/crates/irtt-stats/src/normalization.rs @@ -110,7 +110,20 @@ pub(crate) fn normalize_event(event: &ClientEvent) -> Option { at: received_at.mono, bytes: *bytes, }), - ClientEvent::EchoLoss { timeout_at, .. } => Some(StatsEvent::Loss { at: *timeout_at }), + ClientEvent::EchoLoss { + sent_at, + timeout_at, + .. + } => Some(StatsEvent::Loss { + // `timeout_at` is anchored to the pre-send probe timestamp (see + // irtt-client's AGENTS.md), so it can precede this same probe's + // own post-send `sent_at` when a probe_timeout is small relative + // to the send-call duration. Rolling-window eviction assumes + // events are pushed in non-decreasing `at()` order, so clamp to + // `sent_at` to preserve that invariant without changing the + // public `timeout_at` value on the event itself. + at: (*timeout_at).max(sent_at.mono), + }), ClientEvent::Warning { at, .. } => Some(StatsEvent::Warning { at: at.mono }), ClientEvent::SessionStarted { .. } | ClientEvent::NoTestCompleted { .. } @@ -219,4 +232,29 @@ mod tests { let now = SystemTime::now(); assert!(system_time_ns(now).is_some()); } + + #[test] + fn loss_windowing_timestamp_is_clamped_to_its_own_sent_at() { + let base = Instant::now(); + let sent_at = ClientTimestamp { + mono: base + Duration::from_millis(50), + wall: UNIX_EPOCH + Duration::from_millis(50), + }; + // The pre-send-anchored `timeout_at` can precede this same probe's + // own post-send `sent_at` when probe_timeout is small relative to + // the send-call duration (see irtt-client's AGENTS.md). Rolling- + // window eviction assumes non-decreasing event timestamps, so the + // windowing `at()` must not regress behind the probe's own Sent + // event even though the public `timeout_at` field legitimately can. + let timeout_at = base + Duration::from_millis(10); + + let event = ClientEvent::EchoLoss { + seq: 0, + sent_at, + timeout_at, + }; + + let normalized = normalize_event(&event).expect("loss event normalizes"); + assert_eq!(normalized.at(), sent_at.mono); + } }