Skip to content

storage that stops growing: compact curves, one less index, gzipped backups - #223

Merged
abdulsaheel merged 6 commits into
OpenStrap:mainfrom
svssathvik7:storage-compression
Aug 11, 2026
Merged

storage that stops growing: compact curves, one less index, gzipped backups#223
abdulsaheel merged 6 commits into
OpenStrap:mainfrom
svssathvik7:storage-compression

Conversation

@svssathvik7

@svssathvik7 svssathvik7 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

392 MB → 143 MB on a one-year-old install, measured by rebuilding the real schema both ways and reading per-object byte counts out of dbstat.

Component Before After
1 Hz substrate (3-day window) 37,703,680 34,443,264 1.09x
Derived bundles (365 days) 26,447,872 12,484,608 2.12x
Database file 65,290,240 48,066,560 1.36x
Backups (5 copies) 326,451,200 94,812,300 3.44x
Total on device 392 MB 143 MB 2.74x

Why the bundle, and why not a codec

day_result is the only store that grows without bound — raw and decoded are capped at rawRetentionDays, derived days are kept forever. Its 88 KB bundle was 74.5 KB of series, stored as one JSON object per sample:

[{"t":1783572180,"v":77},{"t":1783572240,"v":80}]

27 bytes to carry two numbers, repeating the full 10-digit epoch in every element, for curves that sample on a fixed 60-second grid. Across the three tracked fixtures, hr_curve, strain_curve, zone_timeline and skin_temp_day are perfectly regular and are 85% of series.

Curves are now written as {t0,dt,v[]} when sampling is regular and {t0,to[],v[]} when it is not. 2.13x, no codec, no decompression on any read path.

It stays plain JSON deliberately. The coach reads this column with SQLjson_each(json_extract(payload_json,'$.series.…')) in v_series/v_hypnogram. A compressed BLOB is opaque to json1 and sqflite cannot register a decompress function, so gzipping the column would have silently stripped every intra-day curve from the AI Coach (invariant 13, and §4.7). Stacking gzip on top of this encoding reaches 5.1–8.1x; rejected — it buys ~7 MB/year for the coach's entire SQL surface.

json_each exposes an array's index as key, so a grid reconstructs as t0 + key*dt in pure SQL.

Migration-free by construction

Three shapes coexist forever — legacy from before this change, plus grid and offset. Nothing runs inside openDatabase under iOS's CPU watchdog (invariant 11).

v_series reads all three, verified row-for-row against the pre-codec view on all three fixtures and on a database holding both shapes at once (no duplicates, equal row counts). Existing rows convert via a bounded, forward-only, resumable pass beside pruneSupersededIntermediates — off the durable-commit path. Every row is gated on a proven-lossless round-trip before it is touched, and only payload_json is written, so no day is re-dated or re-finalized.

No kAlgoVersion bump: values don't change, only their spelling.

Also in here

  • idx_decoded_rr_counter was an exact duplicate of the index PRIMARY KEY (counter, beat_index) already creates. Both measured 3,264,512 bytes on a 3-day fill; after dropping it the planner still serves counter lookups from the auto-index. ~1.09 MB/day plus one b-tree write per beat off the hottest insert path. Dropped inside _createDecodedStore exactly like the idx_decoded_rr_ts drop above it, so no schemaVersion bump is needed.

  • Auto-backups are gzipped, streamed rather than buffered. Two pre-existing leaks fixed while I was in there: the retention pattern matched neither the plain .db names earlier versions wrote nor the -N collision names _uniqueDestination emits, so both were invisible to sortBackupsNewestFirst and never pruned.

  • Import inflates gzip instead of refusing it ("unzip it first"), for both the CSV and database paths, under the existing size ceiling.

Testing

flutter analyze clean; 1830 tests pass (baseline was 1792 passing + 3 failing).

  • series_codec_test.dart — lossless round-trip on all three tracked fixtures, plus embedded nulls, non-monotonic and duplicate timestamps, extra keys, non-int t, and every malformed-envelope case.
  • coach_views_series_shapes_test.dart — the regression pin: same day stored both ways must produce identical v_series rows, and a mixed database must emit each row exactly once.
  • day_result_reencode_test.dart — value-for-value survival, no column but payload_json touched, termination, resumability across a close/reopen, corrupt cursor, unparseable payload, and every algo_version generation converted.
  • series_codec_structural_test.dart — every bundle-decode seam pinned to the codec. Verified to fail when a seam is bypassed rather than passing vacuously.
  • Extensions to db_storage_hygiene_test.dart, auto_backup_test.dart, import_container_test.dart.

Deliberately not here

Chunked columnar blobs for the 1 Hz substrate. It would be a real Gorilla-style win per day, but the substrate is already capped at 3 days — flat, not growing — so the steady-state saving is one-time and modest, while the cost is rewriting the BLE drain through the commit-before-ACK path (invariant 1), whose failure mode is permanent data loss or an infinite re-flood. Dropping the duplicate index already claims 1.09 of its 12.6 MB/day for none of that risk.

Design notes: docs/superpowers/specs/2026-08-10-storage-compression-design.md.

Summary by CodeRabbit

  • New Features

    • Reduced time-series storage size while preserving values and compatibility with existing records.
    • Added compressed .db.gz database backups with improved retention and recovery.
    • Imports now support gzip-compressed SQLite databases and CSV files with size and format safeguards.
    • Existing records are upgraded incrementally during normal processing.
    • Edge backup import now accepts .db and .db.gz files directly.
  • Bug Fixes

    • Improved handling of malformed or unsupported series data with safe fallbacks.
    • Removed redundant database indexing to reduce storage overhead.

…ackups

392 MB -> 143 MB on a one-year-old install, measured by rebuilding the real
schema both ways and reading per-object byte counts out of `dbstat`.

`day_result` is the only store that grows without bound — raw and decoded are
capped at `rawRetentionDays`, derived days are forever. Its 88 KB bundle was
74.5 KB of `series`, stored as one JSON object per sample:

    [{"t":1783572180,"v":77},{"t":1783572240,"v":80}]

27 bytes to carry two numbers, repeating the full 10-digit epoch every element,
for curves that sample on a fixed 60-second grid. That is an encoding problem,
not a compression problem.

Curves are now written as `{t0,dt,v[]}` when the sampling is regular and
`{t0,to[],v[]}` when it is not — 2.13x across the three tracked fixtures, with
no codec and no decompression on any read path.

It stays PLAIN JSON on purpose. The coach reads this column with SQL
(`json_each`/`json_extract` in `v_series`/`v_hypnogram`), a compressed BLOB is
opaque to json1, and sqflite cannot register a decompress function — so
gzipping the column would have silently stripped every intra-day curve from the
AI Coach. Stacking gzip on top would reach 5.1-8.1x; it is rejected for ~7 MB a
year.

Three shapes coexist forever, so there is no rewriting migration and nothing
runs inside `openDatabase` under iOS's CPU watchdog. `v_series` reads all three
and was verified row-for-row against the pre-codec view, including a database
holding both shapes at once. Existing rows are converted by a bounded,
forward-only, resumable pass that runs beside `pruneSupersededIntermediates` —
each row gated on a proven-lossless round-trip before it is touched, and only
`payload_json` is written, so no day is re-dated or re-finalized.

Also:

  * `idx_decoded_rr_counter` was an exact duplicate of the index
    `PRIMARY KEY (counter, beat_index)` already creates. Both measured
    3,264,512 bytes on a 3-day fill; the planner still serves `counter`
    lookups from the auto-index without it. ~1.09 MB/day and one b-tree write
    per beat off the hottest insert path. Dropped in `_createDecodedStore`
    like the `idx_decoded_rr_ts` drop above it, so no schemaVersion bump.

  * Auto-backups are gzipped (3.44x on five copies, ~326 MB -> ~95 MB),
    streamed rather than buffered. The retention pattern now also matches the
    plain `.db` names earlier versions wrote and the `-N` collision names
    `_uniqueDestination` emits — neither matched before, so both leaked
    full-size copies that retention could never see.

  * Import inflates gzip instead of refusing it ("unzip it first"), for both
    the CSV and database paths, under the existing size ceiling.

A structural guard pins every bundle-decode seam to the codec — §4.7 is the
failure mode here, and it is a silent one: a bypassed reader gets a Map where
it expects a List, matches neither, and renders an empty chart.

flutter analyze clean; 1830 tests pass.
@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds compact JSON curve encoding, normalized decoding, SQL support for multiple curve shapes, resumable legacy backfill, gzip backups, gzip import handling, duplicate-index removal, and regression coverage.

Changes

Storage compression

Layer / File(s) Summary
Series codec contract and behavior
docs/superpowers/specs/2026-08-10-storage-compression-design.md, lib/data/series_codec.dart, test/series_codec_test.dart, test/series_codec_structural_test.dart
Defines and tests legacy, grid, and offset JSON shapes, lossless conversion, malformed-input handling, passthrough behavior, and payload-wide encoding and decoding.
Database integration and legacy backfill
lib/data/db.dart, lib/compute/derivation_engine.dart, lib/data/local_repository_impl.dart, lib/health/health_export.dart, lib/import/whoop_import.dart, lib/state/app_state.dart, test/coach_views_series_shapes_test.dart, test/day_result_reencode_test.dart, test/db_storage_hygiene_test.dart
Writes encode payloads before storage. Readers decode compact payloads. v_series supports all three shapes. Legacy rows are re-encoded in bounded resumable batches. The redundant RR index is removed and query plans are tested.
Gzip backups and imports
lib/data/auto_backup.dart, lib/import/import_container.dart, test/auto_backup_test.dart, test/import_container_test.dart, lib/ui/import/import_screen.dart, lib/ui/profile/profile_screen.dart
Automatic backups use .db.gz, retain legacy and collision-suffixed names, stream gzip output, and remove incomplete files. SQLite and CSV imports inflate gzip input under the output-size limit. The import interface and profile text describe compressed backups.
Storage hygiene and validation
.gitattributes, test/db_storage_hygiene_test.dart
Dart diff handling is configured. Tests cover duplicate-index removal, query plans, and superseded-generation pruning.

Estimated code review effort: 5 (Critical) | ~90 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DerivationEngine
  participant LocalDb
  participant SeriesCodec
  participant SQLite
  participant RepositoryReader
  DerivationEngine->>LocalDb: Store day result
  LocalDb->>SeriesCodec: Encode payload JSON
  SeriesCodec->>SQLite: Persist compact JSON
  RepositoryReader->>SQLite: Read payload_json
  RepositoryReader->>SeriesCodec: Decode payload JSON
  SeriesCodec-->>RepositoryReader: Return normalized curves
Loading

Possibly related PRs

Suggested labels: Review effort 5/5

Suggested reviewers: abdulsaheel

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main storage-reduction changes: compact curves, index removal, and gzipped backups.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 3317-3324: The legacy re-encode pass currently runs only through
_pruneOldDecoded when scope.fullHistory is true, so ordinary derives never reach
it. Move or invoke the LocalDb.reencodeLegacyDayResults() housekeeping block
from _pruneOldDecoded to a housekeeping path executed by every derive run, while
preserving its bounded resumable behavior and existing logging.

In `@lib/data/auto_backup.dart`:
- Around line 221-236: The gzip stream in the backup creation flow around
tmp.openRead, dest.openWrite, and the surrounding try/catch must stage output
before publication. Write to a temporary, retention-unrecognized file in the
destination directory, close the gzip sink successfully, then rename it to
dest.path; delete the staging file on any failure and retain existing cleanup
behavior. Add a failure-path test verifying that no final backup filename exists
before publication.

In `@lib/data/db.dart`:
- Around line 4503-4506: Update importFromDbFile to reset the re-encode cursor
after the copy loop when counts['day_result'] is greater than zero, using
LocalDb.putComputeFreshness(kReencodeCursorKey, '') so the walk restarts for
imported legacy-shaped rows; leave the cursor unchanged when no day_result rows
were imported.
- Around line 1768-1805: Update the latest CTE in the v_series view to filter
rows with json_valid(payload_json) before any json_extract calls; place the
guard in the CTE so all three UNION branches inherit it and malformed payloads
are skipped.
- Around line 4508-4525: Refactor reencodeLegacyDayResults to process rows with
a bounded cursor and build a list of (day_id, algo_version, encoded) updates
before opening db.transaction. Move SeriesCodec.needsReencode, verifyLossless,
and encodePayloadJson outside the transaction; keep the transaction limited to
applying prepared updates and advancing the cursor.
- Around line 3144-3154: Move payload encoding for derived writes out of
LocalDb.putDayResult by encoding the decoded bundle in derivation_engine before
the database call, then pass the resulting String through a new
encodedPayloadJson parameter or dedicated API. Update putDayResult to use the
pre-encoded payload without jsonDecode/SeriesCodec.encodePayloadJson/jsonEncode,
while preserving the existing path for skipped, import, and test callers that
lack decoder input.

In `@lib/import/import_container.dart`:
- Around line 190-227: Add regression tests covering inflateGzip and import
resolution for valid gzipped SQLite and CSV files, plus corrupt gzip input. Add
an over-limit gzip case that verifies ImportFormatException is raised and the
partial destination file is removed, using the existing test helpers and
conventions in the import test suite.

In `@test/coach_views_series_shapes_test.dart`:
- Around line 168-185: Update the v_series SQL fixture/setup used by this test
to insert a malformed curve containing both dt and to, then make the offset
branch require that dt is absent. Keep the grid branch unchanged and align the
mutually exclusive routing with SeriesCodec.decodeCurve’s dt-priority behavior.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 2132b4df-7009-4ef8-adfd-d3c46e3d71b4

📥 Commits

Reviewing files that changed from the base of the PR and between c2b76ce and fa467f1.

📒 Files selected for processing (17)
  • docs/superpowers/specs/2026-08-10-storage-compression-design.md
  • lib/compute/derivation_engine.dart
  • lib/data/auto_backup.dart
  • lib/data/db.dart
  • lib/data/local_repository_impl.dart
  • lib/data/series_codec.dart
  • lib/health/health_export.dart
  • lib/import/import_container.dart
  • lib/import/whoop_import.dart
  • lib/state/app_state.dart
  • test/auto_backup_test.dart
  • test/coach_views_series_shapes_test.dart
  • test/day_result_reencode_test.dart
  • test/db_storage_hygiene_test.dart
  • test/import_container_test.dart
  • test/series_codec_structural_test.dart
  • test/series_codec_test.dart

Comment thread lib/compute/derivation_engine.dart Outdated
Comment thread lib/data/auto_backup.dart
Comment thread lib/data/db.dart Outdated
Comment thread lib/data/db.dart
Comment thread lib/data/db.dart
Comment thread lib/data/db.dart Outdated
Comment thread lib/import/import_container.dart
Comment thread test/coach_views_series_shapes_test.dart
Six of the eight review findings were real; two are answered in the threads.

  * The re-encode walk was called from `_pruneOldDecoded`, and BOTH of that
    method's call sites sit behind `if (scope.fullHistory)`. Ordinary
    light/heavy derives run with `fullHistory: false`, so the back-catalogue
    rewrite was resumable but effectively unreachable — a normal install would
    have converted nothing. Moved to `_runStorageHousekeeping`, called on every
    derive including the all-days-finalized early return.

  * Backups compressed straight into the final `.db.gz` path, so the published
    name existed while the file was still being written. A process killed
    mid-stream left a truncated file carrying a name retention matches, which
    counted toward the five and evicted a good backup; `catch` cannot help
    there. Now staged under a `.partial` suffix retention does not match, and
    published by rename. Leftovers from a killed run are swept on the next.

  * `json_extract` RAISES on a malformed document, so one corrupt payload_json
    failed the entire v_series query rather than dropping that day. The `latest`
    CTE now filters on `json_valid` first, the same guard daysWithSleepTst
    already applies for the same reason.

  * `importFromDbFile` writes day_result rows with a raw batch.insert, bypassing
    the encode seam, so imported rows arrive in whatever shape the source device
    stored. The walk latches `done` and is forward-only, so those rows would
    have kept the legacy shape forever — an import silently undoing the
    compression. An import that wrote day_result rows now rewinds the cursor.

  * A curve carrying BOTH `dt` and `to` matched the grid and offset branches at
    once and emitted every point twice. The codec never writes both, but storage
    does not enforce that and an import or corruption can. The offset branch now
    requires `.dt` to be absent, which also matches SeriesCodec.decodeCurve's
    precedence so SQL and Dart read an ambiguous curve the same way.

  * `verifyLossless` + `encodePayloadJson` ran inside `db.transaction`, holding
    the write lock across ~40 x ~88 KB of pure JSON work. Prepared outside now;
    the transaction only applies the updates.

Also: test/import_container_test.dart carried three literal NUL bytes, which
made git, grep and review tooling treat it as BINARY — no diff, no search hits.
That is why the gzip tests in it read as missing. Written as `\x00` escapes so
the file is text; same bytes, same assertions.

flutter analyze clean; 1835 tests pass.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
test/coach_views_series_shapes_test.dart (1)

259-265: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Select the intended encoded fixture for the size assertion.

The earlier test leaves 2026-01-03 in day_result. Therefore rows.last is the ambiguous compact fixture, not the encoded 2026-01-02 row. This test can pass without checking the intended legacy-versus-encoded pair.

Filter by the two fixture day IDs or index rows by day_id.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/coach_views_series_shapes_test.dart` around lines 259 - 265, Update the
size assertion in the test so it explicitly selects the legacy and encoded rows
by their fixture day_id values, rather than relying on rows.first and rows.last.
Ensure the comparison uses the intended 2026-01-01 legacy fixture and 2026-01-02
encoded fixture, excluding the earlier 2026-01-03 row.

Source: Coding guidelines

lib/data/db.dart (1)

1809-1817: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Validate compact-envelope structure before expanding it.

json_valid(payload_json) only validates the outer document. A curve with dt: "bad" or a non-array v enters the grid branch, although SeriesCodec.decodeCurve returns an empty curve. An offset curve with unequal to and v lengths also emits a partial curve, while the codec returns an empty curve.

Require integer t0 and dt values for grid curves. Require integer t0, array to, array v, equal array lengths, and integer offsets for offset curves. Add malformed-envelope fixtures that assert SQL matches SeriesCodec.decodeCurve.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/data/db.dart` around lines 1809 - 1817, Update the compact-envelope SQL
expansion branches around the grid and offset curve queries to validate the same
structure accepted by SeriesCodec.decodeCurve: require integer t0 and dt for
grid curves; require integer t0, array to and v values, equal array lengths, and
integer offsets for offset curves. Exclude malformed envelopes before json_each
expansion, and add fixtures asserting SQL results match SeriesCodec.decodeCurve,
including invalid types and unequal offset/value lengths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 1079-1081: The derivation lifecycle currently skips
_runStorageHousekeeping() on early returns and from runDays() and
rescanRecent(). Move or centralize the housekeeping call at a shared lifecycle
point reached by every derivation entry point and return path, including empty
decoded-row, restored, imported, and finalized-history cases, while keeping the
work bounded.

In `@lib/data/auto_backup.dart`:
- Around line 128-132: Update pruneStagingFiles so it deletes a file only when
the basename ends with kBackupStagingSuffix and the name remaining after
removing that suffix matches _backupNamePattern; preserve unrelated .partial
files. Add a regression test covering an unrelated .partial file that remains
after cleanup.

In `@lib/data/db.dart`:
- Around line 4552-4561: Update the transaction that applies backfill updates so
each day_result write compares the current payload_json with the original value
read before encoding, preventing stale data from overwriting a newer
putDayResult result. When the compare-and-set condition fails, re-read or retry
that row before advancing the cursor; preserve the existing day_id and
algo_version matching.

In `@test/auto_backup_test.dart`:
- Around line 455-467: Update the failure test around runBackup to make
exportSnapshot return a nonexistent snapshot path, allowing staging.openWrite()
and the gzip pipeline to fail during writing. Retain the failed outcome
assertion and verify that neither the final backup nor the staging file remains,
including the existing timestamp-based filename check and directory-count
assertion.

In `@test/day_result_reencode_test.dart`:
- Around line 245-246: Update the regression test around the existing re-encode
setup to create a source database containing a legacy day_result row, import it
via LocalDb.importFromDbFile(), and remove the manual
LocalDb.putComputeFreshness cursor reset. Assert that the subsequent re-encode
pass converts the imported row, ensuring the test exercises the production reset
behavior.

---

Outside diff comments:
In `@lib/data/db.dart`:
- Around line 1809-1817: Update the compact-envelope SQL expansion branches
around the grid and offset curve queries to validate the same structure accepted
by SeriesCodec.decodeCurve: require integer t0 and dt for grid curves; require
integer t0, array to and v values, equal array lengths, and integer offsets for
offset curves. Exclude malformed envelopes before json_each expansion, and add
fixtures asserting SQL results match SeriesCodec.decodeCurve, including invalid
types and unequal offset/value lengths.

In `@test/coach_views_series_shapes_test.dart`:
- Around line 259-265: Update the size assertion in the test so it explicitly
selects the legacy and encoded rows by their fixture day_id values, rather than
relying on rows.first and rows.last. Ensure the comparison uses the intended
2026-01-01 legacy fixture and 2026-01-02 encoded fixture, excluding the earlier
2026-01-03 row.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 02557fae-9b05-4382-b954-ba45f649dccb

📥 Commits

Reviewing files that changed from the base of the PR and between fa467f1 and 19ab6a7.

📒 Files selected for processing (7)
  • lib/compute/derivation_engine.dart
  • lib/data/auto_backup.dart
  • lib/data/db.dart
  • test/auto_backup_test.dart
  • test/coach_views_series_shapes_test.dart
  • test/day_result_reencode_test.dart
  • test/import_container_test.dart

Comment thread lib/compute/derivation_engine.dart Outdated
Comment thread lib/data/auto_backup.dart
Comment thread lib/data/db.dart
Comment thread test/auto_backup_test.dart
Comment thread test/day_result_reencode_test.dart Outdated
@svssathvik7

Copy link
Copy Markdown
Contributor Author

@abdulsaheel — could you take a look when you have time? (No write access here to add you as a reviewer formally.)

What it does: day_result is the only store that grows without bound, and its 88 KB bundle was mostly [{"t":1783572180,"v":77},…] — 27 bytes per sample, repeating a 10-digit epoch, for curves on a fixed 60 s grid. Curves are now written as {t0,dt,v[]} (regular) or {t0,to[],v[]} (irregular). Measured end to end by rebuilding the real schema both ways and reading dbstat: 392 MB → 143 MB on a one-year-old install.

Two things worth your attention specifically:

  1. It stays plain JSON on purpose, not gzip. v_series/v_hypnogram read this column with json_each/json_extract, a BLOB is opaque to json1, and sqflite cannot register a decompress function — so compressing the column would have silently stripped every intra-day curve from the coach. Stacking gzip on top reaches 5.1–8.1x instead of 2.13x; I rejected it for ~7 MB/year. If you disagree, that is the decision to push on — everything else follows from it.

  2. No migration. All three shapes (legacy + the two new ones) stay readable forever, so nothing runs inside openDatabase under the iOS watchdog. v_series was verified row-for-row against the pre-codec view, including a database holding both shapes at once. Old rows convert via a bounded resumable pass, each gated on a proven-lossless round trip before overwrite.

Also drops idx_decoded_rr_counter (an exact duplicate of the PK auto-index — both measured 3,264,512 bytes; ~1.09 MB/day and a b-tree write per beat off the hottest insert path) and gzips auto-backups.

Two caveats for reviewing:

  • The diff for test/import_container_test.dart renders as binary on GitHub. That file carried three literal NUL bytes predating this branch, so git classified the blob as binary — which is also why the CodeRabbit pass reported its gzip tests as missing when they were there all along. I escaped them to \x00, but binary-ness is decided against the base blob, so this PR's diff still hides ~300 lines of test changes. Checking the branch out is the only way to see them; it reads as text from the next PR onward.
  • No CI runs on PRs here, so the suite is manual: flutter analyze clean, 1835 tests pass.

CodeRabbit found 8 issues; 6 were real and fixed in 19ab6a7 (the significant one: the back-catalogue rewrite was gated behind scope.fullHistory and would never have run on a normal install). The other 2 it withdrew.

Design notes and the rejected alternatives are in docs/superpowers/specs/2026-08-10-storage-compression-design.md.

…a derive

The auto-backup writes .db.gz and the restore path handed the picked file
straight to openDatabase, so every backup this code takes came back as "file
is not a database". importFromDbFile now sniffs the magic bytes, inflates to a
temp file, imports and cleans up either way; a plain .db is untouched. The
import screen and the backup sheet said nothing about compression, so they do
now. Nothing crossed that boundary in a test, which is why it shipped.

The re-encode backfill prepared its bundles outside any transaction and then
wrote them back keyed on (day_id, algo_version) alone. Derivation runs in more
than one isolate and this walk starts at the newest day with kAlgoVersion
unbumped, so its first targets are the rows a light derive is rewriting: the
row ended up with the new scalar columns and the old payload. The update is now
a compare-and-set on the payload it read, and a row that has moved is parked
behind the cursor rather than stepped over.

That prepare is also ~0.1-0.4s of synchronous JSON on desktop hardware, on
whichever isolate called the derive, which is the UI one. SeriesCodec is pure,
so the batch runs on a worker isolate.

_runStorageHousekeeping was called unguarded from two points in run(), so a
SQLITE_BUSY or a full disk skipped the raw prune and the timezone re-baseline
and made a finished derive report zero days. It now swallows its own errors and
runs from the finally of run/runDays/rescanRecent — the only place every entry
path and early return reaches, which also fixes it never running on a restored
database that has derived history but no decoded rows.

pruneStagingFiles deleted any *.partial in a folder the user is invited to point
iCloud Drive or Nextcloud at; it now requires the published name to match the
backup pattern. Retention sorted same-second collision names backwards because
'-' sorts before '.', so it ranked the later backup as the older one; it sorts
by parsed timestamp and collision index instead.

inflateGzip used sink.add in an await-for loop, which queues without
back-pressure and buffers the whole inflated database in memory - the OOM this
file's header is about. It pipes through a counting transformer now, against a
2 GiB ceiling rather than 4 GiB, which is not a bound a phone survives reaching.

decodeCurve returned an empty curve for a map it did not recognise; since
decodePayload is the read seam for baselines and freshness rows too, it hands
the value back instead.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/day_result_reencode_test.dart`:
- Around line 228-274: Replace the fixed 80 ms delay in the concurrent reencode
test with a test-only synchronization hook awaited by reencodeLegacyDayResults
after batch preparation and before its transaction. Configure the hook to
perform the competing newest-row update, then await the walk and preserve the
existing assertions that it processes 39 rows and retains the fresh payload.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 19f5f276-c8b2-4ec4-ae71-a8077e371f35

📥 Commits

Reviewing files that changed from the base of the PR and between 19ab6a7 and 1fd948f.

📒 Files selected for processing (11)
  • .gitattributes
  • lib/compute/derivation_engine.dart
  • lib/data/auto_backup.dart
  • lib/data/db.dart
  • lib/data/series_codec.dart
  • lib/import/import_container.dart
  • lib/ui/import/import_screen.dart
  • lib/ui/profile/profile_screen.dart
  • test/auto_backup_test.dart
  • test/day_result_reencode_test.dart
  • test/series_codec_test.dart

Comment thread test/day_result_reencode_test.dart
The two compare-and-set tests waited 80 ms and hoped the competing write landed
between the batch prepare and the transaction. The property under test is that
exact placement, so a test that only holds while the runner stays inside the
delay is one that goes red on a loaded box and, worse, could stop exercising the
race at all while still passing.

The walk awaits a hook at that boundary — null in production, one null check per
batch — and the tests drive the competing derive from it.
The offset branch of v_series joined json_each over `.to` to json_each over
`.v` on `key`. SQLite cannot index a table-valued function, so that join has
no plan except a full cross product and its cost grows with the square of the
curve length. hrv_day, hrv_timeline and resp_day are all irregularly sampled
and therefore all offset-encoded, so it was the normal path: a 365-day
AVG(v) took 877 ms where the pre-codec view took 106, and a 30-day slice of
1440-point curves took 2.1 s. It now walks `.v` once and indexes into `.to` by
that key, which measures 89 ms. The `e.key < json_array_length(.to)` bound
keeps the result row-for-row identical to the join, including for a payload
whose `to` is shorter than its `v`, which the join dropped and an unbounded
index would have emitted with a null timestamp.

v_hypnogram never got the json_valid guard v_series has, so one malformed
payload_json made the whole view throw and took every day's sleep stages away
from the coach rather than only its own.

A truncated .db.gz restored as success. zlib checks the gzip CRC and length,
but only on reaching the end of the stream, and a stream that just stops never
gets there — Dart's decoder returns what it inflated with no error. A backup
cut at 99.9% inflated, sniffed as SQLite, merged and reported success one row
short, which is exactly what a half-synced cloud copy looks like. The trailer
is now read off the file and compared against what came out of the decoder,
and a mismatch is refused with a message about the file being incomplete.
Failure still leaves the live database untouched and nothing half-inflated on
disk.

decodeCurve skipped offset entries that were not integers, handing back a
curve silently missing samples; verifyLossless could not see it because it
compares a decode against a decode rather than against the original. It now
leaves such a curve alone. SQL still adds a fractional offset through rather
than suppressing it, noted where it happens.

Three tests were asserting nothing. The index-drop test asserted an index was
absent from a fresh database that never creates it, so deleting the DROP left
it green; it now plants the index, reopens, and checks the open removed it,
which is the path a real upgrade takes. verifyLossless had no direct coverage
at all despite being the only gate before overwriting a day whose substrate
has aged out. The offset branch's shape is pinned by a query-plan assertion.

.gitattributes asked for `text diff` on Dart sources when `diff` is the part
that keeps a NUL-containing file reviewable; `text` also switches on
end-of-line normalisation repo-wide, which was not the intent.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
lib/data/db.dart (1)

3799-3825: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Close src when database initialization fails. The try block starts after await instance. If initialization throws, src remains open while importFromDbFile deletes the temporary directory, and the deletion failure is swallowed.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/data/db.dart` around lines 3799 - 3825, The database initialization flow
around importFromDbFile must close the source database handle when
initialization fails. Move the relevant try/finally boundary to encompass
instance initialization, or otherwise ensure src is closed on that failure path
before the temporary directory cleanup runs, while preserving normal import
cleanup behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@lib/data/db.dart`:
- Around line 3799-3825: The database initialization flow around
importFromDbFile must close the source database handle when initialization
fails. Move the relevant try/finally boundary to encompass instance
initialization, or otherwise ensure src is closed on that failure path before
the temporary directory cleanup runs, while preserving normal import cleanup
behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f0deec4a-534f-40b9-849b-ab56369ed425

📥 Commits

Reviewing files that changed from the base of the PR and between 1fd948f and d43144e.

📒 Files selected for processing (9)
  • .gitattributes
  • lib/data/db.dart
  • lib/data/series_codec.dart
  • lib/import/import_container.dart
  • test/coach_views_series_shapes_test.dart
  • test/day_result_reencode_test.dart
  • test/db_storage_hygiene_test.dart
  • test/import_container_test.dart
  • test/series_codec_test.dart

@abdulsaheel
abdulsaheel merged commit d25ff9a into OpenStrap:main Aug 11, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants