Let flap dampening engage again after an episode lapses

The arming check tested whether a dampening deadline had ever been set
rather than whether one was still in effect, so the first episode
disarmed the mechanism permanently. A node in a second flap storm went
on switching parents under hold-down alone, and neither the
flap_dampened counter nor the "Flap dampening engaged" warning fired
again, so the storm was invisible to anyone watching that counter.

Retire a lapsed episode explicitly, clearing both the deadline and the
switch counter, so a second episode requires a fresh threshold of
switches within one window rather than re-engaging on the first switch
after lapse. Hold-down was unaffected throughout and continued to limit
discretionary switching, which is why the practical effect at shipped
settings was lost visibility and a lost escalation tier rather than
unrestrained flapping.

An episode engaged through the parent-ancestry update path now reports
the counter and the warning as the other switch paths already did. One
path remains silent, a re-engagement during parent-loss recovery, which
runs inside the tree state where no metrics handle is reachable.

Also cap node.tree.flap_dampening_secs at one year, so a value large
enough to overflow the monotonic clock no longer panics the node when
dampening engages.

Report flap dampening engagement from every path that engages it

One re-engagement path stayed silent, during parent-loss recovery, because
it runs inside the tree state where no metrics handle is reachable. Return
the fact of engagement to the caller that does have one, so every path that
engages dampening reports the counter and the warning rather than most of
them.

Report dampening engagement without moving a published signature

The observability change altered the return type of a published library
function on the maintenance line, which would break a downstream caller at
a patch release. Restore the signature and record the one path that stays
silent as a known gap, which is what the design called for.

CHANGELOG.md

@@ -112,6 +112,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   now also carries the frame's protocol version and flags, which separate a
   short frame from a bad-version or Unencrypted-flagged one.
 
+- Flap dampening can now engage more than once in the lifetime of a node.
+  The arming check tested whether a dampening deadline had ever been set
+  rather than whether one was still in effect, so the first episode
+  disarmed the mechanism permanently: a node in a second flap storm went on
+  switching parents under hold-down alone, and neither the `flap_dampened`
+  counter nor the "Flap dampening engaged" warning fired again, so the
+  storm was invisible to anyone watching that counter. A lapsed episode is
+  now retired explicitly, clearing both the deadline and the switch
+  counter, so a second episode requires a fresh threshold of switches
+  within one window rather than re-engaging on the first switch after
+  lapse. Hold-down was unaffected throughout and continued to limit
+  discretionary switching, which is why the practical effect at shipped
+  settings was lost visibility and a lost escalation tier rather than
+  unrestrained flapping. Every path that can engage an episode now reports
+  it, including a re-engagement during parent-loss recovery, which was
+  previously silent. The warning names which path armed the episode
+  (`trigger`) and how long discretionary parent switching stays suppressed
+  (`dampening_secs`), using the same `trigger` values as the parent-switch
+  logs beside it, so the two can be read together.
+
+- A `node.tree.flap_dampening_secs` large enough to overflow the monotonic
+  clock no longer panics the node when dampening engages; the value is
+  capped at one year, beyond which an episode is indistinguishable from
+  permanent.
+
 - The maintainer address published in package metadata no longer bounces. The
   crate authors field, the Debian package maintainer and upstream contact, and
   both AUR PKGBUILD maintainer lines carried an address that no longer accepts

docs/design/spanning-tree-dynamics.md

@@ -924,7 +924,8 @@ The primary stability mechanisms are implemented:
   `flap_dampening_secs = 120`): if a node switches parents more than 4
   times within 60s, an extended 120s hold-down is imposed. Mandatory
   switches (parent loss, root change) bypass dampening. The flap counter
-  resets when the window expires naturally.
+  resets when the window expires and again when a dampening episode lapses,
+  so each episode requires a fresh threshold of switches within one window.
 
 These mechanisms compose to bound announcement traffic even under rapid link
 flapping. The hold-down timer limits the rate of parent switches (at most

src/node/handlers/mmp.rs

@@ -183,8 +183,7 @@ impl Node {
                     "Parent switched after first RTT measurement"
                 );
                 if flap_dampened {
-                    self.metrics().tree.flap_dampened.inc();
-                    warn!("Flap dampening engaged: excessive parent switches detected");
+                    self.note_flap("first-rtt");
                 }
                 self.send_tree_announce_to_all().await;
                 let all_peers: Vec<crate::NodeAddr> = self.peers.keys().copied().collect();

src/node/tree.rs

@@ -13,6 +13,18 @@ use super::{Node, NodeError};
 use tracing::{debug, info, trace, warn};
 
 impl Node {
+    /// Report a flap-dampening engagement: one counter tick and one warning
+    /// naming which path armed the episode and how long discretionary parent
+    /// switching stays suppressed.
+    pub(super) fn note_flap(&self, trigger: &str) {
+        self.metrics().tree.flap_dampened.inc();
+        warn!(
+            trigger = trigger,
+            dampening_secs = self.tree_state.dampening_secs(),
+            "Flap dampening engaged, discretionary parent switching suppressed"
+        );
+    }
+
     /// Build a TreeAnnounce from our current tree state.
     fn build_tree_announce(&self) -> Result<TreeAnnounce, NodeError> {
         let decl = self.tree_state.my_declaration().clone();
@@ -310,8 +322,7 @@ impl Node {
                 "Parent switched, invalidated downstream coord cache entries, announcing to all peers"
             );
             if flap_dampened {
-                self.metrics().tree.flap_dampened.inc();
-                warn!("Flap dampening engaged: excessive parent switches detected");
+                self.note_flap("announce");
             }
 
             self.send_tree_announce_to_all().await;
@@ -363,7 +374,8 @@ impl Node {
                     .filter(|(_, peer)| peer.has_srtt())
                     .map(|(addr, peer)| (*addr, peer.link_cost()))
                     .collect();
-                if self.tree_state.handle_parent_lost(&peer_costs) {
+                let outcome = self.tree_state.recover(&peer_costs);
+                if outcome.changed {
                     // Clone identity up front to avoid a split borrow against the
                     // &mut self.tree_state / &mut self.coord_cache calls below (cold path).
                     let our_identity = self.identity().clone();
@@ -374,7 +386,7 @@ impl Node {
                             .record_reject(TreeReject::OutboundSignFailed);
                         return;
                     }
-                    // handle_parent_lost may promote to root OR find new parent;
+                    // Recovery may promote to root OR find a new parent;
                     // cover both invalidation classes.
                     self.coord_cache
                         .invalidate_via_node(our_identity.node_addr());
@@ -382,6 +394,9 @@ impl Node {
                         .invalidate_other_roots(self.tree_state.root());
                     self.reset_discovery_backoff();
                     self.send_tree_announce_to_all().await;
+                    if outcome.dampened {
+                        self.note_flap("loop-detected");
+                    }
                 }
                 return;
             }
@@ -411,7 +426,10 @@ impl Node {
             // Clone identity up front to avoid a split borrow against the
             // &mut self.tree_state / &mut self.coord_cache calls below (cold path).
             let our_identity = self.identity().clone();
-            self.tree_state.set_parent(*from, new_seq, timestamp);
+            let flap_dampened = self.tree_state.set_parent(*from, new_seq, timestamp);
+            if flap_dampened {
+                self.note_flap("ancestry-update");
+            }
             self.tree_state.recompute_coords();
             if let Err(e) = self.tree_state.sign_declaration(&our_identity) {
                 warn!(error = %e, "Failed to sign declaration after parent update");
@@ -533,8 +551,7 @@ impl Node {
                 "Parent switched via periodic cost re-evaluation"
             );
             if flap_dampened {
-                self.metrics().tree.flap_dampened.inc();
-                warn!("Flap dampening engaged: excessive parent switches detected");
+                self.note_flap("periodic");
             }
 
             self.send_tree_announce_to_all().await;
@@ -605,7 +622,8 @@ impl Node {
                 .filter(|(_, peer)| peer.has_srtt())
                 .map(|(addr, peer)| (*addr, peer.link_cost()))
                 .collect();
-            let changed = self.tree_state.handle_parent_lost(&peer_costs);
+            let outcome = self.tree_state.recover(&peer_costs);
+            let changed = outcome.changed;
             if changed {
                 // Re-sign the new declaration. Clone identity to avoid a split
                 // borrow against the &mut self.tree_state receiver (cold path).
@@ -616,7 +634,7 @@ impl Node {
                         .tree
                         .record_reject(TreeReject::OutboundSignFailed);
                 }
-                // handle_parent_lost may promote to root OR find new parent;
+                // Recovery may promote to root OR find a new parent;
                 // cover both invalidation classes (same as the loop-detection
                 // branch above). Without this, cached downstream entries keep
                 // our now-stale coordinate prefix until TTL — and get_and_touch
@@ -631,6 +649,9 @@ impl Node {
                     is_root = self.tree_state.is_root(),
                     "Tree state updated after parent loss"
                 );
+                if outcome.dampened {
+                    self.note_flap("parent-loss");
+                }
             }
             changed
         } else {

src/tree/state.rs

@@ -7,6 +7,25 @@ use std::time::{Duration, Instant};
 use super::{CoordEntry, ParentDeclaration, TreeCoordinate, TreeError};
 use crate::{Identity, NodeAddr};
 
+/// Longest dampening episode representable on the monotonic clock. A year
+/// is indistinguishable from permanent for this mechanism; the bound is
+/// what keeps a hostile `flap_dampening_secs` from overflowing the stamp.
+const MAX_FLAP_DAMPENING: Duration = Duration::from_secs(365 * 24 * 60 * 60);
+
+/// What a parent-loss recovery did: whether the tree state changed, and
+/// whether the recovery switch was the one that armed a dampening episode.
+///
+/// Crate-internal on purpose. The published entry point is
+/// [`TreeState::handle_parent_lost`], whose `bool` return this type must not
+/// displace.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(crate) struct ParentLoss {
+    /// Whether the tree state changed and the caller should re-announce.
+    pub(crate) changed: bool,
+    /// Whether the recovery switch armed a flap dampening episode.
+    pub(crate) dampened: bool,
+}
+
 /// Local spanning tree state for a node.
 ///
 /// Contains this node's declaration, coordinates, and view of peers'
@@ -312,7 +331,16 @@ impl TreeState {
     pub fn set_flap_dampening(&mut self, threshold: u32, window_secs: u64, dampening_secs: u64) {
         self.flap_threshold = threshold;
         self.flap_window = Duration::from_secs(window_secs);
-        self.flap_dampening_duration = Duration::from_secs(dampening_secs);
+        self.flap_dampening_duration = Duration::from_secs(dampening_secs).min(MAX_FLAP_DAMPENING);
+    }
+
+    /// How long a dampening episode suppresses discretionary parent switching,
+    /// after the configured value is clamped.
+    ///
+    /// Crate-internal: it feeds a log field, and the published surface of this
+    /// maintenance line does not grow for it.
+    pub(crate) fn dampening_secs(&self) -> u64 {
+        self.flap_dampening_duration.as_secs()
     }
 
     /// Record a parent switch for flap detection.
@@ -320,6 +348,17 @@ impl TreeState {
     pub fn record_parent_switch(&mut self) -> bool {
         let now = Instant::now();
 
+        // Retire a lapsed episode here rather than lazily. Clearing the
+        // deadline and the counter together is what makes each episode cost a
+        // fresh threshold of switches inside one window: switches taken during
+        // an episode (mandatory ones bypass the veto) would otherwise carry
+        // into the next window and re-engage on a single switch after lapse.
+        if self.flap_dampening_until.is_some() && !self.dampened_at(now) {
+            self.flap_dampening_until = None;
+            self.flap_count = 0;
+            self.flap_window_start = None;
+        }
+
         // Reset window if expired or not started
         match self.flap_window_start {
             Some(start) if now.duration_since(start) < self.flap_window => {
@@ -331,20 +370,29 @@ impl TreeState {
             }
         }
 
-        // Check threshold
-        if self.flap_count >= self.flap_threshold && self.flap_dampening_until.is_none() {
-            self.flap_dampening_until = Some(now + self.flap_dampening_duration);
-            return true;
+        // Check threshold. The dampening test is redundant with the retirement
+        // above in this control flow; it is kept so the gate reads correctly on
+        // its own and survives an edit that moves the retirement.
+        if self.flap_count >= self.flap_threshold && !self.dampened_at(now) {
+            match now.checked_add(self.flap_dampening_duration) {
+                Some(until) => {
+                    self.flap_dampening_until = Some(until);
+                    return true;
+                }
+                None => debug_assert!(false, "clamped dampening duration must fit the clock"),
+            }
         }
         false
     }
 
+    /// Whether a dampening episode is still running as of `now`.
+    fn dampened_at(&self, now: Instant) -> bool {
+        self.flap_dampening_until.is_some_and(|until| now < until)
+    }
+
     /// Check if flap dampening is currently active.
     pub fn is_flap_dampened(&self) -> bool {
-        match self.flap_dampening_until {
-            Some(until) => Instant::now() < until,
-            None => false,
-        }
+        self.dampened_at(Instant::now())
     }
 
     /// Evaluate whether to switch parents based on current peer tree state.
@@ -499,6 +547,17 @@ impl TreeState {
     ///
     /// Returns `true` if the tree state changed (caller should re-announce).
     pub fn handle_parent_lost(&mut self, peer_costs: &HashMap<NodeAddr, f64>) -> bool {
+        self.recover(peer_costs).changed
+    }
+
+    /// Handle loss of current parent, reporting whether the recovery switch
+    /// itself armed a flap dampening episode.
+    ///
+    /// Same recovery as [`TreeState::handle_parent_lost`], which delegates
+    /// here. A caller holding a metrics handle uses this one so the
+    /// engagement can be counted and logged; the published signature stays
+    /// `bool`.
+    pub(crate) fn recover(&mut self, peer_costs: &HashMap<NodeAddr, f64>) -> ParentLoss {
         // Try to find an alternative parent
         if let Some(new_parent) = self.evaluate_parent(peer_costs) {
             let timestamp = std::time::SystemTime::now()
@@ -506,12 +565,16 @@ impl TreeState {
                 .map(|d| d.as_secs())
                 .unwrap_or(0);
             let new_seq = self.my_declaration.sequence() + 1;
-            self.set_parent(new_parent, new_seq, timestamp);
+            let dampened = self.set_parent(new_parent, new_seq, timestamp);
             self.recompute_coords();
-            return true;
+            return ParentLoss {
+                changed: true,
+                dampened,
+            };
         }
 
-        // No alternative: become own root
+        // No alternative: become own root. This branch never calls
+        // `set_parent`, so it cannot arm a dampening episode.
         let timestamp = std::time::SystemTime::now()
             .duration_since(std::time::UNIX_EPOCH)
             .map(|d| d.as_secs())
@@ -519,7 +582,10 @@ impl TreeState {
         let new_seq = self.my_declaration.sequence() + 1;
         self.my_declaration = ParentDeclaration::self_root(self.my_node_addr, new_seq, timestamp);
         self.recompute_coords();
-        true
+        ParentLoss {
+            changed: true,
+            dampened: false,
+        }
     }
 
     /// Sign this node's declaration with the given identity.

src/tree/tests.rs

@@ -1635,3 +1635,184 @@ fn test_flap_dampening_same_parent_no_count() {
     // Should NOT be dampened since only the first was a real switch
     assert!(!state.is_flap_dampened());
 }
+
+#[test]
+fn test_flap_dampening_engages_a_second_time_after_first_episode_lapses() {
+    // A lapsed episode must re-arm the mechanism: a second flap storm has to
+    // engage dampening again, and must cost a fresh threshold of switches.
+    // 0-second dampening makes each episode lapse the instant it is stamped.
+    let my_node = make_node_addr(5);
+    let mut state = TreeState::new(my_node);
+    state.set_flap_dampening(3, 60, 0);
+    state.set_hold_down(0);
+
+    let peer_a = make_node_addr(1);
+    let peer_b = make_node_addr(2);
+    let root = make_node_addr(0);
+
+    state.update_peer(
+        ParentDeclaration::new(peer_a, root, 1, 1000),
+        make_coords(&[1, 0]),
+    );
+    state.update_peer(
+        ParentDeclaration::new(peer_b, root, 1, 1000),
+        make_coords(&[2, 0]),
+    );
+
+    // First episode: three switches reach threshold.
+    let first = state.set_parent(peer_a, 1, 1000);
+    state.recompute_coords();
+    let second = state.set_parent(peer_b, 2, 2000);
+    state.recompute_coords();
+    let third = state.set_parent(peer_a, 3, 3000);
+    state.recompute_coords();
+
+    assert!(!first);
+    assert!(!second);
+    assert!(third, "first episode must engage at threshold");
+
+    // Zero-second duration: the episode has already lapsed.
+    assert!(!state.is_flap_dampened());
+
+    // Second episode: a fresh threshold of switches is required, so the first
+    // two switches after the lapse must not re-engage.
+    let fourth = state.set_parent(peer_b, 4, 4000);
+    state.recompute_coords();
+    let fifth = state.set_parent(peer_a, 5, 5000);
+    state.recompute_coords();
+    let sixth = state.set_parent(peer_b, 6, 6000);
+    state.recompute_coords();
+
+    assert!(!fourth, "a lapsed episode must not re-engage on one switch");
+    assert!(
+        !fifth,
+        "a lapsed episode must not re-engage below threshold"
+    );
+    assert!(sixth, "a second episode must engage after the first lapses");
+
+    assert!(!state.is_flap_dampened());
+}
+
+#[test]
+fn test_flap_dampening_duration_at_u64_max_does_not_panic() {
+    // A hostile flap_dampening_secs must be clamped rather than overflow the
+    // monotonic clock when the stamp is taken.
+    let my_node = make_node_addr(5);
+    let mut state = TreeState::new(my_node);
+    state.set_flap_dampening(3, 60, u64::MAX);
+    state.set_hold_down(0);
+
+    let peer_a = make_node_addr(1);
+    let peer_b = make_node_addr(2);
+    let root = make_node_addr(0);
+
+    state.update_peer(
+        ParentDeclaration::new(peer_a, root, 1, 1000),
+        make_coords(&[1, 0]),
+    );
+    state.update_peer(
+        ParentDeclaration::new(peer_b, root, 1, 1000),
+        make_coords(&[2, 0]),
+    );
+
+    state.set_parent(peer_a, 1, 1000);
+    state.recompute_coords();
+    state.set_parent(peer_b, 2, 2000);
+    state.recompute_coords();
+    let dampened = state.set_parent(peer_a, 3, 3000);
+    state.recompute_coords();
+
+    assert!(dampened);
+    assert!(state.is_flap_dampened());
+}
+
+#[test]
+fn test_parent_loss_recovery_reports_the_engagement_that_arms_dampening() {
+    // A parent-loss storm engages dampening on a mandatory recovery switch,
+    // which bypasses the veto but still feeds the flap counter. The recovery
+    // must report that engagement so the caller can surface it.
+    let my_node = make_node_addr(5);
+    let mut state = TreeState::new(my_node);
+    state.set_flap_dampening(2, 60, 120);
+    state.set_hold_down(0);
+
+    let peer_a = make_node_addr(1);
+    let peer_b = make_node_addr(2);
+    let root = make_node_addr(0);
+
+    state.update_peer(
+        ParentDeclaration::new(peer_a, root, 1, 1000),
+        make_coords(&[1, 0]),
+    );
+    state.update_peer(
+        ParentDeclaration::new(peer_b, root, 1, 1000),
+        make_coords(&[2, 0]),
+    );
+
+    // One switch short of the threshold.
+    let first = state.set_parent(peer_a, 1, 1000);
+    state.recompute_coords();
+    assert!(!first, "one switch is below the threshold");
+
+    // Parent disappears; recovery picks peer_b and crosses the threshold.
+    state.remove_peer(&peer_a);
+    let outcome = state.recover(&HashMap::new());
+
+    assert!(outcome.changed);
+    assert_eq!(state.my_declaration().parent_id(), &peer_b);
+    assert!(
+        outcome.dampened,
+        "parent-loss recovery must report the engagement it armed"
+    );
+    assert!(state.is_flap_dampened());
+}
+
+#[test]
+fn test_parent_loss_recovery_to_self_root_reports_no_engagement() {
+    // The self-root fallthrough takes no parent switch, so it can never arm
+    // an episode and must never report one.
+    let my_node = make_node_addr(5);
+    let mut state = TreeState::new(my_node);
+    state.set_flap_dampening(1, 60, 120);
+    state.set_hold_down(0);
+
+    let peer_a = make_node_addr(1);
+    let root = make_node_addr(0);
+
+    state.update_peer(
+        ParentDeclaration::new(peer_a, root, 1, 1000),
+        make_coords(&[1, 0]),
+    );
+    state.set_parent(peer_a, 1, 1000);
+    state.recompute_coords();
+
+    state.remove_peer(&peer_a);
+    let outcome = state.recover(&HashMap::new());
+
+    assert!(outcome.changed);
+    assert!(state.is_root());
+    assert!(!outcome.dampened, "self-root recovery arms no episode");
+}
+
+#[test]
+fn test_handle_parent_lost_keeps_its_published_bool_return() {
+    // `tree` is a public module of a published crate, so the return type of
+    // this entry point is part of the API. Binding the method to an explicitly
+    // typed function pointer is the assertion: any other return type fails to
+    // compile. Calling through the pointer keeps the binding live.
+    let published: fn(&mut TreeState, &HashMap<NodeAddr, f64>) -> bool =
+        TreeState::handle_parent_lost;
+
+    let mut state = TreeState::new(make_node_addr(5));
+    let peer = make_node_addr(1);
+    state.update_peer(
+        ParentDeclaration::new(peer, make_node_addr(0), 1, 1000),
+        make_coords(&[1, 0]),
+    );
+    state.set_parent(peer, 1, 1000);
+    state.recompute_coords();
+
+    state.remove_peer(&peer);
+    assert!(published(&mut state, &HashMap::new()));
+    assert!(state.is_root());
+}