Repository navigation
Hydration: only write imported water that actually changed (#949) - #986
Merged
Merged
Conversation
sync() hands setImportedHydration its whole 31-day window every run, and sync is driven by six HKObserverQuery observers at hourly background delivery. So a watch user was doing ~31 SQLite upserts plus a hydrationSeq bump repeatedly through the day to store numbers that were already there — thirty of those days are historical and immutable, and the honest answer is almost always "nothing moved". The seq bump is the worse half: it makes the Today card re-read its hydration total each time for no change. Read the window's existing imported rows once and upsert only the days whose value actually moved, bumping the seq only when something did. One ranged read replaces the write burst; the common case now writes nothing at all. A day with no row and a value of 0 is skipped rather than written, since an absent row already reads as 0 — that stops a user with no water in Health from accumulating a wall of zeros. A day that HAS a row and is now 0 is still written: that is a deletion propagating, and it must not be confused with the no-op case. For the same reason the existing-row read does not fall back to []. A failed read that looked like "no rows" would send a just-deleted day into the skip path and leave its stale figure behind, so it bails and lets the next sync do it properly. Found re-reviewing this change, not after. Also corrects a comment in TodayView that still described hydrationTotal as reading one row; it has read two since the import landed.
App-target Swift that no CI compiles is a poor place to lean on multi-statement closure inference. No behaviour change.
Owner
Author
|
Re-reviewed. No defects found in the change, and one thing hardened. Verified against the actual definitions rather than memory, since nothing compiles this file:
Hardened: the Noticed, not fixed here: |
ryanbr
added a commit
that referenced
this pull request
Jul 31, 2026
…t for sleep Two fixes from re-reviewing the change. Hydration must NOT be keyed by the record's own offset. Its write is a windowed REPLACE: windowDays is built from LocalDate.now(zone) and HydrationStore.importWindow keeps only keys inside that window, dropping anything else rather than merging it. Keying a record by its own offset can move it a day off the phone zone, so a drink near either edge of the window would land outside it and be discarded without a trace. That is a worse bug than the one being fixed, and the same quiet-loss-in-a-replace-path shape as #986. Fixing it properly means making the window offset-aware too, which is shared code with an iOS twin, so it gets its own PR. Checked the other write paths for the same shape: appleRows/dailyRows are built from acc's own keys with no window clamp, and coveredDays is a semantic gate rather than a range, so hydration was the only one that silently drops a key. Sleep now falls through endZoneOffset to startZoneOffset before the phone zone. The wake day makes the end offset the right first choice, but writers that set only the start are common, and the start is far better evidence of the sleeper's zone than wherever the phone happens to be at import time.
ryanbr
added a commit
that referenced
this pull request
Jul 31, 2026
…) (#1003) * Health Connect: key each record by the offset it was recorded in (#1002) HealthConnectImporter bucketed every record with ZoneId.systemDefault(), the phone's zone at import time, and never read the startZoneOffset / zoneOffset that Health Connect attaches to each record. WINDOW_YEARS is 10, so one import keys a decade of history that way, and importing again after moving re-keys the same records differently. The report attributes this to DST as well as travel. DST was not affected: ZoneId.systemDefault() returns a REGION id, and LocalDate.ofInstant resolves it with the rules in force at the given instant, so a January record is already keyed with January's offset when the import runs in July. Verified on a JVM before writing the fix, and pinned by a test so a later change cannot swap the region zone for a fixed current offset and break what was working. What was broken is travel and relocation, where the record's zone differs from the phone's current one: a record written at 01:30 in Tokyo resolves to the previous day on a phone set to Europe/London. localDayKey(instant, offset, zone) uses the record's offset when it has one and falls back to the phone's zone when it does not, so behaviour is unchanged for the many writers that leave the field null. Threaded through every call site: interval records use startZoneOffset (endZoneOffset for sleep, which keys to the wake day), instantaneous records use zoneOffset, and heart-rate samples inherit their parent record's. Workouts needed the key carried forward — WorkoutRow holds only epoch seconds, so re-deriving it in the #835 calorie post-pass would have fallen back to the phone's zone and undone the fix for exactly the travelled sessions it exists for. workoutDays is index-parallel to workouts. iOS is deliberately not changed here; see the PR for why. * Keep hydration on the phone zone, and fall through to the start offset for sleep Two fixes from re-reviewing the change. Hydration must NOT be keyed by the record's own offset. Its write is a windowed REPLACE: windowDays is built from LocalDate.now(zone) and HydrationStore.importWindow keeps only keys inside that window, dropping anything else rather than merging it. Keying a record by its own offset can move it a day off the phone zone, so a drink near either edge of the window would land outside it and be discarded without a trace. That is a worse bug than the one being fixed, and the same quiet-loss-in-a-replace-path shape as #986. Fixing it properly means making the window offset-aware too, which is shared code with an iOS twin, so it gets its own PR. Checked the other write paths for the same shape: appleRows/dailyRows are built from acc's own keys with no window clamp, and coveredDays is a semantic gate rather than a range, so hydration was the only one that silently drops a key. Sleep now falls through endZoneOffset to startZoneOffset before the phone zone. The wake day makes the end offset the right first choice, but writers that set only the start are common, and the start is far better evidence of the sleeper's zone than wherever the phone happens to be at import time. * Steps top-up: reach back a day so the phone-zone filter can't hide a record the record-zone key claims The top-up's read filter is in the phone's zone but its day key is now the record's own, and the two can disagree. A record whose offset sits east of the phone's zone can belong to today while having started before the phone's midnight: the filter never fetches it, the sum comes back low, and the write replaces the stored count with any non-zero sum — so a miss overwrites a good figure with a low one. The full import keys the same way, so the two would then disagree and flip-flop the number on each run, which is worse than the undercount on its own. Reading from yesterday's midnight closes it. A day of slack covers every real offset (-12..+14), the == dayKey test still decides membership so the extra rows are read and discarded, and step records are coarse interval rows a handful per day, so this stays cheap on a screen refresh. Before the change the filter and the key used the same zone, so the set was consistent by construction; keying by the record's offset is what introduced the asymmetry.
simoncad7
pushed a commit
to simoncad7/noop
that referenced
this pull request
Aug 17, 2026
… (ryanbr#986) * Hydration: only write imported water that actually changed (ryanbr#949) sync() hands setImportedHydration its whole 31-day window every run, and sync is driven by six HKObserverQuery observers at hourly background delivery. So a watch user was doing ~31 SQLite upserts plus a hydrationSeq bump repeatedly through the day to store numbers that were already there — thirty of those days are historical and immutable, and the honest answer is almost always "nothing moved". The seq bump is the worse half: it makes the Today card re-read its hydration total each time for no change. Read the window's existing imported rows once and upsert only the days whose value actually moved, bumping the seq only when something did. One ranged read replaces the write burst; the common case now writes nothing at all. A day with no row and a value of 0 is skipped rather than written, since an absent row already reads as 0 — that stops a user with no water in Health from accumulating a wall of zeros. A day that HAS a row and is now 0 is still written: that is a deletion propagating, and it must not be confused with the no-op case. For the same reason the existing-row read does not fall back to []. A failed read that looked like "no rows" would send a just-deleted day into the skip path and leave its stale figure behind, so it bails and lets the next sync do it properly. Found re-reviewing this change, not after. Also corrects a comment in TodayView that still described hydrationTotal as reading one row; it has read two since the import landed. * Hydration: spell out the closure return type (ryanbr#949) App-target Swift that no CI compiles is a poor place to lean on multi-statement closure inference. No behaviour change.
simoncad7
pushed a commit
to simoncad7/noop
that referenced
this pull request
Aug 17, 2026
…nbr#1002) (ryanbr#1003) * Health Connect: key each record by the offset it was recorded in (ryanbr#1002) HealthConnectImporter bucketed every record with ZoneId.systemDefault(), the phone's zone at import time, and never read the startZoneOffset / zoneOffset that Health Connect attaches to each record. WINDOW_YEARS is 10, so one import keys a decade of history that way, and importing again after moving re-keys the same records differently. The report attributes this to DST as well as travel. DST was not affected: ZoneId.systemDefault() returns a REGION id, and LocalDate.ofInstant resolves it with the rules in force at the given instant, so a January record is already keyed with January's offset when the import runs in July. Verified on a JVM before writing the fix, and pinned by a test so a later change cannot swap the region zone for a fixed current offset and break what was working. What was broken is travel and relocation, where the record's zone differs from the phone's current one: a record written at 01:30 in Tokyo resolves to the previous day on a phone set to Europe/London. localDayKey(instant, offset, zone) uses the record's offset when it has one and falls back to the phone's zone when it does not, so behaviour is unchanged for the many writers that leave the field null. Threaded through every call site: interval records use startZoneOffset (endZoneOffset for sleep, which keys to the wake day), instantaneous records use zoneOffset, and heart-rate samples inherit their parent record's. Workouts needed the key carried forward — WorkoutRow holds only epoch seconds, so re-deriving it in the ryanbr#835 calorie post-pass would have fallen back to the phone's zone and undone the fix for exactly the travelled sessions it exists for. workoutDays is index-parallel to workouts. iOS is deliberately not changed here; see the PR for why. * Keep hydration on the phone zone, and fall through to the start offset for sleep Two fixes from re-reviewing the change. Hydration must NOT be keyed by the record's own offset. Its write is a windowed REPLACE: windowDays is built from LocalDate.now(zone) and HydrationStore.importWindow keeps only keys inside that window, dropping anything else rather than merging it. Keying a record by its own offset can move it a day off the phone zone, so a drink near either edge of the window would land outside it and be discarded without a trace. That is a worse bug than the one being fixed, and the same quiet-loss-in-a-replace-path shape as ryanbr#986. Fixing it properly means making the window offset-aware too, which is shared code with an iOS twin, so it gets its own PR. Checked the other write paths for the same shape: appleRows/dailyRows are built from acc's own keys with no window clamp, and coveredDays is a semantic gate rather than a range, so hydration was the only one that silently drops a key. Sleep now falls through endZoneOffset to startZoneOffset before the phone zone. The wake day makes the end offset the right first choice, but writers that set only the start are common, and the start is far better evidence of the sleeper's zone than wherever the phone happens to be at import time. * Steps top-up: reach back a day so the phone-zone filter can't hide a record the record-zone key claims The top-up's read filter is in the phone's zone but its day key is now the record's own, and the two can disagree. A record whose offset sits east of the phone's zone can belong to today while having started before the phone's midnight: the filter never fetches it, the sum comes back low, and the write replaces the stored count with any non-zero sum — so a miss overwrites a good figure with a low one. The full import keys the same way, so the two would then disagree and flip-flop the number on each run, which is worse than the undercount on its own. Reading from yesterday's midnight closes it. A day of slack covers every real offset (-12..+14), the == dayKey test still decides membership so the extra rows are read and discarded, and step records are coarse interval rows a handful per day, so this stays cheap on a screen refresh. Before the change the filter and the key used the same zone, so the set was consistent by construction; keying by the record's offset is what introduced the asymmetry.
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
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.
Follow-up to #974. Behaviour-neutral; this is about idle background work.
The problem
sync()handssetImportedHydrationits whole 31-day window on every run, andsyncis driven by sixHKObserverQueryobservers at hourly background delivery (heartRateVariabilitySDNN,restingHeartRate,activeEnergyBurned,heartRate,vo2Max, sleep). So a watch user was doing ~31 SQLite upserts plus ahydrationSeqbump repeatedly through the day, to store numbers that were already there. Thirty of those days are historical and immutable — the honest answer is almost always "nothing moved".The seq bump is the worse half: it invalidates the Today card's hydration read, so the card redid that work each time for no change.
The fix
Read the window's existing imported rows once, upsert only the days whose value actually moved, and bump the seq only when something did. One ranged read replaces the write burst, and the common case now writes nothing at all.
Two edges that matter
A day with no row and a value of 0 is skipped, not written — an absent row already reads as 0 via
hydrationImportedTotal. That is what stops a user with no water in Health from accumulating a wall of zero rows on every sync. A day that HAS a row and is now 0 is still written: that is a deletion propagating, and confusing it with the no-op case would strand the old figure. Both paths are explicit in the code.The existing-row read does not fall back to
[]. Found re-reviewing this change, before pushing it: with?? []a failed read looks exactly like "there are no rows", which sends a just-deleted day into the skip path and leaves its stale non-zero figure behind. It bails instead and lets the next sync do it properly — the health store is the source of truth, so nothing is lost by waiting. Same distinction as the read-failure gating in #974; I reintroduced it in the optimisation and caught it on the re-read.Comparison uses a 1e-9 epsilon rather than
==. The same samples re-summed give the sameDouble, but a value that ever drifted by a float hair would re-write every sync forever and quietly undo the point of this. Well below a millilitre, so no real change is swallowed.Also
Corrects a comment in
TodayViewthat still describedhydrationTotalas reading one metricSeries row — it has read two since the import landed.Verification
swiftc -parseclean;doc_comment_lintandi18n_audit --ciboth exit 0.app-build.ymlis disabled, and both files are app-target Swift shared with the macOS target — so a type error would pass every check and break the desktop build too. I have hand-traced the four cases (no row/zero, no row/value, row unchanged, row changed-to-zero), but that is reasoning, not a compiler.