diff --git a/CHANGELOG.md b/CHANGELOG.md index ede84835..3c75f428 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 link was created with. The counters cover authenticated link frames only, so they are not expected to match the transport totals in `show_transports`. The response shape is unchanged. +- `show_peers` (`fipsctl show peers`) now reports a peer that has gone quiet + as `stale`. Its `connectivity` was read from a state that nothing outside + the tests ever changed, so every peer read `connected` until it was + removed, including one that had stopped answering tens of seconds earlier. + The value is now derived from how long the peer has been silent: `connected` + while its idle time is at or below `heartbeat_interval_secs`, and `stale` + above it, the same rule that decides whether discovery re-dials an active + peer on the path it already has. The `reconnecting` and `disconnected` + values the open-discovery tutorial described never occurred, and the + tutorial no longer lists them. The response shape is unchanged. ### Changed diff --git a/docs/tutorials/advertise-your-node.md b/docs/tutorials/advertise-your-node.md index b22b4154..7f2ffeb2 100644 --- a/docs/tutorials/advertise-your-node.md +++ b/docs/tutorials/advertise-your-node.md @@ -284,7 +284,7 @@ sudo fipsctl show peers In addition to your configured `test-us01` peer, you may see an entry for `test-us03` (the open-discovery test mesh node). -It will have `connectivity` active and its own +It will have `connectivity` `connected` and its own `transport_addr`. This peering appeared without you configuring anything — the test-mesh open-discovery node saw your advert, dialed the endpoint, and Noise IK established diff --git a/docs/tutorials/open-discovery.md b/docs/tutorials/open-discovery.md index 815897f8..0f13af04 100644 --- a/docs/tutorials/open-discovery.md +++ b/docs/tutorials/open-discovery.md @@ -200,11 +200,14 @@ You should see considerably more entries than before: Each entry has its own `connectivity` state, and every entry that appears here completed a handshake at least once: a peer whose advert was stale, or that NAT traversal never reached, -produces no entry at all rather than a failed one. Healthy links -read `connected`. A link not heard from recently reads `stale` -and still carries traffic; one that dropped and is being retried -reads `reconnecting`, and one explicitly torn down reads -`disconnected`. Neither of the last two can send. +produces no entry at all rather than a failed one. A link heard +from within the last heartbeat interval +(`node.heartbeat_interval_secs`, 10 seconds by default) reads +`connected`. One silent for longer reads `stale`; it still +carries traffic, and it reads `connected` again as soon as the +peer is heard from. A link that stays silent until it is declared +dead is removed, so its entry disappears rather than changing +state. To get a list of just the connected links: diff --git a/src/control/queries.rs b/src/control/queries.rs index 5768e480..c1e03b73 100644 --- a/src/control/queries.rs +++ b/src/control/queries.rs @@ -250,6 +250,7 @@ pub fn show_peers(node: &Node) -> Value { // start (no peer has SRTT) every peer uses the default link cost of 1.0. let any_peer_has_srtt = node.peers().any(|p| p.has_srtt()); + let now = now_ms(); let peers: Vec = node .peers() .map(|peer| { @@ -267,7 +268,7 @@ pub fn show_peers(node: &Node) -> Value { "npub": peer.npub(), "display_name": node.peer_display_name(&node_addr), "ipv6_addr": format!("{}", peer.address()), - "connectivity": format!("{}", peer.connectivity()), + "connectivity": format!("{}", node.peer_connectivity(peer, now)), "link_id": peer.link_id().as_u64(), "authenticated_at_ms": peer.authenticated_at(), "last_seen_ms": peer.last_seen(), diff --git a/src/node/lifecycle/mod.rs b/src/node/lifecycle/mod.rs index 1e40b272..f30cb3d8 100644 --- a/src/node/lifecycle/mod.rs +++ b/src/node/lifecycle/mod.rs @@ -3171,13 +3171,7 @@ impl Node { let Some(peer) = self.peers.get(peer_node_addr) else { return false; }; - let stale_after_ms = self - .config() - .node - .heartbeat_interval_secs - .saturating_mul(1000) - .max(1000); - peer.idle_time(Self::now_ms()) > stale_after_ms + self.peer_link_is_stale(peer, Self::now_ms()) } fn active_peer_matches_any_candidate( diff --git a/src/node/mod.rs b/src/node/mod.rs index ca4f2877..aff6a43a 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -43,8 +43,8 @@ use self::reloadable::Reloadable; pub(crate) const REKEY_JITTER_SECS: i64 = 15; use crate::cache::CoordCache; use crate::node::session::SessionEntry; -use crate::peer::ActivePeer; use crate::peer::machine::{PeerMachine, TimerKind}; +use crate::peer::{ActivePeer, ConnectivityState}; use crate::proto::bloom::{BloomFilter, BloomState}; use crate::proto::fmp::Fmp; use crate::proto::fmp::wire::{ @@ -2141,6 +2141,7 @@ impl Node { // SRTT) every peer falls back to the default link cost of 1.0. let any_peer_has_srtt = self.peers().any(|p| p.has_srtt()); + let now_ms = Self::now_ms(); let peer_rows: Vec = self .peers() .map(|peer| { @@ -2193,7 +2194,7 @@ impl Node { npub: peer.npub(), display_name: self.peer_display_name(&node_addr), ipv6_addr: format!("{}", peer.address()), - connectivity: format!("{}", peer.connectivity()), + connectivity: format!("{}", self.peer_connectivity(peer, now_ms)), link_id: peer.link_id().as_u64(), authenticated_at_ms: peer.authenticated_at(), last_seen_ms: peer.last_seen(), @@ -2841,6 +2842,35 @@ impl Node { self.peers.values() } + /// Whether an active peer has been silent at `now_ms` for longer than the + /// configured heartbeat interval, floored at one second. + /// + /// The one idle-time liveness rule: the control socket reports such a peer + /// as `stale`, and discovery re-dials it on the path it already has. + pub(in crate::node) fn peer_link_is_stale(&self, peer: &ActivePeer, now_ms: u64) -> bool { + let stale_after_ms = self + .config() + .node + .heartbeat_interval_secs + .saturating_mul(1000) + .max(1000); + peer.idle_time(now_ms) > stale_after_ms + } + + /// Connectivity of an active peer as the control socket reports it: + /// `Stale` when [`Self::peer_link_is_stale`] holds at `now_ms`, otherwise + /// `Connected`. + /// + /// Derived from idle time rather than read from the state stored on the + /// peer, which nothing in production changes after promotion. + pub(crate) fn peer_connectivity(&self, peer: &ActivePeer, now_ms: u64) -> ConnectivityState { + if self.peer_link_is_stale(peer, now_ms) { + ConnectivityState::Stale + } else { + ConnectivityState::Connected + } + } + /// Reference to the Nostr discovery handle if discovery is enabled. /// Used by control queries (`show_peers` per-peer Nostr-traversal /// state) to read failure-state without taking shared ownership. diff --git a/src/node/tests/control.rs b/src/node/tests/control.rs index 4bdf9fb8..dbb08b99 100644 --- a/src/node/tests/control.rs +++ b/src/node/tests/control.rs @@ -6,6 +6,7 @@ //! socket framing. use super::*; +use heartbeat::set_heartbeat_interval; use spanning_tree::{ TestNode, add_loopback_alias, cleanup_nodes, drain_all_packets, make_test_node, process_available_packets, run_tree_test, @@ -421,3 +422,110 @@ async fn show_links_reports_the_traffic_counters_of_the_peer_bound_to_each_link( cleanup_nodes(&mut nodes).await; } + +/// Insert an authenticated peer last heard from at `last_seen_ms` and return +/// its address. +fn insert_peer_last_seen_at(node: &mut Node, link: u64, last_seen_ms: u64) -> NodeAddr { + let identity = PeerIdentity::from_pubkey_full(Identity::generate().pubkey_full()); + let addr = *identity.node_addr(); + node.peers.insert( + addr, + ActivePeer::new(identity, LinkId::new(link), last_seen_ms), + ); + addr +} + +/// The `connectivity` string a `show_peers` response gives the peer at `addr`. +fn connectivity_of(peers: &serde_json::Value, addr: &NodeAddr) -> String { + let addr_hex = hex::encode(addr.as_bytes()); + peers["peers"] + .as_array() + .expect("show_peers returns a peers array") + .iter() + .find(|row| row["node_addr"] == addr_hex.as_str()) + .and_then(|row| row["connectivity"].as_str()) + .expect("show_peers lists the peer with a connectivity string") + .to_string() +} + +/// Render `show_peers` on the loop, then publish a tick and render it again +/// from the snapshot the control socket serves. +fn show_peers_both_renders(node: &mut Node) -> [(&'static str, serde_json::Value); 2] { + let on_loop = crate::control::queries::show_peers(node); + node.record_stats_history(); + let off_loop = crate::control::queries::show_peers_from_handle(&node.control_read_handle()); + [("on-loop", on_loop), ("snapshot", off_loop)] +} + +/// `show_peers` reports a peer silent for longer than the heartbeat interval +/// as `stale`, and a peer heard from just now as `connected`, on both the +/// on-loop render and the tick-published snapshot render. +#[test] +fn show_peers_reports_a_peer_idle_past_the_heartbeat_interval_as_stale() { + let mut node = make_node(); + let interval_ms = node.config().node.heartbeat_interval_secs * 1000; + assert_eq!(interval_ms, 10_000, "the default heartbeat interval"); + let now = Node::now_ms(); + let fresh = insert_peer_last_seen_at(&mut node, 1, now); + let idle = insert_peer_last_seen_at(&mut node, 2, now - interval_ms - 5_000); + + for (render, peers) in show_peers_both_renders(&mut node) { + assert_eq!( + connectivity_of(&peers, &idle), + "stale", + "{render} render, peer silent for 15 s" + ); + assert_eq!( + connectivity_of(&peers, &fresh), + "connected", + "{render} render, peer heard from just now" + ); + } +} + +/// The `stale` threshold is the configured heartbeat interval rather than a +/// fixed ten seconds: with a 30 s interval a peer silent for 15 s still reads +/// `connected`, and one silent for 35 s reads `stale`. +#[test] +fn show_peers_stale_threshold_follows_the_configured_heartbeat_interval() { + let mut node = make_node(); + set_heartbeat_interval(&mut node, 30); + let now = Node::now_ms(); + let quiet = insert_peer_last_seen_at(&mut node, 1, now - 15_000); + let idle = insert_peer_last_seen_at(&mut node, 2, now - 35_000); + + for (render, peers) in show_peers_both_renders(&mut node) { + assert_eq!( + connectivity_of(&peers, &idle), + "stale", + "{render} render, peer silent for 35 s" + ); + assert_eq!( + connectivity_of(&peers, &quiet), + "connected", + "{render} render, peer silent for 15 s" + ); + } +} + +/// The derived connectivity changes at the heartbeat interval exactly: a peer +/// silent for the whole interval still reads `connected`, and one millisecond +/// more reads `stale`. A zero interval is floored at one second, the floor the +/// discovery re-dial gate applies. +#[test] +fn peer_connectivity_turns_stale_one_millisecond_past_the_heartbeat_interval() { + let mut node = make_node(); + let seen = 1_000_000; + let addr = insert_peer_last_seen_at(&mut node, 1, seen); + let at = |node: &Node, now_ms: u64| { + let peer = node.get_peer(&addr).expect("the peer was inserted"); + node.peer_connectivity(peer, now_ms) + }; + + assert_eq!(at(&node, seen + 10_000), ConnectivityState::Connected); + assert_eq!(at(&node, seen + 10_001), ConnectivityState::Stale); + + set_heartbeat_interval(&mut node, 0); + assert_eq!(at(&node, seen + 1_000), ConnectivityState::Connected); + assert_eq!(at(&node, seen + 1_001), ConnectivityState::Stale); +} diff --git a/src/node/tests/heartbeat.rs b/src/node/tests/heartbeat.rs index 11e684f7..008d0968 100644 --- a/src/node/tests/heartbeat.rs +++ b/src/node/tests/heartbeat.rs @@ -37,7 +37,7 @@ fn set_link_dead_timeout(node: &mut crate::node::Node, secs: u64) { /// Set `heartbeat_interval_secs` on an already-constructed node, the same way /// `set_link_dead_timeout` does. -fn set_heartbeat_interval(node: &mut crate::node::Node, secs: u64) { +pub(super) fn set_heartbeat_interval(node: &mut crate::node::Node, secs: u64) { node.replace_context(|ctx| { let mut cfg = (*ctx.config).clone(); cfg.node.heartbeat_interval_secs = secs;