Skip to content

fix: reject same-direction duplicate sessions in addPeer (followup to v0.3.79 PR #4) #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sym-bot merged 1 commit into from
Apr 29, 2026
Merged

fix: reject same-direction duplicate sessions in addPeer (followup to v0.3.79 PR #4) #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 34 additions & 21 deletions Sources/SYM/SymNode.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -978,28 +978,41 @@ public final class SymNode {
let outcome: AddPeerOutcome = peerQueue.sync {
if var existing = self.peers[nodeId] {
if let prev = existing.transports["bonjour"] ?? nil {
// Simultaneous-dial collision: both peers Bonjour-discovered
// each other within ~50ms and both initiated outbound TCP. Each
// side now holds two handshaked sessions for the same nodeId
// (one outbound, one inbound). Without explicit dedup the new
// session silently overwrites the prior in the transports dict
// — the orphan keeps a live NWConnection + read loop, eventually
// fires didDisconnectWith, and `removeTransport` strips the
// surviving winner from the dict. Net effect: every connection
// dies within ~1–2s of handshake, application payloads never
// cross the wire.
// Two cases land here:
//
// Tie-break deterministically by nodeId so both peers agree on
// the same physical connection without exchanging frames: the
// lower nodeId acts as client and keeps its outbound; the
// higher node keeps the matching inbound. The loser's delegate
// is detached before disconnect (see fall-through below) so its
// teardown can't ripple back through removeTransport.
let preferNew = SymNode.preferNewSessionInDualDial(
localNodeId: self.identity.nodeId,
remoteNodeId: nodeId,
newIsOutbound: isOutbound
)
// (1) Dual-dial collision (different directions): both peers
// Bonjour-discovered each other and both initiated outbound
// TCP. Each side holds one outbound + one inbound for the
// same nodeId. Tie-break deterministically by nodeId — the
// lower nodeId keeps its outbound, the higher its inbound —
// so both peers select the same physical socket without
// exchanging coordination frames.
//
// (2) Same-direction duplicate: a second connection in the same
// direction as the established one. Happens when the OS
// newConnectionHandler fires twice for the same advertised
// service (multipath / TCP-retry / Bonjour-republish), or
// when discoveryDidFindPeer fires repeatedly and the dedup
// guard's window let one through. The first is healthy and
// in active use — replacing it with the duplicate would
// disconnect the wire pair on the remote side and trigger
// a peer-left storm. Always reject the duplicate.
//
// The losing session has its delegate detached before disconnect
// (see fall-through below) so its teardown can't ripple through
// removeTransport and clobber the surviving registered session.
let isDualDial = prev.isOutbound != session.isOutbound
let preferNew: Bool
if isDualDial {
preferNew = SymNode.preferNewSessionInDualDial(
localNodeId: self.identity.nodeId,
remoteNodeId: nodeId,
newIsOutbound: isOutbound
)
} else {
// Same direction → keep the established prior, reject duplicate.
preferNew = false
}
if preferNew {
existing.transports["bonjour"] = session
existing.lastSeen = Date()
Expand Down
60 changes: 60 additions & 0 deletions Tests/SYMTests/SYMTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,4 +533,64 @@ final class SimultaneousDialDedupTests: XCTestCase {
XCTAssertTrue(SymNode.preferNewSessionInDualDial(localNodeId: b, remoteNodeId: a, newIsOutbound: false),
"higher nodeId should keep its inbound")
}

/// Documents the dedup decision matrix `addPeer` applies when a prior
/// session already exists for `nodeId`. The dual-dial tie-break is only
/// safe to invoke when prior and new differ in direction. When they
/// match (e.g. two inbound sessions in rapid succession from a TCP
/// retry, multipath, or repeated newConnectionHandler firing), the
/// established prior must be kept and the duplicate rejected.
///
/// Without this rule a second inbound replaces a known-good prior
/// inbound, the prior is force-disconnected, the wire pair on the
/// remote side sees EOF, removeTransport fires there, and peer-left
/// storms ensue. (Observed in iPhone↔Mac Catalyst on v0.3.79 before
/// this rule was added.)
func testSameDirectionDuplicatesAreRejected() {
// The decision in addPeer is:
// isDualDial = (prev.isOutbound != session.isOutbound)
// preferNew = isDualDial ? tieBreak(...) : false
//
// Restate as a pure function so the test reads as a truth table:
func preferNew(localNodeId: String, remoteNodeId: String,
priorIsOutbound: Bool, newIsOutbound: Bool) -> Bool {
let isDualDial = priorIsOutbound != newIsOutbound
guard isDualDial else { return false }
return SymNode.preferNewSessionInDualDial(
localNodeId: localNodeId,
remoteNodeId: remoteNodeId,
newIsOutbound: newIsOutbound
)
}

let a = "00000000-0000-7000-8000-000000000001"
let b = "00000000-0000-7000-8000-000000000002"

// Same direction (both inbound) — reject duplicate regardless of
// nodeId ordering. This is the bug-fix case.
XCTAssertFalse(preferNew(localNodeId: a, remoteNodeId: b,
priorIsOutbound: false, newIsOutbound: false),
"duplicate inbound on lower nodeId must be rejected")
XCTAssertFalse(preferNew(localNodeId: b, remoteNodeId: a,
priorIsOutbound: false, newIsOutbound: false),
"duplicate inbound on higher nodeId must be rejected")

// Same direction (both outbound) — also reject duplicate.
XCTAssertFalse(preferNew(localNodeId: a, remoteNodeId: b,
priorIsOutbound: true, newIsOutbound: true),
"duplicate outbound must be rejected")
XCTAssertFalse(preferNew(localNodeId: b, remoteNodeId: a,
priorIsOutbound: true, newIsOutbound: true),
"duplicate outbound must be rejected")

// Sanity: dual-dial cases still apply tie-break correctly through
// this combined function (regression guard so a future refactor
// doesn't accidentally change the dual-dial behavior here).
XCTAssertFalse(preferNew(localNodeId: a, remoteNodeId: b,
priorIsOutbound: true, newIsOutbound: false),
"lower nodeId keeps its prior outbound, rejects new inbound under dual-dial")
XCTAssertTrue(preferNew(localNodeId: b, remoteNodeId: a,
priorIsOutbound: true, newIsOutbound: false),
"higher nodeId replaces prior outbound with new inbound under dual-dial")
}
}