From fd30ab0994396e77ebcbea0815e2c9abfc0c8222 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Fri, 12 Jun 2026 23:32:56 +0000 Subject: [PATCH] node: notify the peer on manual disconnect so teardown is symmetric A manual disconnect tore down only the local side and sent the peer nothing, so the peer kept its session and never re-emitted its tree and filter announcements; on reconnect it was never re-adopted as a child and its bloom filter was never recorded. Send the disconnected peer a scoped Disconnect, the same message graceful shutdown sends to all peers, so both sides tear down and re-handshake cleanly on the next connection. --- src/control/commands.rs | 6 ++-- src/node/lifecycle.rs | 56 +++++++++++++++++++++++----------- src/node/tests/disconnect.rs | 58 ++++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 20 deletions(-) diff --git a/src/control/commands.rs b/src/control/commands.rs index a0c08ff..69b519d 100644 --- a/src/control/commands.rs +++ b/src/control/commands.rs @@ -12,7 +12,7 @@ use tracing::debug; pub async fn dispatch(node: &mut Node, command: &str, params: Option<&Value>) -> Response { match command { "connect" => connect(node, params).await, - "disconnect" => disconnect(node, params), + "disconnect" => disconnect(node, params).await, _ => Response::error(format!("unknown command: {command}")), } } @@ -49,7 +49,7 @@ async fn connect(node: &mut Node, params: Option<&Value>) -> Response { /// Disconnect a peer. /// /// Params: `{"npub": "npub1..."}` -fn disconnect(node: &mut Node, params: Option<&Value>) -> Response { +async fn disconnect(node: &mut Node, params: Option<&Value>) -> Response { let Some(params) = params else { return Response::error("missing params for disconnect"); }; @@ -61,7 +61,7 @@ fn disconnect(node: &mut Node, params: Option<&Value>) -> Response { debug!(npub = %npub, "API disconnect requested"); - match node.api_disconnect(npub) { + match node.api_disconnect(npub).await { Ok(data) => Response::ok(data), Err(msg) => Response::error(msg), } diff --git a/src/node/lifecycle.rs b/src/node/lifecycle.rs index bd767c6..4e99a6c 100644 --- a/src/node/lifecycle.rs +++ b/src/node/lifecycle.rs @@ -1100,9 +1100,6 @@ impl Node { /// Best-effort: send failures are logged and ignored since the transport /// may already be degraded. This runs before transports are shut down. async fn send_disconnect_to_all_peers(&mut self, reason: DisconnectReason) { - let disconnect = Disconnect::new(reason); - let plaintext = disconnect.encode(); - // Collect node_addrs to avoid borrow conflict with send helper let peer_addrs: Vec = self .peers @@ -1121,24 +1118,41 @@ impl Node { let mut sent = 0usize; for node_addr in &peer_addrs { - match self - .send_encrypted_link_message(node_addr, &plaintext) - .await - { - Ok(()) => sent += 1, - Err(e) => { - debug!( - peer = %self.peer_display_name(node_addr), - error = %e, - "Failed to send disconnect (transport may be down)" - ); - } + if self.send_disconnect_to_peer(node_addr, reason).await { + sent += 1; } } info!(sent, total = peer_addrs.len(), reason = %reason, "Sent disconnect notifications"); } + /// Send a Disconnect notification to a single peer. + /// + /// Best-effort: a send failure (peer already gone, transport down) is + /// logged and swallowed so callers can proceed with teardown regardless. + /// Returns `true` if the message was sent successfully. + async fn send_disconnect_to_peer( + &mut self, + node_addr: &NodeAddr, + reason: DisconnectReason, + ) -> bool { + let plaintext = Disconnect::new(reason).encode(); + match self + .send_encrypted_link_message(node_addr, &plaintext) + .await + { + Ok(()) => true, + Err(e) => { + debug!( + peer = %self.peer_display_name(node_addr), + error = %e, + "Failed to send disconnect (transport may be down)" + ); + false + } + } + } + fn static_peer_addresses(&self, peer_config: &PeerConfig) -> Vec { peer_config .addresses_by_priority() @@ -1885,8 +1899,8 @@ impl Node { /// Disconnect a peer via the control API. /// - /// Removes the peer and suppresses auto-reconnect. - pub(crate) fn api_disconnect(&mut self, npub: &str) -> Result { + /// Notifies the peer, removes it locally, and suppresses auto-reconnect. + pub(crate) async fn api_disconnect(&mut self, npub: &str) -> Result { let peer_identity = PeerIdentity::from_npub(npub).map_err(|e| format!("invalid npub '{npub}': {e}"))?; let node_addr = *peer_identity.node_addr(); @@ -1895,6 +1909,14 @@ impl Node { return Err(format!("peer not found: {npub}")); } + // Notify the peer before we tear down the link, so it drops its own + // session and re-handshakes symmetrically rather than holding a stale + // session that never re-emits its tree/filter announcements. The link + // must still exist for the send, so this runs before removal. + // Best-effort: a send failure must not block the local teardown. + self.send_disconnect_to_peer(&node_addr, DisconnectReason::ConfigurationChange) + .await; + // Remove the peer (full cleanup: sessions, indices, links, tree, bloom) self.remove_active_peer(&node_addr); diff --git a/src/node/tests/disconnect.rs b/src/node/tests/disconnect.rs index 03389da..5323e90 100644 --- a/src/node/tests/disconnect.rs +++ b/src/node/tests/disconnect.rs @@ -324,6 +324,64 @@ async fn test_disconnect_clears_session() { cleanup_nodes(&mut nodes).await; } +/// A manual (control-API) disconnect must notify the peer, not just tear +/// down the local side. +/// +/// Regression test: `api_disconnect` previously removed the peer locally but +/// sent it no Disconnect message. The peer kept its session and never +/// re-emitted its tree/filter announcements, so on reconnect it was never +/// re-adopted as a child and its bloom filter was never recorded. The fix +/// sends the disconnected peer a scoped Disconnect so both sides tear down +/// symmetrically. This test drives `api_disconnect` on node 0 and verifies +/// node 1 receives the notification and removes node 0. +#[tokio::test] +async fn test_api_disconnect_notifies_peer() { + // Two-node topology: 0 -- 1. + let edges = vec![(0, 1)]; + let mut nodes = run_tree_test(2, &edges, false).await; + verify_tree_convergence(&nodes); + + let node0_addr = *nodes[0].node.node_addr(); + let node1_addr = *nodes[1].node.node_addr(); + let node1_npub = nodes[1].node.npub(); + + // Both sides start with each other as a peer. + assert!( + nodes[0].node.get_peer(&node1_addr).is_some(), + "Node 0 should have node 1 before disconnect" + ); + assert!( + nodes[1].node.get_peer(&node0_addr).is_some(), + "Node 1 should have node 0 before disconnect" + ); + + // Operator disconnects node 1 via the control API on node 0. + nodes[0] + .node + .api_disconnect(&node1_npub) + .await + .expect("api_disconnect should succeed"); + + // Node 0 tore down its side immediately. + assert!( + nodes[0].node.get_peer(&node1_addr).is_none(), + "Node 0 should have removed node 1 after api_disconnect" + ); + + // The Disconnect notification must reach node 1. + tokio::time::sleep(Duration::from_millis(50)).await; + process_available_packets(&mut nodes).await; + + // Node 1 must have torn down its side in response — proving the + // notification was actually emitted and received. + assert!( + nodes[1].node.get_peer(&node0_addr).is_none(), + "Node 1 should have removed node 0 after receiving the disconnect notification" + ); + + cleanup_nodes(&mut nodes).await; +} + /// Verify that different disconnect reasons are handled correctly. /// /// Sends each reason code and verifies the peer is removed regardless.