From da9c2d4e3db0a3b2781c3419fe86dcffe3e1c118 Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Mon, 28 Sep 2026 23:33:32 +1300 Subject: [PATCH] refactor(data): one HR union fingerprint, not two that were free to disagree WhoopRepository carried two functions computing the same value. Issue #2566. hrUnionFingerprint/3 line 1185, from #2293 hrFingerprintWindowUnion/3 line 728, which I added in #2543 Same signature, same rawWhoopSourceIds walk, same COUNT plus MAX per id. They differed only in how they punctuated the result: "id=count:maxTs" joined with a comma against "id:count:maxTs" joined with a pipe. Neither encoding is observable. StressLensCache is an in-memory object holding a single key, and PhysiologicalStepCycleEngine's loadCache is a plain HashMap field of a private data class. Nothing persists either string or sends it anywhere, so both doc comments were right that the format is free to change. The redundancy was not the risk. Two witnesses of one fact that are allowed to drift is the risk, and the fact they witness is whether cached scoring work can be reused. Teach one union to walk an extra id or carry a third aggregate and the other keeps answering the older question, silently. So: hrFingerprintWindowUnion is gone, the daytime stress lens memo reads hrUnionFingerprint, and hrUnionFingerprint now calls hrFingerprintWindow for each id rather than repeating the two DAO reads inline. One union, one encoding, one per-id read. The cycle load cache keeps the exact value it had. Strand/Data/Repository.swift referenced the deleted function as its Kotlin twin, so that pointer moves to the survivor. Both sides now also say what kind of twin this is: each compares its own value against its own previous value, neither is persisted or transmitted, and the two encode the same facts differently. There is no byte-identity contract here and no oracle asserting one. Without that note the next reader could take "twin" for the usual byte-level pair this repository means by the word and align the encodings believing they were repairing something. Neither union had a single test, which is how a duplicate survived review. HrUnionFingerprintTest pins the property that motivated the union at all: a row landing under an alias id or under the canonical id must move the witness even when the active id is untouched (#908). A fingerprint keyed on the active id alone passes a naive equality check and fails these. The stubbed DAO throws on any method other than the two index aggregates, so a row fetch creeping into the cheap path fails too. Mutation-verified rather than assumed. Replacing the union walk with the bare active id reddens 4 of 6; dropping maxTs from each entry reddens a different 4 of 6. The survivors are explainable in both cases: stableWhenNothingChanged only asserts determinism, and canonicalActiveIsNotDuplicated sets the active id to the canonical one, where the union and the single id genuinely coincide. --- Strand/Data/Repository.swift | 5 +- Tools/parity_twin_map.json | 4 +- .../java/com/noop/data/WhoopRepository.kt | 49 +++--- .../java/com/noop/ui/StressDaytimeMode.kt | 2 +- .../com/noop/data/HrUnionFingerprintTest.kt | 154 ++++++++++++++++++ 5 files changed, 184 insertions(+), 30 deletions(-) create mode 100644 android/app/src/test/java/com/noop/data/HrUnionFingerprintTest.kt diff --git a/Strand/Data/Repository.swift b/Strand/Data/Repository.swift index 6124cbef0a..9d5c4cc837 100644 --- a/Strand/Data/Repository.swift +++ b/Strand/Data/Repository.swift @@ -1187,7 +1187,10 @@ final class Repository: ObservableObject { /// Cost is one COUNT plus one MAX per id, straight over the `(deviceId, ts)` index with no rows /// materialized, against the per-day row fetches a caller would otherwise repeat. Compared only to /// itself in memory, so the format is free to change. Kotlin twin: - /// `WhoopRepository.hrFingerprintWindowUnion`. + /// `WhoopRepository.hrUnionFingerprint`, which is a twin in ROLE only: each side compares its own + /// value against its own previous value, neither is persisted or sent anywhere, and the two encode + /// the same facts differently. There is no byte-identity contract here and no oracle asserting one, + /// so do not "align" the encodings on the assumption that there is. func hrFingerprintUnion(from: Int, to: Int) async -> String { guard let store = await ensureStore() else { return "" } var parts: [String] = [] diff --git a/Tools/parity_twin_map.json b/Tools/parity_twin_map.json index 641d9fc4af..3a4795b275 100644 --- a/Tools/parity_twin_map.json +++ b/Tools/parity_twin_map.json @@ -19,7 +19,7 @@ }, "authority": { "files": {"count": 505, "sha256": "581ec0dcab0d8486ce113646cb5cbad71769353eb3aef75f45486e03591733a0"}, - "functions": {"count": 4508, "sha256": "e0669951cb737a12aa4e67b8a52c89f9ed407799902e8dfc97c63790c0e9d665"}, + "functions": {"count": 4507, "sha256": "ce9326a53287c00561b1c642cfea7bca0132f8f89bbb9d45711f4c294518d532"}, "properties": {"count": 465, "sha256": "64fcd18c85bf33a4738d154d45ec86967fdc974e78a448d16e9504d9b13609eb"}, "constants": {"count": 1965, "sha256": "bc257e17e354a48ebbade1e5035aaa41caf21784a119a83488e17270573724da"}, "file_pairs": {"count": 75, "sha256": "d581f60241ec523c595a7e5ff123f7987caee501d8be4ebf6df5ba105e26fe1f"}, @@ -27,7 +27,7 @@ "property_pairs": {"count": 150, "sha256": "be7168d309c8a9cf5f3f25438c028cc60ab64202bb6e9c09a6c553e3699ad0e2"}, "constant_pairs": {"count": 683, "sha256": "c3452cb90df3bda98100154f7f2d109b4b473f6358d6ec1d8476ea9fc183eaa3"}, "unpaired_files": {"count": 377, "sha256": "14e70f77d7e11b4eae3b8412cc493c1aa1bf24dfe8fbc5eca71f81b5daf5253f"}, - "unpaired_functions": {"count": 4109, "sha256": "1fd1fd39d23fcf7624e0dd3de1cb0c5f7245bf8dc3c5f385a4f2091bd1fb7726"}, + "unpaired_functions": {"count": 4108, "sha256": "0aad0d7db52de523a86a5e7438ed4e48bc7a4b54aa1300eb3b56ce907c4616ac"}, "unpaired_properties": {"count": 165, "sha256": "fe609dd25384cda6de1616d36753fed68431b1a1f9a73b6fd664c7ff35089955"}, "unpaired_constants": {"count": 599, "sha256": "b7467c65623ecc296604558a639d62eee2e00874c56d21acc7ec6d3b6264fdc7"} } diff --git a/android/app/src/main/java/com/noop/data/WhoopRepository.kt b/android/app/src/main/java/com/noop/data/WhoopRepository.kt index ede27fa1d3..60e4da481a 100644 --- a/android/app/src/main/java/com/noop/data/WhoopRepository.kt +++ b/android/app/src/main/java/com/noop/data/WhoopRepository.kt @@ -714,29 +714,6 @@ class WhoopRepository( suspend fun hrFingerprintWindow(deviceId: String, from: Long, to: Long): Pair = Pair(dao.countHrInWindow(deviceId, from, to), dao.maxHrTsInWindow(deviceId, from, to)) - /** The same HR fingerprint over the UNION [hrSamplesUnion] reads, as one opaque string. - * - * A fingerprint narrower than the read it guards is worse than none: it would serve a cached result - * after a backfill landed rows under an alias id, which is exactly the id set [hrSamplesUnion] exists - * to cover (#908, a re-added strap banking under its own fresh id). So this walks the same ids rather - * than the bare [activeDeviceId]. - * - * Cost is one COUNT plus one MAX per id, index range walks that materialise no rows, against the full - * per-day row fetches a caller would otherwise repeat. Compared only to itself in memory, so the - * format is free to change. - */ - suspend fun hrFingerprintWindowUnion(activeDeviceId: String, from: Long, to: Long): String { - // An explicit loop rather than joinToString: the per-id read suspends and that builder's lambda - // is not a suspend function. - val parts = StringBuilder() - for (id in rawWhoopSourceIds(activeDeviceId)) { - val (count, maxTs) = hrFingerprintWindow(id, from, to) - if (parts.isNotEmpty()) parts.append('|') - parts.append(id).append(':').append(count).append(':').append(maxTs) - } - return parts.toString() - } - /** Whether [deviceId] has ANY heart-rate row in the window, as a scalar EXISTS rather than a fetched * row. The day-owner resolver's per-candidate-per-day probe; see [WhoopDao.hasHrInWindow]. */ suspend fun hasHrInWindow(deviceId: String, from: Long, to: Long): Boolean { @@ -1180,12 +1157,32 @@ class WhoopRepository( if (deviceIds.isEmpty()) emptyList() else mergeHrByTs(deviceIds.map { dao.hrSamples(it, from, to, limit) }) - /** Count and newest timestamp of measured HR per source [hrSamplesUnion] reads, as one string: an - * index-only witness of whether a window's heart rate changed, without fetching a row. */ + /** Count and newest timestamp of measured HR per source [hrSamplesUnion] reads, as one opaque string: + * an index-only witness of whether a window's heart rate changed, without fetching a row. + * + * A fingerprint narrower than the read it guards is worse than none: it would serve a cached result + * after a backfill landed rows under an alias id, which is exactly the id set [hrSamplesUnion] exists + * to cover (#908, a re-added strap banking under its own fresh id). So this walks the same ids rather + * than the bare [activeDeviceId]. + * + * Cost is one COUNT plus one MAX per id via [hrFingerprintWindow], index range walks that materialise + * no rows, against the full per-day row fetches a caller would otherwise repeat. Compared only to + * itself in memory, so the format is free to change, and no caller persists it. The Swift + * `Repository.hrFingerprintUnion` is a twin in ROLE only, encoding the same facts differently; there + * is no byte-identity contract between them and no oracle asserting one. + * + * The single union witness for both callers (#2566): the cycle load cache in + * [com.noop.analytics.PhysiologicalStepCycleEngine] and the daytime stress lens memo in + * [com.noop.ui.selectedDaytimeStressMode]. Two of these that were free to disagree is what #2566 + * removed, so route a new caller here rather than adding a third. + */ suspend fun hrUnionFingerprint(activeDeviceId: String, from: Long, to: Long): String { + // An explicit loop rather than joinToString: the per-id read suspends and that builder's lambda + // is not a suspend function. val parts = ArrayList() for (id in rawWhoopSourceIds(activeDeviceId)) { - parts += "$id=${dao.countHrInWindow(id, from, to)}:${dao.maxHrTsInWindow(id, from, to)}" + val (count, maxTs) = hrFingerprintWindow(id, from, to) + parts += "$id=$count:$maxTs" } return parts.joinToString(",") } diff --git a/android/app/src/main/java/com/noop/ui/StressDaytimeMode.kt b/android/app/src/main/java/com/noop/ui/StressDaytimeMode.kt index 383dc59df8..36f7697001 100644 --- a/android/app/src/main/java/com/noop/ui/StressDaytimeMode.kt +++ b/android/app/src/main/java/com/noop/ui/StressDaytimeMode.kt @@ -103,7 +103,7 @@ internal suspend fun selectedDaytimeStressMode( val spanFrom = stressLocalDayWindow(todayLocalDay.minusDays(baselineHistoryDays.toLong()), zone).fromEpochSecond val spanTo = stressLocalDayWindow(todayLocalDay.minusDays(1L), zone).toEpochSecondInclusive val cacheKey = "$deviceId|$todayLocalDay|$zone|" + - repo.hrFingerprintWindowUnion(deviceId, spanFrom, spanTo) + repo.hrUnionFingerprint(deviceId, spanFrom, spanTo) return StressLensCache.resolve(cacheKey) { val aggregates = ArrayList(baselineHistoryDays) // Oldest -> newest so the EWMA fold replays history in order. Reduce each day immediately: the diff --git a/android/app/src/test/java/com/noop/data/HrUnionFingerprintTest.kt b/android/app/src/test/java/com/noop/data/HrUnionFingerprintTest.kt new file mode 100644 index 0000000000..6255b8f1d8 --- /dev/null +++ b/android/app/src/test/java/com/noop/data/HrUnionFingerprintTest.kt @@ -0,0 +1,154 @@ +package com.noop.data + +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals +import org.junit.Test +import java.lang.reflect.Proxy + +/** + * #2566 — [WhoopRepository.hrUnionFingerprint] is the SINGLE index-only witness of whether a window's + * heart rate changed, shared by the cycle load cache and the daytime stress lens memo. Until #2566 there + * were two of these computing the same facts in different encodings, and nothing stopped them drifting. + * + * The property that matters is #908: a strap re-added through the device manager banks its live raw under + * a FRESH id, so a fingerprint keyed on the active id alone would serve a stale cached result after a + * backfill landed rows under the alias. These pin that the witness walks the same id set + * [WhoopRepository.hrSamplesUnion] reads, and that it does so with index aggregates only. + * + * Stubbed through a Proxy [WhoopDao] (no Room), answering per-id so a change under ONE id is visible. + */ +class HrUnionFingerprintTest { + + private fun row(id: String) = PairedDeviceRow( + id = id, brand = "WHOOP", model = "4.0", nickname = null, + sourceKind = "strap", capabilities = "hr", status = "active", + addedAt = 0L, lastSeenAt = 0L, + ) + + /** [counts] and [maxTs] are keyed by deviceId, so each union id answers independently. */ + private fun repo( + registered: List, + counts: Map, + maxTs: Map, + seen: MutableList? = null, + ): WhoopRepository { + val dao = Proxy.newProxyInstance( + WhoopDao::class.java.classLoader, + arrayOf(WhoopDao::class.java), + ) { _, method, args -> + when (method.name) { + "pairedDevices" -> registered.map(::row) + "countHrInWindow" -> { + val id = args[0] as String + seen?.add("count:$id") + counts[id] ?: 0 + } + "maxHrTsInWindow" -> { + val id = args[0] as String + seen?.add("maxTs:$id") + maxTs[id] ?: 0L + } + // Any row fetch here would defeat the point: this is the cheap witness that AVOIDS one. + else -> throw UnsupportedOperationException("hrUnionFingerprint must not call ${method.name}") + } + } as WhoopDao + return WhoopRepository(dao) + } + + // The union is (active, other registered WHOOPs, canonical "my-whoop"), so a single-strap install + // still reads two ids and a re-added strap reads three. + @Test fun walksEveryUnionIdNotJustTheActive() = runBlocking { + val seen = mutableListOf() + val fp = repo( + registered = listOf("whoop-alias"), + counts = mapOf("whoop-active" to 3, "whoop-alias" to 7, "my-whoop" to 11), + maxTs = mapOf("whoop-active" to 300L, "whoop-alias" to 700L, "my-whoop" to 1100L), + seen = seen, + ).hrUnionFingerprint("whoop-active", 0L, 9_999L) + + assertEquals("whoop-active=3:300,whoop-alias=7:700,my-whoop=11:1100", fp) + // Exactly one COUNT and one MAX per id, never a repeat and never a fourth id. + assertEquals( + listOf( + "count:whoop-active", "maxTs:whoop-active", + "count:whoop-alias", "maxTs:whoop-alias", + "count:my-whoop", "maxTs:my-whoop", + ), + seen, + ) + } + + // #908, the reason this is a union at all: rows landing under the ALIAS id must move the witness, + // although the active id is untouched. A fingerprint keyed on the active id alone stays equal here + // and serves a stale cached score. + @Test fun movesWhenARowLandsUnderAnAliasIdOnly() = runBlocking { + val before = repo( + registered = listOf("whoop-alias"), + counts = mapOf("whoop-active" to 5, "whoop-alias" to 0, "my-whoop" to 0), + maxTs = mapOf("whoop-active" to 500L, "whoop-alias" to 0L, "my-whoop" to 0L), + ).hrUnionFingerprint("whoop-active", 0L, 9_999L) + + val after = repo( + registered = listOf("whoop-alias"), + counts = mapOf("whoop-active" to 5, "whoop-alias" to 1, "my-whoop" to 0), + maxTs = mapOf("whoop-active" to 500L, "whoop-alias" to 900L, "my-whoop" to 0L), + ).hrUnionFingerprint("whoop-active", 0L, 9_999L) + + assertNotEquals(before, after) + } + + // The canonical import id is in the union too, so a backfill under "my-whoop" moves it as well. + @Test fun movesWhenARowLandsUnderTheCanonicalIdOnly() = runBlocking { + val before = repo( + registered = emptyList(), + counts = mapOf("whoop-active" to 5, "my-whoop" to 0), + maxTs = mapOf("whoop-active" to 500L, "my-whoop" to 0L), + ).hrUnionFingerprint("whoop-active", 0L, 9_999L) + + val after = repo( + registered = emptyList(), + counts = mapOf("whoop-active" to 5, "my-whoop" to 2), + maxTs = mapOf("whoop-active" to 500L, "my-whoop" to 800L), + ).hrUnionFingerprint("whoop-active", 0L, 9_999L) + + assertEquals("whoop-active=5:500,my-whoop=0:0", before) + assertNotEquals(before, after) + } + + // Unchanged inputs must produce the SAME string, or every caller re-folds on every pass and the + // memo is worse than no memo at all. + @Test fun stableWhenNothingChanged() = runBlocking { + fun fp() = runBlocking { + repo( + registered = listOf("whoop-alias"), + counts = mapOf("whoop-active" to 4, "whoop-alias" to 6, "my-whoop" to 8), + maxTs = mapOf("whoop-active" to 400L, "whoop-alias" to 600L, "my-whoop" to 800L), + ).hrUnionFingerprint("whoop-active", 0L, 9_999L) + } + assertEquals(fp(), fp()) + } + + // An empty window is a stable non-null value per id, so a first run still differs from an unset + // watermark while two empty reads match and nothing churns. + @Test fun emptyWindowIsStableZeroes() = runBlocking { + assertEquals( + "whoop-active=0:0,my-whoop=0:0", + repo(registered = emptyList(), counts = emptyMap(), maxTs = emptyMap()) + .hrUnionFingerprint("whoop-active", 0L, 9_999L), + ) + } + + // The active id is never duplicated when it is also the canonical id (single-strap install that + // never re-paired), so the witness stays one entry rather than comparing "my-whoop" against itself. + @Test fun canonicalActiveIsNotDuplicated() = runBlocking { + assertEquals( + "my-whoop=9:900", + repo( + registered = emptyList(), + counts = mapOf("my-whoop" to 9), + maxTs = mapOf("my-whoop" to 900L), + ).hrUnionFingerprint("my-whoop", 0L, 9_999L), + ) + } +}