Skip to content

fix: only claim a held connection when the OS actually reports one #2003

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ryanbr merged 1 commit into from
Sep 8, 2026
Merged

fix: only claim a held connection when the OS actually reports one #2003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 21 additions & 25 deletions android/app/src/main/java/com/noop/ble/BondRefusalGiveUp.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,25 @@ class BondRefusalGiveUp(
"so it stops draining both batteries. Tap Connect to try again, and if it keeps happening " +
"please share your strap log."

/**
* #1997: the three inputs the guide choice turns on, logged at the pause.
*
* The held-link branch is now deliberately conservative: it fires only when the OS itself reports
* the connection still held. Nothing in the field log that prompted it actually shows that signal
* on the silent links, because those took the give-up path, which prints no ACL marker. So the
* branch may be right and may simply never fire, and without this line the next log would not say
* which.
*
* The counters ship rather than just the behaviour, for the same reason the windowed-read ones do.
*
* [held] is PASSED IN rather than re-derived here. Restating `heldLinkWithoutTraffic`'s rule in a
* second file would let the line report a verdict the code did not reach the moment either copy
* changed. The caller evaluates the predicate once and hands the answer to both this and the guide,
* so all three cannot disagree.
*/
fun heldLinkDiagLine(aclHeld: Boolean, inboundFrames: Int, held: Boolean): String =
"held-link check: aclHeld=$aclHeld inbound=$inboundFrames -> ${if (held) "held" else "not held"}"

/**
* #1997: which reconnect guide the never-bonded pause should show.
*
Expand Down Expand Up @@ -182,34 +201,11 @@ class BondRefusalGiveUp(
answering on it: the connection size negotiation is refused and no data arrives. Re-pairing
will not change this, so it is not worth doing.

1. If the official WHOOP app is running, quit it. A strap talks to one phone at a time.
2. Otherwise turn Bluetooth off and back on, which releases the held connection.
1. Turn Bluetooth off and back on. That releases the held connection.
2. If you have the official WHOOP app installed, quit it too. A strap talks to one phone at a time.
3. Come back here and tap Connect.
""".trimIndent()

/**
* #1997: the paused hint for a link the OS was already HOLDING, rather than a strap refusing.
*
* [pausedHintHandshakeUnanswered] deliberately declines to name a cause, and that is right for an
* unanswered handshake: several things produce it and "close the WHOOP app" would be a guess
* dressed as instruction. This branch exists because there IS evidence. The MTU exchange was
* refused, which happens when it already took place on a connection the phone still holds, and not
* one frame arrived on the link. Naming what was observed is then reporting, not guessing.
*
* It still does not assert WHICH owner holds the link, because NOOP cannot see that. It names the
* two candidates and the actions that belong to the user. Crucially it does NOT send them to
* re-pair: nothing was exchanged for the strap to refuse, so re-pairing cannot change this state,
* and the reporter had been doing exactly that on a loop.
*
* Pure; no em-dash.
*/
fun pausedHintLinkHeld(): String =
"NOOP stopped retrying because the phone's Bluetooth is still holding a connection to your " +
"strap that the strap is not answering on: the connection size negotiation is refused and " +
"no data arrives. Re-pairing will not change this, so it is not worth doing. If the " +
"official WHOOP app is running, quit it. Otherwise turn Bluetooth off and on again, which " +
"releases the held connection. Then tap Connect."

/**
* #750: a short OPAQUE token for the epitaph, derived from the strap's device id.
*
Expand Down
46 changes: 17 additions & 29 deletions android/app/src/main/java/com/noop/ble/WhoopBleClient.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1048,8 +1048,13 @@ class WhoopBleClient(
/**
* #1997: was this link one the strap never spoke on, on a connection the OS was already holding?
*
* The MTU exchange is refused (non-zero status, so the link sits at the 23-byte default) and not a
* single frame arrived. On the reporting device every re-attach looked exactly like this while the
* The OS reports the connection still held ([isStrapAclHeld]) and not a single frame arrived on it.
*
* That is the whole claim the guide makes, so it is the whole predicate. An earlier version also
* required the MTU exchange to have been refused, which was how this inferred a held link BEFORE
* asking the OS directly. Once the direct signal is required the refusal adds no part of the claim
* and only narrows when it can fire, which on a branch with no field sighting yet is a cost with
* no benefit. The MTU refusal still reads as a refusal in the log, where it helps triage. On the reporting device every re-attach looked exactly like this while the
* one link that negotiated 247 carried traffic, so this is the shape of attaching to a stale
* connection rather than of a strap declining to pair.
*
Expand All @@ -1059,8 +1064,8 @@ class WhoopBleClient(
* connect-drop-retry cycle would have run forever draining both batteries. That is the exact
* failure #982 exists to prevent. The pause is right; only the explanation was wrong.
*/
internal fun heldLinkWithoutTraffic(mtuStatus: Int, inboundFrames: Int): Boolean =
mtuStatus != 0 && inboundFrames == 0
internal fun heldLinkWithoutTraffic(aclHeld: Boolean, inboundFrames: Int): Boolean =
aclHeld && inboundFrames == 0

/** Pure guard for a delayed service-discovery kick. The operation belongs only to the exact
* connection that scheduled it, and a temporarily-missing GATT wrapper must not consume the
Expand Down Expand Up @@ -6512,12 +6517,6 @@ class WhoopBleClient(
it.copy(pairingHint = when {
suppress -> BondRefusalGiveUp.helloSuppressedHint()
authRefusal -> BondRefusalGiveUp.pausedHint()
// #1997: BEFORE the unanswered-handshake hint, because this is the more specific
// observation. A refused MTU exchange with no inbound traffic says the link is held
// and the strap is not on it, which the generic hint cannot say and which changes
// what the user should do: not re-pair.
heldLinkWithoutTraffic(lastMtuStatus, inboundFrames) ->
BondRefusalGiveUp.pausedHintLinkHeld()
else -> BondRefusalGiveUp.pausedHintHandshakeUnanswered()
})
}
Expand Down Expand Up @@ -6575,20 +6574,6 @@ class WhoopBleClient(
* stable telemetry and to avoid repeating any future callback-side work. */
private var lastMtuValue = -1

/**
* #1997: the status the last MTU exchange reported, and the reason a refused one matters.
*
* A re-attach to a link the OS still holds answers the MTU request with a non-zero status and leaves
* the link at the 23-byte default. On the reporter's log every such link then carried ZERO inbound
* frames, while the one link that negotiated 247 carried traffic. So a refused exchange is a cheap,
* early tell that this link is not one the strap is actually talking on.
*
* Plain `var`, like [lastMtuValue] and `lastMtuAtMs` beside it, which are written in the same callback
* and read on the same paths. Marking only this one `@Volatile` would advertise a thread-safety
* property it cannot deliver: it is read in a single expression with `inboundFrames`, also a plain
* var, so the pair would be no more coherent than before while looking as though it had been made so.
*/
private var lastMtuStatus = 0
private var lastMtuAtMs = 0L

/** #1066 follow-up: wall-clock of the `requestMtu` attempt, so `onMtuChanged` can log how long the MTU
Expand Down Expand Up @@ -6836,7 +6821,6 @@ class WhoopBleClient(
}
lastMtuValue = mtu
lastMtuAtMs = now
lastMtuStatus = status
// Whatever the strap granted (≤ requested). Telemetry only: Android can emit this callback
// for the connection itself as well as requestMtu, without saying which. Starting discovery
// here can overlap the still-running MTU operation and wedge service discovery.
Expand Down Expand Up @@ -10909,9 +10893,14 @@ class WhoopBleClient(
// was wrong is blaming the strap. When the MTU exchange was refused and nothing arrived,
// the phone is holding a connection the strap is not on, and re-pairing cannot change
// that: the reporter had been doing it several times a day on this guide's advice.
val guide = BondRefusalGiveUp.reconnectGuideFor(
heldLinkWithoutTraffic(lastMtuStatus, inboundFrames),
)
// Read the OS signal ONCE: isStrapAclHeld is a binder call, and the diagnostic and the
// guide must agree about what was observed anyway.
val aclHeldNow = lastDeviceAddress?.let { isStrapAclHeld(it) } == true
// Say what the choice turned on, so a shared log can tell whether the held-link branch
// fired or was simply never reachable in the field.
val heldLink = heldLinkWithoutTraffic(aclHeldNow, inboundFrames)
log(BondRefusalGiveUp.heldLinkDiagLine(aclHeldNow, inboundFrames, heldLink))
val guide = BondRefusalGiveUp.reconnectGuideFor(heldLink)
_state.update { it.copy(reconnectGuide = guide) }
}
}
Expand Down Expand Up @@ -11204,7 +11193,6 @@ class WhoopBleClient(
// Clear the onMtuChanged dedup (#50) so the first MTU callback of the NEXT connection — even to
// the same strap with the same granted mtu — is never mistaken for a duplicate of the last one.
lastMtuValue = -1
lastMtuStatus = 0
lastMtuAtMs = 0L
mtuRequestedAtMs = 0L // #1066: don't measure settle time across a connection boundary
// The strap forgets the realtime-HR toggle across a disconnect; the post-bond branch re-arms it
Expand Down
59 changes: 20 additions & 39 deletions android/app/src/test/java/com/noop/ble/BondRefusalGiveUpTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,44 +105,6 @@ class BondRefusalGiveUpTest {

// --- #1997: the held-link hint ---

/**
* The whole point of this branch: it must NOT send the user to re-pair. The reporter was re-pairing
* and rebooting on a loop because the generic guide told them to, for a state where nothing was
* exchanged for the strap to refuse.
*/
@Test
fun `the held-link hint does not tell the user to re-pair`() {
val h = BondRefusalGiveUp.pausedHintLinkHeld().lowercase()
// It DOES say the word, to rule the action out. What it must not do is instruct it: no Forget,
// no Unpair, no numbered steps. Asserting the word is absent was wrong and this test caught it.
assertTrue(h, h.contains("re-pairing will not change this"))
assertFalse(h, h.contains("forget"))
assertFalse(h, h.contains("unpair"))
assertFalse(h, h.contains("1."))
}

/**
* It names both candidate owners and the action for each, without asserting WHICH holds the link,
* because NOOP cannot see that. Naming the observation is reporting; naming the owner would be a
* guess, which is the standard `pausedHintHandshakeUnanswered` already sets.
*/
@Test
fun `the held-link hint offers both actions without claiming which owner holds it`() {
val h = BondRefusalGiveUp.pausedHintLinkHeld().lowercase()
assertTrue(h, h.contains("whoop app"))
assertTrue(h, h.contains("bluetooth off and on"))
assertFalse(h, h.contains("is holding it"))
}

/** Distinct from the generic hint, or the more specific observation would be invisible. */
@Test
fun `the held-link hint is not the unanswered-handshake hint`() {
assertNotEquals(
BondRefusalGiveUp.pausedHintHandshakeUnanswered(),
BondRefusalGiveUp.pausedHintLinkHeld(),
)
}

/**
* The guide is what the user actually sees, and it is where the harm was: the re-pair steps sent the
* reporter to forget and re-pair several times a day for a state re-pairing cannot change.
Expand All @@ -157,8 +119,10 @@ class BondRefusalGiveUpTest {
assertFalse(g, g.contains("unpair"))
assertFalse(g, g.contains("flash blue"))
// and it still gives them something to do
assertTrue(g, g.contains("quit it"))
assertTrue(g, g.contains("bluetooth off and back on"))
// Universal action first, the conditional one second, for the same reason.
assertTrue(g, g.indexOf("bluetooth off and back on") < g.indexOf("whoop app"))
assertTrue(g, g.contains("if you have the official whoop app installed"))
}

/**
Expand All @@ -183,4 +147,21 @@ class BondRefusalGiveUpTest {
assertTrue(g, g.contains("Tap the band repeatedly until its LEDs flash blue (pairing mode)."))
assertTrue(g, g.startsWith("Your strap connects but never finishes pairing with NOOP"))
}

/**
* The diagnostic renders the verdict it is GIVEN, which is the verdict the guide is chosen from, so a
* log cannot report a decision the code did not make. Driven from `heldLinkWithoutTraffic` here so the
* inputs and the rendered verdict are checked against the real rule across every combination.
*/
@Test
fun `the held-link diagnostic reports the same verdict the predicate reaches`() {
val cases = listOf(true to 0, false to 0, true to 6, false to 6)
for ((acl, inbound) in cases) {
val expected = WhoopBleClient.heldLinkWithoutTraffic(acl, inbound)
val line = BondRefusalGiveUp.heldLinkDiagLine(acl, inbound, expected)
assertTrue(line, line.endsWith(if (expected) "-> held" else "-> not held"))
assertTrue(line, line.contains("aclHeld=$acl"))
assertTrue(line, line.contains("inbound=$inbound"))
}
}
}
19 changes: 14 additions & 5 deletions android/app/src/test/java/com/noop/ble/NeverBondedSelfDropGiveUpTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,11 +189,20 @@ class NeverBondedSelfDropGiveUpTest {
* which this must not claim.
*/
@Test
fun `a held link is a refused MTU exchange AND no traffic`() {
assertTrue(WhoopBleClient.heldLinkWithoutTraffic(mtuStatus = 4, inboundFrames = 0))
assertFalse(WhoopBleClient.heldLinkWithoutTraffic(mtuStatus = 4, inboundFrames = 6))
assertFalse(WhoopBleClient.heldLinkWithoutTraffic(mtuStatus = 0, inboundFrames = 0))
assertFalse(WhoopBleClient.heldLinkWithoutTraffic(mtuStatus = 0, inboundFrames = 6))
fun `a held link is the OS signal AND no traffic`() {
assertTrue(WhoopBleClient.heldLinkWithoutTraffic(aclHeld = true, inboundFrames = 0))
assertFalse(WhoopBleClient.heldLinkWithoutTraffic(aclHeld = true, inboundFrames = 6))
}

/**
* The narrowing that matters, and the reason it was added. A refused exchange with no traffic ALSO
* describes a stale pairing, where the right advice is the opposite: re-pair. The #1997 reporter
* turned out to be exactly that, with no official WHOOP app installed and a bond that had gone stale.
* Without the OS actually reporting the connection held, this must not claim it.
*/
@Test
fun `a stale pairing is not mistaken for a held connection`() {
assertFalse(WhoopBleClient.heldLinkWithoutTraffic(aclHeld = false, inboundFrames = 0))
}

/**
Expand Down