Skip to content

fix(registrar): ensure onDisconnect respects notifyOnLimitedConnection#3376

Open
paschal533 wants to merge 3 commits intolibp2p:mainfrom
paschal533:fix/sdp
Open

fix(registrar): ensure onDisconnect respects notifyOnLimitedConnection#3376
paschal533 wants to merge 3 commits intolibp2p:mainfrom
paschal533:fix/sdp

Conversation

@paschal533
Copy link

Title

fix(registrar): ensure onDisconnect respects notifyOnLimitedConnection

Description

Fixed an inconsistency in topology notification where onDisconnect could fire for connections that never triggered onConnect.

When a topology has notifyOnLimitedConnection unset (default false), connections with limits (like circuit relay connections used for WebRTC SDP signaling) correctly skip onConnect. However, when these connections closed, onDisconnect was still being called due to a logic error in _onDisconnect:

// Before: when topology.filter is undefined, this evaluates to
// undefined === false → false, so the early return never triggers
if (topology.filter?.has(remotePeer) === false) { return }

// After: explicitly checks filter exists before checking membership
if (topology.filter != null && topology.filter.has(remotePeer) !== true) { return }

This caused spurious disconnect events in protocol handlers like GossipSub when WebRTC browser-to-browser connections use temporary relay connections for SDP exchange.

Fixes #2647
Fixes #2369

Notes & open questions

  • No changes were needed in transport-webrtc because circuit relay v2 connections already have connection.limits set. The bug was purely in the registrar's disconnect notification logic.
  • The optional chaining behavior (undefined === false → false) is a subtle JS gotcha that allowed this to go undetected.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@paschal533 paschal533 requested a review from a team as a code owner February 5, 2026 14:22
@dozyio dozyio self-requested a review February 18, 2026 17:09
@Faolain
Copy link

Faolain commented Mar 1, 2026

gentle bump @dozyio for review

Copy link
Collaborator

Choose a reason for hiding this comment

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

this also needs the fix and a test added 🙏

// was previously added to the filter (which happens on onConnect).
// This ensures limited connections that were never notified via
// onConnect don't trigger onDisconnect.
if (topology.filter != null && topology.filter.has(remotePeer) !== true) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (topology.filter != null && topology.filter.has(remotePeer) !== true) {
if (topology.filter?.has(remotePeer) !== true) {

follows style from other handler methods

@paschal533
Copy link
Author

Hi @tabcat , Thanks for the review. I've applied the fix to line 244 in _onPeerUpdate and added a test for that
case.

However, I need to keep the explicit null check (topology.filter != null &&) rather than simplifying
to optional chaining style. Here's the reason...

The optional chaining pattern doesn't work correctly for topologies without filters

// This breaks topologies without filters:
if (topology.filter?.has(peer) !== true) { return }
// When filter is undefined: undefined !== true → true → always returns early!

This causes the test "should call topology onConnect handler for limited connection when explicitly
requested" to fail (times out waiting for onConnect), because that topology has no filter.

The explicit check handles all three cases correctly:

 if (topology.filter != null && topology.filter.has(peer) !== true) { return }
 // When filter is undefined: false && ... → false → calls handler ✓
 // When filter exists but peer not in it: true && true → true → skips ✓
 // When filter exists and peer is in it: true && false → false → calls handler ✓

The style difference exists because:

  • _onPeerIdentify checks === true (works with optional chaining)
  • _onDisconnect and _onPeerUpdate check !== true (requires explicit null check for topologies without
    filters)

All tests now pass including the limited connection tests. Let me know if you'd prefer a different
approach!

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.

WebRTC SDP handshake connection transient? notifyOnTransient also ignore disconnection events?

4 participants