Repository navigation
docs(review): Phase 1 review passes, AD-2/5/6 verdicts and the real-backup gate - #24
Merged
Merged
Conversation
RealBackupGateTests imports each .noopbak named in NOOP_GATE_BACKUPS through DataBackup.restore, migrates it to head with WhoopStore(path:), exports it with the app's writer and imports the export again. Every table is compared by an order-independent row digest over every column, and every whitelisted setting by value. Tables a migration rewrites on purpose are listed with the migration that does it. The test skips when the variable is unset, so CI never runs it. It is the Phase 1 exit gate in docs/review/README.md.
… AD-6 Passes 1 Map, 2 Static sweep, 3 Deep read and 5 Adversarial ran as one workflow over W1 Protocol, W2 Storage and the safe-trim, backfill and live/historical-split part of W6 (5 reviewers, 3 adversaries). - W1: W01-001 and W01-002 closed as not a bug (ms R-R confirmed against the strap's own HR; the v26 base was already fixed). W01-003 to W01-009 added; W01-009 refuted. - W2: W02-001 (found while building the gate test) to W02-016; W02-015 refuted. S2: restore snapshot misses the WAL, export can ship a torn file, deleting a device leaves its computed rows. - W6: W06-001 lowered to S4; W06-002 to W06-024. S2: the 5/MG raw IMU session writer is unreachable. - AD-2, AD-5 and AD-6: Amend, with evidence and a design row each. - Exit gate, first half: RealBackupGateTests passes on three backups. All rows stay Reported until V0 (pass 4).
W02-005. "Delete all of this device's data" and "Forget device" call deleteAllData with the bare id, which left the device's computed sibling (<id>-noop: scored days, sleeps, detected workouts, metric series) on disk. The Repository kept reading it, so the deleted recordings' scores stayed on screen, and no rescore could evict them once their raw input was gone. deleteAllData now clears <id>-noop in the same transaction. With a re-added strap the engine writes every computed row under the canonical my-whoop-noop, so splitting those needs per-day ownership; recorded as W07-002 for Phase 2. V0: testDeleteAllDataAlsoClearsTheComputedSibling failed on main (the sibling row survived). V1: it passes, fails with the fix reverted, and the WhoopStore suite passes (546 tests).
W02-003. An export checkpointed the WAL and then zipped the live main file. Two things broke that: - A TRUNCATE checkpoint that a reader on the other pool cuts short reports busy in its result row, not as an error, so checkpointWAL() returned normally. The main file it left was malformed: the checkpoint writes only the pages whose newest frame predates the reader's mark. The pre-zip quick_check read through the WAL and passed, so the export reported success. - Rows committed after the checkpoint (a sync running while the macOS save panel is open, or the folder backup at launch overlapping the reconnect offload) were missing, and an autocheckpoint during the zip could tear the file. Exports now zip a snapshot that WhoopStore.writeSnapshot(to:) writes through SQLite's backup API from one read snapshot, staged in a temp file and removed afterwards. The quick_check now reads the bytes the archive carries. macOS takes the snapshot after the save panel, not before. checkpointWAL() throws CheckpointBlocked when a reader stops it. The failure message no longer claims recent changes are in the WAL; the new string carries all nine translations. V0: CheckpointBusyTests reproduced the malformed main file. V1: the package tests pass and fail with the busy check reverted; DataBackupLiveStoreTests restores every row from a folder backup taken under a reader and a writer, and fails with the old path. WhoopStore suite passes (546 tests); Strand builds.
W02-002. Before a restore replaces the live store, it copies it to a whoop-replaced-<ts>.sqlite side file, and a failed copy or a failed post-swap quick_check rolls back from that file. The copy took the main file alone and the swap then deleted the live -wal, so every commit not yet checkpointed was in neither place. A rollback put back a store without them while telling the user their data was kept. After a partial checkpoint the main file alone can even be malformed. The side file is now a consistent snapshot written through SQLite's backup API on a fresh connection, which reads through the WAL. A live file too damaged to read that way is kept byte for byte with its WAL, because a user restoring over a broken store still needs those bytes. Rollbacks put back whichever was kept. V0: with 50 committed rows still in the WAL the side file did not even hold the hrSample table. V1: it now holds all 50.
…d a live restore The gate now exports with DataBackup.writeBackup and a store open on the file, as the app does, and restores over that open store to check the side file it keeps. Settings still round-trip through the test seam, because the app's writer carries this device's own settings. On three real backups every table matches after the snapshot export, and the side file matches the store it replaced (W02-002, W02-003 V3).
…amp heal W02-006. The ryanbr#547 heal deleted every raw row dated before the plausibility floor (2023-11-14) from every source. An imported FIT, GPX or TCX activity stores its per-second heart rate under activity-file with the activity's real dates, so a heal re-run (after a bad-clock sync) stripped the heart rate from any workout older than the floor. The same function already spared imported computed rows. The far-past floor now spares WhoopStore.importedRawSourceIds; a future-dated row is still removed whatever wrote it. A StrandImport test pins the id to ActivityFileImporter.sourceId. V0: testImportedActivityHeartRateBelowFloorSurvives failed on main. V1: it passes and fails with the fix reverted; WhoopStore suite passes (547 tests).
…ther W02-008. BLEManager and Repository each hold a DatabasePool on the same file. Write transactions were DEFERRED, and SQLite cannot use the busy handler to upgrade a deferred transaction that has already read, so any write that reads first (upsertSleepSessions, for one) failed at once with SQLITE_BUSY while the other pool was writing. The 5 s busy timeout never applied, and IntelligenceEngine swallowed the failure, so that pass's rescored night was not stored. Write transactions are now IMMEDIATE, GRDB's documented setting for several writers on one file; readers stay deferred. V0: testReadFirstWriteWaitsForTheOtherInstancesWrite failed in 0.2 s with database is locked. V1: it passes; WhoopStore suite passes (548 tests).
W02-012. StreamStore.insert counted a v18 aux row for every statement it ran, while the comment beside the return promised rows accepted, like the other eight channels. A reconnect that re-offloaded records already on disk therefore printed v18aux=N as newly banked beside zeros for every other channel, and spent the retention-sweep budget on rows that were never written. V0: testResyncReportsNoNewAuxRows reported 2 on the duplicate insert. V1: it reports 0; WhoopStore suite passes (549 tests).
W02-011. Both migration comments justified deleting ecgCandidateSample with "the strap re-offloads v16 records on the next sync". The strap trims history once NOOP acks it, so the records behind those rows are gone and the purge was permanent. The v49 comment's escape hatch, the Test Centre export, only helps on the old build, before the migration runs. Comment-only; no migration body changes.
W02-013. stepDataRevisionSignature was documented as process-local, but its index lives on each WhoopStore instance, and the app opens two on one file. Strap steps are recorded on BLEManager's instance while the cycle cache reads Repository's, so for strap data the component never moves; the cache's days witness is what invalidates it. The doc now says so. Doc-only.
W02-010. WhoopStoreInfo.schemaVersion was the constant 18 from the first commit while the schema grew to 49 migrations, so every .noopbak manifest and every APP_VERSION_CHANGED event recorded 18 and could not tell one schema from another. It is now the registered migration count. Four tests that pinned 18 are gone; SchemaOracleTests pins the value to the oracle's migration list, so the constant cannot fall behind again. WhoopStore suite passes (545 tests).
W02-014. The activity-file import stored the file's per-second heart rate with try?, so a failed insert was dropped while the import reported success, and its comment said an identical timestamp overwrites when the insert keeps the first row. The insert now throws into the import's existing catch, which reports the failure; a retry is idempotent. The comment states first-wins, and InsertTests pins it.
W06-002. A Raw Data Collector session on a WHOOP 5/MG never recorded an IMU sample. The only writer, ImuSessionFileStore.append, sits in Collector.ingest(frame:parsed:), and nothing in the app calls that: ryanbr#1709 moved IMU storage behind the ingest seam, which the 5/MG frame loop does not enter. The session started the strap's 100 Hz stream, then exported nothing. The 5/MG loop now hands every frame to Collector.bankImuForSessions before the offload branch, so a session takes buffers from the live stream and from history sync, as ryanbr#1709 intended. Only intact 1244-byte buffers are banked, because Whoop5RawImu decodes without checking the CRC. V1: CollectorImuBankingTests (an intact real buffer is banked, a CRC-flipped copy is not); Strand and NOOPiOS (without the watch app) build. Not yet tested on hardware.
…#2429) The reassembler guarded a declared length below the family floor and above the 8 KB ceiling, and trusted everything between. indexOfSOF matches a bare 0xAA, which occurs inside payload as readily as at a frame start, so a stream that desyncs can put the read cursor on a payload byte whose next bytes read as a PLAUSIBLE length: past the floor, under the ceiling, accepted by both guards. feed() then waited for that many bytes and emitted them as one frame, consuming whatever valid frames fell inside it. They never reached a parser and nothing downstream could return them, because the CRC32 that rejects the bad frame runs after the cursor has advanced past them. The header checksum settles it without new protocol knowledge, because it already covers the length field on both families: CRC-8 over the two length bytes on WHOOP 4.0, CRC-16-Modbus over the first six on 5/MG. A real frame always passes; a false SOF passes with probability 1/256 or 1/65536. The gate sits AFTER the floor and ceiling checks so those keep their counters and their tests, and it only sees lengths the old code accepted. headerChecksumDrops counts the rejections and is folded into the connection's reject tally, under HEADER_CHECKSUM_MISMATCH rather than the floor bucket: a floor drop is a malformed frame, this is a read cursor that was not on a frame boundary at all. Without that fold the drops would have vanished with no trace, which is the failure the neighbouring absorbReassemblerDrops exists to prevent. Both absorbed totals clear on the tally's reset, or the fold reads as "already folded" against a total that never goes down and goes silent for every later connection. Both platforms start a connection with the counters and the absorbed totals at zero, so the fold cannot be silenced by a lifecycle mismatch: Apple reassigns the reassembler and sets router.family on adjacent lines, and its didSet clears the tally; Android resets the tally in STATE_CONNECTED, before service discovery reassigns the reassembler. trigger, "a bit-flip, or a spurious 0xAA injected mid-frame". This is the in-range half of the same problem. Mutation-checked on both platforms independently. With the gate removed, feed() returns nothing where two valid frames should be: the false SOF eats them both while waiting for 104 bytes. With the reset line removed, only the post-reset assertion fails. Every pre-existing framing test stays green under both mutations, which is what separates a gate that catches a false SOF from one that rejects real frames. Swift 791 tests, Kotlin 17 and 39, parity clean, 16 of 16 in CI. Not reproduced on hardware: the trigger needs a desync, and the reassembler is rebuilt per connection, so the residual window is a subscribe landing mid-frame. Found by an AI code review, which reported it as corruption under the ceiling. The mechanism is a false SOF after a desync rather than corruption, since BLE hands up only CRC-valid PDUs, and the severity is lower than it judged. The gap it pointed at is real, and the header-checksum resync it recommended is what this implements. (cherry picked from commit f15360d) Fork note (W01-003, docs/review/workstreams/W01-protocol.md): the fork has only the 5/MG family, so the header gate keeps the CRC-16 branch alone, the two WHOOP 4.0 tests are rewritten on 5/MG frames, and the Android half is dropped. Added: real-frame tests pinning the review's oracle (3 of 12 and 1 of 10 records intact before the gate; 12 and 10 after) and a Swift test for the reject-tally fold. WhoopProtocol suite passes (743 tests); Strand builds. Not tested on hardware.
W01-005. PROTOCOL_SENSORS.md, which the project treats as the authority, told readers that packet-40 and R18 R-R words are 1/1024 s ticks to be converted with (ticks * 1000 + 512) // 1024. The decoder reads them as milliseconds (ryanbr#2195), and the inspector note beside every R-R field said "ms from 1/1024 s ticks". A contributor following the doc would shorten every stored R-R interval and HRV value by 2.4 %. The doc rows and text now say milliseconds and why, PROTOCOL_CONCEPTS notes that WHOOP 5/MG departs from the BLE spec's unit, the tick arithmetic leaves validate_examples.py, and the note says the word is already milliseconds. Whoop5RRTests fails if an R-R note mentions 1024 again. The rr_raw_ticks key is kept: tests pin it.
…tact grade W01-008. R18 byte 63 is published twice, as motion_wear_quality and as activity_class. The first note read "0=still/good, 1, 2=poor contact" beside the second's "0=still, 1=walk, 2=run", so the inspector and whoop-decode gave two contradictory meanings for one byte, and PROTOCOL_SENSORS.md defines it as the activity class. The note now names motion_wear_quality as the older name of activity_class; the key and its value are unchanged. Nothing stores the older name.
W01-007. PROTOCOL_TRANSPORT.md said format-1 acceptance requires a frame longer than 15 bytes with (declared length - 4) divisible by four, a rule nothing implements. verifyFrame accepts 13-byte and larger frames without an alignment check (FrameLimits records the 13-byte floor as a deliberate, empirical policy), and PROTOCOL_IMPLEMENTATION.md already says 13. Both checksums still gate every frame. The doc now keeps the construction rule and states the acceptance NOOP enforces. FrameIntegrityTests pins 15-, 16- and 17-byte frames as accepted, so the doc and the code cannot drift apart unnoticed.
W01-004. The ryanbr#547 gate in extractHistoricalStreams drops a record whose own timestamp is implausible (before 2023-11 or more than a day ahead) so it cannot be filed on the wrong day. rejectedHistoricalRecords, the screen that archives every record NOOP cannot turn into rows before the trim ack, did not know about that gate: an intact v18 record passed it because it has a timestamp and heart rate, and an intact v16 or v26 record is skipped because its lane normally stores it. So the record was neither stored nor archived, and the ack freed the strap's only copy, although its bytes were fine and a clock fix could re-date it. rejectedHistoricalRecords now takes the same gate inputs (wall time and the session markers) and archives an intact record the gate refuses. The Backfiller passes the values its extraction used, and its drop log says the raw records go to the archive. V1: RejectedHistoryTests archives a far-past and a future-dated record and not a plausible one, and fails with the check disabled. WhoopProtocol suite passes (747 tests); the Backfiller app tests pass.
…evice W02-005 follow-up, from the V2 review of a617df3. That fix cleared <id>-noop with a device's recordings. But the engine writes every day it scores under the canonical my-whoop-noop, whichever registered source supplied it: a re-added strap (whoop-<uuid>), an activity file, Apple Health. So Forget on an archived my-whoop after a re-add, the very case Forget exists for, deleted every computed day, including days scored from the active strap, and every hand-edited night. Only the last 21 days rescore, and edits never come back. deleteAllData now clears the sibling only when it is the device's alone: a non-canonical id, or my-whoop with no other device registered. User- edited and hand-added nights are never cleared this way. With other devices registered, the canonical namespace stays, which leaves the W02-005 symptom in multi-device installs; that remainder is W07-002 and needs per-day attribution. Tests: the V2 scenario keeps the shared rows, and fails with the old rule; the single-device and non-canonical cases clear their own rows and keep edited nights. WhoopStore suite passes (549 tests).
… partial copy W02-002 follow-up, from the V2 review of b2d18a5. - A snapshot that failed part way (a full disk, an I/O error) left an empty side file behind. The restore's byte-copy fallback then failed with "already exists" and the whole restore aborted. The copy helper now removes a partial destination on any failure, and the fallback clears the side file before copying. - The snapshot's source connection was read-write, so when nothing else had the file open, closing it checkpointed the live WAL into the main file. The source is now read-only and never changes the live file; when it cannot open read-only, the byte-copy fallback keeps the files as they are. V0: under a file-size limit the copy failed and left a partial file. V1: CheckpointBusyTests.testAFailedSnapshotLeavesNoPartialCopy passes, and fails without the cleanup; a live file that cannot be snapshotted is kept byte for byte (DataBackupLiveStoreTests).
…iled W02-003 follow-up, from the V2 review of cf63992. A snapshot is a full physical copy of the store: the old export zipped the live file, and the old pre-restore copy was an APFS clone, so neither needed the space. On a nearly full volume the backup API filled the disk and only then failed; while the disk was full the strap sync's own writes could fail too. The export then said "Try again in a moment", a transient cause the code could not back, and folder auto-backup retried on every launch. The snapshot now checks that the volume can take the copy plus a 64 MB margin before writing anything, and throws SnapshotNoSpace otherwise. A restore falls back to the byte copy, which clones and needs no space. The snapshot closure now throws, and the export message carries the reason (not enough space, no store open yet, a SQLite error). The new string replaces the unreleased one, with all nine translations; the i18n gate passes.
W02-017 to W02-020, all pre-existing: a rollback that reports data kept without checking its copy, an iOS export left in tmp, the staging import copying another app's live database file by file, and no dashboard refresh after a device delete.
Owner decision, 2026-09-25: W02-004 goes to Phase 5 with the single store owner (W02-016), W02-007 to Phase 4 with AD-9, and W02-009 to Phase 3 with AD-14. The W2 log records this batch.
W01-006 gets a storage lane: a nullable rawRecord BLOB on the ECG and PPG tables holding the full intact record. The reject archive is not used, because it is a rolling 5 MB file that v20 and v21 records already saturate (W06-003).
…'s decisions on the board
BLE (W6): record IMU in 5/MG Raw Data Collector sessions
Protocol (W1): reassembler header gate, archive what the timestamp gate refuses, and doc truths
Storage (W2): backups that restore, deletes that keep what they should, and smaller fixes
The watch app compiles WhoopStore, and volumeAvailableCapacityForImportantUsageKey is unavailable on watchOS, so the NOOPiOS build failed. On watchOS the check now reads the plain available capacity. Verified by cross-compiling StrandAnalytics and its dependencies for the watchOS 10 simulator: the two CI errors without the change, a clean build with it.
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
2 participants
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.
Phase 1 (Data integrity) of the whole-project review: the review passes, the decisions they settle, and the real-backup check the phase closes on. Tracker only, plus one env-gated test. The fixes are in three stacked PRs based on this branch:
review/w02-fixes,review/w01-fixesandreview/w06-fixes.Review passes
Passes 1 Map, 2 Static sweep, 3 Deep read and 5 Adversarial ran as one workflow over W1 Protocol, W2 Storage and the safe-trim, backfill and live/historical-split part of W6: five reviewers, then one adversary per area that tried to refute every finding and to find what the first pass missed. Findings are recorded as rows in
docs/review/workstreams/W01-protocol.md,W02-storage.mdandW06-ble-collect.md; duplicates across slices are merged, and two claims the adversary refuted stay asNot a bugrows.Decisions
AD-2, AD-5 and AD-6 are Amend, with the evidence in
docs/review/DECISIONS.mdand a design row for each (W02-016, W06-023, W06-024). In short: oneWhoopStoreper process instead of two pools on one file; describe the live/historical split the 5/MG code actually has; and restate safe-trim as "every record the strap will free is in a durable lane or the archive before the ack".Real-backup gate
StrandTests/RealBackupGateTestsimports each.noopbaknamed inNOOP_GATE_BACKUPSthrough the app's restore code, migrates it to head, exports it and imports it again, comparing every table by an order-independent row digest and every whitelisted setting by value. It skips when the variable is unset, so CI never runs it:On three real backups (schema v46, v48, v49; 1.7M to 3.5M rows) every table that existed before is unchanged after migrating, apart from the ECG purge v48 and v49 document, and the round trip reproduces every row and setting.
Owner decisions recorded
W02-004 deferred to Phase 5, W02-007 to Phase 4, W02-009 to Phase 3; W01-006 decided (a storage lane). A strap run is agreed for the BLE-path fixes.
Verification
StrandTestspasses (1,966 tests, the gate skipped); the gate itself passes on three real backups; hygiene gates pass. No app behaviour changes in this PR.