From 70181f390ab22f5f23a22ca3003f830a77d023af Mon Sep 17 00:00:00 2001 From: dignifiedquire Date: Wed, 18 Mar 2026 18:55:51 +0100 Subject: [PATCH] fix(proto): handle NoViablePath timer in closed/drained state (#397, #398, #400) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The NoViablePath grace timer (added in fd562f184 for #397) could fire after the connection was already closing/drained — e.g. when the application calls close() before the timer expires. This caused a panic on the invalid state transition Drained -> closed. Also adds: - Regression tests for #398/#400 last-validated-path abandon scenarios - Fix proptest error string to match new NO_VIABLE_PATH message - Proptest regression seeds for the discovered panic --- .../proptest-regressions/tests/proptests.txt | 8 + noq-proto/src/connection/mod.rs | 22 +- noq-proto/src/tests/multipath.rs | 232 ++++++++++++++++++ noq-proto/src/tests/proptests.rs | 2 +- 4 files changed, 255 insertions(+), 9 deletions(-) create mode 100644 noq-proto/proptest-regressions/tests/proptests.txt diff --git a/noq-proto/proptest-regressions/tests/proptests.txt b/noq-proto/proptest-regressions/tests/proptests.txt new file mode 100644 index 000000000..4fbcf9848 --- /dev/null +++ b/noq-proto/proptest-regressions/tests/proptests.txt @@ -0,0 +1,8 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 91184c7b6b718961d2dc03365f02098a18ac0035ca85b95654fbafa430d93664 # shrinks to input = _RandomInteractionWithMultipathComplexRoutingArgs { seed: [143, 24, 215, 92, 97, 107, 192, 143, 158, 46, 179, 30, 168, 66, 54, 199, 174, 170, 78, 124, 66, 235, 14, 97, 211, 127, 253, 141, 194, 196, 222, 138], interactions: [ClosePath { side: Server, path_idx: 0, error_code: 0 }, Drive { side: Client }], routes: RoutingTable { client_routes: [([::ffff:1.1.1.0]:44433, 0)], server_routes: [([::ffff:2.2.2.0]:4433, 0)] } } +cc ec5baef3027436b012a332a97d46814ed157f259337c3f651bbb7a3233bd9c7f # shrinks to input = _RandomInteractionWithMultipathSimpleRoutingArgs { seed: [121, 74, 209, 215, 123, 149, 7, 227, 67, 200, 91, 12, 216, 81, 208, 77, 83, 181, 39, 2, 207, 186, 233, 211, 254, 178, 230, 22, 100, 197, 215, 43], interactions: [Drive { side: Client }, ClosePath { side: Client, path_idx: 0, error_code: 0 }] } diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index e9ea1bf3c..9ce7eceb0 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -2351,14 +2351,20 @@ impl Connection { // Grace period expired: all paths were abandoned and no new path // was opened. Close the connection with CONNECTION_CLOSE. // See draft-ietf-quic-multipath-21 Section 3.4 para 7. - trace!("no viable path grace period expired, closing connection"); - let err = TransportError::NO_VIABLE_PATH( - "last path abandoned, no new path opened", - ); - self.close_common(); - self.set_close_timer(now); - self.connection_close_pending = true; - self.state.move_to_closed(err); + if self.state.is_closed() || self.state.is_drained() { + // Connection already closing/drained (e.g. application called + // close() before the grace timer fired). Nothing to do. + trace!("no viable path timer fired, but connection already closing"); + } else { + trace!("no viable path grace period expired, closing connection"); + let err = TransportError::NO_VIABLE_PATH( + "last path abandoned, no new path opened", + ); + self.close_common(); + self.set_close_timer(now); + self.connection_close_pending = true; + self.state.move_to_closed(err); + } } }, // TODO: add path_id as span somehow diff --git a/noq-proto/src/tests/multipath.rs b/noq-proto/src/tests/multipath.rs index aa294b804..ee8e47b2a 100644 --- a/noq-proto/src/tests/multipath.rs +++ b/noq-proto/src/tests/multipath.rs @@ -1837,3 +1837,235 @@ fn abandon_cycle() -> TestResult { // which is already tested in `network_change_multipath_no_hint_replaces_path` and // `network_change_selective_hint`. Link-breaking via silent drop is covered by the // existing `open_path_validation_fails_client_side` test using `blackhole_step`. + +// --- Tests for #398/#400: last-path and last-validated-path abandon scenarios --- +// +// These are regression tests for issues #398 and #400. Both were fixed by our #397 fix +// (commit fd562f184) which: +// - Always accepts remote PATH_ABANDONs (even for the last path) +// - Starts a NoViablePath grace timer (~1 PTO) when the last path is abandoned +// - Cancels the timer when a new path appears (in ensure_path) +// +// History: the old code had separate checks for local vs remote abandon: +// - Local: blocked if no remaining *validated* paths (checked path.data.validated) +// - Remote: blocked if no remaining paths at all +// Both returned Err(LastOpenPath), killing the connection. +// +// The fix unified both: always accept, start grace timer for last-path case. +// This means #400 ("connection killed when remote abandons last validated path") +// is also fixed — the old remote check didn't care about validation state, and the +// new code doesn't block at all. + +/// Regression test for #400: remote abandons the only validated path while an +/// unvalidated path exists. The connection should survive if the unvalidated +/// path validates. +/// +/// Pre-fix: the remote check didn't consider validation, so this case was +/// accepted (the bug in #400 was actually about the *local* check blocking +/// when only unvalidated paths remained). Post-fix (#397): always accepted, +/// no guard at all. +/// +/// draft-ietf-quic-multipath-21 Section 3.4 para 4 (MUST accept) +#[test] +fn remote_abandon_last_validated_path_unvalidated_remains() -> TestResult { + let _guard = subscribe(); + let mut pair = multipath_pair(); + + // Set up routing for two address pairs + let mut second_client_addr = pair.client.addr; + let mut second_server_addr = pair.server.addr; + second_client_addr.set_port(second_client_addr.port() + 1); + second_server_addr.set_port(second_server_addr.port() + 1); + pair.routes = Some(RoutingTable::simple_symmetric( + [pair.client.addr, second_client_addr], + [pair.server.addr, second_server_addr], + )); + + // Open path 1 from client but don't fully drive — leave it unvalidated. + let path1_net = FourTuple { + local_ip: Some(second_client_addr.ip()), + remote: second_server_addr, + }; + info!("client opens path 1 (will remain unvalidated)"); + let path1 = pair.open_path(Client, path1_net, PathStatus::Available)?; + // Drive client → server: PATH_CHALLENGE sent and received + pair.drive_client(); + pair.drive_server(); + // Don't drive further — PATH_RESPONSE not yet delivered to client + + // Server abandons path 0 — the only validated path from the client's perspective. + info!("server abandons path 0 (client's only validated path)"); + pair.close_path(Server, PathId::ZERO, 0u8.into())?; + pair.drive(); + + // Client must accept the abandon (Section 3.4 para 4) + let client_stats = pair.stats(Client); + assert!( + client_stats.frame_rx.path_abandon >= 1, + "client should have received PATH_ABANDON for path 0" + ); + + // Drive to let path 1 validate (PATH_RESPONSE arrives during drive) + pair.drive(); + + // After path 1 validates, the connection should survive + assert!( + !pair.is_closed(Client), + "client should survive — path 1 validated" + ); + assert!( + !pair.is_closed(Server), + "server should survive — path 1 is usable" + ); + assert!( + pair.path_status(Client, path1).is_ok(), + "path 1 should still be alive" + ); + + Ok(()) +} + +/// Regression test for #398/#400: remote abandons the only validated path and +/// the remaining unvalidated path fails validation. The connection should +/// eventually close. +/// +/// The close happens through the chain: path validation timeout → path 1 +/// abandoned → is_last_path = true → NoViablePath grace timer → CONNECTION_CLOSE. +/// +/// draft-ietf-quic-multipath-21 Section 3.4 para 7 +#[test] +fn remote_abandon_last_validated_path_validation_fails() -> TestResult { + let _guard = subscribe(); + + let mut cfg = TransportConfig::default(); + cfg.max_concurrent_multipath_paths(MAX_PATHS); + cfg.initial_rtt(Duration::from_millis(10)); + + let mut pair = ConnPair::with_transport_cfg(cfg.clone(), cfg); + pair.drive(); + + // Use an unreachable address for path 1 so validation will fail + let unreachable_server: SocketAddr = SocketAddr::new( + [9, 8, 7, 6].into(), + SERVER_PORTS.lock()?.next().ok_or("no port")?, + ); + + // Open path 1 to unreachable address — it will never validate + let path1_net = FourTuple { + remote: unreachable_server, + local_ip: None, + }; + info!("client opens path 1 to unreachable address"); + let _path1 = pair.open_path(Client, path1_net, PathStatus::Available)?; + pair.drive_client(); + pair.drive_server(); + + // Server abandons path 0 — the only validated path + info!("server abandons path 0"); + pair.close_path(Server, PathId::ZERO, 0u8.into())?; + + // Drive to idle — path 1 validation will time out, then connection closes + pair.drive(); + while pair.step() {} + + // The connection should be closed: + // - path 0: abandoned by server + // - path 1: validation failed (unreachable) → abandoned → NoViablePath timer fires + assert!( + pair.is_closed(Client), + "client should close — no viable path after validation failure" + ); + + let mut saw_connection_lost = false; + while let Some(event) = pair.poll(Client) { + if matches!(&event, Event::ConnectionLost { .. }) { + saw_connection_lost = true; + } + } + assert!( + saw_connection_lost, + "client should emit ConnectionLost after no viable path" + ); + + Ok(()) +} + +/// Regression test for #398: when the remote abandons all paths, the client +/// can recover by opening a new path within the grace period. +/// +/// Tests the combination of: +/// - #400: last validated path abandoned with unvalidated remaining +/// - #398: application opens new path to recover +/// +/// draft-ietf-quic-multipath-21 Section 3.4 para 7: +/// "Alternatively, a client MAY instead try to open a new path" +#[test] +fn remote_abandon_last_validated_path_recovery_via_new_path() -> TestResult { + let _guard = subscribe(); + + let mut cfg = TransportConfig::default(); + cfg.max_concurrent_multipath_paths(4); + cfg.initial_rtt(Duration::from_millis(10)); + + let mut pair = ConnPair::with_transport_cfg(cfg.clone(), cfg); + pair.drive(); + + // Set up 3 address pairs + let mut addrs_client = vec![pair.client.addr]; + let mut addrs_server = vec![pair.server.addr]; + for i in 1..3u16 { + let mut ca = pair.client.addr; + ca.set_port(ca.port() + i); + addrs_client.push(ca); + let mut sa = pair.server.addr; + sa.set_port(sa.port() + i); + addrs_server.push(sa); + } + pair.routes = Some(RoutingTable::simple_symmetric( + addrs_client.clone(), + addrs_server.clone(), + )); + + // Open path 1 but leave it unvalidated + let path1_net = FourTuple { + local_ip: Some(addrs_client[1].ip()), + remote: addrs_server[1], + }; + info!("client opens path 1 (will remain unvalidated)"); + let _path1 = pair.open_path(Client, path1_net, PathStatus::Available)?; + pair.drive_client(); + pair.drive_server(); + + // Server abandons path 0 (only validated path) + info!("server abandons path 0"); + pair.close_path(Server, PathId::ZERO, 0u8.into())?; + pair.drive_server(); + pair.drive_client(); + + // Client opens a new path (path 2) to a reachable address + let path2_net = FourTuple { + local_ip: Some(addrs_client[2].ip()), + remote: addrs_server[2], + }; + info!("client opens path 2 within grace period"); + let path2 = pair.open_path(Client, path2_net, PathStatus::Available)?; + + // Full drive — path 2 should validate + pair.drive(); + + // Connection should survive + assert!( + !pair.is_closed(Client), + "client should survive — new path validated" + ); + assert!( + !pair.is_closed(Server), + "server should survive — new path validated" + ); + assert!( + pair.path_status(Client, path2).is_ok(), + "path 2 should be alive" + ); + + Ok(()) +} diff --git a/noq-proto/src/tests/proptests.rs b/noq-proto/src/tests/proptests.rs index ce893c51c..d85a0eab2 100644 --- a/noq-proto/src/tests/proptests.rs +++ b/noq-proto/src/tests/proptests.rs @@ -200,7 +200,7 @@ fn allowed_error(err: Option) -> bool { None => true, Some(ConnectionError::TransportError(err)) => { // keep in sync with connection/mod.rs - &err.reason == "last path abandoned by peer" + &err.reason == "last path abandoned, no new path opened" } Some(ConnectionError::ConnectionClosed(ConnectionClose { error_code, .. })) => { *error_code != TransportErrorCode::PROTOCOL_VIOLATION