From a18eddeda9a478bed04adb95f31f3d61732cd555 Mon Sep 17 00:00:00 2001 From: dignifiedquire Date: Mon, 23 Mar 2026 11:12:16 +0100 Subject: [PATCH] fix(proto): match picoquic's PTO cap logic Replace flat 2s PTO cap with picoquic's two-tier approach (timing.c:42-88): 1. Bind PTO to idle_timeout/16 so ~16 retransmits fit before the idle timer fires (only when idle_timeout > 15s) 2. Hard cap at 2s, or 1.5 * smoothed_rtt for satellite paths (RTT > 610ms) 3. Use max(last_ack_eliciting, now) as PTO base to prevent scheduling in the past Update proptests to explicitly close connections before asserting idle, since with the PTO cap active paths with reachable peers correctly stay alive instead of timing out. --- noq-proto/src/connection/mod.rs | 41 ++++++++++++++++++++++++++++---- noq-proto/src/tests/proptests.rs | 14 ++++++++++- 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index f56f93a78..08b7bbbe5 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -3382,13 +3382,33 @@ impl Connection { } // Include max_ack_delay and backoff for ApplicationData. duration += self.ack_frequency.max_ack_delay_for_pto() * backoff; - // Cap PTO post-handshake to prevent long retransmit gaps after connectivity loss. - duration = duration.min(MAX_PTO_INTERVAL); + + // Cap PTO post-handshake, matching picoquic's + // picoquic_current_retransmit_timer() (timing.c:42-88): + // + // 1. Bind to idle_timeout / 16 so ~16 retransmits fit before + // the idle timer fires (timing.c:59-63). + if let Some(idle) = self.idle_timeout { + if idle > MIN_IDLE_FOR_PTO_CAP { + duration = duration.min(idle / IDLE_TIMEOUT_PTO_DIVISOR); + } + } + // 2. Hard cap at 2s, or 1.5 * smoothed_rtt for satellite paths + // with RTT > 610ms (timing.c:77-85). + let hard_cap = if path.rtt.get() > SATELLITE_RTT_THRESHOLD { + (path.rtt.get() * 3) / 2 + } else { + MAX_PTO_INTERVAL + }; + duration = duration.min(hard_cap); } let Some(last_ack_eliciting) = pns.time_of_last_ack_eliciting_packet else { continue; }; - let pto = last_ack_eliciting + duration; + // Use the later of (last_ack_eliciting + duration) and (now + duration) + // to prevent the PTO from firing in the past when the cap makes duration + // small relative to how long ago the last packet was sent. + let pto = last_ack_eliciting.max(now) + duration; if result.is_none_or(|(earliest_pto, _)| pto < earliest_pto) { if path.anti_amplification_blocked(1) { // Nothing would be able to be sent. @@ -7233,9 +7253,22 @@ fn get_max_ack_delay(params: &TransportParameters) -> Duration { // Prevents overflow and improves behavior in extreme circumstances const MAX_BACKOFF_EXPONENT: u32 = 16; -// Cap on PTO after backoff, matching picoquic's PICOQUIC_LARGE_RETRANSMIT_TIMER. +// Hard cap on PTO after backoff, matching picoquic's PICOQUIC_LARGE_RETRANSMIT_TIMER. const MAX_PTO_INTERVAL: Duration = Duration::from_secs(2); +// PTO is capped at idle_timeout / IDLE_TIMEOUT_PTO_DIVISOR so ~16 retransmits +// fit before the idle timer fires. Matches picoquic (idle_timeout >> 4). +const IDLE_TIMEOUT_PTO_DIVISOR: u32 = 16; + +// RTT threshold above which the PTO cap uses 1.5 * smoothed_rtt instead of +// MAX_PTO_INTERVAL. Matches picoquic's PICOQUIC_TARGET_SATELLITE_RTT (610ms). +const SATELLITE_RTT_THRESHOLD: Duration = Duration::from_millis(610); + +// Idle timeout must exceed this for the idle-bound PTO cap to apply. +// With short idle timeouts, the cap would make PTO too aggressive. +// Matches picoquic's guard (idle_timeout > 15). +const MIN_IDLE_FOR_PTO_CAP: Duration = Duration::from_secs(15); + /// Minimal remaining size to allow packet coalescing, excluding cryptographic tag /// /// This must be at least as large as the header for a well-formed empty packet to be coalesced, diff --git a/noq-proto/src/tests/proptests.rs b/noq-proto/src/tests/proptests.rs index ce893c51c..db1e32438 100644 --- a/noq-proto/src/tests/proptests.rs +++ b/noq-proto/src/tests/proptests.rs @@ -11,9 +11,11 @@ use proptest::{ use test_strategy::proptest; use tracing::error; +use bytes::Bytes; + use crate::{ Connection, ConnectionClose, ConnectionError, Event, PathStatus, Side, TransportConfig, - TransportErrorCode, + TransportErrorCode, VarInt, tests::{ Pair, RoutingTable, random_interaction::{TestOp, run_random_interaction}, @@ -114,6 +116,11 @@ fn random_interaction_with_multipath_simple_routing( let (client_ch, server_ch) = run_random_interaction(&mut pair, interactions, multipath_transport_config(prefix)); + // Close the connection explicitly — with PTO caps, active paths with + // reachable peers never idle on their own (correct behavior). + let now = pair.time; + pair.client_conn_mut(client_ch) + .close(now, VarInt(0), Bytes::new()); prop_assert!(!pair.drive_bounded(1000), "connection never became idle"); prop_assert!(allowed_error(poll_to_close( pair.client_conn_mut(client_ch) @@ -167,6 +174,11 @@ fn random_interaction_with_multipath_complex_routing( let (client_ch, server_ch) = run_random_interaction(&mut pair, interactions, multipath_transport_config(prefix)); + // Close the connection explicitly — with PTO caps, active paths with + // reachable peers never idle on their own (correct behavior). + let now = pair.time; + pair.client_conn_mut(client_ch) + .close(now, VarInt(0), Bytes::new()); prop_assert!(!pair.drive_bounded(1000), "connection never became idle"); prop_assert!(allowed_error(poll_to_close( pair.client_conn_mut(client_ch)