Invalidate daily analysis cache when late scored streams arrive - #1907
Conversation
…HR alone A history offload does not commit its channels together — HR can land first and R-R, respiration or SpO2 minutes later — and an offloaded HR row duplicating a live one is dropped on conflict. So a banked night could be scored once from HR alone and then gain its R-R with the night's HR (count, maxTs) completely unmoved. The per-day reuse key was HR-only, so it said "reuse" and re-served the HRV-less scan for the rest of the session, force refreshes included: force only bypasses the whole-pass watermark gate, never the day cache. The whole-pass gate had already been widened to every raw stream (analysisFingerprint v2). This is the same widening at day granularity, which is where the reuse decision is actually made: a new per-day, per-owner witness over ppgHrSample, rrInterval, respSample, spo2Sample, gravitySample, stepSample, skinTempSample and event, folded into the key. ppgHrSample is in it because the day's HR read is measured-union-PPG while the key's fingerprint counted hrSample only, so a PPG-derived second changed the scored series without moving the key — the same defect, second instance. COUNT(*) is the load-bearing half: it moves when a backfill lands rows inside a window already covered, which MAX(ts) alone misses. Every arm is an index range walk over the same (deviceId, ts) keys the HR fingerprint uses, materialising no rows, against the 7 full stream reads a hit skips. rrInterval is filtered exactly as the R-R read filters, so the witness counts the beats actually scored. The alternative the report also listed — let force bypass the day cache — was rejected: force defaults to true and every real update path uses the default, so it would disable the cache on essentially every pass. Verified: swift test WhoopStore ReadTests/testDayStreamFingerprint (2 passed) and StrandAnalytics --filter AnalyzeRecentDayCache (9 passed, including a red-first reproduction driving analyzeDay and a real store); gradle testFullDebugUnitTest AnalyzeRecentDayCacheTest (8 passed) on the pinned JDK 17. IntelligenceEngine.swift is app-target Swift and was not compiled — no Xcode on this host. No strap test.
analyzeDay is handed nine streams and the day witness covered eight. The missing one is the band's own sleep_state, and it sits on exactly the mechanism this witness exists for: HR and sleep_state are appended from the SAME v18 record at the same ts. When a re-offloaded HR row duplicates a live one it is dropped on conflict while the sleepStateSample row is new — HR count and maxTs unmoved, the day key unmoved, and the night re-served though its re-onset confirm and persisted per-session band state change. The whole-pass gate already sees it; the day key defeated it. Same defect as #29, one stream later. One COUNT(*) + COALESCE(MAX(ts), 0) arm per platform, letter `b` (band) — `s` is spent on the version prefix `s1|`. Same cost profile as the other arms: an index range walk over the (deviceId, ts) primary key. The regression loop in ReadTests committed six of the eight streams, so ppgHrSample and stepSample had no test at all: an arm reading a NEIGHBOURING table is valid SQL, ships, and is invisible. All nine streams are committed now — sleepState fails against the previous witness before the fix. Two doc claims corrected: the Swift witness test named a Kotlin twin that does not exist, and WhoopDao claimed Room and a plain-JVM SQLite test run the same statement — the day SQL carries binds the existing parameterless harness cannot execute, so Room's KSP verification is what checks it. Verified: swift test WhoopStore ReadTests/testDayStreamFingerprint — red on sleepState first (store-red.log), 2 passed after the fix (store-green.log). The Kotlin arm and the app-target call site are unbuilt here; CI on the PR covers the Room @query and the app module.
|
Thanks @bhelm — this is a well-diagnosed bug and a clean fix. The mechanism is exactly right: a history offload does not commit its channels together, and an offloaded HR row duplicating a live one is dropped on conflict, so a night scored from HR alone can gain its R-R with I checked the things that would make this fix incomplete rather than taking them on trust:
Locally on a fresh checkout with One follow-up, not a blockerThe cost lands on the cache-hit path, which is the path this cache exists to keep cheap (#1005 was a battery change). Per day it goes from one statement to two, the second holding 18 aggregates across 9 tables. That is almost certainly still far cheaper than re-scoring a night, and correctness beats a stale HRV-less night either way — so this is worth knowing rather than worth blocking on. The pass already prints One idea if it does show up: the whole-pass Good catch, and the writeup made this quick to verify. |
2 participants
Late R-R and other scored streams can change a night's result while the per-day cache key still matches the earlier HR-only scan. This can retain a night without HRV after the missing beats arrive.
Add a device/window-scoped count/max witness for all other consumed streams to the day-cache key on Swift and Kotlin, including band sleep state. Unchanged inputs continue to reuse cached analysis. The default force=true path does not bypass the cache, since doing so would effectively disable normal reuse.
Verification: actual HRV-changing red reproduction before the fix; targeted Swift cache/oracle and store tests and Kotlin cache tests passed. Independent source review found a missing sleep-state arm, now fixed with a targeted red/green store regression; comments no longer claim nonexistent twin tests. The current PR revision has now passed all 16 CI checks, including the final Kotlin SQL delta, Swift packages, and macOS/iOS app compile. No device testing or local full-suite coverage is claimed.
Addresses bhelm#29. Kept draft for maintainer review; no merge or deployment performed.
A separate draft in the author's older fork exercises its CI: bhelm#103. That port is on a different base and omits the newer end-to-end oracle whose API is absent there; its CI is supporting evidence, not a claim that this upstream revision has passed the same jobs.