fix: reject same-direction duplicate sessions in addPeer (followup to v0.3.79 PR #4)#5
Merged
Merged
Conversation
v0.3.79 introduced dual-dial dedup but applied the tie-break unconditionally whenever a prior bonjour transport existed. The tie-break assumes the prior is in the OPPOSITE direction of the new session — which is true for genuine simultaneous-dial collisions but NOT for same-direction duplicates. When two inbound sessions arrive in rapid succession (TCP retry, multipath, or repeated newConnectionHandler firing), the tie-break was applied as if the prior were an outbound. For higher-nodeId peers this returns preferNew=true, replacing the established healthy inbound with the duplicate. The replaced session was force-disconnected; its remote wire pair saw EOF; removeTransport fired on the remote side; peer-left storm. Observed in the field on iPhone↔Mac Catalyst v0.3.79: iPhone log shows two consecutive "replacing prior with new inbound" lines back-to-back — the second replace tearing down the survivor of the first. Fix: in addPeer, only apply the dual-dial tie-break when prior and new differ in direction. Same-direction prior → keep established session, reject duplicate (delegate detached + disconnect). Test: testSameDirectionDuplicatesAreRejected — truth-table coverage of both same-direction cases (both inbound / both outbound) for both nodeId orderings, plus regression guard that dual-dial tie-break still applies correctly through the combined decision. 71/71 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Followup to #4 (v0.3.79). The dual-dial dedup introduced there applied the nodeId tie-break unconditionally whenever a prior bonjour transport existed. The tie-break assumes the prior is in the opposite direction of the new session — true for genuine simultaneous-dial collisions, but NOT for same-direction duplicates.
When two inbound sessions arrive in rapid succession (TCP retry, multipath race, repeated
newConnectionHandlerfiring for the same advertised service), the tie-break was applied as if the prior were an outbound. For the higher-nodeId peer this returnspreferNew=true, replacing the established healthy inbound with the duplicate. The replaced session was force-disconnected; its remote wire pair saw EOF;removeTransportfired on the remote side; peer-left storm continued.Field evidence (iPhone↔Mac Catalyst on v0.3.79)
iPhone (higher nodeId) log shows the bug — two consecutive "replacing prior with new inbound" lines:
The second "replacing prior" is tearing down the survivor of the first — exactly the symptom described above.
Mac (lower nodeId) consequence — peer flap continued:
Fix
In
addPeer, only apply the dual-dial tie-break when prior and new differ in direction. Same-direction prior → keep established session, reject duplicate. The duplicate's delegate is detached beforedisconnect()so its teardown can't ripple back throughremoveTransport.Tests
New test
testSameDirectionDuplicatesAreRejected— truth-table coverage of both same-direction cases (both inbound / both outbound) for both nodeId orderings, plus a regression guard that the dual-dial tie-break still applies correctly through the combined decision.71/71 tests pass.
Test plan
swift test)🤖 Generated with Claude Code