From f9931ee292ea66f2c008c701f45bd15b735326db Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Wed, 24 Jun 2026 15:53:46 +0100 Subject: [PATCH 1/4] refactor(proto): do not set timers in poll_transmit We had one case where we are setting timers in poll_transmit, this happened to work around a bug in noq that was not waking up proto in a timely manner. However the code style is very much not following this. In general poll_transmit only tries to only concern itself with building packets that need to be build and contains the bare minimum of application logic that we can manage. Other timers which need to be set when packets are sent are also set at the time the packets are scheduled. The time in between should be negligible if the driver is functioning correctly. Furthermore path challenges are special in that there can be several reasons why they are being sent. Only when they are being sent is this known, and poll_transmit having to figure out what the reason was so it can set the right timers is triky. Bringing this separation back will also allow us to schedule on-path path challenges for liveness checks for example, without needing strict validation, something RFC9000 suggests and is a nice thing to have available in our toolbox should we have a use. Fixes #686 --- noq-proto/src/connection/mod.rs | 52 ++++++---- noq-proto/src/tests/mod.rs | 66 ------------ noq-proto/src/tests/multipath.rs | 169 ++++++++++++------------------- 3 files changed, 97 insertions(+), 190 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 881e9536d..faac7c3dc 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -590,7 +590,7 @@ impl Connection { return Err(PathError::RemoteCidsExhausted); } - let path = self.ensure_path(path_id, network_path, now, None); + let path = self.create_path(path_id, network_path, now, None); path.status.local_update(initial_status); Ok(path_id) @@ -929,7 +929,7 @@ impl Connection { /// Creates the [`PathData`] for a new [`PathId`]. /// /// Called for incoming packets as well as when opening a new path locally. - fn ensure_path( + fn create_path( &mut self, path_id: PathId, network_path: FourTuple, @@ -978,9 +978,31 @@ impl Connection { .address_discovery_role .should_report(&self.peer_params.address_discovery_role); + // Challenges are ack-eliciting but not retransmittable, so set a timer + // manually. This timer interval keeps increasing with lost challenges. At some + // point the interval will exceed the path idle timeout and that is when this path + // will be closed. + self.timers.set( + Timer::PerPath(path_id, PathTimer::PathChallengeLost), + now + data.on_path_challenge_expiry(), + self.qlog.with_time(now), + ); + if !data.validated { + // Hard cap on how long to try validation for. + // TODO(flub): 3 * PTO is way too short, it makes opening paths very susceptible + // to packet loss. I prefer using the path idle timeout probably. + let pto = self.ack_frequency.max_ack_delay_for_pto() + data.rtt.pto_base(); + self.timers.set( + Timer::PerPath(path_id, PathTimer::AbandonFromValidation), + now + 3 * pto, + self.qlog.with_time(now), + ); + } + let path = vacant_entry.insert(PathState { data, prev: None }); let mut pn_space = spaces::PacketNumberSpace::new(now, SpaceId::Data, &mut self.rng); + pn_space.open_status = OpenStatus::Sent; if let Some(pn) = pn { pn_space.dedup.insert(pn); } @@ -2527,6 +2549,11 @@ impl Connection { trace!(?path.data.lost_challenge_count, "path challenge deemed lost"); path.data.pending_challenge = true; path.data.lost_challenge_count += 1; + self.timers.set( + Timer::PerPath(path_id, PathTimer::PathChallengeLost), + now + path.data.on_path_challenge_expiry(), + self.qlog.with_time(now), + ); } PathTimer::AbandonFromValidation => { let Some(path) = self.paths.get_mut(&path_id) else { @@ -4349,7 +4376,7 @@ impl Connection { if self.side().is_server() && !self.abandoned_paths.contains(&path_id) { // Only the client is allowed to open paths - self.ensure_path(path_id, network_path, now, pn); + self.create_path(path_id, network_path, now, pn); } if self.paths.contains_key(&path_id) { self.on_packet_authenticated( @@ -6179,25 +6206,6 @@ impl Connection { let challenge = frame::PathChallenge(token); builder.write_frame(challenge, stats); builder.require_padding(); - let pto = self.ack_frequency.max_ack_delay_for_pto() + path.rtt.pto_base(); - let pns = space.for_path(path_id); - match pns.open_status { - OpenStatus::Sent | OpenStatus::Informed => {} - OpenStatus::Pending => { - pns.open_status = OpenStatus::Sent; - self.timers.set( - Timer::PerPath(path_id, PathTimer::AbandonFromValidation), - now + 3 * pto, - self.qlog.with_time(now), - ); - } - } - - self.timers.set( - Timer::PerPath(path_id, PathTimer::PathChallengeLost), - now + path.on_path_challenge_expiry(), - self.qlog.with_time(now), - ); if is_multipath_negotiated && !path.validated && path.pending_challenge { // queue informing the path status along with the challenge diff --git a/noq-proto/src/tests/mod.rs b/noq-proto/src/tests/mod.rs index 34c62e26c..fb6e6dcf0 100644 --- a/noq-proto/src/tests/mod.rs +++ b/noq-proto/src/tests/mod.rs @@ -1506,72 +1506,6 @@ fn migration() { ); } -#[test] -fn path_challenge_retransmit() { - let _guard = subscribe(); - let mut pair = Pair::default(); - let (client_ch, server_ch) = pair.connect(); - pair.drive(); - - pair.client_conn_mut(client_ch).ping(); - pair.drive(); - - println!("-------- server wants path validation --------"); - pair.server_conn_mut(server_ch).trigger_path_validation(); - pair.drive_server(); // Send the path challenge - println!("-------- client loses messages --------"); - // Have the client lose the challenge - pair.client.inbound.clear(); - - pair.drive(); - - let client_tx = pair.client_conn_mut(client_ch).stats().frame_tx; - let server_tx = pair.server_conn_mut(server_ch).stats().frame_tx; - - assert_eq!( - server_tx.path_challenge, 2, - "expected server to send two path challenges" - ); - assert_eq!( - client_tx.path_response, 1, - "expected client to send one path response" - ); -} - -#[test] -fn path_response_retransmit() { - let _guard = subscribe(); - let mut pair = Pair::default(); - let (client_ch, server_ch) = pair.connect(); - pair.drive(); - - pair.client_conn_mut(client_ch).ping(); - pair.drive(); - - println!("-------- server wants path validation --------"); - pair.server_conn_mut(server_ch).trigger_path_validation(); - pair.drive_server(); // Send the path challenge - pair.drive_client(); // Send the path response - println!("-------- server loses messages --------"); - // Have the server lose the path response - pair.server.inbound.clear(); - - // The server should decide to re-send the path challenge - pair.drive(); - - let client_tx = pair.client_conn_mut(client_ch).stats().frame_tx; - let server_tx = pair.server_conn_mut(server_ch).stats().frame_tx; - - assert_eq!( - server_tx.path_challenge, 2, - "expected server to send two path challenges" - ); - assert_eq!( - client_tx.path_response, 2, - "expected client to send two path responses" - ); -} - /// Regression test: handle sent challenges that are waiting for a response while a passive /// migration occurs. /// diff --git a/noq-proto/src/tests/multipath.rs b/noq-proto/src/tests/multipath.rs index 4a45c37a6..4223535fd 100644 --- a/noq-proto/src/tests/multipath.rs +++ b/noq-proto/src/tests/multipath.rs @@ -2009,113 +2009,78 @@ fn test_peer_may_probe() -> TestResult { } #[test] -fn on_path_challenge_lost_backoff() { +fn path_open_challenge_lost() -> TestResult { let _guard = subscribe(); + let mut pair = ConnPair::builder() + .enable_multipath() + .disable_mtud_discovery() + .connect(); - let mut pair = ConnPair::default(); - - // We use two helpers so we can "skip over" unrelated stopping points from `next_wakeup` - - /// Attempts to drive the client until it has sent the expected amount of path challenges. - /// - /// Tries for a maximum of 10 iterations. Panics if that fails. - fn drive_client_until_challenge_sent(pair: &mut ConnPair, expected_path_challenges_sent: u64) { - for _ in 0..10 { - pair.drive_client(); - if pair.stats(Client).frame_tx.path_challenge == expected_path_challenges_sent { - return; - } - pair.time = pair - .client - .next_wakeup() - .expect("couldn't drive client forward"); - info!("advancing to {:?} for client", pair.time - pair.epoch); - } - panic!( - "client never sent PATH_CHALLENGE #{}, actual: {}", - expected_path_challenges_sent, - pair.stats(Client).frame_tx.path_challenge - ); - } - - /// Attempts to drive the server until it has received the expected amount of path challenges. - /// - /// Tries for a maximum of 10 iterations. Panics if that fails. - fn drive_server_until_challenge_received( - pair: &mut ConnPair, - expected_path_challenges_received: u64, - ) { - for _ in 0..10 { - // attempt to drive the server until it receives the PATH_CHALLENGE and sends a PATH_RESPONSE - pair.time = pair - .server - .next_wakeup() - .expect("couldn't drive server forward"); - info!("advancing to {:?} for server", pair.time - pair.epoch); - pair.drive_server(); - if pair.stats(Server).frame_rx.path_challenge == expected_path_challenges_received - && pair.stats(Server).frame_tx.path_response == expected_path_challenges_received - { - return; - } - } - panic!( - "server never received PATH_CHALLENGE #{}, actual: {}", - expected_path_challenges_received, - pair.stats(Server).frame_rx.path_challenge - ); - } - - pair.conn_mut(Client).trigger_path_validation(); - - // Kickstart the process once to get a reference point for the last_challenge_sent - drive_client_until_challenge_sent(&mut pair, 1); - let mut last_challenge_send = pair.time; - let mut last_duration = Duration::ZERO; - - const MAX_DURATION: Duration = Duration::from_secs(2); // equivalent to MAX_PTO_INTERVAL - const MAX_ITERS: u64 = MAX_DURATION.as_millis().ilog2() as u64 + 2; - - for i in 1..=MAX_ITERS { - drive_server_until_challenge_received(&mut pair, i); - pair.client.inbound.clear(); // we drop the client-inbound PATH_RESPONSE - info!("dropped client inbound"); - - drive_client_until_challenge_sent(&mut pair, i + 1); - let time = pair.time.duration_since(last_challenge_send); - info!(?time, ?last_duration, "time since last PATH_CHALLENGE send"); - assert!( - time >= last_duration, - "duration between PATH_CHALLENGE sends must be monotonically increasing (backing off)" - ); - assert!( - time <= MAX_DURATION, - "duration between PATH_CHALLENGE sends must be bound to a maximum of 2s" - ); - if i == MAX_ITERS { - assert_eq!(time, MAX_DURATION); - } - last_duration = time; - last_challenge_send = pair.time; - } - - // Now we stop dropping the client inbound once and the backoff should return back to normal: + info!("opening path"); + let server_addr = pair.routes.public_server_addr(); + let path_id = pair.open_path( + Client, + FourTuple::from_remote(server_addr), + PathStatus::Available, + )?; + let first_challenge = pair.time; + pair.drive_client(); - drive_server_until_challenge_received(&mut pair, MAX_ITERS + 1); - pair.drive(); // Ensure the client fully processes the PATH_RESPONSE this time. - info!("client should have processed PATH_RESPONSE"); + // Drop the 1st PATH_CHALLENGE + let client_stats = pair.path_stats(Client, path_id).unwrap(); + assert_eq!(client_stats.frame_tx.path_challenge, 1); + assert!(pair.path_stats(Server, path_id).is_none()); + info!("dropping client's packet"); + pair.server.inbound.clear(); + + // Drop the 2nd PATH_CHALLENGE + pair.time = pair + .client + .next_wakeup() + .expect("couldn't drive client forward"); + info!("advancing to {:?} for client", pair.time - pair.epoch); + let second_challenge = pair.time; + pair.drive_client(); + let client_stats = pair.path_stats(Client, path_id).unwrap(); + assert_eq!(client_stats.frame_tx.path_challenge, 2); + assert!(pair.path_stats(Server, path_id).is_none()); + info!("dropping client's packet"); + pair.server.inbound.clear(); + + // Send the 3rd PATH_CHALLENGE + pair.time = pair + .client + .next_wakeup() + .expect("couldn't drive client forward"); + info!("advancing to {:?} for client", pair.time - pair.epoch); + let third_challenge = pair.time; + pair.drive_client(); + let client_stats = pair.path_stats(Client, path_id).unwrap(); + assert_eq!(client_stats.frame_tx.path_challenge, 3); + assert!(pair.path_stats(Server, path_id).is_none()); - // The next time challenges are sent, the backoff should be reset. + let first_delay = second_challenge - first_challenge; + let second_delay = third_challenge - second_challenge; + assert!( + first_delay < second_delay, + "delays must monotonically increase" + ); - pair.conn_mut(Client).trigger_path_validation(); - drive_client_until_challenge_sent(&mut pair, MAX_ITERS + 2); - last_challenge_send = pair.time; - drive_server_until_challenge_received(&mut pair, MAX_ITERS + 2); - pair.client.inbound.clear(); // we drop the client-inbound PATH_RESPONSE - info!("dropped client inbound"); - drive_client_until_challenge_sent(&mut pair, MAX_ITERS + 3); - let duration = pair.time.duration_since(last_challenge_send); - assert_eq!(duration, Duration::from_millis(1)); + // Open path, drive to completion + pair.drive_server(); + let server_stats = pair.path_stats(Server, path_id).unwrap(); + assert_eq!(server_stats.frame_rx.path_challenge, 1); + pair.advance_time(); + pair.drive(); + assert_matches!( + pair.poll(Client), + Some(Event::Path(crate::PathEvent::Established { id })) if id == path_id + ); + assert_matches!( + pair.poll(Server), + Some(Event::Path(crate::PathEvent::Established { id })) if id == path_id + ); + Ok(()) } #[test] From c4f853b85429cf61b0b04f57702c1edb257b0c11 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Thu, 25 Jun 2026 14:36:58 +0100 Subject: [PATCH 2/4] remove unused state --- noq-proto/src/connection/mod.rs | 1 - noq-proto/src/connection/spaces.rs | 5 +---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index faac7c3dc..abd274dc2 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -1002,7 +1002,6 @@ impl Connection { let path = vacant_entry.insert(PathState { data, prev: None }); let mut pn_space = spaces::PacketNumberSpace::new(now, SpaceId::Data, &mut self.rng); - pn_space.open_status = OpenStatus::Sent; if let Some(pn) = pn { pn_space.dedup.insert(pn); } diff --git a/noq-proto/src/connection/spaces.rs b/noq-proto/src/connection/spaces.rs index 0f9d70d72..ebaa14739 100644 --- a/noq-proto/src/connection/spaces.rs +++ b/noq-proto/src/connection/spaces.rs @@ -496,12 +496,9 @@ impl PacketNumberSpace { #[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] pub(super) enum OpenStatus { - /// A first packet has not been sent using this [`PathId`]. + /// The application has not yet been informed of this path. #[default] Pending, - /// The first packet has been sent using this [`PathId`]. However, it is not yet deemed good - /// enough to be reported to the application. - Sent, /// The application has been informed of this path. Informed, } From 4e5eb9b8208e288087361ae22513981ad0c10695 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Thu, 25 Jun 2026 15:30:57 +0100 Subject: [PATCH 3/4] move on-path challenge pto timer back to send time --- noq-proto/src/connection/mod.rs | 21 +++++++++++---------- noq-proto/src/connection/paths.rs | 12 ++++++++++-- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index abd274dc2..9d42beec7 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -978,15 +978,6 @@ impl Connection { .address_discovery_role .should_report(&self.peer_params.address_discovery_role); - // Challenges are ack-eliciting but not retransmittable, so set a timer - // manually. This timer interval keeps increasing with lost challenges. At some - // point the interval will exceed the path idle timeout and that is when this path - // will be closed. - self.timers.set( - Timer::PerPath(path_id, PathTimer::PathChallengeLost), - now + data.on_path_challenge_expiry(), - self.qlog.with_time(now), - ); if !data.validated { // Hard cap on how long to try validation for. // TODO(flub): 3 * PTO is way too short, it makes opening paths very susceptible @@ -2550,7 +2541,7 @@ impl Connection { path.data.lost_challenge_count += 1; self.timers.set( Timer::PerPath(path_id, PathTimer::PathChallengeLost), - now + path.data.on_path_challenge_expiry(), + now + path.data.on_path_challenge_pto(), self.qlog.with_time(now), ); } @@ -6206,6 +6197,16 @@ impl Connection { builder.write_frame(challenge, stats); builder.require_padding(); + // On-path challenges need a PATH_RESPONSE and not only an ACK which can be + // received on any path. So set a timer manually instead of relying on the usual + // LossDetection/PTO timer. This timer interval keeps exponentially increasing + // with missing responses like the normal PTO interval. + self.timers.set( + Timer::PerPath(path_id, PathTimer::PathChallengeLost), + now + path.on_path_challenge_pto(), + self.qlog.with_time(now), + ); + if is_multipath_negotiated && !path.validated && path.pending_challenge { // queue informing the path status along with the challenge space.pending.path_status.insert(path_id); diff --git a/noq-proto/src/connection/paths.rs b/noq-proto/src/connection/paths.rs index e7a6ed8c6..4ccfe4c1b 100644 --- a/noq-proto/src/connection/paths.rs +++ b/noq-proto/src/connection/paths.rs @@ -477,14 +477,22 @@ impl PathData { if self.unconfirmed_challenges.is_empty() { return None; } - let duration = self.on_path_challenge_expiry(); + let duration = self.on_path_challenge_pto(); self.unconfirmed_challenges .values() .map(|info| info.sent_instant + duration) .min() } - pub(super) fn on_path_challenge_expiry(&self) -> Duration { + /// The duration after which a PTO expires for an on-path challenge, if sent now. + /// + /// Since challenges need an on-path response rather than just an ACK that can be sent + /// on any path they need a different timer from the + /// [`PathTimer::LossDetection`]. Functionally this behaves as the probe timeout + /// however. + /// + /// [`PathTimer::LossDetection`]: super::timer::PathTimer::LossDetection + pub(super) fn on_path_challenge_pto(&self) -> Duration { let backoff = 2u32.pow(self.lost_challenge_count.min(MAX_BACKOFF_EXPONENT)); let duration = self.rtt.pto_base() * backoff; duration.min(MAX_PTO_INTERVAL) From b3f2fdacbe810c426fa9d1f3dc169dffd941df13 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Mon, 29 Jun 2026 18:35:38 +0100 Subject: [PATCH 4/4] Remove PathValidationFailed time, use path timeout instead This is probably not so simple. --- noq-proto/src/connection/mod.rs | 48 +++------- noq-proto/src/connection/paths.rs | 4 + noq-proto/src/connection/qlog.rs | 3 - noq-proto/src/connection/timer.rs | 20 +--- noq-proto/src/tests/multipath.rs | 149 +++++++++++++++++++----------- noq-proto/src/tests/util.rs | 4 +- 6 files changed, 119 insertions(+), 109 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 9d42beec7..5c5701c9d 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -717,9 +717,7 @@ impl Connection { // These timers deal with sending and receiving PATH_CHALLENGE and // PATH_RESPONSE, but now that the path is abandoned, we no longer care about // these frames or their timing - PathTimer::PathValidationFailed - | PathTimer::PathChallengeLost - | PathTimer::AbandonFromValidation => false, + PathTimer::PathValidationFailed | PathTimer::PathChallengeLost => false, // These timers deal with the lifetime of the path. Now that the path is abandoned, // these are not relevant. PathTimer::PathKeepAlive | PathTimer::PathIdle => false, @@ -978,17 +976,17 @@ impl Connection { .address_discovery_role .should_report(&self.peer_params.address_discovery_role); - if !data.validated { - // Hard cap on how long to try validation for. - // TODO(flub): 3 * PTO is way too short, it makes opening paths very susceptible - // to packet loss. I prefer using the path idle timeout probably. - let pto = self.ack_frequency.max_ack_delay_for_pto() + data.rtt.pto_base(); - self.timers.set( - Timer::PerPath(path_id, PathTimer::AbandonFromValidation), - now + 3 * pto, - self.qlog.with_time(now), - ); - } + // if !data.validated { + // // Hard cap on how long to try validation for. + // // TODO(flub): 3 * PTO is way too short, it makes opening paths very susceptible + // // to packet loss. I prefer using the path idle timeout probably. + // let pto = self.ack_frequency.max_ack_delay_for_pto() + data.rtt.pto_base(); + // self.timers.set( + // Timer::PerPath(path_id, PathTimer::AbandonFromValidation), + // now + 3 * pto, + // self.qlog.with_time(now), + // ); + // } let path = vacant_entry.insert(PathState { data, prev: None }); @@ -2545,24 +2543,6 @@ impl Connection { self.qlog.with_time(now), ); } - PathTimer::AbandonFromValidation => { - let Some(path) = self.paths.get_mut(&path_id) else { - continue; - }; - path.data.reset_on_path_challenges(); - self.timers.stop( - Timer::PerPath(path_id, PathTimer::PathChallengeLost), - self.qlog.with_time(now), - ); - debug!("new path validation failed"); - if let Err(err) = self.close_path_inner( - now, - path_id, - PathAbandonReason::ValidationFailed, - ) { - warn!(?err, "failed closing path"); - } - } PathTimer::Pacing => {} PathTimer::MaxAckDelay => { // This timer is only armed in the Data space @@ -5639,10 +5619,6 @@ impl Connection { Timer::PerPath(path_id, PathTimer::PathValidationFailed), qlog.clone(), ); - self.timers.stop( - Timer::PerPath(path_id, PathTimer::AbandonFromValidation), - qlog.clone(), - ); let next_challenge = path .data .earliest_on_path_expiring_challenge() diff --git a/noq-proto/src/connection/paths.rs b/noq-proto/src/connection/paths.rs index 4ccfe4c1b..cca56bcea 100644 --- a/noq-proto/src/connection/paths.rs +++ b/noq-proto/src/connection/paths.rs @@ -1103,6 +1103,10 @@ pub enum PathAbandonReason { error_code: VarInt, }, /// We didn't receive a path response in time after opening this path. + /// + /// This event is no longer emitted, when validation fails a path is only abandoned once + /// there's a path timeout and the [`Self::TimedOut`] event would be emitted instead. + // TODO(flub): should this be deprecated? ValidationFailed, /// We didn't receive any data from the remote within the path's idle timeout. TimedOut, diff --git a/noq-proto/src/connection/qlog.rs b/noq-proto/src/connection/qlog.rs index ed18865e9..5925a9f7c 100644 --- a/noq-proto/src/connection/qlog.rs +++ b/noq-proto/src/connection/qlog.rs @@ -323,9 +323,6 @@ impl QlogSink { PathTimer::PathIdle => Some(TimerType::custom("path_idle")), PathTimer::PathValidationFailed => Some(QlogTimerType::PathValidation.into()), PathTimer::PathChallengeLost => Some(TimerType::custom("path_challenge_lost")), - PathTimer::AbandonFromValidation => { - Some(TimerType::custom("abandon_from_validation")) - } PathTimer::PathKeepAlive => Some(TimerType::custom("path_keep_alive")), PathTimer::Pacing => Some(TimerType::custom("pacing")), PathTimer::MaxAckDelay => Some(QlogTimerType::Ack.into()), diff --git a/noq-proto/src/connection/timer.rs b/noq-proto/src/connection/timer.rs index 258140f28..1613b41e2 100644 --- a/noq-proto/src/connection/timer.rs +++ b/noq-proto/src/connection/timer.rs @@ -60,32 +60,22 @@ pub(crate) enum PathTimer { PathValidationFailed = 2, /// When to resend an on-path path challenge deemed lost PathChallengeLost = 3, - /// When to abandon a path due to failed validation. - /// - /// There are two situations in which we give up a path from validation. - /// 1. When opening a path we validate it according to RFC9000 ยง8.2 as required by the - /// multipath spec. This timer is armed to time-bound that validation. - /// 2. When validating an already opened multipath path for various reasons in which we - /// expect the path to either work or not work and want to respond correspondingly in - /// a timely manner. - AbandonFromValidation = 4, /// When to send a `PING` frame to keep the path alive - PathKeepAlive = 5, + PathKeepAlive = 4, /// When pacing will allow us to send a packet - Pacing = 6, + Pacing = 5, /// When to send an immediate ACK if there are unacked ack-eliciting packets of the peer - MaxAckDelay = 7, + MaxAckDelay = 6, /// When to clean up state for an abandoned path - PathDrained = 8, + PathDrained = 7, } impl PathTimer { - pub(super) const VALUES: [Self; 9] = [ + pub(super) const VALUES: [Self; 8] = [ Self::LossDetection, Self::PathIdle, Self::PathValidationFailed, Self::PathChallengeLost, - Self::AbandonFromValidation, Self::PathKeepAlive, Self::Pacing, Self::MaxAckDelay, diff --git a/noq-proto/src/tests/multipath.rs b/noq-proto/src/tests/multipath.rs index 4223535fd..e7d136188 100644 --- a/noq-proto/src/tests/multipath.rs +++ b/noq-proto/src/tests/multipath.rs @@ -412,72 +412,115 @@ fn open_path_key_update() -> TestResult { Ok(()) } -/// Client starts opening a path but the server fails to validate the path -/// -/// The client should receive an event closing the path. -#[test] -fn open_path_validation_fails_server_side() -> TestResult { - let _guard = subscribe(); - let mut pair = ConnPair::builder().enable_multipath().connect(); - - let different_addr = FourTuple { - remote: SocketAddr::new([9, 8, 7, 6].into(), 5), - local_ip: None, - }; - assert_ne!(different_addr.remote, Pair::SERVER_ADDR); - assert_ne!(different_addr.remote, Pair::CLIENT_ADDR); - let path_id = pair.open_path(Client, different_addr, PathStatus::Available)?; - - // block the server from receiving anything - while pair.blackhole_step(true, false) {} - assert_matches!( - pair.poll(Client), - Some(Event::Path(crate::PathEvent::Abandoned { id, reason: PathAbandonReason::ValidationFailed })) if id == path_id - ); - - assert!(pair.poll(Server).is_none()); - Ok(()) -} - -/// Client starts opening a path but the client fails to validate the path +// /// Client starts opening a path but the server fails to validate the path +// /// +// /// The client should receive an event closing the path. +// #[test] +// fn open_path_validation_fails_server_side() -> TestResult { +// let _guard = subscribe(); +// let mut pair = ConnPair::builder().enable_multipath().connect(); + +// let different_addr = FourTuple { +// remote: SocketAddr::new([9, 8, 7, 6].into(), 5), +// local_ip: None, +// }; +// assert_ne!(different_addr.remote, Pair::SERVER_ADDR); +// assert_ne!(different_addr.remote, Pair::CLIENT_ADDR); +// let path_id = pair.open_path(Client, different_addr, PathStatus::Available)?; + +// // block the server from receiving anything +// while pair.blackhole_step(true, false) {} +// assert_matches!( +// pair.poll(Client), +// Some(Event::Path(crate::PathEvent::Abandoned { id, reason: PathAbandonReason::ValidationFailed })) if id == path_id +// ); + +// assert!(pair.poll(Server).is_none()); +// Ok(()) +// } + +/// client starts opening a path but the client fails to validate the path /// /// The server should receive an event close the path #[test] fn open_path_validation_fails_client_side() -> TestResult { let _guard = subscribe(); - let mut pair = ConnPair::builder().enable_multipath().connect(); - - // make sure the new path cannot be validated using the existing path - let new_addr = SocketAddr::new([9, 8, 7, 6].into(), 5); - assert_ne!(new_addr, Pair::SERVER_ADDR); - assert_ne!(new_addr, Pair::CLIENT_ADDR); - pair.routes.as_basic_mut().client_addr = new_addr; + let client_addr_two = "[::1:2]:1".parse()?; + let server_addr_two = "[::2:2]:1".parse()?; + assert_ne!(client_addr_two, Pair::SERVER_ADDR); + assert_ne!(client_addr_two, Pair::CLIENT_ADDR); + assert_ne!(server_addr_two, Pair::SERVER_ADDR); + assert_ne!(server_addr_two, Pair::CLIENT_ADDR); + let mut builder = ConnPair::builder() + .enable_multipath() + .disable_mtud_discovery() + .with_routes( + // client_addr_two can send to server_addr_two, but the reverse is broken. + ManyToManyRouting::from_routes( + vec![(Pair::CLIENT_ADDR, 0), (client_addr_two, 0)], + vec![(Pair::SERVER_ADDR, 0), (server_addr_two, 1)], + ), + ); + builder + .server_transport_cfg + .default_path_max_idle_timeout(Some(Duration::from_secs(30))); + builder + .client_transport_cfg + .default_path_max_idle_timeout(Some(Duration::from_secs(30))); + let mut pair = builder.connect(); + // Open a path from ::1:2 to ::2:2. Client datagrams can be routed, server datagrams + // will come from ::2:2 from ::1:2 but only ::2:1 is allowed to send to ::1:2, so server + // datagrams will be dropped.. This might be crazy, please file complaints to + // ManyToManyRouting. let network_path = FourTuple { - remote: pair.routes.public_server_addr(), + remote: server_addr_two, local_ip: None, }; let path_id = pair.open_path(Client, network_path, PathStatus::Available)?; - // Make sure the client's path open makes it through to the server and is processed. - pair.drive_client(); - pair.drive_server(); + // // make sure the client's path open makes it through to the server and is processed. + // pair.drive_client(); + // pair.drive_server(); + + // info!("dropping client inbound queue"); + // pair.client.inbound.clear(); + + // // Sever the connection and run it to idle. + // // This makes sure that + // // - path validation can't succeed because path responses don't make it through and + // // - the server needs to decide to close the path on its own, because path abandons + // // don't make it through. + // while pair.blackhole_step(true, true) {} + + info!("drive to idle"); + pair.drive(); - info!("dropping client inbound queue"); - pair.client.inbound.clear(); + info!("manual keep-alive of PathId::ZERO"); + // Though some ACKs will have been flowing over this path already, keeping it alive. + pair.ping_path(Client, PathId::ZERO)?; + pair.ping_path(Client, PathId::ZERO)?; - // Sever the connection and run it to idle. - // This makes sure that - // - path validation can't succeed because path responses don't make it through and - // - the server needs to decide to close the path on its own, because path abandons - // don't make it through. - while pair.blackhole_step(true, true) {} + info!("drive past idle"); + // min_opt(pair.client.next_wakeup(), pair.server.next_wakeup()).map(|t| pair.time = t); + // pair.blackhole_step(true, true); + pair.drive(); + // The client gave up first and timed out. assert_matches!( - pair.poll(Server), - Some(Event::Path(PathEvent::Abandoned { id, reason: PathAbandonReason::ValidationFailed })) + pair.poll(Client), + Some(Event::Path(PathEvent::Abandoned { id, reason: PathAbandonReason::TimedOut })) if id == path_id ); + // Server would also fail to validate, but the client already abandoned. + assert_matches!( + pair.poll(Server), + Some(Event::Path(PathEvent::Abandoned { + id, + reason: PathAbandonReason::RemoteAbandoned { .. } + })) + if id == path_id + ); Ok(()) } @@ -1772,7 +1815,7 @@ fn test_simple_nat_traveral_opens_path() -> TestResult { let mut pair = ConnPair::builder() .enable_multipath() .enable_nat_traversal() - .with_routes(SimpleFirewallRouting::new().into()) + .with_routes(SimpleFirewallRouting::new()) .connect(); info!("adding addrs"); @@ -1876,7 +1919,7 @@ fn test_hard_nat_client_opens_path() -> TestResult { let mut pair = ConnPair::builder() .enable_multipath() .enable_nat_traversal() - .with_routes(routing.into()) + .with_routes(routing) .connect(); info!("adding addrs"); @@ -1926,7 +1969,7 @@ fn test_hard_nat_server_opens_path() -> TestResult { let mut pair = ConnPair::builder() .enable_multipath() .enable_nat_traversal() - .with_routes(routing.into()) + .with_routes(routing) .connect(); info!("adding addrs"); @@ -1972,7 +2015,7 @@ fn test_peer_may_probe() -> TestResult { .enable_multipath() .enable_nat_traversal() .disable_mtud_discovery() - .with_routes(SimpleFirewallRouting::new().into()); + .with_routes(SimpleFirewallRouting::new()); let server_cfg = ServerConfig { transport: Arc::new(builder.server_transport_cfg.clone()), migration: false, diff --git a/noq-proto/src/tests/util.rs b/noq-proto/src/tests/util.rs index fb48a4edf..088819b16 100644 --- a/noq-proto/src/tests/util.rs +++ b/noq-proto/src/tests/util.rs @@ -569,8 +569,8 @@ impl ConnPairBuilder { } /// Sets the [`Routing`] to use. - pub(super) fn with_routes(mut self, routes: Routing) -> Self { - self.routes = Some(routes); + pub(super) fn with_routes(mut self, routes: impl Into) -> Self { + self.routes = Some(routes.into()); self }