fix: require the RSSI probe before a bond refresh on every path - #113
Merged
Conversation
#103 shipped the bond refresh deliberately narrow — its title names the condition, "when a bonded device is in range but refuses to open." #108 extended it to takeovers by replacing that requirement with `(skipRangeCheck && autoReconnectIsOn)`. An absent peer and an absent peripheral are different facts. A registered peripheral that is merely switched off matches "bonded but refuses openConnection" exactly, so a takeover removed its healthy pairing record. `takeAllPeripherals` loops every registered peripheral, so the headline menu-bar flow reaches it, as do the display-dock trigger and ADOPT_RELEASED; auto-reconnect defaults to on, which is all the old gate required. The documented recovery cannot fire: `probeAndReclaim` gates on RSSI, and an unbonded Magic device stops answering until it is power cycled — see the comment above `directReclaimAfterWake` and README's double-sleep note. The refresh now requires the probe on every path, which also drops the last use of `autoReconnectIsOn`. Also from the same review: - removeStaleBond re-checks the attempt token immediately before `-remove`. The caller's gate runs a blocking `rssi()` after its own check, and a cancel landing in that window lost a bond it could not re-pair. - The bonded-open success arm takes the same `isCurrentAttempt` guard the failure arm three lines below already had, so a cancel during the blocking open is no longer silently undone. - bondsAwaitingRepair clears when the pair succeeds, not only on `.connected`. A re-pair that succeeded but whose openConnection failed reported "Pairing Was Reset" and told the user to rebuild a pairing that was intact — and README documents that stuck-device case as common. - cancelConnect reports a bond the attempt already removed. It stands the watcher down, so nothing else would have rebuilt it. - handlePairTimeout supersedes the attempt it just retired, so a Bluetooth-queue block still behind a blocking open cannot start a pair with no watchdog. - HOLDS_ONE answers from live Bluetooth state again. The registered-list precheck made this Mac reply "not holding" for a peripheral it was actively using, and the peer treats any failure as permission to reclaim; the two lists only converge on a manual Sync. - takeReleasesInFlight counts instead of flagging, so the first of two overlapping takes cannot clear the guard while the other is on the wire. - The connected-row adoption in fetchConnectedPeripherals goes through setConnectionState, so it completes the peer's connect waiter instead of leaving it to time out. - An unproven resolve can no longer relocate a record parked behind an Identity Mismatch, which also silently cleared the parking. - Corrects the comment claiming Magic devices stay bonded to both Macs.
|
🎉 This PR is included in version 2.25.7 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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
1 participant
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.
Findings from a full review of
v2.25.6's six commits (1cacb70..40b55b4). Full rationale is in the commit message.The one that matters
#103shipped the bond refresh deliberately narrow — its title names the safety condition, "when a bonded device is in range but refuses to open."#108extended it to takeovers and replaced that hardrssi() != invalidRSSIrequirement with(skipRangeCheck && autoReconnectIsOn) || rssi() != invalidRSSI.An absent peer and an absent peripheral are different facts.
#108's goal was unsticking a take from an absent peer — but peer absence is already established over TCP before we touch Bluetooth. RSSI is the only thing that says the peripheral is there. "Refuses to open" alone is not a signature of anything; a powered-off device looks identical to a stale bond.Why it bites:
takeAllPeripheralsloops every registered peripheral (AppDelegate.swift:755), so the headline menu-bar flow reaches it — as do the display-dock trigger andADOPT_RELEASED, both unattended.autoReconnectdefaults totrue, which is all the old gate required.performUnregisterFromPCearly-returns when the device isn't connected (:732) and keeps the bond, so the app itself produces the bonded-but-unreachable state that matches the signature.And the stated safety net can't fire:
probeAndReclaimgates on RSSI, and an unbonded Magic device stops answering until power cycled — per the comment abovedirectReclaimAfterWakeand README's double-sleep note. The refresh deletes one of the two pairings README lists as a prerequisite and can't rebuild it.The probe is now required on every path. This drops the last use of
autoReconnectIsOn.Also fixed
removeStaleBondre-checks the attempt token immediately before-remove— the caller's gate runs a blockingrssi()after its own check, and a cancel in that window lost a bond it couldn't re-pair.isCurrentAttemptguard its failure arm already had; a cancel during the blocking open was silently undone.bondsAwaitingRepairclears on pair success, not only.connected— a re-pair that succeeded but whoseopenConnectionfailed told the user to rebuild an intact pairing (README:141 documents that case as common).cancelConnectreports a bond the attempt already removed, since it stands the watcher down and nothing else would rebuild it.handlePairTimeoutsupersedes the attempt it retires, so a queued block can't start a pair with no watchdog.takeReleasesInFlightcounts instead of flags.setConnectionState, so the peer's connect waiter fires instead of timing out.