fix(sleep): stop deleting stored sleep history on every connect - #44
Merged
Conversation
Fixes #43 (zaggash, Colmi R11 / Da Rings): "whenever a new night is over and registered, the previous are deleted and not available anymore." EventPersistenceSubscriber ran unscoped DELETEs on sleep_sessions and sleep_stage_blocks on every connect for every family except YCBT, then rebuilt from the ring. That assumed the ring could re-supply what was deleted. None can re-supply more than its own buffer, and the two smallest re-supply a single day: CRPSyncEngine sends queryHistorySleep(daysAgo = 0), and jring's syncWindowDays is 1. So each connect discarded every night older than the last one, and the next night arrived into an emptied table instead of joining the nights before it. Colmi survived only incidentally, by requesting a multi-day big-data range. The rebuild was never load-bearing. upsertSleepSessionAtomic already reconciles one waking day at a time and is documented as idempotent, and it already folds in legacy rows keyed to the wrong day — "include legacy rows keyed to the wrong day if they overlap this packet; reconciliation re-points their surviving blocks to the correct waking day" — which was the blanket clear's stated reason to exist. So the clear was doing nothing the per-day reconcile did not already do, except destroying days the ring would never send again. A connect now retires demo rows and nothing else, for every family. That makes preservesSleepOnConnect vacuous, so it is deleted rather than left as dead code with a passing test. isConnectTransition stays: it is what keeps a device-info reply from being read as a connection, and it still gates the demo clear. Behaviour worth stating explicitly: nothing bulk-deletes real sleep any more, so Forget followed by pairing a different ring now carries the previous ring's history over instead of silently wiping it. That seems right — the data is the user's, not the ring's — but if it should change it belongs on DeviceForgotten as a deliberate choice, not as a side effect of connecting. Noted in AGENTS.md. Not included: deepening the pull so a night is not lost when a sync straddles it. The CRP vendor exposes CRPHistoryDay.YESTERDAY = 1 and jring's makeHistoryQueryCommand accepts up to 27 days, but that is per-family protocol work and wants zaggash to confirm the R11 answers daysAgo = 1 before we rely on it. The delete is the data loss; this fix stops it on its own. 791 unit tests pass. Test coverage changed shape: the three preservesSleepOnConnect cases are gone with the function, replaced by one that asserts the property that now matters — re-syncing a night leaves the nights on either side of it intact. The end-to-end "connect does not delete rows" invariant still has no unit coverage; it needs a Room harness this suite does not have.
…es nothing" structural Follow-up to e11be6e from an adversarial review of #44. That commit stopped the bleeding but shipped three problems. Backfill the prior week's sleep, once per connection. Stopping the deletion only stops further loss — it recovers nothing, because the poll pass only ever asked `queryHistorySleep(daysAgo = 0)`. zaggash's stored history would have rebuilt one night at a time from today forward while the ring already held the back-catalogue. The earlier PR deferred this as needing hardware confirmation, which overstated the risk: CRPDecoder.decodeSleep already reads the day index out of `payload[0]` and accepts up to 14, so a reply dates itself rather than trusting what we asked for, and a day the ring has no record of produces no reply — exactly today's behaviour for that day. Sent once per connection, not per pass, and stopped at a week rather than the decoder's 14-day ceiling: runStartup is also the ~30-minute background sync and this ring funnels everything through one fdd2 channel. Replace the fake regression test. `re-syncing one night leaves the nights around it untouched` exercised replaceOverlappingSleepBlocks, which e11be6e never touched — it passed identically before the fix and would keep passing if someone re-added sleepSessionDao().clear() tomorrow. It guarded nothing, and it replaced eight assertions that at least pinned real per-family behaviour, so coverage went down in a commit whose purpose was preventing data loss. The replacement is structural rather than another assertion: ConnectPurge has members NOTHING and DEMO_ROWS and deliberately none meaning "real rows", so restoring the old behaviour now requires a new member, which fails to compile against exhaustive `when`s in both the subscriber and the test. Verified by temporarily adding an ALL_SLEEP member — the build fails with "'when' expression must be exhaustive" — rather than assuming it. Correct an AGENTS.md claim the code does not deliver. It said isConnectTransition "stops a device-info reply from being mistaken for a connection". The gate only governs what a CONNECTED event may delete; the row write below it — stateRaw, lastConnectedAt, lastSyncAt — is outside the gate and still runs for every decoder Status, so a jring 0x0C reply does restamp the device row each sync pass. Harmless, but not something the gate prevents. Also recorded two things the PR left unsaid: no bulk-delete path exists anywhere now, so there is no retention or pruning mechanism at all, and reconcileWakingDay's `if (groups.isEmpty())` is the one delete a re-sync can still perform — it should be unreachable, but it is where to look first if history goes missing again. jring has the same one-day gap (`syncWindowDays = 1` against a command that accepts 27) and is deliberately untouched: that constant also drives the activity-sync progress window, so it is not the one-line change it appears to be. 792 unit tests pass.
Three comments and an AGENTS.md paragraph attributed jring's one-day sleep pull
to RingSyncCoordinator.syncWindowDays. That constant has exactly one use —
sizing the sync-progress window in beginSyncProgress — and it applies to every
family, not just jring. jring's actual depth is makeHistoryQueryCommand()'s
default of 1, called with no argument at JringDriver.kt:105.
The misattribution came from grepping for makeHistoryQueryCommand callers: one
of the hits was syncWindowDays' doc comment ("must match makeHistoryQueryCommand's
default"), and the constant sitting under it was read as the control. Both values
are 1 and the comment asserts they must match, so the wrong mechanism kept
producing the right answer and nothing contradicted it.
It was not just a mislabel. The rationale for deferring the jring fix was reasoned
from it — "that constant also drives the activity-sync progress window, so
widening it is not the one-line change it looks like" — which is true of
syncWindowDays and irrelevant to jring's request depth. AGENTS.md now says
plainly that syncWindowDays is not a per-family request depth, and records the two
things that do make jring harder than CRP: JringSyncEngine.runStartup has no
once-per-connection gate, so a wider `days` re-pulls the span on every ~30-minute
background pass instead of once; and 0x10 returns activity and sleep together,
with no sleep-only request, so each extra day costs ~96 activity packets
(15x 1-minute buckets per packet) on top of the night.
Comments only, no behaviour change. 792 unit tests pass.
… day per pass Closes the gap left by 7649513, which deepened CRP's sleep pull and left jring on a single day. Same defect: the ring holds days the app never asks for, so once connecting stopped deleting the stored copy (e11be6e), a user's history could still only grow one night at a time from install and never recovered what the ring already had. JringSyncEngine now asks for JRING_BACKFILL_DAYS on the first pass of a connection and one day on every pass after it. A fresh engine is built per connection (JringDriver.makeSyncEngine runs on connect), so instance state gives "once per connection" for free — the same trick CRPSyncEngine uses for its read-backs. The gate is the whole difficulty, and it is why this was previously deferred as a larger piece of work. runStartup is also the ~30-minute background sync, and refresh()/querySleep() default to it, so an unconditional wider window would re-pull the span every half hour forever rather than once. That earlier deferral also cited RingSyncCoordinator.syncWindowDays as the control, which was wrong — see 893aebb; the real constraint is volume. Window is 3 days rather than CRP's 7 because 0x10 returns activity *and* sleep and there is no sleep-only request, so each extra day costs roughly 96 more packets (activity arrives as 15x 1-minute buckets each) to reach one more night. Re-syncing the same days is harmless either way: activity buckets upsert by timestamp with the day total recomputed from distinct buckets, and sleep reconciles one waking day at a time. Tested that the gate actually holds rather than assuming it: commenting out the `historyBackfilled = true` latch — a realistic regression that still compiles — fails `Jring pulls a deeper history window once per connection` and nothing else. The existing refresh/querySleep contract test builds a fresh engine per capture, so all three take the first-pass branch and it still passes unchanged; a comment now says so, since that is no longer self-evident. Still unverified on hardware: that the ring honours days > 1. The command coerces to 0..27 and mirrors Gadgetbridge's triggerActivityReportByDays, and a ring that ignores it returns what it has, which is today's behaviour. 794 unit tests pass.
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.
Fixes #43 (@zaggash, Colmi R11 / Da Rings):
Reproduced from source; not hardware-validated.
What was happening
EventPersistenceSubscriberran unscoped deletes on every connect for every family except YCBT, then rebuilt from the ring:That assumed the ring could re-supply what was deleted. None can re-supply more than its own buffer, and the two smallest re-supply one day:
queryHistorySleep(daysAgo = 0)→ todaymakeHistoryQueryCommand()default → 1 daybigDataRequest(BIG_DATA_SLEEP, [0xFF, 0x01])— multi-dayCRPSyncEngine.kt:118sendsqueryHistorySleep()once, with the defaultdaysAgo = 0. So each connect discarded every night older than the last one, and the next night arrived into an emptied table instead of joining the nights before it — exactly the reported symptom.The rebuild was never load-bearing
upsertSleepSessionAtomicalready reconciles one waking day at a time and is documented as idempotent. It also already folds in legacy rows keyed to the wrong day:…which was the blanket clear's own stated reason for existing. So the clear did nothing the per-day reconcile didn't already do, except destroy days the ring would never send again.
Fix
A connect now retires demo rows only, for every family. That connect path was the sole caller of the unscoped deletes, so nothing bulk-deletes real sleep any more.
preservesSleepOnConnectbecame vacuous once both branches were identical, so it's deleted rather than left as dead code with a passing test.isConnectTransitionstays — it still keeps a device-info reply from being read as a connection, and still gates the demo clear.Behaviour change worth a decision
Nothing bulk-deletes real sleep now, so Forget → pair a different ring carries the previous ring's history over instead of silently wiping it. That seems right — the data is the user's, not the ring's — but if it should change, it belongs on
DeviceForgottenas a deliberate choice, not as a side effect of connecting. Noted in AGENTS.md.Not included
Deepening the pull.Since added for CRP in7649513:CRPSyncEngine.sendSleepBackfillpulls the prior week once per connection. Deferring it was a mistake — stopping the deletion only stops further loss and recovers nothing, and the risk was overstated:CRPDecoder.decodeSleepalready reads the day index frompayload[0]and accepts up to 14, so a reply dates itself and an unsupported day simply produces no reply.jring is still not deepened. See the correction at the bottom for what that actually requires.
Testing
791 unit tests pass. Coverage changed shape: the three
preservesSleepOnConnectcases are gone with the function, replaced by one asserting the property that now matters — re-syncing a night leaves the nights on either side intact. The end-to-end "connecting deletes nothing" invariant still has no unit coverage; it needs a Room harness this suite doesn't have.versionCodebumped to 33.Note for testers
This stops future loss — it can't recover what's already gone. History that was already discarded stays gone, and the app will accumulate night by night from here rather than have it reappear.
Correction (post-review)
An earlier revision of this description cited
RingSyncCoordinator.syncWindowDays = 1as jring's sleep-request depth. That was wrong.syncWindowDayshas exactly one use — sizing the sync-progress window inbeginSyncProgress— and it applies to every family, not just jring. jring's actual depth ismakeHistoryQueryCommand()'s default of 1, called with no argument atJringDriver.kt:105.The conclusion (jring pulls one day) was independently correct; only the citation was wrong. It mattered because the deferral rationale was reasoned from the bad citation — "that constant also drives the progress window, so widening it isn't a one-line change" — which is true of
syncWindowDaysand irrelevant to jring's request depth. The real reasons jring is harder than CRP are thatJringSyncEngine.runStartuphas no once-per-connection gate (a widerdayswould re-pull the span every ~30-minute background pass, not once) and that0x10returns activity and sleep together — no sleep-only request exists — so each extra day costs ~96 activity packets on top of the night.