Repository navigation
Stop asking Android to pair once the strap has refused the bond (#1635) - #1763
Merged
Merged
Conversation
Asking Android to pair was the only probe in this area with no bound at all. The CLIENT_HELLO has a give-up, the unbonded offload probe has a silence budget, and this one fired once per link forever on a strap that answers SMP "Pairing Not Supported". That is worse than a quiet loop: every declined request surfaces a system "Pairing rejected" notice, so the cost lands on the user rather than in a log nobody reads. It was reported from the field as exactly that. The request now stands down once the bond give-up has latched for the device. The signal is the PERSISTED suppression latch rather than BondRefusalGiveUp.gaveUp, which lives in one process — a bound that dies with the process is the bug this area has had to fix twice already. Gating one experiment on the other is deliberate. BondRefusalGiveUp does not track the hello; it tracks whether this strap will form an encrypted bond, which is what both routes are trying to achieve and neither has. By the time it latches, the pairing request has had its five links and returned BOND_NONE every time. That is the experiment concluding, not being cut short. It ends the loop, not the experiment. Both things that clear the latch are user actions that could have changed the answer: tapping Connect, and forgetting the device. Putting a 5/MG into pairing mode and tapping Connect is the one flow known to have worked on real hardware, and it still asks. Nothing here touches connecting. The give-up this rides is the suppress branch, which stays connected and keeps live HR streaming; the pause branch is for an auth refusal and is untouched. A one-shot line reports the stand-down, because a switch that is on and silently doing nothing reads as a broken switch. The latch is now read once, above both decisions that consult it — the pairing request is made before the hello is considered, so a read placed only at the hello could not be seen by the request that needs it.
One read now serves both the pairing request and the hello, and that is only correct because nothing between them writes the latch — which became true when the pairing request stopped clearing it. Left implicit, the next person to add a write in that window gives the hello a stale verdict and gets no warning. The comment names the constraint and what to do instead.
) It read "not asking Android to pair on this connect". The line is printed once per process and governs every connect after it, so a reader finding it above twenty further connects would conclude either that the line was stale or that the gate ran per connect. Both wrong, and the reader has no way to tell which. Its two siblings both state the permanent thing - the hello override's "stopping", the offload probe's "Not asking this strap again" - and this one now matches: "not asking Android to pair on this strap again". The test pins the retirement wording and rejects the skip wording, because the difference is two words and a wrong conclusion.
Two drift risks in the branch that reports the stand-down. It restated three of shouldRequestExplicitBond's five conditions to work out that the give-up was the reason. Add a sixth condition to that gate and this branch keeps claiming the give-up for a decision the new check made - silently, in the one line a reader consults to find out why the requests stopped. It now calls the gate with bondGivenUpForDevice = false, which is literally the question being asked: would we be asking to pair if the give-up were not latched? Forgetting a device clears the suppression latch but did not re-arm the one-shot line, so a re-added strap would stop asking with nothing in the log saying why. That is the silent stop this area exists to stop repeating, and clearPairingHint already had the same reset for the same reason.
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.
It does not stop connecting
Stating this first because it is the obvious worry. The give-up this rides is the
suppress branch, the one whose own log line reads "hello suppressed, staying
connected":
This change gates exactly one thing —
createBond(). The diff touches no connect,reconnect, scan or pause path. Live HR, battery and auto-reconnect are unaffected.
The bound this request never had
Every probe in this area is bounded and this one was not:
shouldRequestExplicitBondwas gated only byoptedIn, family, the two bond flags,and
alreadyRequestedThisLink— a per-link flag. On a strap answering SMP"Pairing Not Supported" neither bond flag ever becomes true, so it fired on every
connect indefinitely.
That is worse than a quiet loop. Every declined request surfaces a system "Pairing
rejected" notice, so the cost lands on the user rather than in a log nobody reads.
It was reported from the field as exactly that.
The change
The request stands down once the bond give-up has latched for that device.
The signal is the persisted suppression latch, not
BondRefusalGiveUp.gaveUp.That flag lives in one process, and a bound that dies with the process is the same
defect corrected twice already this week: #1761 (a probe budget that re-armed on
every service restart) and #1762 (a latch cleared on every link).
Gating one experiment on the other is deliberate.
BondRefusalGiveUpdoes nottrack the hello; it tracks whether this strap will form an encrypted bond, which is
the single thing both routes are trying to achieve and neither has. Its threshold is
reached on the strap's own refusals, so by the time it latches the pairing request
has had its five links and returned
BOND_NONEevery time, with an HCI captureshowing SMP 0x05 underneath. The experiment concludes; it is not cut short.
It ends the loop, not the experiment. Both things that clear the latch are user
actions that could have changed the answer — tapping Connect, and forgetting the
device. Putting a 5/MG into pairing mode and tapping Connect is the one flow known
to have worked on real hardware (@Zebsi235's MG), and it still asks.
A one-shot line reports the stand-down. A switch that is on and silently doing
nothing reads as a broken switch, and the user has just been reading Android's
rejection notices — they are owed the app's side of it.
The latch is now read once, above both decisions that consult it. The pairing
request is made before the hello is considered, so a read placed only at the hello
could not be seen by the request that needs it.
Field evidence
Build 398, same strap, after the two earlier repairs landed:
That last number is what this PR removes. The UI reached "Live HR (not fully
paired)" — the #1635 end state — and the only thing still firing was the pairing
request the user could see being rejected.
Tests
ExplicitBondTest+2. The give-up case and the cleared-latch case are one testbecause the contrast is the point: the loop ends, the experiment does not. The
retirement line is pinned to name both ways out and to mention the system notice, so
it connects to what the user was actually looking at.
608 tests in
com.noop.ble, zero failures on a forced clean run.compileFullDebugKotlin,doc_comment_lint.py,i18n_audit.pyclean.Parity
Android only, and permanently so —
ExplicitBond.kt's own header already recordswhy: CoreBluetooth exposes no explicit pairing API, so there is nothing to mirror and
an audit finding this one-sided should leave it rather than delete it as untwinned.
Refs #1635.