Repository navigation
ble: let the bond give-up see an unanswered handshake - #1640
Merged
Merged
Conversation
A WHOOP 5/MG whose CLIENT_HELLO is never answered reconnects forever. Eight
consecutive cycles in a field capture all logged "attempt 1", retrying every
~11 seconds with no backoff, draining both batteries.
The give-up machinery already exists and works — it fired twice earlier the
same day. It could not see this failure because its gate tested only the
disconnect status:
if (!isInsufficientAuthStatus(status)) return
An unanswered handshake presents as a plain local terminate (status 22), so
the streak never incremented and the give-up never latched. Two observations
mean the same thing for the purpose of stopping: the strap answered the bond
write with INSUFFICIENT_AUTHENTICATION / _ENCRYPTION, or it never answered
at all.
The two signals are carried SEPARATELY rather than merged, because only one
of them supports naming a cause. PAIRING_HINT_TEXT and pausedHint() both
tell the user their strap is "still held by the official WHOOP app" — well
founded when the strap actively said no, and an unfounded guess when it
simply went quiet. Telling someone to close an app that may be irrelevant,
in the voice of an instruction, is worse than saying less. So an unanswered
handshake gets a new hint that describes what was observed, offers the one
action that is definitely theirs, and asserts no cause; and the
over-threshold pairing guidance stays auth-only.
Counted only when the status is NOT an auth rejection, since those already
reach the same accounting from onCharacteristicWrite — counting a cycle
twice would trip the give-up at half the intended streak.
This does NOT make the strap bond. It stops NOOP hammering a strap that
cannot, and hands the user an honest paused hint instead of a silent
11-second cycle. Why the handshake goes unanswered is still open, and the
bond-state trace is what should answer it.
Verified: compileFullDebugKotlin clean; com.noop.ble suite (438 tests)
green; 6 tests on the pure rule including the over-reach guard (an ordinary
disconnect with neither signal must not accrue) and one asserting the new
hint does NOT name a cause it cannot know; doc_comment_lint and i18n_audit
clean, gated. BLE behaviour change — needs app-build and a real-strap
capture to confirm the loop actually stops.
Re-review of this PR against the one it builds on, and the two defects share a cause: clientHelloWriteAtMs was cleared only on the SUCCESS path, so a failed completion left it set. 1. THIS PR DOUBLE-COUNTED. A write callback failing with INSUFFICIENT_AUTHENTICATION already reaches noteBondRefusalForPairingHint from onCharacteristicWrite. With the timestamp still set, the link then drops as status 22, helloWasUnacked reads true, and my `!isInsufficientAuthStatus(status)` guard - which tests the DISCONNECT status, not the write status - lets it count a second time. The give-up would trip at half the intended streak on exactly the auth refusals that already worked before this change. 2. #1638's DIAGNOSTIC LIED. A failed callback produced no outcome line at all, and then "NO write callback after Nms" at disconnect - which is false, the callback arrived and carried a status. A diagnostic that confidently reports the wrong thing is worse than one that says nothing, and this one would have sent the next investigation looking for a missing callback that was never missing. Report the failed completion where it happens, with its real status, and clear the timestamp. One fix, both defects, and the line now distinguishes "the strap refused" from "the strap never answered" - which is the whole point of the outcome field. Neither showed up in compilation or tests: the pure formatter is correct in isolation, and nothing exercises the failure branch. It took reading the success path and the failure path against each other. compileFullDebugKotlin clean; com.noop.ble suite (438 tests) green; doc_comment_lint and i18n_audit clean, gated.
ryanbr
force-pushed
the
fix/bond-giveup-covers-unacked-hello
branch
from
August 26, 2026 04:59
9113227 to
4b7806e
Compare
ryanbr
added a commit
that referenced
this pull request
Aug 26, 2026
…y 5s (#1642) The #1635 captures show the drop is locked to the hello, not the connect - 3.158/3.159/3.155s after the write, cycle after cycle - while live HR streams over the standard profile throughout. Sixteen writes across two sessions, zero acks, and the bond-state trace records no OS pairing attempt at all. After the give-up latches, the handshake is left off for that strap and the link stays up on the standard profile: the 'Live HR, not fully paired' state the app already models, reached deliberately instead of never. The give-up's two causes now diverge - an auth refusal still pauses, an unanswered handshake suppresses and stays connected. An explicit Connect always re-attempts, so it is never permanent. Two self-defeating interactions caught in re-review: the bond watchdog would have bounced the link at 7s instead of 4.8s, and the #982 never-bonded detector would have paused auto-reconnect after a few ordinary drops. Both key off didBond, which suppression makes permanently false by design. Android only, matching #1640. Does not make the strap pair - it converts a five-second loop into a stable live-HR link.
ryanbr
added a commit
that referenced
this pull request
Aug 27, 2026
Android has stopped the 5/MG reconnect loop since #1642. iOS never got it, so a 5/MG here still wrote the CLIENT_HELLO every connect, lost the link ~3s later and reconnected forever - the behaviour v10.6.0 shipped on both platforms. An HCI capture settled the cause: the phone sends an SMP Pairing Request and the strap answers Pairing Not Supported (0x05), so the encrypted bond the hello waits behind can never arrive on a 5/MG. Suppressing it after the give-up latches trades an unreachable capability (puffin commands, history offload) for one that demonstrably works - continuous live HR, the "Live HR, not fully paired" state the app already models (#69). Ported, mirroring the Kotlin twins: shouldSendClientHello, giveUpSuppressesHello, countsAsBondRefusal and helloSuppressionPrefKey in a new pure Strand/BLE/HelloSuppression.swift; helloSuppressedEpitaph, helloSuppressedHint and pausedHintHandshakeUnanswered on BondRefusalGiveUp; and recordWhoop5BondRefusal, shared by both causes so the split cannot drift. #1640 had to come with it. An unanswered hello produces no write error at all - the link just drops - so it never reached this platform's give-up, which only ever saw INSUFFICIENT_AUTHENTICATION. Without that half the suppression could never trigger. didBond stays permanently false once suppressed, and CLAUDE.md names that the trap. The audit: iOS has no bond watchdog and no #982 detector, so neither can undo it; countsAsBondRefusal gates on family, so a 4.0 can never latch; an auth refusal cannot be double-counted or mislabelled, because the write-error branch clears clientHelloWriteAt before the count; and only user-driven paths reach connect(), so the one-shot retry cannot leak into an automatic reconnect. Three re-review passes found three defects, all of that same family: - keepAliveFire ALSO read didBond, so the timer - and with it the liveness watchdog - would have been switched off for exactly the link the suppression keeps up for hours, leaving a stalled stream with nothing to bounce it. The first correction was dead code: both existing startKeepAlive() sites are post-bond, so on a suppressed link the timer never started at all. - the latch outlived the strap. releaseStrap/forgetDevice reset the give-up under the invariant that no state outlives the strap it belonged to, but the latch is persisted and survived a removal. ANDROID HAD THIS TOO, since #1642; the Kotlin half is in this PR, per the parity contract. - the keep-alive started in the wrong place. Android starts it on the live-HR bonded transition, which also covers an unbonded 5/MG that has not latched yet; iOS would have had no watchdog during the give-up's first five cycles. SettingsView carried a comment left for whoever did this port: its status line falls through to "Finishing the secure pairing handshake" and would describe a handshake no longer being attempted. It now carries the same "streaming but not fully paired" arm as SettingsLogic.strapStatusLine, in all ten locales, placed after the hint so the actionable text still wins. NOT ported: the #1669 opt-in override, which is experimental, default-off, and produced 57 reconnect cycles in an hour on hardware before #1670 bounded it. KNOWN REMAINING GAP: Android reads the Device Information Service on an unbonded link (#1647/#1652/#1653); iOS reads it post-bond only, so a suppressed iOS 5/MG never learns its firmware or whether it is an MG, leaving MG-only capabilities locked. Deliberately left as a follow-up - it is a separate feature that was never ported here at all. Verification: the three hint strings are proven byte-identical to the Kotlin source by extracting and comparing both, not by eye. 17 tests over the pure rules, incl. the family gate across every DeviceFamily case, the keep-alive gate either side, and the latch's case-insensitivity; the pure file also compiled standalone with swiftc and every rule exercised. Android 4552 tests green. app-build green on Build Strand, Test Strand and Build NOOPiOS. NOT HARDWARE-VALIDATED. BLE behaviour cannot be CI-tested and no 5/MG was available on iOS. The falsifiable prediction is the one Android's change carried: after the latch, the loop stops and live HR streams continuously. If a link still drops on a cycle with no CLIENT_HELLO line, the port is wrong.
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 loop
A WHOOP 5/MG whose CLIENT_HELLO is never answered reconnects forever. From a field capture, eight consecutive cycles:
Every one logs
attempt 1— the counter resets each cycle, so there is no backoff. It hammers the strap indefinitely, draining both batteries.Why the existing give-up couldn't see it
The machinery already exists and works — it fired twice earlier the same day. Its gate is the first line of
noteBondRefusalForPairingHint:It counts a failure only when the disconnect status is
INSUFFICIENT_AUTHENTICATION(5)orINSUFFICIENT_ENCRYPTION(15). An unanswered handshake presents as a plain local terminate (22), so the streak never incremented and the give-up never latched.Two observations mean the same thing for the purpose of stopping: the strap answered the bond write with a refusal, or it never answered at all.
The part I want reviewed most
The two signals are carried separately rather than merged, because only one of them supports naming a cause.
PAIRING_HINT_TEXTandpausedHint()both tell the user their strap is "still held by the official WHOOP app" and instruct them to close it and forget the device. That is well founded when the strap actively said no. It is an unfounded guess when the strap simply went quiet — and delivered as an instruction, an unfounded guess is worse than saying less, because the user has no way to know it might be irrelevant.So an unanswered handshake gets a hint that describes what was observed, offers the one action that is definitely theirs, and asserts nothing about why:
The over-threshold pairing guidance stays auth-only. There is a test asserting the new hint does not contain "official WHOOP app" or "Forget This Device".
What it does not do
This does not make the strap bond. It stops NOOP hammering one that cannot, and replaces a silent 11-second cycle with an honest paused state. Why the handshake goes unanswered is still open — #1639's bond-state trace is what should answer that.
Verification
onCharacteristicWrite; counting a cycle twice would trip the give-up at half the intended streak.compileFullDebugKotlinclean; fullcom.noop.blesuite (438 tests) green.doc_comment_lintandi18n_audit --ci mainclean, gated on rather than run alongside.