mirror of
https://github.com/jmcorgan/fips.git
synced 2026-07-30 19:46:15 +00:00
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.
This commit is contained in:
@@ -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),
|
||||
}
|
||||
|
||||
+39
-17
@@ -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<NodeAddr> = 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<PeerAddress> {
|
||||
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<serde_json::Value, String> {
|
||||
/// Notifies the peer, removes it locally, and suppresses auto-reconnect.
|
||||
pub(crate) async fn api_disconnect(&mut self, npub: &str) -> Result<serde_json::Value, String> {
|
||||
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);
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user