fix(dht): let runtime re-bootstrap recover isolated and client-starved routing tables

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.

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;

tests/client_rebootstrap.rs

@@ -0,0 +1,175 @@
+// Copyright 2024 Saorsa Labs Limited
+//
+// This software is licensed under the MIT license <LICENSE-MIT or
+// https://opensource.org/licenses/MIT> or the Apache License, Version 2.0
+// <LICENSE-APACHE or https://www.apache.org/licenses/LICENSE-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<MultiAddr>) -> 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>) -> 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"
+    );
+}