Decide the discovery path-MTU write with the shared keep-tighter rule

The LookupResponse handler open-coded the keep-tighter comparison that
should_apply_path_mtu already holds for the session carriers. It now
calls that function, so the discovery write and the session tightens
follow one rule. The equal-value case still keeps the stored entry and
its learn time, so a later answer of the same value cannot extend the
entry's deadline.

No handler test reached that equal-value case: a replayed response is
dropped as unsolicited before it gets to the write, and the keep-tighter
test uses a strictly looser value. A new test answers two genuine
lookups for one target with the same path MTU and checks the entry is
left exactly as it was. The replay test's comment and the handler's
comment on the keep-tighter arm are corrected to say what that arm
bounds and what actually stops a replay, and the shared function's
documentation now says it writes only a strictly tighter value, which
is what it does.
This commit is contained in:
Johnathan Corgan
2026-10-02 18:14:01 +00:00
parent 3c2dfa9d41
commit e431390381
4 changed files with 76 additions and 17 deletions
+13 -7
View File
@@ -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!(
+56 -4
View File
@@ -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
// ============================================================================
+6 -5
View File
@@ -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<u16>, candidate: u16) -> bool {
!matches!(existing, Some(existing) if existing <= candidate)
}
+1 -1
View File
@@ -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,