diff --git a/src/node/handlers/lookup.rs b/src/node/handlers/lookup.rs index 88cae3c0..3f783115 100644 --- a/src/node/handlers/lookup.rs +++ b/src/node/handlers/lookup.rs @@ -7,6 +7,7 @@ use crate::node::Node; use crate::node::reject::DiscoveryReject; +use crate::proto::fsp::should_apply_path_mtu; use crate::proto::lookup::{ LookupAction, LookupRequest, LookupResponse, MAX_RECENT_LOOKUP_REQUESTS, }; @@ -416,7 +417,9 @@ impl Node { let fips_addr = crate::FipsAddress::from_node_addr(&target); match self.path_mtu_lookup.write() { Ok(mut map) => match map.get(&fips_addr).copied() { - Some(existing) if existing.mtu <= path_mtu => { + Some(existing) + if !should_apply_path_mtu(Some(existing.mtu), path_mtu) => + { // Keep the tighter learned value; never loosen // the clamp. A reactive MtuExceeded or // PathMtuNotification tighten takes precedence @@ -424,12 +427,15 @@ impl Node { // (cross-carrier keep-tighter). // // This arm deliberately leaves `learned_ms` - // alone. That is what bounds a replayed - // response: the replay of a value already - // stored takes this arm, so the entry still - // expires at first-write plus the TTL rather - // than being pushed out again on every - // injection. Refreshing the stamp here would + // alone. A later answered lookup that reports + // the value already stored takes this arm, so + // the entry still expires at first-write plus + // the TTL rather than being pushed out again + // by every answer of the same value. (A + // replayed response never gets here: the + // pending lookup is gone once the first answer + // is accepted, so the copy is dropped as + // unsolicited.) Refreshing the stamp here would // read as a tidy-up and would silently restore // indefinite pinning. debug!( diff --git a/src/node/tests/discovery.rs b/src/node/tests/discovery.rs index 2bf4b64c..3b29b4a0 100644 --- a/src/node/tests/discovery.rs +++ b/src/node/tests/discovery.rs @@ -1653,10 +1653,11 @@ async fn test_lookup_response_path_mtu_expires_without_a_session() { #[tokio::test] async fn test_replayed_lookup_response_does_not_extend_the_path_mtu_deadline() { - // The response carries no replay dedupe, so a captured one can be - // re-injected indefinitely. What bounds the damage is that a replay of a - // value already stored takes the keep-tighter arm, which does not touch - // the learn time: each injection buys one TTL, not one per packet. + // The response carries no replay dedupe of its own, so a captured one can + // be re-injected indefinitely. Accepting the first response clears the + // pending lookup, so each replay is dropped as unsolicited before it + // reaches the path-MTU write: each injection buys one TTL, not one per + // packet. The equal-value arm of that write is pinned by the next test. let mut node = make_node(); let from = make_node_addr(0xAA); @@ -1694,6 +1695,57 @@ async fn test_replayed_lookup_response_does_not_extend_the_path_mtu_deadline() { ); } +#[tokio::test] +async fn test_a_later_solicited_response_of_the_same_path_mtu_keeps_the_learn_time() { + // Two genuine lookups for one target, answered with the same path_mtu. + // The second answer is solicited, so it reaches the path-MTU write, and an + // equal value must keep the stored entry, learn time included. Refreshing + // the stamp on equality would let every answer of the same value push the + // deadline out again. + let mut node = make_node(); + let from = make_node_addr(0xAA); + + let target_identity = Identity::generate(); + let target = *target_identity.node_addr(); + let target_fips = crate::FipsAddress::from_node_addr(&target); + let root = make_node_addr(0xF0); + let coords = TreeCoordinate::from_addrs(vec![target, root]).unwrap(); + node.register_identity(target, target_identity.pubkey_full()); + + let answer = |request_id: u64| { + let proof = + target_identity.sign(&LookupResponse::proof_bytes(request_id, &target, &coords)); + let mut response = LookupResponse::new(request_id, target, coords.clone(), proof); + response.path_mtu = 1300; + response.encode()[1..].to_vec() + }; + + seed_pending_lookup(&mut node, target, 805); + node.handle_lookup_response(&from, &answer(805)).await; + let first = node + .path_mtu_lookup_entry(&target_fips) + .expect("precondition: the first response wrote an entry"); + assert!( + first.learned_ms.is_some(), + "precondition: the entry carries a learn time" + ); + + // Real elapsed wall-clock, so a refreshed stamp would differ. + std::thread::sleep(std::time::Duration::from_millis(5)); + seed_pending_lookup(&mut node, target, 806); + node.handle_lookup_response(&from, &answer(806)).await; + + assert!( + !node.lookup.pending_lookups.contains_key(&target), + "precondition: the second response was accepted as solicited" + ); + assert_eq!( + node.path_mtu_lookup_entry(&target_fips), + Some(first), + "an equal path_mtu must leave the entry exactly as it was, learn time included" + ); +} + // ============================================================================ // Open-Discovery Sweep — cache-injection unit test // ============================================================================ diff --git a/src/proto/fsp/core.rs b/src/proto/fsp/core.rs index f45526a0..8a3b25c4 100644 --- a/src/proto/fsp/core.rs +++ b/src/proto/fsp/core.rs @@ -483,10 +483,10 @@ impl Fsp { } /// Decide whether a path-MTU update should tighten the shared lookup: emit - /// `TightenPathMtuLookup` only when `candidate` is at least as tight as the - /// `existing` value (keep-tighter, never loosen). The `existing` read and - /// the applied write are performed shell-side under one `path_mtu_lookup` - /// write guard, so the decision stays atomic. + /// `TightenPathMtuLookup` only when there is no `existing` value or + /// `candidate` is strictly tighter than it (keep-tighter, never loosen). + /// The `existing` read and the applied write are performed shell-side + /// under one `path_mtu_lookup` write guard, so the decision stays atomic. pub(crate) fn plan_path_mtu_tighten( &self, fips_addr: FipsAddress, @@ -524,7 +524,8 @@ pub(crate) fn initiation_winner(our_node_addr: &NodeAddr, their_node_addr: &Node /// Decide whether a path-MTU update should be applied to the shared /// `FipsAddress`-keyed lookup: keep the tighter of existing-or-candidate, never /// loosen. Returns `true` when `candidate` should be written (there is no -/// existing value, or the candidate is at least as tight). +/// existing value, or the candidate is strictly tighter). An equal candidate is +/// not written, so the stored entry, and any learn time it carries, is kept. pub(crate) fn should_apply_path_mtu(existing: Option, candidate: u16) -> bool { !matches!(existing, Some(existing) if existing <= candidate) } diff --git a/src/proto/fsp/mod.rs b/src/proto/fsp/mod.rs index 7c9b37f5..15df2ff5 100644 --- a/src/proto/fsp/mod.rs +++ b/src/proto/fsp/mod.rs @@ -33,7 +33,7 @@ mod tests; pub(crate) use core::{ DecryptSlot, EpochReaction, Fsp, FspAction, InitialMsg3ResendSnapshot, RekeyCfg, RekeyMsg3ResendSnapshot, SessionSnapshot, cutover_timer_elapsed, initiation_winner, - mark_ipv6_ecn_ce, push_bounded_pending, + mark_ipv6_ecn_ce, push_bounded_pending, should_apply_path_mtu, }; pub use wire::{ FspInnerFlags, SessionAck, SessionFlags, SessionMessageType, SessionMsg3, SessionSetup,