Skip to content

Stop a connection bug from invalidating its own capture, and tag the fifth teardown route (#1040) #1046

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
Aug 2, 2026
Merged

Stop a connection bug from invalidating its own capture, and tag the fifth teardown route (#1040) #1046

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
17 changes: 14 additions & 3 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 @@ -2436,9 +2436,6 @@ class WhoopBleClient(
@SuppressLint("MissingPermission")
fun connect(model: WhoopModel = WhoopModel.WHOOP4) {
intentionalDisconnect = false
// Connection test mode: stamp when this connect attempt began so onConnectionStateChange can report
// the connect latency. A plain timestamp, no behaviour change; only read behind the CONNECTION gate.
connectAttemptStartedAtMs = System.currentTimeMillis()
// PR #588: an explicit user-driven Connect is never an out-of-range retry — clear the involuntary-
// reconnect streak so this scan (and any reconnects it spawns) starts back at the snappy
// LOW_LATENCY scan mode + the 3s backoff base, never inheriting a backed-off lower-power scan.
Expand Down Expand Up @@ -4299,6 +4296,11 @@ class WhoopBleClient(
// connect — onScanResult (pin now null) connects to the working strap when it advertises.
resetReconnectBackoff() // a deliberate re-adopt, not an out-of-range retry — start fresh
intentionalDisconnect = false
// #1040: the FIFTH local-teardown route, and the only one that was never stamped — so the status-22
// drop it causes printed `via=unknown`, which the reason doc calls out as meaning exactly this: a
// teardown path that exists and is not tagged. A multi-WHOOP re-adopt looks identical in the trace
// to a bond-watchdog bounce without it.
noteLocalTeardown("readopt") // #1020
try {
gatt?.disconnect() // drop the dead-pin link → handleDisconnect → rescan (pin cleared)
} catch (t: Throwable) {
Expand Down Expand Up @@ -4414,6 +4416,15 @@ class WhoopBleClient(
// Close any prior/pending GATT so a direct-reconnect attempt doesn't leak the old client.
// close() can throw on a dead binder (#314); swallow it — we're replacing the handle anyway.
try { gatt?.close() } catch (t: Throwable) { log("prior gatt.close() threw ${t.javaClass.simpleName} (ignored)") }
// Connection test mode: stamp when THIS GATT attempt began, so onConnectionStateChange reports the
// latency of the attempt that just succeeded. Stamped here rather than in connect() (#1040): the
// auto-reconnect path never calls connect(), so the mark went stale and `latencyMs` was measured
// from the original user-initiated connect — #1040 reported latencyMs=7847081 (2.18 h) for a link
// that came up 6 seconds after the previous drop, and it grows without bound the longer a reconnect
// loop runs. connectToDevice is the single funnel every connect passes through, so every attempt
// now gets a fresh mark. Latency is therefore GATT-attempt time, no longer including scan time on
// the first connect. A plain timestamp; only read behind the CONNECTION gate.
connectAttemptStartedAtMs = System.currentTimeMillis()
// autoConnect=false → a fast, direct connect (CoreBluetooth central.connect default), used for
// the scan-discovered first connect. autoConnect=true → the OS reconnects whenever the bonded
// strap is reachable WITHOUT needing an advertisement (used by the dropout auto-reconnect, #61).
Expand Down
36 changes: 33 additions & 3 deletions android/app/src/main/java/com/noop/testcentre/ReportCompleteness.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,29 @@ object ReportCompleteness {
*/
val evidenceTokens: Map<TestDomain, String> = linkedMapOf(
TestDomain.SLEEP to "sleep day=",
// #1040: CONNECTION's killer token `clockDrift ` is emitted from the strap-clock/history read, so a
// connection that never stays up long enough to reach that read cannot produce it — i.e. the worse
// the connection bug, the more certainly the Connection capture reads MISSING. #1040 arrived with
// 816 reconnects, a log full of `[connection] connect up/down` lines, and `INCOMPLETE: missing
// connection` stamped on it, which reads as an unusable capture when it is in fact the report we
// want. `reconnect n=` is the token that survives every way a connection can fail: it is emitted
// under TestDomain.CONNECTION on BOTH sides of the wasConnected branch — `reconnect n=<n>
// reason=<…>` after a link that came up and dropped, and `reconnect n=<n> failedConnect reason=<…>`
// when it never reached CONNECTED at all. So it is present precisely when the connection is broken,
// in either failure mode.
//
// Deliberately NOT `bondState`, the Swift twin's second token
// (`CaptureCompleteness.tokens[.connection] == ["clockDrift", "bondState"]`): that fires from only
// two sites, both on a COMPLETED encrypted bond, so a loop whose bond never completes — a prime
// suspect for a ~3 s bounce — would not emit it and the capture would still read MISSING. Nor
// `connect up gen=`, which covers a link that comes up and drops but NOT one that never connects,
// leaving a whole failure mode still self-invalidating. Swift wants the same widening; tracked with
// #1040 rather than diverging its map from here.
//
// Residual, accepted: a HEALTHY session that never dropped and never synced history has neither
// token. That capture reads MISSING as it always has — but it records no connection problem, so it
// is not a bug report being obstructed, which is what this rescue exists to prevent.
TestDomain.CONNECTION to "reconnect n=",
// #141: the NIGHTLY HRV trace proves the HRV mode captured, even when the user never took a manual
// (spot) reading — `hrv rmssd=` only fires on the Live-screen snapshot, but the overnight per-window
// trace emits `hrv nightSummary …`. So a wear-overnight-and-export HRV capture reads complete.
Expand Down Expand Up @@ -112,10 +135,17 @@ object ReportCompleteness {
sb.append("=== Capture check ===")
for ((d, status) in statuses) {
val matched = matchedToken(reportText, d)
// #1040: QUOTE the token. These are raw grep literals — they end in `=` or a significant
// trailing space (`clockDrift `), so unquoted they render as a dangling fragment: a real
// report read `connection: MISSING (expected clockDrift )`, and an evidence match would read
// `present (via reconnect n=)`. Quoting makes it unambiguous that the text IS the substring to
// search for, which is the whole point of naming it (#386). Does not affect the #950
// self-poisoning guard: the bare token still occurs inside the quotes, so the assembler must
// keep passing the PRE-append body either way.
val label = when {
status == Status.MISSING -> "expected ${killerTokens[d]}"
matched == killerTokens[d] -> matched
else -> "via $matched"
status == Status.MISSING -> "expected \"${killerTokens[d]}\""
matched == killerTokens[d] -> "\"$matched\""
else -> "via \"$matched\""
}
sb.append('\n')
.append(d.id).append(": ").append(status.token)
Expand Down
67 changes: 59 additions & 8 deletions android/app/src/test/java/com/noop/testcentre/ReportCompletenessParityTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,58 @@ class ReportCompletenessParityTest {
// the absent killer trace — `present (gate run=)` over an evidence-only match sent a maintainer
// hunting for gate lines the report never contained.
assertTrue("an evidence-only match must be labelled `via <evidence>`",
section.contains("sleep: present (via sleep day=)"))
section.contains("sleep: present (via \"sleep day=\")"))
}

@Test fun connectionIsPresentFromConnectChurnWhenTheLinkNeverReachesTheClockRead() {
// #1040, reproduced verbatim: CONNECTION's killer token `clockDrift ` is emitted from the strap-clock
// read, which a link that dies ~3 s after every connect never reaches. So the WORSE the connection
// bug, the more certainly its own capture reads MISSING — the report arrived with 816 reconnects, a
// log full of `[connection]` lines, and `INCOMPLETE: missing connection` stamped on exactly the
// capture we needed.
//
// No `bondState` in this fixture ON PURPOSE: that line only fires on a COMPLETED encrypted bond, so
// a loop whose bond never completes wouldn't emit it and the rescue would not fire.
val report = buildString {
appendLine("dayOwner day=2026-08-02 readId=whoop-D0 writeActiveId=whoop-D0 hrRows=2439 provenance=measured")
appendLine("[connection] connect up gen=852 latencyMs=6120 uptimeStart=1785663906")
appendLine("[connection] connect down (uptime ends after 3.0s)")
appendLine("[connection] reconnect n=816 reason=localTerminate via=bondWatchdog")
// NOTE: neither `clockDrift ` nor `bondState` — the link never got far enough for either.
}
val s = ReportCompleteness.statuses(report, active = setOf(TestDomain.CONNECTION))
assertEquals(ReportCompleteness.Status.PRESENT, s[TestDomain.CONNECTION])
val section = ReportCompleteness.captureCheckSection(report, active = setOf(TestDomain.CONNECTION))
assertTrue("a reconnect-loop capture must read complete, not INCOMPLETE",
section.endsWith("complete: all active traces present"))
assertTrue("an evidence-only match must be labelled `via <evidence>`",
section.contains("connection: present (via \"reconnect n=\")"))
}

@Test fun connectionIsPresentWhenTheLinkNEVERConnectsAtAll() {
// The other half of the same bug class, and the reason the token is `reconnect n=` rather than
// `connect up gen=`: a strap that never reaches CONNECTED emits ONLY the failedConnect branch, so a
// token keyed on a successful connect-up would leave "can't connect at all" reports still stamped
// INCOMPLETE — a connection failure invalidating its own capture, exactly what #1040 was about.
val report = buildString {
appendLine("dayOwner day=2026-08-02 readId=my-whoop writeActiveId=my-whoop hrRows=0 provenance=measured")
appendLine("[connection] reconnect n=42 failedConnect reason=connectionTimeout")
// NOTE: no `connect up gen=` — the link never came up even once.
}
val s = ReportCompleteness.statuses(report, active = setOf(TestDomain.CONNECTION))
assertEquals(ReportCompleteness.Status.PRESENT, s[TestDomain.CONNECTION])
assertTrue(ReportCompleteness.captureCheckSection(report, active = setOf(TestDomain.CONNECTION))
.endsWith("complete: all active traces present"))
}

@Test fun connectionStillPrefersTheClockDriftKillerTraceWhenItIsPresent() {
// The churn line is the rescue path, not a replacement: a capture that DID reach the strap-clock read
// must still name the deeper trace, so `via` continues to mean "the killer trace is genuinely absent".
val both = "clockDrift newest=2026-08-02 wall=2026-08-02 ok\nreconnect n=3 reason=connectionTimeout"
assertEquals("clockDrift ", ReportCompleteness.matchedToken(both, TestDomain.CONNECTION))
assertEquals("reconnect n=",
ReportCompleteness.matchedToken("reconnect n=3 reason=connectionTimeout", TestDomain.CONNECTION))
assertEquals(null, ReportCompleteness.matchedToken("no connection traces here", TestDomain.CONNECTION))
}

@Test fun matchedTokenPrefersTheKillerTraceOverTheEvidenceLine() {
Expand All @@ -107,8 +158,8 @@ class ReportCompletenessParityTest {
val section = ReportCompleteness.captureCheckSection(report, active = setOf(TestDomain.SLEEP))
assertEquals(
"=== Capture check ===\n" +
"universal: present (dayOwner day=)\n" +
"sleep: present (gate run=)\n" +
"universal: present (\"dayOwner day=\")\n" +
"sleep: present (\"gate run=\")\n" +
"complete: all active traces present",
section,
)
Expand All @@ -117,7 +168,7 @@ class ReportCompletenessParityTest {
/**
* #950 — the capture check must not read its OWN output as evidence.
*
* The section renders a missing domain as `<id>: MISSING (expected <killer token>)`, and that line
* The section renders a missing domain as `<id>: MISSING (expected \"<killer token>\")`, and that line
* contains the killer token. meta.json's capture_check was computed over report.txt AFTER the section
* was appended, so the re-scan found the token inside its own "expected …" text and recorded the
* domain as present. Every missing trace flipped and `complete` went true while report.txt said
Expand Down Expand Up @@ -156,9 +207,9 @@ class ReportCompletenessParityTest {
)
assertEquals(
"=== Capture check ===\n" +
"universal: present (dayOwner day=)\n" +
"sleep: MISSING (expected gate run=)\n" +
"connection: MISSING (expected clockDrift )\n" +
"universal: present (\"dayOwner day=\")\n" +
"sleep: MISSING (expected \"gate run=\")\n" +
"connection: MISSING (expected \"clockDrift \")\n" +
"INCOMPLETE: missing sleep, connection",
section,
)
Expand All @@ -170,7 +221,7 @@ class ReportCompletenessParityTest {
val section = ReportCompleteness.captureCheckSection("nothing here", active = emptySet())
assertEquals(
"=== Capture check ===\n" +
"universal: MISSING (expected dayOwner day=)\n" +
"universal: MISSING (expected \"dayOwner day=\")\n" +
"complete: all active traces present",
section,
)
Expand Down