Repository navigation
Restore: don't sync into a closed DB, and never ack past un-stored history (#57) - #62
Merged
Merged
Conversation
…nd never ack past un-stored history (#57) After loading a backup, strap sync stayed at 0 (live HR worked) and — worse — real banked history was discarded. Two bugs, both fixed here. 1. Restore leaves the app writing to a CLOSED database. NoopApplication.repository is a `by lazy` that caches a WhoopDao from the DB instance built at startup. A restore calls WhoopDatabase.close() (closes + nulls the singleton so the file can be swapped; the next get() rebuilds against the restored file), but the long-lived repository + BLE client keep the DAO on the now-closed instance, so every offload write throws "connection pool has been closed" until the PROCESS restarts. The code already documented that a restore requires a restart (DataBackup) but only showed a dismissable toast. Enforce it: auto-relaunch the app after a successful restore. 2. An empty/metadata HISTORY_END acks past HELD records. finishChunk correctly holds the ack when a records-chunk fails to persist — but repository.insert short-circuits EMPTY batches without touching the store, so an empty END never throws, reaches ackTrim, and advances the strap's trim PAST the held records-chunks, freeing history we never stored. Add a session `persistStalled` flag: the moment any persist (rows / reject archive / trim cursor) fails, halt ALL further acks (incl. empty ENDs) so the trim can't advance past un-stored data. The offload stalls safely; a fresh session re-offers everything past the last GOOD ack. Confirmed against the reporter's 8.3.4 log: 208 "failed to persist ... connection pool has been closed" while 200 distinct trims were acked and advanced — real records freed. Android compiles; BLE unit tests pass. The Swift Backfiller twin has the same empty-END ack-past-held latent bug (bug 2) — a parity follow-up.
…e restart uncancellable (#57) Re-review follow-ups: - iOS twin (bug 2): Strand/Collect/Backfiller.swift had the same latent data-loss — an empty/metadata HISTORY_END skips store.insert (never throws) and would ack past a held records-chunk. Mirror the Android persistStalled guard: set it on any persist failure (decoded / reject archive / raw enqueue / trim cursor) and refuse to ack while set, so the trim can't advance past un-stored history. - Android restart hardening: the auto-relaunch ran in the screen's cancellable scope, so navigating away during the 800ms window skipped the restart and left the app on the closed DB. Wrap it in NonCancellable so the data-safety restart always completes. Android compiles + BLE tests pass; iOS via CI.
…ility) #57 was invisible in a report — the UI (and 'Last sync', which fires on any completed offload) showed '0 synced' with no way to tell 'strap was empty' from 'writes FAILED'. Add a write-health signal to the debug export, both platforms: - Persist sync.lastWriteOkAt when an offload actually persisted rows, and sync.lastWriteStalledAt when a session stalled on a persist failure (Backfiller now exposes persistStalled read-only; WhoopBleClient/BLEManager record it at session end). - Persist backup.lastRestoreAt when a restore swaps the DB, so the export can correlate a restore with a later stall (a restore not followed by a restart IS the #57 failure). - Export shows: 'Data write: rows last landed X ago' and, when stalled, a '⚠ history NOT persisting — last offload STALLED X ago (if you restored a backup, fully restart — #57)' line, plus 'Last restore: X ago'. Android compiles + BLE tests pass; iOS via CI.
DX23876
pushed a commit
to DX23876/noop
that referenced
this pull request
Jul 22, 2026
Restore: don't sync into a closed DB, and never ack past un-stored history (ryanbr#57)
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.
What #57 hit
On Android, after loading a backup from 8.2.2, strap sync stayed at 0 (live HR worked), and the log showed hundreds of:
Two bugs, both here — and together they discarded real banked history.
Bug 1 — restore leaves the app writing to a closed DB
NoopApplication.repositoryis aby lazythat caches aWhoopDaofrom the DB instance built at startup. A restore callsWhoopDatabase.close()(closes + nulls the singleton so the file can be swapped; the nextget()rebuilds against the restored file). But the long-lived repository + BLE client keep the DAO on the now-closed instance, so every offload write throws "connection pool has been closed" until the process restarts. The code already says a restore requires a restart (DataBackup.kt:40/61/144) but only showed a dismissable toast — exactly how the reporter ended up syncing into a dead DB.Fix: auto-relaunch the app after a successful restore (relaunch intent +
exitProcess), instead of trusting a toast.Bug 2 — an empty END acks past held records (data loss)
finishChunkcorrectly holds the ack when a records-chunk fails to persist. Butrepository.insertshort-circuits empty batches (WhoopRepository:170) without touching the store — so an empty/metadataHISTORY_ENDnever throws, reachesackTrim, and advances the strap's trim past the held records-chunks, freeing history we never stored. Backfiller's own comment (line 79) names this hazard.Fix: a session
persistStalledflag — the moment any persist (rows / reject archive / trim cursor) fails, halt all further acks (including empty ENDs) so the trim can't advance past un-stored data. The offload stalls safely; a fresh session re-offers everything past the last good ack.Evidence
The reporter's 8.3.4 log: 208 "failed to persist … connection pool has been closed" interleaved 1:1 with 200 distinct acked+advancing trims — real records freed while the UI showed "0 synced."
Scope
compileFullReleaseKotlin), BLE unit tests pass.Fixes #57.