Repository navigation
WHOOP 5/MG: a completed CLIENT_HELLO write is not an encrypted link - #1744
Merged
Merged
Conversation
The second false bond, and unlike the first one this is not a mis-attributed
completion - the completion really is the hello's. The error is what was concluded
from it.
The ack branch took "the CLIENT_HELLO write completed" as proof of an encrypted
just-works bond and set encryptedBond, so the app showed "Bonded, streaming." on a
strap that an HCI capture already shows answering SMP "Pairing Not Supported",
where an encrypted bond is not merely absent but impossible. The 22:47 capture has
all three lines within two seconds of each other on one link:
22:46:31 CLIENT_HELLO outcome: acked by fd4b0002 after 5ms status=GATT_SUCCESS(0)
22:46:31 [connection] bondState encryptedBond family=whoop5 (CLIENT_HELLO acked)
22:46:33 bond state poll: BOND_NONE
A write completion says the STACK finished the write. Encryption is a property of
the LINK, and on Android the only thing that attests it is the OS bond state. Those
are different facts and are now recorded separately: encryptedBond waits for the OS
bond state or the strap's own BLE_BONDED event, while the completion still drives
the HANDSHAKE - subscribe the puffin chars, clock the strap, offload - all of which
need the strap to be listening, not the link to be encrypted.
Everything else in that branch that claimed something about the strap's WILLINGNESS
to bond is now gated on real encryption too: noteGenuineBond ("this strap bonds
fine"), clearPairingHint, staleDirectFailures, the deferral run and the override
budget. Clearing those on an unencrypted completion is how a strap that has never
bonded gets recorded as one that bonds fine - and it retires the very counters that
exist to notice it never did. cancelBondWatchdog stays unconditional: the hello is
answered either way and there is nothing left to time out.
The timing is diagnostic, never a gate. Across 41 captures every hello either
"completed" in 0, 4, 5 or 7ms or produced no callback at all about 3150ms later -
starkly bimodal, nothing in between, and 0ms cannot be a round trip. The line points
at that when the elapsed is under one connection interval, because it is the
clearest evidence in the set that the callback came from the local stack rather than
the strap. It does not decide anything: a peripheral may answer inside the same
connection event, so a fast round trip is unusual, not impossible, and a strap that
genuinely bonds must never have its bond rejected for being quick.
Android-only, and this one has no Swift twin on purpose. CoreBluetooth exposes no
bond or link-encryption state at all, so Apple has nothing to supply - a shared
helper would be handed a constant false and would suppress bonds that are genuine
there. The gap is real on Apple too; it cannot be closed the same way, and saying so
beats a twin that is wrong.
Staging 391 / iOS 272. 4898 tests, lintVital, doc lint and i18n clean.
ryanbr
pushed a commit
that referenced
this pull request
Aug 31, 2026
…1746) GET_DATA_RANGE answers twice - a short PENDING(2) acknowledgement, then the payload with SUCCESS(1) - and the #689 backlog decode ran on both. The ack carries no ring pointers by construction, so it decoded to null and logged "offsets may have moved" once per sync on healthy hardware, pointing every reader at an alignment bug that was not there. The tree already knew: Framing's own result-code table records "2=PENDING precedes SUCCESS on GET_DATA_RANGE (hardware-confirmed, #78 fork)". The decode site simply had no way to act on it, because a too-short frame and a misaligned one look the same to pagesBehind. A pure isPendingResponse(frame, cmdOff) on both platforms reads the result byte at cmdOff+2 and gates the decode at the two call sites. The SUCCESS frame that will not parse still logs exactly as loudly as before - that is the case #818's comment protects, and it is untouched. The #451 raw dump stays unconditional above it. Verified against the captured bytes rather than the description. On the PENDING frame, index 10 is 0x22 (34, GET_DATA_RANGE), 11 is the origin sequence, 12 is 0x02 - the cmdOff+2 claim is exact. That fixture is a whole frame: declLen 12, +8, and it is 20 bytes. The SUCCESS fixture is trimmed from its declared 84 bytes to 40, which is deliberate rather than damaged: pagesBehind reads to index 37, so the trim keeps precisely what the decoder needs, and decoding it by hand gives w=48833, u=48820, t=131072 -> 13, matching the log excerpt in the PR. The two predicates are exact twins, including the detail that Kotlin masks the signed Byte where Swift's UInt8 does not need it. Five new cases per platform, one-to-one. Merged onto a main that had moved a long way under it - WhoopBleClient.kt was rewritten substantially by #1744, #1748 and #1749 after this branch was cut - so it was merged locally and run rather than trusted: 4,926 tests, 0 failures, DataRangeScanTest 17/17, doc lint and i18n clean, all fifteen CI checks green. One thing left on the table for later, since the fixture already supports it: nothing asserts pagesBehind == 13 on that SUCCESS frame. The pair currently pins only that the two are distinguishable; asserting the value would pin that the pair behaves exactly as the log shows, for free, on both platforms. The PR body was edited before merge to remove a closing verb adjacent to an issue number. It was a denial - the contributor was explicitly saying the issue should stay open - but GitHub matches those keywords literally and does not read negations, so it would have been actioned on merge. Harmless this time only because that issue was already closed. Thanks @Zebsi235 - real captured frames rather than synthetic ones, the failing stub run to prove the tests actually catch the regression, and the Swift gap stated up front rather than left to be found.
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.
The second false bond, from the 30 Aug 22:47 capture. Unlike the first one this is not a mis-attributed completion — the completion really is the hello's. The error is what was concluded from it.
What the capture shows
Three lines, one link, two seconds apart:
The app displayed "Bonded, streaming." for that link — on a strap an HCI capture already shows answering SMP
Pairing Not Supported, where an encrypted bond is not merely absent but impossible.The confusion
A write completion says the stack finished the write. Encryption is a property of the link, and on Android the only thing that attests it is the OS bond state. The ack branch treated the first as proof of the second.
They are now recorded separately:
encryptedBondwaits for something that actually attests encryption — the OS bond state, or the strap's ownBLE_BONDEDeventThat split is deliberate and is the useful half: it keeps the only path that has ever plausibly led to history on this strap, while dropping the claim that was false.
The other claims in that branch
Everything asserting something about the strap's willingness to bond is gated on real encryption now:
noteGenuineBond("this strap bonds fine"),clearPairingHint,staleDirectFailures, the deferral run, and the hello-override budget.Clearing those on an unencrypted completion is how a strap that has never bonded gets recorded as one that bonds fine — and it retires the very counters that exist to notice it never did. That is a self-erasing bug: the more it fires, the less evidence survives that it fired.
cancelBondWatchdogstays unconditional. The hello is answered either way and there is nothing left to time out.Timing is diagnostic, never a gate
Across 41 captures the hello outcome is starkly bimodal:
Nothing in between, and 0ms cannot be a round trip. The line points at this when the elapsed is under one connection interval, because it is the clearest evidence in the set that the callback came from the local stack rather than the strap.
It decides nothing. A peripheral may answer inside the same connection event, so a fast round trip is unusual rather than impossible, and a strap that genuinely bonds must never have its bond rejected for being quick. The gate is the OS bond state; the timing is a pointer for whoever reads the capture next.
Android-only on purpose
No Swift twin here. CoreBluetooth exposes no bond or link-encryption state at all, so Apple has nothing to supply — a shared helper would be handed a constant
falseand would suppress bonds that are genuine there. The gap is real on Apple too; it cannot be closed the same way, and saying so beats a twin that is wrong.Verification
4898tests, 0 failureslintVitalFullRelease, doc-comment lint, i18n--cicleanRelated to #1635.