Skip to content

refactor(libp2p): use precise connection cleanup in handshake error handling#5330

Merged
gacevicljubisa merged 6 commits into
masterfrom
refactor/precise-connection-cleanup
Jul 11, 2026
Merged

refactor(libp2p): use precise connection cleanup in handshake error handling#5330
gacevicljubisa merged 6 commits into
masterfrom
refactor/precise-connection-cleanup

Conversation

@gacevicljubisa

@gacevicljubisa gacevicljubisa commented Jan 24, 2026

Copy link
Copy Markdown
Member

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues.

Description

Problem

In the handshake error paths, we called s.host.Network().ClosePeer(peerID), which terminates all libp2p connections to that peer, not just the one that failed. With NAT'd peers and simultaneous dials, duplicate connection attempts are common: a failed handshake on a new, redundant connection would tear down an existing healthy session, notify kademlia, depeer the neighbor, and force a full reconnect cycle. Worst case was the duplicate-peer path in Connect(): a mere FullClose() failure on a redundant handshake stream called s.Disconnect(overlay, ...), actively removing a healthy peer from the topology.

Fix

Close only the connection that carried the failed handshake (stream.Conn().Close()) and leave established sessions untouched:

  • Use stream.Conn().Close() instead of ClosePeer() in the handshake error paths of handleIncoming and Connect
  • In the duplicate-peer path of Connect, close only the new redundant connection instead of disconnecting the peer
  • Also apply the same cleanup to the buildFullMAs fallback error path in Connect (was missed in the first pass)

Why this is safe

  • The peer registry is already designed for multiple connections per peer: addIfNotExists tracks every connection, and the Disconnected notifiee only removes the peer when its last connection drops. Closing a single connection is handled correctly end to end.
  • The inbound duplicate-peer path (handleIncoming) already used stream.Conn().Close() on master, this PR makes the outbound path and remaining error paths consistent with it.
  • Blocklisted peers intentionally keep ClosePeer(): for those we do want to sever everything.

Expected impact

Lower peer churn and a more stable topology: fewer spurious bee_kademlia_total_inbound_disconnections, fewer reconnect attempts, longer peer session lifetimes. Being an error-path change, the effect is most visible in NAT-heavy environments; we'll verify with a canary comparing disconnection rate and session lifetime split by bee version.

Open API Spec Version Changes (if applicable)

N/A

Motivation and Context (Optional)

Prevents disrupting healthy concurrent connections when rejecting redundant or failed connection attempts, reducing unnecessary kademlia topology churn.

Related Issue (Optional)

Screenshots (if appropriate):

@janos janos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@acud acud added this to the v2.8.0 milestone Feb 4, 2026
@gacevicljubisa gacevicljubisa added the on hold Temporarily halted by other development label Mar 10, 2026
@gacevicljubisa gacevicljubisa removed the on hold Temporarily halted by other development label Jul 7, 2026
@gacevicljubisa gacevicljubisa merged commit 82b0761 into master Jul 11, 2026
21 checks passed
@gacevicljubisa gacevicljubisa deleted the refactor/precise-connection-cleanup branch July 11, 2026 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants