diff --git a/CHANGELOG.md b/CHANGELOG.md index 9419d84..0db297c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -392,6 +392,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the TUN-side `path_mtu_lookup` so later flows pick up forward-path bottlenecks without re-discovery. Windows TUN reader receives the same per-destination plumbing. +- Proactive end-to-end `PathMtuNotification` now mirrors into the + TUN-side `path_mtu_lookup` (TCP MSS clamp store), parallel to the + reactive `MtuExceeded` mirror that already existed. Previously the + proactive handler only updated the session-canonical + `MmpSessionState.path_mtu`; on stable long-lived paths where the + destination's echo had tightened the session MTU but no transit + router had emitted a fresh `MtuExceeded` (because all current + traffic was already sized by the tighter session value), new TCP + flows opened in that window kept getting clamped by the staler + discovery-time value. The proactive mirror closes that gap with + the same tighter-only semantics — never loosens the clamp. - Auto-connect peers now reconnect after a graceful `Disconnect` notification from the remote side. `handle_disconnect` previously removed the peer without scheduling a reconnect, orphaning the diff --git a/src/node/handlers/session.rs b/src/node/handlers/session.rs index 0f9a795..695b3ba 100644 --- a/src/node/handlers/session.rs +++ b/src/node/handlers/session.rs @@ -949,7 +949,11 @@ impl Node { /// /// The destination is telling us the path MTU has changed. /// Apply source-side rules (decrease immediate, increase validated). - fn handle_session_path_mtu_notification(&mut self, src_addr: &NodeAddr, body: &[u8]) { + pub(in crate::node) fn handle_session_path_mtu_notification( + &mut self, + src_addr: &NodeAddr, + body: &[u8], + ) { let notif = match PathMtuNotification::decode(body) { Ok(n) => n, Err(e) => { @@ -973,16 +977,59 @@ impl Node { let old_mtu = mmp.path_mtu.current_mtu(); let now = std::time::Instant::now(); - mmp.path_mtu.apply_notification(notif.path_mtu, now); + let changed = mmp.path_mtu.apply_notification(notif.path_mtu, now); let new_mtu = mmp.path_mtu.current_mtu(); - if new_mtu != old_mtu { - debug!( - src = %peer_name, - old_mtu, - new_mtu, - "Path MTU changed via notification" - ); + if !changed { + return; + } + + debug!( + src = %peer_name, + old_mtu, + new_mtu, + "Path MTU changed via notification" + ); + + // Mirror the new effective MTU into the FipsAddress-keyed lookup used + // by the TUN reader/writer at TCP MSS clamp time. Without this, new + // TCP flows opened on a path the proactive end-to-end echo has + // already tightened keep getting clamped by the staler discovery- + // time value until a reactive MtuExceeded happens to fire. Keep the + // tighter of existing-or-new — never loosen the clamp. + let fips_addr = crate::FipsAddress::from_node_addr(src_addr); + match self.path_mtu_lookup.write() { + Ok(mut map) => match map.get(&fips_addr).copied() { + Some(existing) if existing <= new_mtu => { + debug!( + dest = %peer_name, + fips_addr = %fips_addr, + new_mtu, + existing, + "PathMtuNotification: keeping tighter existing path_mtu_lookup value" + ); + } + other => { + map.insert(fips_addr, new_mtu); + debug!( + dest = %peer_name, + fips_addr = %fips_addr, + new_mtu, + prior = ?other, + map_len = map.len(), + "PathMtuNotification: tightened path_mtu_lookup" + ); + } + }, + Err(e) => { + warn!( + dest = %peer_name, + fips_addr = %fips_addr, + new_mtu, + error = %e, + "path_mtu_lookup write lock poisoned; PathMtuNotification not reflected" + ); + } } } diff --git a/src/node/tests/session.rs b/src/node/tests/session.rs index 4897d40..decab10 100644 --- a/src/node/tests/session.rs +++ b/src/node/tests/session.rs @@ -2207,3 +2207,123 @@ async fn test_handle_mtu_exceeded_keeps_tighter_existing_path_mtu_lookup() { "MtuExceeded with looser bottleneck must not loosen a tighter existing value" ); } + +// ============================================================================ +// Proactive PathMtuNotification → path_mtu_lookup focused unit tests +// +// These exercise the receive-side write path that mirrors the proactive +// end-to-end echo into `path_mtu_lookup`. Without this mirror, new TCP +// flows opened on a path the proactive notification has tightened keep +// getting clamped by the staler discovery-time value until a reactive +// MtuExceeded fires for those flows — long-lived stable paths can sit +// in the gap indefinitely. +// ============================================================================ + +/// Build a PathMtuNotification body (2 bytes: path_mtu LE). +fn build_path_mtu_notification_body(mtu: u16) -> Vec { + mtu.to_le_bytes().to_vec() +} + +/// Insert an Established session with MMP initialized so the proactive +/// PathMtuNotification handler can apply notifications. +fn install_established_session_with_mmp(node: &mut Node, remote: &Identity) { + let session = make_noise_session(node.identity(), remote); + let remote_addr = *remote.node_addr(); + let mut entry = crate::node::session::SessionEntry::new( + remote_addr, + remote.pubkey_full(), + EndToEndState::Established(session), + 1000, + true, + ); + entry.init_mmp(&node.config.node.session_mmp); + node.sessions.insert(remote_addr, entry); +} + +#[test] +fn test_handle_path_mtu_notification_writes_path_mtu_lookup_when_empty() { + let mut node = make_node(); + let remote = Identity::generate(); + let remote_addr = *remote.node_addr(); + let remote_fips = crate::FipsAddress::from_node_addr(&remote_addr); + + install_established_session_with_mmp(&mut node, &remote); + + assert!( + node.path_mtu_lookup_get(&remote_fips).is_none(), + "lookup should start empty for this destination" + ); + + let body = build_path_mtu_notification_body(1280); + node.handle_session_path_mtu_notification(&remote_addr, &body); + + assert_eq!( + node.path_mtu_lookup_get(&remote_fips), + Some(1280), + "PathMtuNotification should populate path_mtu_lookup with the reported MTU" + ); +} + +#[test] +fn test_handle_path_mtu_notification_tightens_existing_path_mtu_lookup() { + let mut node = make_node(); + let remote = Identity::generate(); + let remote_addr = *remote.node_addr(); + let remote_fips = crate::FipsAddress::from_node_addr(&remote_addr); + + install_established_session_with_mmp(&mut node, &remote); + + // Pre-seed with a generous value (e.g., from the discovery seed at link + // promotion time, before the destination's proactive echo arrived). + node.path_mtu_lookup_insert(remote_fips, 1500); + + let body = build_path_mtu_notification_body(1280); + node.handle_session_path_mtu_notification(&remote_addr, &body); + + assert_eq!( + node.path_mtu_lookup_get(&remote_fips), + Some(1280), + "PathMtuNotification with smaller MTU must tighten the lookup" + ); +} + +#[test] +fn test_handle_path_mtu_notification_keeps_tighter_existing_path_mtu_lookup() { + let mut node = make_node(); + let remote = Identity::generate(); + let remote_addr = *remote.node_addr(); + let remote_fips = crate::FipsAddress::from_node_addr(&remote_addr); + + install_established_session_with_mmp(&mut node, &remote); + + // Pre-seed with a tighter value than what the proactive notification + // reports (e.g., from a prior reactive MtuExceeded on a narrower hop). + // The mirror must never loosen the clamp. + node.path_mtu_lookup_insert(remote_fips, 1200); + + let body = build_path_mtu_notification_body(1400); + node.handle_session_path_mtu_notification(&remote_addr, &body); + + assert_eq!( + node.path_mtu_lookup_get(&remote_fips), + Some(1200), + "PathMtuNotification with looser MTU must not loosen a tighter existing value" + ); +} + +#[test] +fn test_handle_path_mtu_notification_no_session_no_op() { + let mut node = make_node(); + let remote = Identity::generate(); + let remote_addr = *remote.node_addr(); + let remote_fips = crate::FipsAddress::from_node_addr(&remote_addr); + + // No session installed. The handler should drop the notification entirely. + let body = build_path_mtu_notification_body(1280); + node.handle_session_path_mtu_notification(&remote_addr, &body); + + assert!( + node.path_mtu_lookup_get(&remote_fips).is_none(), + "PathMtuNotification with no session must not touch path_mtu_lookup" + ); +}