From 66e908df08bc37f92e67b9d4f61d033895498ca4 Mon Sep 17 00:00:00 2001 From: ryanbr Date: Sat, 1 Aug 2026 11:24:01 -0700 Subject: [PATCH 1/5] Connection trace: name which of our own paths dropped the link (#1020) A local teardown surfaces as GATT status 22 and the trace printed the bare `reason=localTerminate`. At least FIVE of our paths produce it: the bond watchdog, the keep-alive stall bounce, a user disconnect, releaseStrap, and a safeGatt teardown after a dead binder. One string for five causes. #1020 reports nearly 2,800 reconnects, every one localTerminate, and the log cannot say which path is responsible - so the report cannot be diagnosed without a round trip. The comment beside the reason string could only guess ("our own bounce (e.g. #971 bond watchdog)"), which is the tell that this was already costing someone time. Each site now records its origin and the trace prints `via=`. An untagged local teardown reads `via=unknown`, which is a signal rather than a fallback: it means a teardown route exists that this missed. `connect down` also carries the session length. That alone separates the candidates - a bond watchdog fires seconds in, the stall bounce two minutes in, a radio drop anywhere. An unknown session start prints no suffix rather than "after 0.0s", which would read as an instant drop and mean something different. DIAGNOSTIC ONLY. No reconnect logic, timing or teardown behaviour changes; the new lines ride the existing CONNECTION test-domain gate. The origin setter is one volatile assignment on paths already performing a teardown. Composition extracted to connectionDownReason/sessionHeldSuffix beside the other pure BLE policy helpers, with 7 tests. Locale.US on the duration so a decimal comma cannot follow the phone language into a pasted log. --- .../main/java/com/noop/ble/WhoopBleClient.kt | 69 ++++++++++++++++-- .../com/noop/ble/ConnectionDownReasonTest.kt | 73 +++++++++++++++++++ 2 files changed, 136 insertions(+), 6 deletions(-) create mode 100644 android/app/src/test/java/com/noop/ble/ConnectionDownReasonTest.kt diff --git a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt index d87153f0ed..5ee5c1e330 100644 --- a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt +++ b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt @@ -538,6 +538,31 @@ class WhoopBleClient( else -> BluetoothGatt.CONNECTION_PRIORITY_BALANCED } + /** + * The `reason=` token for a connection-down trace line (#1020), pure so the composition is + * unit-testable without a BLE stack. + * + * A local teardown arrives as GATT status 22 and used to print the bare `localTerminate`, which + * five different paths of ours produce. [localTeardownOrigin] names which one; `unknown` when the + * drop was local but no path claimed it, which is itself worth seeing — it means a teardown route + * exists that is not tagged. + */ + fun connectionDownReason(status: Int, localTeardownOrigin: String?): String = when (status) { + GATT_CONN_TIMEOUT -> "connectionTimeout" + GATT_CONN_TERMINATE_LOCAL_HOST -> "localTerminate via=${localTeardownOrigin ?: "unknown"}" + else -> "status$status" + } + + /** + * The ` after s` suffix on `connect down` (#1020), or empty when the session start is unknown. + * + * Separates the culprits at a glance: a bond watchdog fires seconds in, the keep-alive stall + * bounce two minutes in, a radio drop anywhere. Locale.US so the decimal point does not follow the + * phone language into a log people paste into issues. + */ + fun sessionHeldSuffix(heldMs: Long): String = + if (heldMs < 0L) "" else " after ${"%.1f".format(java.util.Locale.US, heldMs / 1000.0)}s" + /** Pure battery-adaptive gate (#477), unit-testable without a BLE stack. Keyed on the STRAP's * battery (WHOOP/Oura/Fitbit): the lever is ARMED by [thresholdPct] > 0 (the Settings slider is * 10–30; 0 disables it) and engages while the strap is DISCHARGING at/below [thresholdPct]. The @@ -2515,6 +2540,7 @@ class WhoopBleClient( */ @SuppressLint("MissingPermission") fun disconnect() { + noteLocalTeardown("userDisconnect") // #1020 intentionalDisconnect = true handler.removeCallbacks(scanTimeoutRunnable) // #1030 (ryanbr): a user teardown supersedes any pending involuntary reconnect timer. @@ -2668,6 +2694,7 @@ class WhoopBleClient( * `BLEManager.forgetDevice` (which iOS already wires from DevicesView's Remove). Runs on the main looper. */ fun releaseStrap() { + noteLocalTeardown("releaseStrap") // #1020 handler.post { intentionalDisconnect = true // defuse the disconnect→3s-reconnect loop's guard handler.removeCallbacks(scanTimeoutRunnable) @@ -3971,6 +3998,30 @@ class WhoopBleClient( * nag the user. Reset to 0 on any genuine bond. (5/MG firmware reset parity, 2026-06) */ private var staleDirectFailures = 0 + /** + * Which of OUR OWN paths last tore the link down, and when the current session started (#1020). + * + * A local teardown surfaces as GATT status 22 (`GATT_CONN_TERMINATE_LOCAL_HOST`), which the + * connection trace has always reported as the bare `reason=localTerminate`. At least five paths + * produce it — the bond watchdog, the keep-alive stall bounce, a user disconnect, releaseStrap, and + * a safeGatt teardown after a dead binder — and the log could not tell them apart. A report showing + * thousands of localTerminate reconnects (#1020) therefore could not be diagnosed from the log at + * all; the existing comment beside the reason string just guesses ("e.g. #971 bond watchdog"). + * + * Set at each initiating site, cleared when a new session comes up, and printed alongside the + * reason. @Volatile because the setters run on the main looper and timer callbacks while + * handleDisconnect reads it from a GATT binder thread on API 26/27. + */ + @Volatile private var lastLocalTeardown: String? = null + + /** Wall clock when the current session reached STATE_CONNECTED, for the session-duration readout. + * 0 when not connected. @Volatile for the same reason as [lastLocalTeardown]. */ + @Volatile private var connectedAtMs = 0L + + /** Record which local path is about to drop the link. Cheap and unconditional: the string is a + * constant at every call site, and knowing the origin is worth more than the trace being gated. */ + private fun noteLocalTeardown(origin: String) { lastLocalTeardown = origin } + /** Consecutive involuntary reconnect attempts, feeding the capped-exponential [ReconnectBackoff] * (3, 6, 12, 24, 48, 60s…). Replaces the old fixed [RECONNECT_DELAY_MS] rescan loop so a strap * that's genuinely out of range stops hammering BLE — the Android twin of the iOS @@ -4097,6 +4148,7 @@ class WhoopBleClient( // through to a clean teardown if it does (mirrors the keep-alive bounce). When we gave up above, // [handleDisconnect] takes its paused branch and schedules NO reconnect; otherwise it backoff- // reconnects and [armBondWatchdog] arms the NEXT (wider) window from [bondWatchdogBackoff]. + noteLocalTeardown("bondWatchdog") // #1020: name the origin of the localTerminate that follows try { gatt?.disconnect() // → handleDisconnect → reset() (cancels this) → (paused | backoff reconnect) } catch (t: Throwable) { @@ -4421,6 +4473,8 @@ class WhoopBleClient( if (testCentre.active(com.noop.testcentre.TestDomain.CONNECTION)) { val nowUnix = System.currentTimeMillis() / 1000L val latencyMs = connectAttemptStartedAtMs?.let { System.currentTimeMillis() - it } + connectedAtMs = System.currentTimeMillis() // #1020: for the session-duration readout + lastLocalTeardown = null // #1020: a new session, no teardown origin yet log("connect up gen=$connectGeneration latencyMs=${latencyMs ?: "?"} uptimeStart=$nowUnix", com.noop.testcentre.TestDomain.CONNECTION) } @@ -5473,6 +5527,7 @@ class WhoopBleClient( // Nothing for the fuse window — the live stream/link stalled. Bounce it: the auto-rescan on // disconnect re-bonds and resumes streaming (the automatic version of the manual fix). log("No data for ${silentMs / 1000}s — bouncing link to resume live stream") + noteLocalTeardown("keepAliveStall") // #1020 intentionalDisconnect = false // make sure the auto-reconnect fires // disconnect() throwing on a dead binder (#314) would crash from the keep-alive timer; // tear down directly so the bounce degrades to a clean disconnect. @@ -6846,6 +6901,7 @@ class WhoopBleClient( // policy (always tear down) is single-sourced in shouldTeardownOnGattThrow so it's testable. if (shouldTeardownOnGattThrow(t)) { log("GATT op '$reason' failed (${t.javaClass.simpleName}); tearing down link") + noteLocalTeardown("gattThrow:$reason") // #1020 teardownAfterGattFailure() } false @@ -7049,13 +7105,14 @@ class WhoopBleClient( // so the reconnect-churn count means the same thing on both platforms. if (wasConnected) connReconnectCount += 1 if (testCentre.active(com.noop.testcentre.TestDomain.CONNECTION)) { - val reason = when (status) { - GATT_CONN_TIMEOUT -> "connectionTimeout" - GATT_CONN_TERMINATE_LOCAL_HOST -> "localTerminate" // our own bounce (e.g. #971 bond watchdog) - else -> "status$status" - } + // #1020: `via=` names WHICH of our own paths dropped it — bondWatchdog, keepAliveStall, + // userDisconnect, releaseStrap or gattThrow:. Five paths produced one string before, + // which made a thousands-of-reconnects report undiagnosable from the log alone. + val reason = connectionDownReason(status, lastLocalTeardown) if (wasConnected) { - log("connect down (uptime ends)", com.noop.testcentre.TestDomain.CONNECTION) + val heldMs = if (connectedAtMs > 0L) System.currentTimeMillis() - connectedAtMs else -1L + log("connect down (uptime ends${sessionHeldSuffix(heldMs)})", + com.noop.testcentre.TestDomain.CONNECTION) log("reconnect n=$connReconnectCount reason=$reason", com.noop.testcentre.TestDomain.CONNECTION) } else { log("reconnect n=$connReconnectCount failedConnect reason=$reason", com.noop.testcentre.TestDomain.CONNECTION) diff --git a/android/app/src/test/java/com/noop/ble/ConnectionDownReasonTest.kt b/android/app/src/test/java/com/noop/ble/ConnectionDownReasonTest.kt new file mode 100644 index 0000000000..59e527b292 --- /dev/null +++ b/android/app/src/test/java/com/noop/ble/ConnectionDownReasonTest.kt @@ -0,0 +1,73 @@ +package com.noop.ble + +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * #1020 — the connection-down trace has to say WHICH path dropped the link. + * + * The report that prompted this showed nearly 2,800 reconnects, every one `reason=localTerminate`. + * That token is GATT status 22, and at least five of NOOP's own paths produce it — the bond watchdog, + * the keep-alive stall bounce, a user disconnect, releaseStrap, and a safeGatt teardown after a dead + * binder. With one string for five causes the log could not be diagnosed at all; the comment beside it + * in the source could only guess ("e.g. #971 bond watchdog"). + */ +class ConnectionDownReasonTest { + + /** The regression itself: a local teardown now names its origin. */ + @Test fun aLocalTeardownNamesThePathThatCausedIt() { + assertEquals( + "localTerminate via=bondWatchdog", + WhoopBleClient.connectionDownReason(22, "bondWatchdog"), + ) + assertEquals( + "localTerminate via=keepAliveStall", + WhoopBleClient.connectionDownReason(22, "keepAliveStall"), + ) + } + + /** + * A safeGatt teardown carries the failing operation with it, so `gattThrow:requestMtu` is + * distinguishable from `gattThrow:readRemoteRssi` — those point at different bugs. + */ + @Test fun aGattThrowCarriesTheOperationThatFailed() { + assertEquals( + "localTerminate via=gattThrow:requestMtu", + WhoopBleClient.connectionDownReason(22, "gattThrow:requestMtu"), + ) + } + + /** + * `unknown` is a real signal, not a fallback to ignore: the drop was local but no tagged path + * claimed it, which means a teardown route exists that this change missed. + */ + @Test fun anUntaggedLocalTeardownReadsAsUnknown() { + assertEquals("localTerminate via=unknown", WhoopBleClient.connectionDownReason(22, null)) + } + + /** A remote timeout is not ours, so no origin is attached — the strap or the radio dropped it. */ + @Test fun aRemoteTimeoutIsNotAttributedToUs() { + assertEquals("connectionTimeout", WhoopBleClient.connectionDownReason(8, "bondWatchdog")) + } + + /** Any other status passes through verbatim, as before. */ + @Test fun otherStatusesPassThrough() { + assertEquals("status19", WhoopBleClient.connectionDownReason(19, null)) + assertEquals("status133", WhoopBleClient.connectionDownReason(133, "userDisconnect")) + } + + /** The session length is what separates a 7-second bond watchdog from a 120-second stall bounce. */ + @Test fun theSessionLengthIsReportedToATenthOfASecond() { + assertEquals(" after 6.8s", WhoopBleClient.sessionHeldSuffix(6_800)) + assertEquals(" after 120.0s", WhoopBleClient.sessionHeldSuffix(120_000)) + assertEquals(" after 0.4s", WhoopBleClient.sessionHeldSuffix(432)) + } + + /** + * Unknown session start yields no suffix rather than a misleading zero — "after 0.0s" would read as + * an instant drop, which is a different diagnosis from "we do not know". + */ + @Test fun anUnknownSessionStartYieldsNoSuffix() { + assertEquals("", WhoopBleClient.sessionHeldSuffix(-1)) + } +} From 7e8370b6c23cbba9efdcba57719992c823398093 Mon Sep 17 00:00:00 2001 From: ryanbr Date: Sat, 1 Aug 2026 11:53:58 -0700 Subject: [PATCH 2/5] Put the connect-down session duration on both platforms (#1020) The duration half of the teardown diagnostics was Android-only. Both platforms emit the same "connect down (uptime ends)" line and both feed the shared ConnectionReadout parser, so a one-sided suffix would have split a trace format that is meant to read identically whichever platform wrote it. sessionHeldSuffix moves into ConnectionTrace on both sides, next to the other shared trace formatters, rather than living in the Android BLE client. That also puts the Swift half under Packages/, where CI compiles and runs it. The via= origin stays Android-only: it names which local path tore the link down for GATT status 22, and CoreBluetooth has no equivalent status to attribute. Parser is unaffected - both sides match on contains("connect down"), so the appended suffix passes through. --- .../StrandAnalytics/ConnectionReadout.swift | 17 ++++++++++ .../SessionHeldSuffixTests.swift | 31 +++++++++++++++++++ Strand/BLE/BLEManager.swift | 13 +++++++- .../com/noop/analytics/ConnectionReadout.kt | 16 ++++++++++ .../main/java/com/noop/ble/WhoopBleClient.kt | 12 +------ .../com/noop/ble/ConnectionDownReasonTest.kt | 9 +++--- 6 files changed, 82 insertions(+), 16 deletions(-) create mode 100644 Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SessionHeldSuffixTests.swift diff --git a/Packages/StrandAnalytics/Sources/StrandAnalytics/ConnectionReadout.swift b/Packages/StrandAnalytics/Sources/StrandAnalytics/ConnectionReadout.swift index 7e06e1fa13..ab13b16bc6 100644 --- a/Packages/StrandAnalytics/Sources/StrandAnalytics/ConnectionReadout.swift +++ b/Packages/StrandAnalytics/Sources/StrandAnalytics/ConnectionReadout.swift @@ -13,6 +13,23 @@ import Foundation // counts, durations and ISO dates only. No em-dashes. The Kotlin twin is ConnectionReadout.kt. public enum ConnectionTrace { + /// The ` after s` suffix on a `connect down` trace line, or empty when the session start is + /// unknown (#1020). + /// + /// A session's length separates the causes of a drop at a glance: a bond watchdog fires seconds in, + /// a keep-alive stall bounce minutes in, a radio drop anywhere. The bare `connect down (uptime ends)` + /// could not distinguish them, which is why a report of thousands of reconnects needed a round trip + /// before anyone could start on it. + /// + /// An unknown start yields NO suffix rather than `after 0.0s` — "instant drop" and "we do not know" + /// are different diagnoses. Locale-fixed so a decimal comma cannot follow the phone language into a + /// log people paste into issues. Twin of the Kotlin `ConnectionTrace.sessionHeldSuffix`. + public static func sessionHeldSuffix(millis: Int) -> String { + guard millis >= 0 else { return "" } + return " after " + String(format: "%.1f", locale: Locale(identifier: "en_US_POSIX"), + Double(millis) / 1000.0) + "s" + } + /// The CLOCK-DRIFT summary line (#767 / #754 cluster): the strap-reported banked-record window /// [oldest, newest] against the wall clock, with a FUTURE-DATE flag when the strap's newest record is diff --git a/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SessionHeldSuffixTests.swift b/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SessionHeldSuffixTests.swift new file mode 100644 index 0000000000..5bc4da1f87 --- /dev/null +++ b/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SessionHeldSuffixTests.swift @@ -0,0 +1,31 @@ +import XCTest +@testable import StrandAnalytics + +/// #1020 — the ` after s` suffix on a `connect down` trace line. Twin of the Kotlin +/// `ConnectionDownReasonTest`'s duration cases; the same inputs, because a log pasted into an issue must +/// read the same whichever platform wrote it. +/// +/// The reason this exists: a report of thousands of reconnects arrived with a log whose every drop read +/// `connect down (uptime ends)`. Nothing in that line said whether the sessions lasted seconds or +/// minutes, which is the first split between a bond watchdog firing and a radio-side drop, so the issue +/// needed a round trip before anyone could start on it. +final class SessionHeldSuffixTests: XCTestCase { + + /// The ordinary case, to a tenth: enough to separate a watchdog from a stall, no false precision. + func testAHeldSessionReportsItsLengthToATenth() { + XCTAssertEqual(ConnectionTrace.sessionHeldSuffix(millis: 6_800), " after 6.8s") + XCTAssertEqual(ConnectionTrace.sessionHeldSuffix(millis: 120_000), " after 120.0s") + XCTAssertEqual(ConnectionTrace.sessionHeldSuffix(millis: 432), " after 0.4s") + } + + /// An unknown start yields NO suffix rather than `after 0.0s` — "dropped instantly" and "we do not + /// know when it came up" are different diagnoses and must not render alike. + func testAnUnknownStartAddsNothing() { + XCTAssertEqual(ConnectionTrace.sessionHeldSuffix(millis: -1), "") + } + + /// A genuine instant drop is still reported: zero is a measurement, unlike the negative sentinel. + func testAnInstantDropIsStillReported() { + XCTAssertEqual(ConnectionTrace.sessionHeldSuffix(millis: 0), " after 0.0s") + } +} diff --git a/Strand/BLE/BLEManager.swift b/Strand/BLE/BLEManager.swift index 927706e0db..6b6c0a2daf 100644 --- a/Strand/BLE/BLEManager.swift +++ b/Strand/BLE/BLEManager.swift @@ -617,6 +617,12 @@ public final class BLEManager: NSObject, ObservableObject { private var connectAttemptStartedAt: Date? /// Count of INVOLUNTARY reconnects this run (a drop or failed-connect that was not user-initiated), /// surfaced as the reconnect-churn count. Reset by an intentional disconnect. + /// When the current session reached connected, for the `connect down` duration readout (#1020). + /// A session's length separates the causes of a drop at a glance — a bond watchdog fires seconds + /// in, a stall bounce minutes in, a radio drop anywhere. `nil` when not connected. Android twin: + /// `WhoopBleClient.connectedAtMs`. + private var connSessionStartedAt: Date? + private var connReconnectCount = 0 /// #126 false-alarm guard: tracks CONSECUTIVE console-only completed syncs so the "clock has lost /// sync" banner only fires on sustained emptiness, not a single transient empty cycle on a healthy strap. @@ -3924,6 +3930,7 @@ extension BLEManager: @preconcurrency CBCentralManagerDelegate { if TestCentre.active(.connection) { let nowUnix = Int(Date().timeIntervalSince1970) let latencyMs = connectAttemptStartedAt.map { Int(Date().timeIntervalSince($0) * 1000) } + connSessionStartedAt = Date() // #1020: for the session-duration readout on the way down state.append(log: "connect up gen=\(connectGeneration) " + "latencyMs=\(latencyMs.map(String.init) ?? "?") uptimeStart=\(nowUnix)", domain: .connection) } @@ -4103,7 +4110,11 @@ extension BLEManager: @preconcurrency CBCentralManagerDelegate { if TestCentre.active(.connection) { let reason = (error as? CBError)?.code == .connectionTimeout ? "connectionTimeout" : connErrorToken(error) - state.append(log: "connect down (uptime ends)", domain: .connection) + // #1020: the session's length separates the causes of a drop at a glance. Same suffix the + // Android twin emits, so the shared ConnectionReadout parser sees one format. + let held = ConnectionTrace.sessionHeldSuffix( + millis: connSessionStartedAt.map { Int(Date().timeIntervalSince($0) * 1000) } ?? -1) + state.append(log: "connect down (uptime ends\(held))", domain: .connection) state.append(log: "reconnect n=\(connReconnectCount) reason=\(reason)", domain: .connection) } DispatchQueue.main.asyncAfter(deadline: .now() + 3) { [weak self] in diff --git a/android/app/src/main/java/com/noop/analytics/ConnectionReadout.kt b/android/app/src/main/java/com/noop/analytics/ConnectionReadout.kt index 6246889829..2fdfd976de 100644 --- a/android/app/src/main/java/com/noop/analytics/ConnectionReadout.kt +++ b/android/app/src/main/java/com/noop/analytics/ConnectionReadout.kt @@ -12,6 +12,22 @@ import java.util.TimeZone // Swift line shapes so a shared report reads identically on either platform. object ConnectionTrace { + /** + * The ` after s` suffix on a `connect down` trace line, or empty when the session start is + * unknown (#1020). + * + * A session's length separates the causes of a drop at a glance: a bond watchdog fires seconds in, a + * keep-alive stall bounce minutes in, a radio drop anywhere. The bare `connect down (uptime ends)` + * could not distinguish them, which is why a report of thousands of reconnects needed a round trip + * before anyone could start on it. + * + * An unknown start yields NO suffix rather than `after 0.0s` — "instant drop" and "we do not know" + * are different diagnoses. Locale-fixed so a decimal comma cannot follow the phone language into a + * log people paste into issues. Twin of the Swift `ConnectionTrace.sessionHeldSuffix`. + */ + fun sessionHeldSuffix(millis: Long): String = + if (millis < 0L) "" else " after ${"%.1f".format(java.util.Locale.US, millis / 1000.0)}s" + /** * The CLOCK-DRIFT summary line (#767 / #754 cluster): the strap-reported banked-record window diff --git a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt index 5ee5c1e330..49d2169dc5 100644 --- a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt +++ b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt @@ -553,16 +553,6 @@ class WhoopBleClient( else -> "status$status" } - /** - * The ` after s` suffix on `connect down` (#1020), or empty when the session start is unknown. - * - * Separates the culprits at a glance: a bond watchdog fires seconds in, the keep-alive stall - * bounce two minutes in, a radio drop anywhere. Locale.US so the decimal point does not follow the - * phone language into a log people paste into issues. - */ - fun sessionHeldSuffix(heldMs: Long): String = - if (heldMs < 0L) "" else " after ${"%.1f".format(java.util.Locale.US, heldMs / 1000.0)}s" - /** Pure battery-adaptive gate (#477), unit-testable without a BLE stack. Keyed on the STRAP's * battery (WHOOP/Oura/Fitbit): the lever is ARMED by [thresholdPct] > 0 (the Settings slider is * 10–30; 0 disables it) and engages while the strap is DISCHARGING at/below [thresholdPct]. The @@ -7111,7 +7101,7 @@ class WhoopBleClient( val reason = connectionDownReason(status, lastLocalTeardown) if (wasConnected) { val heldMs = if (connectedAtMs > 0L) System.currentTimeMillis() - connectedAtMs else -1L - log("connect down (uptime ends${sessionHeldSuffix(heldMs)})", + log("connect down (uptime ends${com.noop.analytics.ConnectionTrace.sessionHeldSuffix(heldMs)})", com.noop.testcentre.TestDomain.CONNECTION) log("reconnect n=$connReconnectCount reason=$reason", com.noop.testcentre.TestDomain.CONNECTION) } else { diff --git a/android/app/src/test/java/com/noop/ble/ConnectionDownReasonTest.kt b/android/app/src/test/java/com/noop/ble/ConnectionDownReasonTest.kt index 59e527b292..b10d540eed 100644 --- a/android/app/src/test/java/com/noop/ble/ConnectionDownReasonTest.kt +++ b/android/app/src/test/java/com/noop/ble/ConnectionDownReasonTest.kt @@ -1,5 +1,6 @@ package com.noop.ble +import com.noop.analytics.ConnectionTrace import org.junit.Assert.assertEquals import org.junit.Test @@ -58,9 +59,9 @@ class ConnectionDownReasonTest { /** The session length is what separates a 7-second bond watchdog from a 120-second stall bounce. */ @Test fun theSessionLengthIsReportedToATenthOfASecond() { - assertEquals(" after 6.8s", WhoopBleClient.sessionHeldSuffix(6_800)) - assertEquals(" after 120.0s", WhoopBleClient.sessionHeldSuffix(120_000)) - assertEquals(" after 0.4s", WhoopBleClient.sessionHeldSuffix(432)) + assertEquals(" after 6.8s", ConnectionTrace.sessionHeldSuffix(6_800)) + assertEquals(" after 120.0s", ConnectionTrace.sessionHeldSuffix(120_000)) + assertEquals(" after 0.4s", ConnectionTrace.sessionHeldSuffix(432)) } /** @@ -68,6 +69,6 @@ class ConnectionDownReasonTest { * an instant drop, which is a different diagnosis from "we do not know". */ @Test fun anUnknownSessionStartYieldsNoSuffix() { - assertEquals("", WhoopBleClient.sessionHeldSuffix(-1)) + assertEquals("", ConnectionTrace.sessionHeldSuffix(-1)) } } From 21c16c5775272479f3cb059ed8099ebe8feacbd9 Mon Sep 17 00:00:00 2001 From: ryanbr Date: Sat, 1 Aug 2026 12:03:54 -0700 Subject: [PATCH 3/5] Stamp the session start and clear the teardown origin outside the test gate (#1020) Both were assigned inside `if (testCentre.active(CONNECTION))`, which is wrong for the way the mode is actually used: people switch Test Centre on AFTER something looks wrong, so the mode is usually off at connect time and on by the time the drop is logged. That left two stale reads. The session start was either unset (no suffix, merely unhelpful) or still held a PREVIOUS session's value, reporting a duration spanning the gap between two sessions as though it were one. Worse on Android: noteLocalTeardown() writes are ungated but the clear was not, so an untagged local teardown could print a stale via=bondWatchdog where via=unknown is the honest answer - defeating the signal this change exists to provide. Both are field assignments rather than log lines, so moving them above the gate keeps the mode-off cost at nothing. The log lines stay gated. The bondLoop-paused branch still emits a bare `connect down (uptime ends)` on both platforms. Left as is: no suffix already means "start unknown", which is the truthful reading for a terminal give-up state. --- Strand/BLE/BLEManager.swift | 6 +++++- android/app/src/main/java/com/noop/ble/WhoopBleClient.kt | 9 +++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/Strand/BLE/BLEManager.swift b/Strand/BLE/BLEManager.swift index 6b6c0a2daf..9d52ff861c 100644 --- a/Strand/BLE/BLEManager.swift +++ b/Strand/BLE/BLEManager.swift @@ -3924,13 +3924,17 @@ extension BLEManager: @preconcurrency CBCentralManagerDelegate { } lastDataAt = Date() log("Connected — discovering services") + // #1020: stamped OUTSIDE the gate below. Test Centre is usually switched on AFTER something looks + // wrong, so a stamp taken only when the mode happened to be on at connect time would either be + // missing or, worse, still hold a PREVIOUS session's start - reporting a duration that spans the + // gap between them. A field assignment, not a log line, so it costs nothing when the mode is off. + connSessionStartedAt = Date() // Connection test mode: report the connect latency + the uptime-start marker the readout reads. // Gated zero-cost: the .connection bool is read before any string is built, so this is a no-op // when the mode is off. Behaviour-neutral diagnostics only - the connect flow above is unchanged. if TestCentre.active(.connection) { let nowUnix = Int(Date().timeIntervalSince1970) let latencyMs = connectAttemptStartedAt.map { Int(Date().timeIntervalSince($0) * 1000) } - connSessionStartedAt = Date() // #1020: for the session-duration readout on the way down state.append(log: "connect up gen=\(connectGeneration) " + "latencyMs=\(latencyMs.map(String.init) ?? "?") uptimeStart=\(nowUnix)", domain: .connection) } diff --git a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt index 49d2169dc5..3400deff5e 100644 --- a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt +++ b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt @@ -4460,11 +4460,16 @@ class WhoopBleClient( // Connection test mode: report the connect latency + the uptime-start marker the readout // reads. Gated zero-cost (the CONNECTION bool is read before any string is built). // Behaviour-neutral diagnostics only - the connect flow below is unchanged. Twin of macOS. + // #1020: both stamped OUTSIDE the gate. Test Centre is usually switched on AFTER + // something looks wrong, and noteLocalTeardown() writes are themselves ungated - so a + // clear that only ran when the mode was already on would let a PREVIOUS session's origin + // survive, printing a stale via=bondWatchdog where via=unknown is the honest answer. + // Field assignments, not log lines: no cost when the mode is off. + connectedAtMs = System.currentTimeMillis() + lastLocalTeardown = null if (testCentre.active(com.noop.testcentre.TestDomain.CONNECTION)) { val nowUnix = System.currentTimeMillis() / 1000L val latencyMs = connectAttemptStartedAtMs?.let { System.currentTimeMillis() - it } - connectedAtMs = System.currentTimeMillis() // #1020: for the session-duration readout - lastLocalTeardown = null // #1020: a new session, no teardown origin yet log("connect up gen=$connectGeneration latencyMs=${latencyMs ?: "?"} uptimeStart=$nowUnix", com.noop.testcentre.TestDomain.CONNECTION) } From dedb947ed0bb35b0dfdc7b04f55d149c6ceaacae Mon Sep 17 00:00:00 2001 From: ryanbr Date: Sat, 1 Aug 2026 12:13:08 -0700 Subject: [PATCH 4/5] Pin the emitter/parser contract and fix the field docs (#1020) Regression sweep turned up three things. The suffix was appended on the strength of ConnectionReadout matching with contains("connect down"), but no test fed the parser the format the emitter actually produces - both fixtures still carried only the bare line. Added the suffixed case on both platforms, so the two halves can no longer drift apart silently. The Swift field was inserted between connReconnectCount's doc comment and its declaration, leaving that field undocumented and prepending an unrelated sentence to the new one. Restored. Both field docs claimed an invariant nothing maintains - "nil when not connected" / "0 when not connected". Neither is ever cleared on the way down. Documented what is actually true, including why the stamp is ungated and the guard a reader needs. --- .../ConnectionReadoutTests.swift | 11 +++++++++++ Strand/BLE/BLEManager.swift | 12 ++++++++---- .../src/main/java/com/noop/ble/WhoopBleClient.kt | 4 +++- .../com/noop/analytics/ConnectionReadoutTest.kt | 13 +++++++++++++ 4 files changed, 35 insertions(+), 5 deletions(-) diff --git a/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/ConnectionReadoutTests.swift b/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/ConnectionReadoutTests.swift index be0f8d1853..e0465378f3 100644 --- a/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/ConnectionReadoutTests.swift +++ b/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/ConnectionReadoutTests.swift @@ -93,6 +93,17 @@ final class ConnectionReadoutTests: XCTestCase { XCTAssertEqual(ConnectionReadout.uptimeLabel(taggedTail: tail, nowUnix: 5000), "not connected") } + /// #1020: the emitter appends a session duration, so the line the parser actually receives is no + /// longer the bare one above. Pinned because the two are edited independently — the suffix was added + /// on the strength of this match being a `contains`, and nothing was asserting that. + func testUptimeLabelDownWithASessionDuration() { + let tail = [ + "[connection] connect up gen=1 latencyMs=420 uptimeStart=1000", + "[connection] connect down (uptime ends after 6.8s)", + ] + XCTAssertEqual(ConnectionReadout.uptimeLabel(taggedTail: tail, nowUnix: 5000), "not connected") + } + func testUptimeLabelEmptyTail() { XCTAssertEqual(ConnectionReadout.uptimeLabel(taggedTail: [], nowUnix: 5000), "not connected") } diff --git a/Strand/BLE/BLEManager.swift b/Strand/BLE/BLEManager.swift index 9d52ff861c..52170a224a 100644 --- a/Strand/BLE/BLEManager.swift +++ b/Strand/BLE/BLEManager.swift @@ -617,13 +617,17 @@ public final class BLEManager: NSObject, ObservableObject { private var connectAttemptStartedAt: Date? /// Count of INVOLUNTARY reconnects this run (a drop or failed-connect that was not user-initiated), /// surfaced as the reconnect-churn count. Reset by an intentional disconnect. + private var connReconnectCount = 0 /// When the current session reached connected, for the `connect down` duration readout (#1020). /// A session's length separates the causes of a drop at a glance — a bond watchdog fires seconds - /// in, a stall bounce minutes in, a radio drop anywhere. `nil` when not connected. Android twin: - /// `WhoopBleClient.connectedAtMs`. + /// in, a stall bounce minutes in, a radio drop anywhere. + /// + /// Set on EVERY connect, not only when the connection test mode is active: the mode is usually + /// switched on after something already looks wrong, so a stamp taken under the gate would carry a + /// previous session's start. Never cleared on the way down, so read it only where a session is known + /// to have been held (`didDisconnectPeripheral`, which fires only for a peripheral that connected). + /// Android twin: `WhoopBleClient.connectedAtMs`. private var connSessionStartedAt: Date? - - private var connReconnectCount = 0 /// #126 false-alarm guard: tracks CONSECUTIVE console-only completed syncs so the "clock has lost /// sync" banner only fires on sustained emptiness, not a single transient empty cycle on a healthy strap. private var emptySyncTracker = EmptySyncTracker() diff --git a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt index 3400deff5e..de9652eb42 100644 --- a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt +++ b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt @@ -4005,7 +4005,9 @@ class WhoopBleClient( @Volatile private var lastLocalTeardown: String? = null /** Wall clock when the current session reached STATE_CONNECTED, for the session-duration readout. - * 0 when not connected. @Volatile for the same reason as [lastLocalTeardown]. */ + * 0 until the first connect; set on EVERY connect (not gated on the test mode, which is usually + * switched on only after something looks wrong) and never cleared on the way down, so read it only + * under a `wasConnected` guard. @Volatile for the same reason as [lastLocalTeardown]. */ @Volatile private var connectedAtMs = 0L /** Record which local path is about to drop the link. Cheap and unconditional: the string is a diff --git a/android/app/src/test/java/com/noop/analytics/ConnectionReadoutTest.kt b/android/app/src/test/java/com/noop/analytics/ConnectionReadoutTest.kt index eda8bda1e1..03bdf84ba4 100644 --- a/android/app/src/test/java/com/noop/analytics/ConnectionReadoutTest.kt +++ b/android/app/src/test/java/com/noop/analytics/ConnectionReadoutTest.kt @@ -98,6 +98,19 @@ class ConnectionReadoutTest { assertEquals("not connected", ConnectionReadout.uptimeLabel(tail, nowUnix = 5000)) } + /** + * #1020: the emitter appends a session duration, so the line the parser actually receives is no + * longer the bare one above. Pinned because the two are edited independently - the suffix was added + * on the strength of this match being a `contains`, and nothing was asserting that. + */ + @Test fun uptimeLabelDownWithASessionDuration() { + val tail = listOf( + "[connection] connect up gen=1 latencyMs=420 uptimeStart=1000", + "[connection] connect down (uptime ends after 6.8s)", + ) + assertEquals("not connected", ConnectionReadout.uptimeLabel(tail, nowUnix = 5000)) + } + @Test fun uptimeLabelEmptyTail() { assertEquals("not connected", ConnectionReadout.uptimeLabel(emptyList(), nowUnix = 5000)) } From 77b1300cdfe14315bbf704b283248226ff56c6c4 Mon Sep 17 00:00:00 2001 From: ryanbr Date: Sat, 1 Aug 2026 12:21:12 -0700 Subject: [PATCH 5/5] Correct the cost claim on noteLocalTeardown (#1020) The doc said the origin string is "a constant at every call site". Four of the five are literals; safeGatt interpolates the failing op name. That allocation is noise against the teardown it precedes, but the doc should say what is true. Also records what the sweep established: none of the five sites is on a per-record path. --- android/app/src/main/java/com/noop/ble/WhoopBleClient.kt | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt index de9652eb42..c980ed8c6d 100644 --- a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt +++ b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt @@ -4010,8 +4010,11 @@ class WhoopBleClient( * under a `wasConnected` guard. @Volatile for the same reason as [lastLocalTeardown]. */ @Volatile private var connectedAtMs = 0L - /** Record which local path is about to drop the link. Cheap and unconditional: the string is a - * constant at every call site, and knowing the origin is worth more than the trace being gated. */ + /** Record which local path is about to drop the link. Unconditional rather than gated on the test + * mode, because the clear in the connect path is ungated too - gating one and not the other is what + * let a previous session's origin survive. Four of the five call sites pass a literal; `safeGatt` + * interpolates the failing op, but that is an exception path where a String allocation is noise + * against the teardown it is about to perform. None of the five is on a per-record path. */ private fun noteLocalTeardown(origin: String) { lastLocalTeardown = origin } /** Consecutive involuntary reconnect attempts, feeding the capped-exponential [ReconnectBackoff]