Skip to content

Preserve repeated corrupt-DB quarantines instead of dropping the newest (#661) - #681

Merged
ryanbr merged 1 commit into
mainfrom
fix/corrupt-db-quarantine-661
Jul 20, 2026
Merged

Preserve repeated corrupt-DB quarantines instead of dropping the newest (#661)#681
ryanbr merged 1 commit into
mainfrom
fix/corrupt-db-quarantine-661

Conversation

@ryanbr

@ryanbr ryanbr commented Jul 20, 2026

Copy link
Copy Markdown
Owner

Fixes #661.

The bug

The #1014 corruption handler (CorruptionPreservingOpenHelperFactory.onCorruption) kept a single fixed .corrupt slot: the first corrupt copy was preserved and every later one deleted (the else { original.delete() } branch). But a later corruption is a rebuilt store that has banked new, non-resendable strap history since — so the copy it discarded was the most recovery-worthy one, silently defeating the handler's whole purpose. tigercraft4 flagged this in a static data-integrity audit.

The behaviour was a deliberate disk bound ("repeated failed opens can't multiply 100 MB files") — but it picked disk-safety over the newest data.

The fix

  • Timestamped quarantine names whoop.db.corrupt.<epochMillis> + keep newest MAX_CORRUPT_QUARANTINES (3), pruning older ones. Caps disk (a crash loop can't multiply files) and preserves the latest data plus a couple of priors for diagnosing a recurring corruption path. Naming/pruning is a pure, unit-tested function (planCorruptQuarantine).
  • Move WAL/SHM sidecars alongside the quarantine rather than deleting them — the WAL can hold the most recent un-checkpointed writes. The live path is still cleared (rename, or copy+delete on the cross-storage fallback), so the fresh rebuild can never inherit a stale write-ahead log. (A stale live WAL could otherwise be applied to the new DB by salt match and re-inject corrupt pages — hence live-path clearing always wins over preserving an un-movable sidecar.)
  • Doc fix: the class-doc claimed the factory "deletes NOTHING", but the old else-branch deleted the original and the sidecars were always dropped.

Scope / parity

Android-only. iOS/GRDB has no delete-on-corruption default (this Android handler exists to neutralise the platform default), and WhoopStore.quarantineIncompatibleDatabase already uses timestamped names — so this converges the two rather than diverging. No analytics/stored-data change → parity contract untouched.

Verification

  • ./gradlew compileFullDebugKotlin ✓
  • ./gradlew testFullDebugUnitTest --tests com.noop.data.CorruptDbQuarantineTest ✓ (first/under-cap/keep-newest-N/eviction-order/never-evict-new/dedup)
  • ./gradlew testFullDebugUnitTest --tests com.noop.data.* ✓ (no existing regressions)
    The filesystem move/prune is I/O and validated by inspection; the decision logic is unit-tested.

Reported-by: tigercraft4

@ryanbr
Preserve repeated corrupt-DB quarantines instead of dropping the newe…
9d59aff 
…st (#661)

The #1014 corruption handler kept a single fixed `.corrupt` slot: the FIRST
corrupt copy was preserved and every later one deleted (the `else` branch).
But a later corruption is a rebuilt store that has banked new, non-resendable
strap history since — so the copy it discarded was the most recovery-worthy
one, silently defeating the handler's whole purpose.

Quarantine under a TIMESTAMPED name (`whoop.db.corrupt.<epochMillis>`) and keep
the newest MAX_CORRUPT_QUARANTINES (3), pruning older ones. This caps disk (a
crash loop can't multiply 100 MB files — the original concern) AND preserves
the latest data plus a couple of priors for diagnosing a recurring corruption
path. The naming/pruning is a pure, unit-tested function.

Also move the WAL/SHM sidecars ALONGSIDE the quarantine rather than deleting
them: the WAL can hold the most recent un-checkpointed writes. The live path is
still cleared (rename, or copy+delete on the cross-storage fallback), so the
fresh rebuild can never inherit a stale write-ahead log — a stale live WAL
could otherwise be applied to the new DB by salt and re-inject corrupt pages.

Corrected the class-doc, which claimed the factory 'deletes NOTHING' — the old
else-branch deleted the original and the sidecars were always dropped.

Android-only: iOS/GRDB has no delete-on-corruption default (this handler exists
to neutralise Android's), and already quarantines with timestamped names.

Reported-by: tigercraft4 (static data-integrity audit). Verified:
compileFullDebugKotlin + CorruptDbQuarantineTest + com.noop.data.*.

Author: ryanbr <mp3geek@gmail.com>
@ryanbr
ryanbr merged commit cb4f8eb into main Jul 20, 2026
1 check passed
@ryanbr
ryanbr deleted the fix/corrupt-db-quarantine-661 branch July 20, 2026 17:28
ryanbr added a commit that referenced this pull request Jul 20, 2026
@ryanbr
Bump build numbers for a fresh 9.0.3 testing build
29a7586 
Marketing version stays 9.0.3; Android versionCode 296->297 and iOS
CURRENT_PROJECT_VERSION 206->207 so the new staging build installs over the
previous one. Content since the last testing build: Health Connect writeback
failure surfacing (#672), SpO2/respiration empty-track explanation (#673),
WHOOP 5/MG raw-IMU offload persist (#675), hide stale sleep recompute entries
(#679), Oura activity MET research corpus + cap/rotation (#676/#680), trend
chart date alignment (#528), and corrupt-DB quarantine preservation (#681).
DX23876 pushed a commit to DX23876/noop that referenced this pull request Jul 22, 2026
@ryanbr
Preserve repeated corrupt-DB quarantines instead of dropping the newe…
059220e 
…st (ryanbr#661) (ryanbr#681)

Timestamped .corrupt.<epoch> quarantine + keep-newest-3 pruning replaces the single fixed .corrupt slot that deleted every later corrupt copy (the most recovery-worthy one). WAL/SHM moved alongside the quarantine; live path always cleared so the rebuild can't inherit a stale WAL (ryanbr#1014 crash-loop invariant preserved). Pure planCorruptQuarantine + unit test. Android-only; iOS/GRDB has no delete-on-corruption default. Reported-by: tigercraft4.
DX23876 pushed a commit to DX23876/noop that referenced this pull request Jul 22, 2026
@ryanbr
Bump build numbers for a fresh 9.0.3 testing build
ed3b09d 
Marketing version stays 9.0.3; Android versionCode 296->297 and iOS
CURRENT_PROJECT_VERSION 206->207 so the new staging build installs over the
previous one. Content since the last testing build: Health Connect writeback
failure surfacing (ryanbr#672), SpO2/respiration empty-track explanation (ryanbr#673),
WHOOP 5/MG raw-IMU offload persist (ryanbr#675), hide stale sleep recompute entries
(ryanbr#679), Oura activity MET research corpus + cap/rotation (ryanbr#676/ryanbr#680), trend
chart date alignment (ryanbr#528), and corrupt-DB quarantine preservation (ryanbr#681).
brendankowitz pushed a commit to brendankowitz/noop that referenced this pull request Aug 9, 2026
@claude
Live workout: heart-rate-led Hearth hero on its own dark-plum sky
ce15d40 
Rebuilds the in-exercise screen (ryanbr#238) to the Hearth mockup's §1e heart-rate-led
take. It stops being a stack of cream cards on a light scenic backdrop and
becomes one full-bleed moment: a dark-plum sky, a recording pill, HR leading at
96pt, Effort second, the HR zone rail, a frosted Avg/Peak/Effort strip, and a
floating control bar pinned to the bottom.

Visual only — every number is the same live value the card layout showed, from
the same sources: smoothed `AppModel.bpm`, `HRZones` off the user's HR-max,
`ActiveWorkout.liveStrain`/avgHr/peakHr, and the TimelineView elapsed tick. The
LiveState isolation (`SensorRowIfPresent` owning its own observation so a 1 Hz
packet doesn't invalidate the hero), the ryanbr#681 realtime-HR arm/disarm balance,
the ryanbr#703 keep-awake hold and the ryanbr#517 end-confirm are all preserved verbatim.

Notes:
- Drops the last live `StrainGauge` call site in the app (the layered bevel
  gauge that the rest of the retheme already flattened elsewhere). The Effort
  numeral replaces it; the band word it used to draw (LIGHT / MODERATE /
  STRENUOUS / HIGH / ALL-OUT, `showsLabel` defaulted true) is kept on the SAME
  thresholds, so the screen says exactly what it said before — no new
  classifier was invented for it.
- No pause/resume exists in the recorder (AppModel has start/end only), so the
  bar's right-hand control is a close-and-keep-recording chevron, not the
  mockup's pause glyph. Both controls are 56pt, clear of the 44pt minimum.
- Zone rail: inactive rungs are a tint of THAT zone's own colour; the active
  rung's label picks ink or off-white by the fill's luminance, since the Hearth
  zone ramp runs light sage to dark maroon.
- Colours route through the override layer: two additions to HearthTheme (the
  mockup's live-workout sky ramp, and an on-sky rose — metricRose is authored
  for warm paper and is below AA on the plum). No hex literals in the view.
- DEBUG `--demo-workout` renders one fixed synthetic reading so the populated
  hero can be screenshotted at all (the Simulator has no BLE); same in-file
  launch-flag idiom as `--demo-reveal`, stripped from Release.

Verified: iOS NOOPiOS and macOS Strand both build; screenshotted the populated
hero and the honest no-reading state in the Simulator. Not strap-tested (no
BLE path was touched).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GPcFU4xvYzxovzd4aQ7uGB
simoncad7 pushed a commit to simoncad7/noop that referenced this pull request Aug 17, 2026
@ryanbr
Preserve repeated corrupt-DB quarantines instead of dropping the newe…
e1ff688 
…st (ryanbr#661) (ryanbr#681)

Timestamped .corrupt.<epoch> quarantine + keep-newest-3 pruning replaces the single fixed .corrupt slot that deleted every later corrupt copy (the most recovery-worthy one). WAL/SHM moved alongside the quarantine; live path always cleared so the rebuild can't inherit a stale WAL (ryanbr#1014 crash-loop invariant preserved). Pure planCorruptQuarantine + unit test. Android-only; iOS/GRDB has no delete-on-corruption default. Reported-by: tigercraft4.
simoncad7 pushed a commit to simoncad7/noop that referenced this pull request Aug 17, 2026
@ryanbr
Bump build numbers for a fresh 9.0.3 testing build
fdf4c1a 
Marketing version stays 9.0.3; Android versionCode 296->297 and iOS
CURRENT_PROJECT_VERSION 206->207 so the new staging build installs over the
previous one. Content since the last testing build: Health Connect writeback
failure surfacing (ryanbr#672), SpO2/respiration empty-track explanation (ryanbr#673),
WHOOP 5/MG raw-IMU offload persist (ryanbr#675), hide stale sleep recompute entries
(ryanbr#679), Oura activity MET research corpus + cap/rotation (ryanbr#676/ryanbr#680), trend
chart date alignment (ryanbr#528), and corrupt-DB quarantine preservation (ryanbr#681).
xxblyxx added a commit to xxblyxx/noop that referenced this pull request Aug 21, 2026
@xxblyxx
Keep realtime HR armed for the life of a workout, not the screen
8050e27 
startWorkout() took no realtime-HR reference; every existing caller
(LiveWorkoutView's sheet, the Live tab) was screen-scoped. Dismissing
the sheet right after Start dropped the ref-count to zero and silently
stopped capturing samples on a WHOOP 5/MG, since live HR there only
flows while the puffin realtime stream is armed (ryanbr#681-shaped) —
captureWorkoutSample bailed on every sample and endWorkout discarded
the session with nothing to save.

startWorkout() and rehydrateActiveWorkout() now take a realtime-HR
reference for as long as activeWorkout is non-nil, released in
endWorkout(). Ref-counted and clamped at 0, so it composes with
LiveWorkoutView's own start/stop pair without either disarming the
other.

Android's requestRealtimeHr/releaseRealtimeHr stays screen-scoped
(ui/LiveWorkoutScreen.kt:79-80) — a known, deliberate divergence, not
a formula or stored value, so no Kotlin twin is required for this
behavior change.
@kiesstein kiesstein mentioned this pull request Sep 9, 2026
Merged
ryanbr pushed a commit that referenced this pull request Sep 10, 2026
@kiesstein
Arm the realtime HR stream while the Breathe screen is visible (#2036)
822aba8 
The Breathe screen reads live beats but never asked for them. On a WHOOP 5 or MG, which only streams
while armed, that left the BPM hero and the RMSSD tile on a dash and the coherence card on "No data"
unless the wearer happened to have opened Live first, which is why it looked intermittent rather than
broken. Both platforms consume the stream: Swift ingests R-R through `onRRPackets` and Kotlin collects
`rrPackets()` into the buffer behind `Hrv.rmssd`. Neither armed it.

The screen now takes the stream while it is visible, through the ref-counted request that Live and the
in-exercise view already use. Because it is ref-counted, moving between Breathe and Live never stops
the stream out from under whichever screen stays.

Arming on visible rather than on a running session is deliberate. The BPM hero and the RMSSD tile are
on screen before a session starts, so gating on the session would leave a wearer looking at two dashes
and deciding the feature is broken, which is the report this came from. The cost is bounded the same
way Live's is, by a foreground screen the wearer opened, and the lightweight HR recording is unchanged
either way.

The two sides were verified against each other rather than trusted for looking alike: both hold the
same counter, both arm on the zero to one edge, both blank the stale smoothing window there and only
there, and both clamp the release at zero. Nothing else on this screen touches the count, so the
appear and disappear pair is balanced.

Relates to #681, which is where the same only-streams-while-armed behaviour was first pinned down.

Thanks to @kiesstein.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Data integrity: preserve multiple corrupt database quarantines instead of deleting repeated corrupt originals

1 participant

@ryanbr