diff --git a/docs/adr/ADR-018-runtime-rebootstrap-recovery.md b/docs/adr/ADR-018-runtime-rebootstrap-recovery.md new file mode 100644 index 0000000..13c6272 --- /dev/null +++ b/docs/adr/ADR-018-runtime-rebootstrap-recovery.md @@ -0,0 +1,88 @@ +# ADR-018: Runtime Re-bootstrap Must Be Able to Recover + +## Status + +Proposed + +## Context + +`maybe_rebootstrap` is the routing table's runtime repair mechanism: when the +table falls below `AUTO_REBOOTSTRAP_THRESHOLD` (3), it re-seeds via FIND_NODE +against currently connected peers, rate-limited by a five-minute cooldown. Two +of its paths were dead ends — states the repair fired from but could never +leave: + +1. **Full isolation.** With zero connections there is nothing to gossip from, + and the repair returned early. Nothing at runtime ever re-dialed the + *configured* bootstrap peers — only process startup does that — so a node + that lost its last connection could only recover by being restarted. +2. **Client-mode starvation.** `bootstrap_from_peers` skipped every + gossiped-peer dial in client mode (clients dial on demand). But + routing-table admission is connection-driven (`handle_peer_connected`), so + a starved client rediscovered the same peers every cycle, dialed none of + them, and stayed starved. The repair could not, by construction, affect + its own exit condition. + +Both dead ends are observed, not theoretical. A mainnet daemon +(WithAutonomi/ant-sdk#232) sat at routing table size 0 for ~34 hours while +auto-re-bootstrap ran continuously; a restart recovered it immediately. In a +controlled reproduction, a client held 10 identity-verified connections while +its table sat pinned below threshold for 10+ hours across 98 repair cycles — +each one logging `Auto re-bootstrap discovered 10 peers`, because the success +metric counted gossip seen rather than admissions gained. + +## Decision + +Make the repair able to reach its own exit condition in both states, without +changing what routing-table membership means. + +1. **Zero-connections fallback.** When no peers are connected, + `maybe_rebootstrap` re-dials the configured bootstrap peers — the same + seeds initial startup uses — and proceeds with whatever connects. Only if + all of them are unreachable does it give up until the next cooldown. +2. **Starved clients dial.** In client mode, `bootstrap_from_peers` dials + gossiped peers while the routing table is below the threshold, stopping as + soon as it clears. Non-starved clients keep the existing skip — the + ADR-017 §8 rationale (clients don't serve the DHT) is untouched; this only + restores the client's ability to answer its *own* lookups and report its + own health. Admission runs asynchronously on the peer-connected event, so + the size check can lag a dial and over-dial slightly; that is harmless and + bounded by the gossip set. +3. **Honest observability.** The completion log now reports the routing-table + size after repair alongside the discovered count, and + `AUTO_REBOOTSTRAP_THRESHOLD` is public so consumers reporting network + health (e.g. a daemon `/health` showing "0 of 3") use the same floor the + DHT repairs toward. +4. **`maybe_rebootstrap` is public.** Daemons gain a manual recovery hook + (e.g. behind an admin endpoint). The threshold and cooldown checks make an + extra call cheap and safe. + +## Alternatives considered + +- **Admit connected, identity-verified peers into the routing table + directly.** Rejected as broader than the defect: admission policy + (user-agent gating, IP-diversity, trust-aware swap) is deliberate, and + changing when peers *qualify* is a different decision from making the + repair able to dial at all. +- **Insert gossiped peers without dialing.** Rejected: gossip must not grant + routing-table membership without identity verification through the dial + path — the same principle as ADR-017 §5. +- **Leave recovery to a supervisor restart.** Rejected: it treats a + recoverable transport state as fatal, and field experience shows operators + discover the state only after user-visible write failures. +- **Futility detection for structurally capped tables.** On single-host + devnets, same-IP diversity caps can pin the table below the threshold + permanently, so the repair fires every cooldown forever. Deferred: the + cooldown already bounds the cost, and detecting "cannot possibly reach + threshold" needs admission-policy introspection that doesn't exist yet. + +## Consequences + +- Fully isolated nodes and starved clients now recover at runtime; the + reporter's restart-only failure mode is closed. +- Dial rate increases only for nodes already below the threshold, bounded by + the gossip set size and the five-minute cooldown. +- New public API: `AUTO_REBOOTSTRAP_THRESHOLD` and + `DhtNetworkManager::maybe_rebootstrap` (semver: feature). +- `tests/client_rebootstrap.rs` holds regression tests for both dead ends, + verified to fail against the previous behavior. diff --git a/src/dht_network_manager.rs b/src/dht_network_manager.rs index 6b8692b..3066de4 100644 --- a/src/dht_network_manager.rs +++ b/src/dht_network_manager.rs @@ -206,7 +206,11 @@ const MAX_CONCURRENT_BUCKET_REFRESH_LOOKUPS: usize = 1; const BUCKET_REFRESH_SELECTION_JITTER: Duration = Duration::from_secs(60); /// Routing table size below which automatic re-bootstrap is triggered. -const AUTO_REBOOTSTRAP_THRESHOLD: usize = 3; +/// +/// Public so consumers reporting network health (e.g. daemon `/health` +/// endpoints) can show the observed routing-table size against the same +/// floor the DHT itself repairs toward. +pub const AUTO_REBOOTSTRAP_THRESHOLD: usize = 3; /// Maximum time to wait for a background task to stop during shutdown before /// aborting it. Defense in depth against tasks that fail to respond to the @@ -2156,7 +2160,13 @@ impl DhtNetworkManager { /// deliberately not guarded by `bucket_refresh_lookup_semaphore`: once the /// routing table is below the recovery threshold, bootstrap repair should /// not be skipped just because a best-effort bucket refresh is running. - async fn maybe_rebootstrap(&self) { + /// Trigger a re-bootstrap if the routing table has fallen below + /// [`AUTO_REBOOTSTRAP_THRESHOLD`] and the cooldown has elapsed. + /// + /// Runs automatically from the maintenance driver; public so consumers + /// (e.g. daemons with a recovery endpoint) can also request an attempt — + /// the threshold and cooldown checks make an extra call cheap and safe. + pub async fn maybe_rebootstrap(&self) { let rt_size = self.get_routing_table_size().await; if rt_size >= AUTO_REBOOTSTRAP_THRESHOLD { return; @@ -2182,16 +2192,48 @@ impl DhtNetworkManager { AUTO_REBOOTSTRAP_THRESHOLD ); - // Collect currently connected peers to use as bootstrap seeds. - let connected = self.transport.connected_peers().await; + // Collect currently connected peers to use as bootstrap seeds. With + // no connections left there is nothing to gossip from, so fall back + // to re-dialing the configured bootstrap peers — the same seeds + // initial startup uses. Without this fallback a fully isolated node + // could never recover at runtime (only a process restart re-dials + // the configured peers), while its logs claimed re-bootstrap was + // running. + let mut connected = self.transport.connected_peers().await; if connected.is_empty() { - debug!("Auto re-bootstrap: no connected peers to bootstrap from"); - return; + let configured = &self.config.node_config.bootstrap_peers; + if configured.is_empty() { + debug!( + "Auto re-bootstrap: no connected peers and no configured bootstrap peers to fall back to" + ); + return; + } + info!( + "Auto re-bootstrap: no connected peers — re-dialing {} configured bootstrap peer(s)", + configured.len() + ); + for addr in configured { + match self.transport.connect_peer(addr).await { + Ok(_) => debug!("Auto re-bootstrap: reconnected to {addr}"), + Err(e) => debug!("Auto re-bootstrap: dial of {addr} failed: {e}"), + } + } + connected = self.transport.connected_peers().await; + if connected.is_empty() { + warn!( + "Auto re-bootstrap: all {} configured bootstrap peer(s) unreachable", + configured.len() + ); + return; + } } match self.bootstrap_from_peers(&connected).await { Ok(discovered) => { - info!("Auto re-bootstrap discovered {discovered} peers"); + let rt_after = self.get_routing_table_size().await; + info!( + "Auto re-bootstrap discovered {discovered} peers, routing table size now {rt_after}" + ); } Err(e) => { warn!("Auto re-bootstrap failed: {e}"); @@ -2310,11 +2352,36 @@ impl DhtNetworkManager { // demand when the client needs to reach one, which is enough for its // own requests — matching the rationale for skipping post-bootstrap // self-lookups in `P2PNode::start()`. + // + // EXCEPT when the routing table is starved: admission is + // connection-driven (`handle_peer_connected`), so a client below + // `AUTO_REBOOTSTRAP_THRESHOLD` that dials nothing can never repair + // its own table — `maybe_rebootstrap` would rediscover the same + // gossiped peers and skip them again, forever. Dial just enough of + // them to lift the table over the threshold. Admission runs + // asynchronously on the peer-connected event, so the size check may + // lag a dial by one iteration and over-dial slightly; that is + // harmless (the connections are usable) and bounded by `to_dial`. if matches!(self.config.node_config.mode, NodeMode::Client) { - debug!( - "DHT bootstrap: client mode — skipping {} gossiped-peer dial(s)", - to_dial.len() - ); + if self.get_routing_table_size().await >= AUTO_REBOOTSTRAP_THRESHOLD { + debug!( + "DHT bootstrap: client mode — skipping {} gossiped-peer dial(s)", + to_dial.len() + ); + } else { + let candidates = to_dial.len(); + let mut dialed = 0usize; + for (peer_id, typed) in to_dial { + if self.get_routing_table_size().await >= AUTO_REBOOTSTRAP_THRESHOLD { + break; + } + self.dial_addresses(&peer_id, &typed).await; + dialed += 1; + } + info!( + "DHT bootstrap: client mode with starved routing table — dialed {dialed} of {candidates} gossiped peer(s)" + ); + } } else { for (peer_id, typed) in to_dial { self.dial_addresses(&peer_id, &typed).await; diff --git a/tests/client_rebootstrap.rs b/tests/client_rebootstrap.rs new file mode 100644 index 0000000..9a7e1c9 --- /dev/null +++ b/tests/client_rebootstrap.rs @@ -0,0 +1,175 @@ +// Copyright 2024 Saorsa Labs Limited +// +// This software is licensed under the MIT license or the Apache License, Version 2.0 +// , at your +// option. This file may not be copied, modified, or distributed except +// according to those terms. +// +// Unless required by applicable law or agreed to in writing, software +// distributed under these licenses is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + +//! Regression tests for client-mode routing-table repair (V2-1036). +//! +//! Two failure modes are covered, both previously unrecoverable at runtime +//! (only a process restart re-dialed the configured bootstrap peers): +//! +//! 1. `bootstrap_from_peers` skipped ALL gossiped-peer dials in client mode. +//! Routing-table admission is connection-driven (`handle_peer_connected`), +//! so a client with a starved table rediscovered the same peers every +//! cycle, dialed none of them, and stayed starved forever. +//! 2. `maybe_rebootstrap` seeded only from currently connected peers and gave +//! up when there were none, never falling back to the configured +//! bootstrap peers that initial startup uses. + +#![allow(clippy::unwrap_used, clippy::expect_used)] + +use saorsa_core::{MultiAddr, NodeConfig, NodeMode, P2PNode}; +use std::time::Duration; +use tokio::time::timeout; + +fn node_config() -> NodeConfig { + NodeConfig::builder() + .local(true) + .port(0) + .ipv6(false) + .build() + .expect("node config should be valid") +} + +fn client_config(bootstrap: Option) -> NodeConfig { + let mut builder = NodeConfig::builder() + .local(true) + .port(0) + .ipv6(false) + .mode(NodeMode::Client); + if let Some(addr) = bootstrap { + builder = builder.bootstrap_peer(addr); + } + builder.build().expect("client config should be valid") +} + +async fn started_node() -> P2PNode { + let node = P2PNode::new(node_config()).await.unwrap(); + node.start().await.unwrap(); + node +} + +fn ipv4_listen_addr(addrs: Vec) -> MultiAddr { + addrs + .into_iter() + .find(|a| a.is_ipv4()) + .expect("node should have an IPv4 listen address") +} + +/// Poll the routing table until it reaches `want` entries or `wait` elapses. +/// Returns the final observed size either way. +async fn wait_for_routing_table(node: &P2PNode, want: usize, wait: Duration) -> usize { + let deadline = tokio::time::Instant::now() + wait; + loop { + let size = node.dht_manager().get_routing_table_size().await; + if size >= want || tokio::time::Instant::now() >= deadline { + return size; + } + tokio::time::sleep(Duration::from_millis(200)).await; + } +} + +/// A client whose routing table is below the auto-re-bootstrap threshold must +/// dial gossiped peers during `bootstrap_from_peers` so admission (which is +/// connection-driven) can actually repair the table. Before the fix the +/// client skipped every dial and the table stayed at 1 forever. +#[tokio::test] +async fn starved_client_dials_gossiped_peers_to_repair_routing_table() { + // Hub-and-spoke mesh: c and d dial hub b, so b's routing table knows + // both and FIND_NODE against b gossips them. + let node_b = started_node().await; + let node_c = started_node().await; + let node_d = started_node().await; + tokio::time::sleep(Duration::from_millis(50)).await; + + let b_addr = ipv4_listen_addr(node_b.listen_addrs().await); + for spoke in [&node_c, &node_d] { + timeout(Duration::from_secs(5), spoke.connect_peer(&b_addr)) + .await + .expect("spoke connect should not time out") + .expect("spoke connect should succeed"); + } + let hub_table = wait_for_routing_table(&node_b, 2, Duration::from_secs(10)).await; + assert!( + hub_table >= 2, + "hub should admit both spokes, got {hub_table}" + ); + + // Client connects to the hub only: routing table = 1, below threshold. + let client = P2PNode::new(client_config(None)).await.unwrap(); + client.start().await.unwrap(); + timeout(Duration::from_secs(5), client.connect_peer(&b_addr)) + .await + .expect("client connect should not time out") + .expect("client connect should succeed"); + let before = wait_for_routing_table(&client, 1, Duration::from_secs(10)).await; + assert!(before >= 1, "client should admit the hub, got {before}"); + + // Drive the repair path directly (the maintenance driver would do the + // same on its next cycle). + let seeds: Vec<_> = client.connected_peers().await; + assert!(!seeds.is_empty(), "client should be connected to the hub"); + client + .dht_manager() + .bootstrap_from_peers(&seeds) + .await + .expect("bootstrap_from_peers should succeed"); + + // Admission runs on the async peer-connected path; poll briefly. + let after = wait_for_routing_table(&client, before + 1, Duration::from_secs(10)).await; + assert!( + after > before, + "client routing table should grow after repair dials (before={before}, after={after})" + ); +} + +/// A client that has lost every connection must fall back to re-dialing its +/// configured bootstrap peers during `maybe_rebootstrap`. Before the fix it +/// returned early on "no connected peers" and could only recover via a +/// process restart. +#[tokio::test] +async fn isolated_client_rebootstraps_from_configured_peers() { + let node_b = started_node().await; + tokio::time::sleep(Duration::from_millis(50)).await; + let b_addr = ipv4_listen_addr(node_b.listen_addrs().await); + + let client = P2PNode::new(client_config(Some(b_addr))).await.unwrap(); + client.start().await.unwrap(); + let before = wait_for_routing_table(&client, 1, Duration::from_secs(10)).await; + assert!( + before >= 1, + "client should bootstrap to the node, got {before}" + ); + + // Sever every connection — the state the reporter's daemon was stuck in. + for peer in client.connected_peers().await { + client.disconnect_peer(&peer).await.ok(); + } + let deadline = tokio::time::Instant::now() + Duration::from_secs(5); + while !client.connected_peers().await.is_empty() && tokio::time::Instant::now() < deadline { + tokio::time::sleep(Duration::from_millis(100)).await; + } + assert!( + client.connected_peers().await.is_empty(), + "disconnect should leave the client with no connections" + ); + + // The repair must reconnect using the configured bootstrap peers. + client.dht_manager().maybe_rebootstrap().await; + + let deadline = tokio::time::Instant::now() + Duration::from_secs(10); + while client.connected_peers().await.is_empty() && tokio::time::Instant::now() < deadline { + tokio::time::sleep(Duration::from_millis(200)).await; + } + assert!( + !client.connected_peers().await.is_empty(), + "maybe_rebootstrap should have re-dialed the configured bootstrap peer" + ); +}