mirror of
https://github.com/jmcorgan/fips.git
synced 2026-10-05 11:08:25 +00:00
fix(control): report a peer silent past the heartbeat interval as stale
show_peers, and the peer row the tick publishes for the control socket to serve, printed a peer's connectivity from a state stored on the active peer. That state starts at connected on promotion and nothing outside the tests ever changes it, so every peer read connected for as long as it stayed in the peer map, including one that had stopped answering and was waiting out its link-dead timeout. Both render sites now derive the value from how long the peer has been silent: connected while its idle time is at or below heartbeat_interval_secs, floored at one second, and stale above it. That is the rule discovery already applies when deciding whether to re-dial an active peer on the path it already has, so the rule moves to one helper on Node that the re-dial gate and both renders call. stale was already one of the field's values, so the set of values a client can see does not grow, and the response shape is unchanged. The stored state and its other readers are left as they are. The open-discovery tutorial described reconnecting and disconnected values that never occur, and the advertise-your-node tutorial said a new peer would read active, which the field never reports. Both now describe what the field reports. The new tests insert peers last heard from at chosen times and read show_peers both on the loop and from the tick-published snapshot. Under the default 10 s interval a peer silent for 15 s reads stale and one heard from just now reads connected; with a 30 s interval a peer silent for 15 s still reads connected, so the threshold is the configured interval and not a fixed ten seconds. Both failed on the unfixed code, which reported the silent peer as connected. A third test pins the boundary: connected at exactly the interval, stale one millisecond past it, and a zero interval floored at one second. Restoring the stored read at either render site alone fails both show_peers tests on that render, making the comparison inclusive fails the boundary test, and a fixed ten-second threshold fails the configured-interval and boundary tests.
This commit is contained in:
@@ -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
|
link was created with. The counters cover authenticated link frames only, so
|
||||||
they are not expected to match the transport totals in `show_transports`.
|
they are not expected to match the transport totals in `show_transports`.
|
||||||
The response shape is unchanged.
|
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
|
### Changed
|
||||||
|
|
||||||
|
|||||||
@@ -284,7 +284,7 @@ sudo fipsctl show peers
|
|||||||
|
|
||||||
In addition to your configured `test-us01` peer, you may see
|
In addition to your configured `test-us01` peer, you may see
|
||||||
an entry for `test-us03` (the open-discovery test mesh node).
|
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
|
`transport_addr`. This peering appeared without you
|
||||||
configuring anything — the test-mesh open-discovery node saw
|
configuring anything — the test-mesh open-discovery node saw
|
||||||
your advert, dialed the endpoint, and Noise IK established
|
your advert, dialed the endpoint, and Noise IK established
|
||||||
|
|||||||
@@ -200,11 +200,14 @@ You should see considerably more entries than before:
|
|||||||
Each entry has its own `connectivity` state, and every entry
|
Each entry has its own `connectivity` state, and every entry
|
||||||
that appears here completed a handshake at least once: a peer
|
that appears here completed a handshake at least once: a peer
|
||||||
whose advert was stale, or that NAT traversal never reached,
|
whose advert was stale, or that NAT traversal never reached,
|
||||||
produces no entry at all rather than a failed one. Healthy links
|
produces no entry at all rather than a failed one. A link heard
|
||||||
read `connected`. A link not heard from recently reads `stale`
|
from within the last heartbeat interval
|
||||||
and still carries traffic; one that dropped and is being retried
|
(`node.heartbeat_interval_secs`, 10 seconds by default) reads
|
||||||
reads `reconnecting`, and one explicitly torn down reads
|
`connected`. One silent for longer reads `stale`; it still
|
||||||
`disconnected`. Neither of the last two can send.
|
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:
|
To get a list of just the connected links:
|
||||||
|
|
||||||
|
|||||||
@@ -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.
|
// 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 any_peer_has_srtt = node.peers().any(|p| p.has_srtt());
|
||||||
|
|
||||||
|
let now = now_ms();
|
||||||
let peers: Vec<Value> = node
|
let peers: Vec<Value> = node
|
||||||
.peers()
|
.peers()
|
||||||
.map(|peer| {
|
.map(|peer| {
|
||||||
@@ -267,7 +268,7 @@ pub fn show_peers(node: &Node) -> Value {
|
|||||||
"npub": peer.npub(),
|
"npub": peer.npub(),
|
||||||
"display_name": node.peer_display_name(&node_addr),
|
"display_name": node.peer_display_name(&node_addr),
|
||||||
"ipv6_addr": format!("{}", peer.address()),
|
"ipv6_addr": format!("{}", peer.address()),
|
||||||
"connectivity": format!("{}", peer.connectivity()),
|
"connectivity": format!("{}", node.peer_connectivity(peer, now)),
|
||||||
"link_id": peer.link_id().as_u64(),
|
"link_id": peer.link_id().as_u64(),
|
||||||
"authenticated_at_ms": peer.authenticated_at(),
|
"authenticated_at_ms": peer.authenticated_at(),
|
||||||
"last_seen_ms": peer.last_seen(),
|
"last_seen_ms": peer.last_seen(),
|
||||||
|
|||||||
@@ -3171,13 +3171,7 @@ impl Node {
|
|||||||
let Some(peer) = self.peers.get(peer_node_addr) else {
|
let Some(peer) = self.peers.get(peer_node_addr) else {
|
||||||
return false;
|
return false;
|
||||||
};
|
};
|
||||||
let stale_after_ms = self
|
self.peer_link_is_stale(peer, Self::now_ms())
|
||||||
.config()
|
|
||||||
.node
|
|
||||||
.heartbeat_interval_secs
|
|
||||||
.saturating_mul(1000)
|
|
||||||
.max(1000);
|
|
||||||
peer.idle_time(Self::now_ms()) > stale_after_ms
|
|
||||||
}
|
}
|
||||||
|
|
||||||
fn active_peer_matches_any_candidate(
|
fn active_peer_matches_any_candidate(
|
||||||
|
|||||||
+32
-2
@@ -43,8 +43,8 @@ use self::reloadable::Reloadable;
|
|||||||
pub(crate) const REKEY_JITTER_SECS: i64 = 15;
|
pub(crate) const REKEY_JITTER_SECS: i64 = 15;
|
||||||
use crate::cache::CoordCache;
|
use crate::cache::CoordCache;
|
||||||
use crate::node::session::SessionEntry;
|
use crate::node::session::SessionEntry;
|
||||||
use crate::peer::ActivePeer;
|
|
||||||
use crate::peer::machine::{PeerMachine, TimerKind};
|
use crate::peer::machine::{PeerMachine, TimerKind};
|
||||||
|
use crate::peer::{ActivePeer, ConnectivityState};
|
||||||
use crate::proto::bloom::{BloomFilter, BloomState};
|
use crate::proto::bloom::{BloomFilter, BloomState};
|
||||||
use crate::proto::fmp::Fmp;
|
use crate::proto::fmp::Fmp;
|
||||||
use crate::proto::fmp::wire::{
|
use crate::proto::fmp::wire::{
|
||||||
@@ -2141,6 +2141,7 @@ impl Node {
|
|||||||
// SRTT) every peer falls back to the default link cost of 1.0.
|
// 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 any_peer_has_srtt = self.peers().any(|p| p.has_srtt());
|
||||||
|
|
||||||
|
let now_ms = Self::now_ms();
|
||||||
let peer_rows: Vec<snap::PeerRow> = self
|
let peer_rows: Vec<snap::PeerRow> = self
|
||||||
.peers()
|
.peers()
|
||||||
.map(|peer| {
|
.map(|peer| {
|
||||||
@@ -2193,7 +2194,7 @@ impl Node {
|
|||||||
npub: peer.npub(),
|
npub: peer.npub(),
|
||||||
display_name: self.peer_display_name(&node_addr),
|
display_name: self.peer_display_name(&node_addr),
|
||||||
ipv6_addr: format!("{}", peer.address()),
|
ipv6_addr: format!("{}", peer.address()),
|
||||||
connectivity: format!("{}", peer.connectivity()),
|
connectivity: format!("{}", self.peer_connectivity(peer, now_ms)),
|
||||||
link_id: peer.link_id().as_u64(),
|
link_id: peer.link_id().as_u64(),
|
||||||
authenticated_at_ms: peer.authenticated_at(),
|
authenticated_at_ms: peer.authenticated_at(),
|
||||||
last_seen_ms: peer.last_seen(),
|
last_seen_ms: peer.last_seen(),
|
||||||
@@ -2841,6 +2842,35 @@ impl Node {
|
|||||||
self.peers.values()
|
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.
|
/// Reference to the Nostr discovery handle if discovery is enabled.
|
||||||
/// Used by control queries (`show_peers` per-peer Nostr-traversal
|
/// Used by control queries (`show_peers` per-peer Nostr-traversal
|
||||||
/// state) to read failure-state without taking shared ownership.
|
/// state) to read failure-state without taking shared ownership.
|
||||||
|
|||||||
@@ -6,6 +6,7 @@
|
|||||||
//! socket framing.
|
//! socket framing.
|
||||||
|
|
||||||
use super::*;
|
use super::*;
|
||||||
|
use heartbeat::set_heartbeat_interval;
|
||||||
use spanning_tree::{
|
use spanning_tree::{
|
||||||
TestNode, add_loopback_alias, cleanup_nodes, drain_all_packets, make_test_node,
|
TestNode, add_loopback_alias, cleanup_nodes, drain_all_packets, make_test_node,
|
||||||
process_available_packets, run_tree_test,
|
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;
|
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);
|
||||||
|
}
|
||||||
|
|||||||
@@ -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 `heartbeat_interval_secs` on an already-constructed node, the same way
|
||||||
/// `set_link_dead_timeout` does.
|
/// `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| {
|
node.replace_context(|ctx| {
|
||||||
let mut cfg = (*ctx.config).clone();
|
let mut cfg = (*ctx.config).clone();
|
||||||
cfg.node.heartbeat_interval_secs = secs;
|
cfg.node.heartbeat_interval_secs = secs;
|
||||||
|
|||||||
Reference in New Issue
Block a user