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. Ported onto this fork (behind upstream): the upstream commit's whole-pass `analysisFingerprint`/`ANALYSIS_FINGERPRINT_SQL` widening is not present here, so only the per-day witness is taken and the doc lines that named it were reworded to describe the whole-pass gate generically. The upstream integration test AnalyzeRecentDayCacheStreamWitnessTests.swift is left out: it drives SleepStager.hrOnlySessions, which this fork does not have. The key contract is still pinned by AnalyzeRecentDayCacheTests / AnalyzeRecentDayCacheTest and the witness by ReadTests testDayStreamFingerprint*. Not verified locally: nothing was built or run for this port beyond Tools/doc_comment_lint.py (clean) and static checks that every cacheKey caller, table, column and RR read filter it relies on exists on this fork. CI on the pull request is the evidence.
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 the per-day witness itself, 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. One doc claim corrected: WhoopDao said Room and a plain-JVM SQLite test run the same statement — the day SQL carries binds no parameterless harness can execute, so Room's KSP verification is what checks it. Ported onto this fork from the original-branch delta. The upstream commit's comment-only edit to AnalyzeRecentDayCacheStreamWitnessTests.swift does not apply: that test is not on this fork (it drives SleepStager.hrOnlySessions, which this fork does not have). The corrected WhoopDao wording is written without naming ANALYSIS_FINGERPRINT_SQL, which this fork also does not have. Not verified locally: nothing was built or run for this port beyond Tools/doc_comment_lint.py (clean) and source checks that sleepStateSample, PpgHrSample(ts:bpm:conf:), StepSample(ts:counter:) and SleepStateSample(ts:state:) exist on this fork in both the GRDB and the Room schema. The red/green run belongs to the original branch. CI on the pull request is the evidence here.
The per-day cache key gained a second suspend store read (dayStreamFingerprint) alongside hrFingerprintWindow. Each suspension point enlarges analyzeRecentOnCpu's single coroutine state machine, and the Jacoco-instrumented method crossed the 61,535-byte budget IntelligenceEngineJacocoBudgetTest asserts. Move the key assembly into a private suspend helper so both reads live in their own state machine. The call site keeps one awaited call in the same position, with the same read order, window, arguments and miss fallback, and cacheKey itself is untouched, so the key string stays byte-identical.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Draft CI test for #29 on this fork's existing main. Only the day-cache fix is ported; this does not synchronize the fork with upstream.
The per-day reuse key previously watched HR rows only. Late-arriving scored streams could change a night's result without invalidating that key. The fix adds a device/window-scoped count/max witness for the other consumed streams, including sleep state, on Swift and Kotlin. It preserves cache reuse for unchanged inputs and avoids bypassing on the default force=true path.
Verification
On the upstream-based candidate: actual HRV-changing red reproduction before the fix; 9 targeted Swift cache/oracle tests, 2 targeted Swift store tests and 8 Kotlin cache tests passed. Independent source review found the missing sleep-state arm; that small correction has a further targeted Swift red/green test. The final Kotlin SQL delta and this older fork integration have now passed this PR's CI. Existing results are not claimed as green for a different fork revision.
Fork adaptation preserves the key/store tests. The newer end-to-end StreamWitness test is omitted here because its hrOnlySessions API does not exist on this older fork; its original reproduction remains part of the upstream candidate. No missing upstream feature was imported just to make this test compile.
Expected CI: Swift Packages, Android, Apple app compile, source hygiene and i18n. No device verification claimed. No local full-suite reruns; this PR is intended to obtain the missing platform/integration evidence through CI.
Fork CI follow-up
The first fork run passed all Swift packages and both Apple app builds. Android compiled and ran 4,512 tests, with only the existing JaCoCo method-size budget failing. The exact fork base had passed that test; the newer upstream candidate also passed Android CI.
The fork-only follow-up extracts the same two cache-key reads into one private suspend helper in IntelligenceEngine.kt, reducing the large caller's coroutine body without changing the key or weakening the budget. Existing tests remain the regression gate. No local build or full-suite rerun was performed; the new CI run has now passed all 16 checks, including the existing JaCoCo method-size budget and both Apple app builds. Prior green checks cover unchanged relevant content but are not presented as newly executed at this commit.