From f0ccfc2ece291d88d2d4185df644616dbb9840fe Mon Sep 17 00:00:00 2001 From: Franz Heinzmann Date: Tue, 14 Apr 2026 11:01:39 +0200 Subject: [PATCH] fix(proto): let server-side paths use recoverability hint on network change (#579) ## Description Currently `handle_network_change` only checks `is_path_recoverable()` on the client side. The server side always assumed paths are recoverable, based on the assumption that servers have stable addresses. This assumption breaks when the server's uplink changes (e.g., mobile device acting as server replugs to a different network). The dead direct path would linger until the 3*PTO idle timeout instead of being immediately abandoned. Now both sides use the hint when provided. When no hint is given, clients default to non-recoverable (existing behavior) and servers default to recoverable (existing behavior). ## Breaking Changes ## Notes & open questions --------- Co-authored-by: Floris Bruynooghe --- noq-proto/src/connection/mod.rs | 14 ++-- noq-proto/src/tests/multipath.rs | 128 +++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 9 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 9975617f9..cce725def 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -5614,15 +5614,11 @@ impl Connection { // multipath, even in a single-path scenario, we attempt to migrate the path to a new // PathId. let attempt_to_recover = if is_multipath_negotiated { - if is_client { - hint.map(|h| h.is_path_recoverable(*path_id, network_path)) - .unwrap_or(false) - } else { - // Servers should have stable addresses so this scenario is generally discouraged. - // There is no way to prevent this, so the best hope is to attempt to recover the - // path - true - } + // Use the hint to determine if the path can recover. When no hint is + // provided, clients default to non-recoverable (abandon and re-open) + // while servers default to recoverable (attempt in-place recovery). + hint.map(|h| h.is_path_recoverable(*path_id, network_path)) + .unwrap_or(!is_client) } else { // In the non multipath case, we try to recover the single active path true diff --git a/noq-proto/src/tests/multipath.rs b/noq-proto/src/tests/multipath.rs index 2f9f3c767..571f8aaeb 100644 --- a/noq-proto/src/tests/multipath.rs +++ b/noq-proto/src/tests/multipath.rs @@ -967,6 +967,134 @@ fn network_change_selective_hint() -> TestResult { Ok(()) } +/// Server-side network change with two paths and a selective hint. +/// +/// The non-recoverable path is abandoned, leaving only the recoverable one. +#[test] +fn network_change_server_two_paths_selective_hint() -> TestResult { + let _guard = subscribe(); + let mut pair = multipath_pair(); + + // Open a second path from the client side. + let server_addr = pair.addrs_to_server(); + let second_path = pair.open_path(Client, server_addr, PathStatus::Available)?; + pair.drive(); + + assert_matches!( + pair.poll(Client), + Some(Event::Path(PathEvent::Opened { id })) if id == second_path + ); + assert_matches!( + pair.poll(Server), + Some(Event::Path(PathEvent::Opened { id })) if id == second_path + ); + + // Hint: The provided PathId is recoverable, others are not. + #[derive(Debug)] + struct SelectiveHint(PathId); + impl NetworkChangeHint for SelectiveHint { + fn is_path_recoverable(&self, path_id: PathId, _network_path: FourTuple) -> bool { + path_id == self.0 + } + } + + pair.handle_network_change(Server, Some(&SelectiveHint(second_path))); + + pair.drive(); + + // The non-recoverable path is abandoned on the server. No replacement opens because + // servers cannot call open_path. + assert_matches!( + pair.poll(Server), + Some(Event::Path(PathEvent::Abandoned { + id, + reason: PathAbandonReason::UnusableAfterNetworkChange, + })) if id == PathId::ZERO + ); + assert_matches!( + pair.poll(Server), + Some(Event::Path(PathEvent::Discarded { id, .. })) if id == PathId::ZERO + ); + assert_matches!(pair.poll(Server), None); + + // The client sees PathId::ZERO abandoned by the remote, then discards it. + assert_matches!( + pair.poll(Client), + Some(Event::Path(PathEvent::Abandoned { + id: PathId::ZERO, + reason: PathAbandonReason::RemoteAbandoned { .. }, + })) + ); + assert_matches!( + pair.poll(Client), + Some(Event::Path(PathEvent::Discarded { + id: PathId::ZERO, + .. + })) + ); + assert_matches!(pair.poll(Client), None); + + Ok(()) +} + +/// Server-side network change with a single path and a non-recoverable hint. +/// +/// The path cannot be closed because it is the last one. +#[test] +fn network_change_server_single_path_non_recoverable_falls_back() -> TestResult { + let _guard = subscribe(); + let mut pair = multipath_pair(); + + // Hint that says all paths are non-recoverable + #[derive(Debug)] + struct NonRecoverableHint; + impl NetworkChangeHint for NonRecoverableHint { + fn is_path_recoverable(&self, _path_id: PathId, _network_path: FourTuple) -> bool { + false + } + } + + pair.handle_network_change(Server, Some(&NonRecoverableHint)); + pair.drive(); + + // The path should NOT be abandoned. The last open path cannot be closed. + assert_matches!(pair.poll(Server), None); + assert_matches!(pair.poll(Client), None); + + Ok(()) +} + +/// Server-side network change with no hint defaults to recoverable. Both paths stay open. +#[test] +fn network_change_server_no_hint_recovers() -> TestResult { + let _guard = subscribe(); + let mut pair = multipath_pair(); + + // Open a second path from the client side. + let server_addr = pair.addrs_to_server(); + let second_path = pair.open_path(Client, server_addr, PathStatus::Available)?; + pair.drive(); + + assert_matches!( + pair.poll(Client), + Some(Event::Path(PathEvent::Opened { id })) if id == second_path + ); + assert_matches!( + pair.poll(Server), + Some(Event::Path(PathEvent::Opened { id })) if id == second_path + ); + + pair.handle_network_change(Server, None); + pair.drive(); + + // No path events: the server defaults to recoverable when no hint is provided. + // Neither path should be abandoned. + assert_matches!(pair.poll(Server), None); + assert_matches!(pair.poll(Client), None); + + Ok(()) +} + /// Checks that the deadline given before a path fails to be considered open start only when the /// first packet is sent. ///