From 8a91aad8658782d37d2af07c821570341ace3639 Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 3 Oct 2026 17:04:38 +0200 Subject: [PATCH 01/13] fix(store): deleting a strap's data clears the scores attributed to it from the shared namespace (W07-002) The engine writes every scored day under the canonical my-whoop-noop, whichever strap supplied it. Since W02-005's follow-up, deleteAllData left that namespace alone whenever another device was registered, so deleting a re-added strap's data, or the canonical strap's with a second one registered, kept every score computed from it on screen. scoreInputProvenance already names the day owner for each scored day's recovery, strain and Rest-point cells. deleteAttributedCanonicalRows runs first in the delete transaction: each cell naming the device goes; a day's row goes when no recovery or strain cell names another source, together with the day's unattributed cells; cells naming another source or an estimator stay. Sleeps and detected workouts carry no day key and go when only the device's heart rate covers them; edited nights stay. Days scored before provenance existed have no cells and stay. --- .../WhoopStore/DeviceRegistryStore.swift | 69 ++++++++++++- .../DeviceRegistryStoreTests.swift | 99 +++++++++++++++++++ docs/review/DECISIONS.md | 3 +- docs/review/workstreams/W07-app-data.md | 3 +- 4 files changed, 171 insertions(+), 3 deletions(-) diff --git a/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift b/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift index 539c7594b1..bfa867682b 100644 --- a/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift +++ b/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift @@ -183,8 +183,15 @@ public struct DeviceRegistryStore: Sendable { /// series) goes with it when it is the device's alone. Left behind, the scores computed from the deleted /// recordings stayed on screen, and no rescore could evict them once their raw input was gone (W02-005). /// Nights the user edited or added by hand are user data, not scores, and are never cleared this way. + /// + /// When the device's scores share the canonical namespace instead, only what is attributed to it goes + /// (`deleteAttributedCanonicalRows`, W07-002). That runs first, while the device's heart rate still exists. public func deleteAllData(deviceId: String) throws { try dbQueue.write { db in + let exclusiveSibling = try Self.exclusiveComputedSibling(db, of: deviceId) + if exclusiveSibling != Self.canonicalComputedId { + try Self.deleteAttributedCanonicalRows(db, of: deviceId) + } for table in Self.deviceScopedTables { try db.execute(sql: "DELETE FROM \(table) WHERE deviceId = ?", arguments: [deviceId]) } @@ -192,7 +199,7 @@ public struct DeviceRegistryStore: Sendable { // namespace. Forgetting a provider must remove those associations too. try db.execute(sql: "DELETE FROM scoreInputProvenance WHERE sourceId = ?", arguments: [deviceId]) - guard let sibling = try Self.exclusiveComputedSibling(db, of: deviceId) else { return } + guard let sibling = exclusiveSibling else { return } for table in Self.deviceScopedTables where table != "sleepSession" { try db.execute(sql: "DELETE FROM \(table) WHERE deviceId = ?", arguments: [sibling]) } @@ -216,6 +223,66 @@ public struct DeviceRegistryStore: Sendable { return others == 0 ? sibling : nil } + /// The computed namespace the engine writes every scored day under, whichever source supplied it. + static let canonicalComputedId = "my-whoop" + computedSuffix + + /// Removes from the shared canonical namespace what was computed from `deviceId` (W07-002). + /// + /// Scored days are attributed per cell by `scoreInputProvenance`, which names the source of each + /// day's recovery, strain and Rest-point series. Each series cell naming the device goes; a day's + /// `dailyMetric` row goes only when no recovery or strain cell of that day names another source, so a + /// mixed day keeps its row. A cell sourced from an estimator (weekly VO₂max) never names a device, and + /// a day scored before provenance existed has no cells, so both stay. + /// + /// Sleeps and detected workouts carry no day key, so they are attributed by heart rate: a session goes + /// when the device has heart rate inside it and no other source does. Nights the user edited or added + /// stay, as in the exclusive case. Must run before the device's heart rate is deleted. + static func deleteAttributedCanonicalRows(_ db: Database, of deviceId: String) throws { + let canonical = canonicalComputedId + let cells = try Row.fetchAll(db, sql: """ + SELECT day, key FROM scoreInputProvenance WHERE deviceId = ? AND sourceId = ? + """, arguments: [canonical, deviceId]) + var days = Set() + for cell in cells { + let day: String = cell["day"], key: String = cell["key"] + days.insert(day) + try db.execute(sql: "DELETE FROM metricSeries WHERE deviceId = ? AND day = ? AND key = ?", + arguments: [canonical, day, key]) + } + for day in days { + let otherSource = try Bool.fetchOne(db, sql: """ + SELECT EXISTS(SELECT 1 FROM scoreInputProvenance + WHERE deviceId = ? AND day = ? AND key IN ('recovery', 'strain') AND sourceId != ?) + """, arguments: [canonical, day, deviceId]) ?? true + guard !otherSource else { continue } + try db.execute(sql: "DELETE FROM dailyMetric WHERE deviceId = ? AND day = ?", arguments: [canonical, day]) + // The day was the device's alone, so its other computed cells were derived from it too: values a + // later pass stopped producing (which lose their provenance but keep their value) and the + // Healthspan and Fitness Age points, which are never attributed. A cell another source is + // named for stays. + try db.execute(sql: """ + DELETE FROM metricSeries AS m WHERE m.deviceId = ? AND m.day = ? + AND NOT EXISTS(SELECT 1 FROM scoreInputProvenance p + WHERE p.deviceId = m.deviceId AND p.day = m.day AND p.key = m.key AND p.sourceId != ?) + """, arguments: [canonical, day, deviceId]) + } + try db.execute(sql: "DELETE FROM scoreInputProvenance WHERE deviceId = ? AND sourceId = ?", + arguments: [canonical, deviceId]) + + let coveredOnlyByDevice = """ + EXISTS(SELECT 1 FROM hrSample h WHERE h.deviceId = :device AND h.ts BETWEEN s.startTs AND s.endTs) + AND NOT EXISTS(SELECT 1 FROM hrSample h WHERE h.deviceId != :device AND h.deviceId NOT LIKE '%' || :suffix + AND h.ts BETWEEN s.startTs AND s.endTs) + """ + let args: StatementArguments = ["canonical": canonical, "device": deviceId, "suffix": computedSuffix] + try db.execute(sql: """ + DELETE FROM sleepSession AS s WHERE s.deviceId = :canonical AND s.userEdited = 0 AND \(coveredOnlyByDevice) + """, arguments: args) + try db.execute(sql: """ + DELETE FROM workout AS s WHERE s.deviceId = :canonical AND \(coveredOnlyByDevice) + """, arguments: args) + } + /// #771: re-point the ACTIVE Oura device from its CoreBluetooth-UUID id (`activeId`, e.g. /// "oura-4DD70E24-…") onto its STABLE serial id (`serialId`, e.g. "oura-2H3B2405003655"), so a re-pair — /// which mints a fresh CB UUID — never orphans the ring's history again. The serial is read from the ring diff --git a/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift b/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift index c6f8e854c3..3348a6c9e0 100644 --- a/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift +++ b/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift @@ -240,6 +240,105 @@ final class DeviceRegistryStoreTests: XCTestCase { XCTAssertEqual(try rows(dbq, "sleepSession", "whoop-new-noop"), 1) } + // MARK: W07-002 — the shared canonical namespace, attributed per cell + + /// Seeds the canonical computed namespace the way the engine leaves it with two straps registered: + /// day A scored from `whoop-new`, day B from `my-whoop`, day M mixed (recovery from a legacy snapshot of + /// `my-whoop`, strain from `whoop-new`), day L from before provenance existed, and a weekly VO₂max cell on + /// day A whose source is an estimator, not a device. An unattributed cell sits on days A, B and M, as a + /// value a later pass stopped producing does. Sleeps and a detected workout carry no day key, so they + /// are attributed by whose heart rate covers them. + private func seedSharedNamespace(_ dbq: DatabaseQueue) throws { + try dbq.write { db in + for day in ["A", "B", "M", "L"] { + try db.execute(sql: "INSERT INTO dailyMetric (deviceId, day, recovery, strain) VALUES ('my-whoop-noop', ?, 60, 10)", + arguments: [day]) + try db.execute(sql: "INSERT INTO metricSeries (deviceId, day, key, value) VALUES ('my-whoop-noop', ?, 'sleep_performance', 80)", + arguments: [day]) + } + try db.execute(sql: "INSERT INTO metricSeries (deviceId, day, key, value) VALUES ('my-whoop-noop', 'A', 'vo2max_est', 45)") + // Unattributed cells: a value a later pass stopped producing (it keeps its value, loses its + // provenance) on day A, day M and day B, and the Healthspan model marker on its sentinel day. + for day in ["A", "M", "B"] { + try db.execute(sql: "INSERT INTO metricSeries (deviceId, day, key, value) VALUES ('my-whoop-noop', ?, 'spo2_candidate', 95)", + arguments: [day]) + } + try db.execute(sql: "INSERT INTO metricSeries (deviceId, day, key, value) VALUES ('my-whoop-noop', '1970-01-01', 'healthspan_model', 2)") + let cells: [(String, String, String)] = [ + ("A", "recovery", "whoop-new"), ("A", "strain", "whoop-new"), ("A", "sleep_performance", "whoop-new"), + ("A", "vo2max_est", "nes"), + ("B", "recovery", "my-whoop"), ("B", "strain", "my-whoop"), ("B", "sleep_performance", "my-whoop"), + ("M", "recovery", "my-whoop"), ("M", "strain", "whoop-new"), ("M", "sleep_performance", "whoop-new"), + ] + for (day, key, source) in cells { + try db.execute(sql: "INSERT INTO scoreInputProvenance (deviceId, day, key, sourceId) VALUES ('my-whoop-noop', ?, ?, ?)", + arguments: [day, key, source]) + } + // Heart rate: whoop-new alone over 1000…2000, both straps over 3000…4000, my-whoop alone over 7000…8000. + for (dev, ts) in [("whoop-new", 1500), ("whoop-new", 3500), ("my-whoop", 3600), ("whoop-new", 5500), ("my-whoop", 7500)] { + try db.execute(sql: "INSERT INTO hrSample (deviceId, ts, bpm) VALUES (?, ?, 60)", arguments: [dev, ts]) + } + for (start, end, edited) in [(1000, 2000, 0), (3000, 4000, 0), (5000, 6000, 1), (7000, 8000, 0)] { + try db.execute(sql: "INSERT INTO sleepSession (deviceId, startTs, endTs, userEdited) VALUES ('my-whoop-noop', ?, ?, ?)", + arguments: [start, end, edited]) + } + for (start, end) in [(1000, 2000), (7000, 8000)] { + try db.execute(sql: "INSERT INTO workout (deviceId, startTs, endTs, sport, source) VALUES ('my-whoop-noop', ?, ?, 'Running', 'detected')", + arguments: [start, end]) + } + } + } + + private func canonicalCells(_ dbq: DatabaseQueue) throws -> (days: [String], series: [String], sleeps: [Int], workouts: [Int]) { + try dbq.read { db in + (try String.fetchAll(db, sql: "SELECT day FROM dailyMetric WHERE deviceId = 'my-whoop-noop' ORDER BY day"), + try String.fetchAll(db, sql: "SELECT day || ':' || key FROM metricSeries WHERE deviceId = 'my-whoop-noop' ORDER BY day, key"), + try Int.fetchAll(db, sql: "SELECT startTs FROM sleepSession WHERE deviceId = 'my-whoop-noop' ORDER BY startTs"), + try Int.fetchAll(db, sql: "SELECT startTs FROM workout WHERE deviceId = 'my-whoop-noop' ORDER BY startTs")) + } + } + + /// Deleting a re-added strap's data takes the scores computed from it out of the canonical namespace: + /// the cells its provenance names, the day rows it alone supplied, and the sleeps and detected workouts + /// only its heart rate covers. Another strap's cells, a mixed day's row, a day with no provenance, the + /// estimator-sourced VO₂max cell and a night the user edited all stay. + func testDeletingAReAddedStrapClearsTheCanonicalCellsAttributedToIt() throws { + let dbq = try makeDB() + let store = DeviceRegistryStore(dbQueue: dbq) + try store.add(PairedDevice(id: "whoop-new", brand: "WHOOP", model: "WHOOP 5.0 / MG", sourceKind: .liveBLE, + capabilities: [.hr], status: .active, addedAt: 1, lastSeenAt: 1)) + try seedSharedNamespace(dbq) + + try store.deleteAllData(deviceId: "whoop-new") + + let left = try canonicalCells(dbq) + XCTAssertEqual(left.days, ["B", "L", "M"], "day A was whoop-new's alone; a mixed day keeps its row") + XCTAssertEqual(left.series, ["1970-01-01:healthspan_model", "A:vo2max_est", "B:sleep_performance", + "B:spo2_candidate", "L:sleep_performance", "M:spo2_candidate"], + "day A's unattributed cell goes with the day; a mixed day keeps its unattributed cell") + XCTAssertEqual(left.sleeps, [3000, 5000, 7000], "a night only whoop-new's heart rate covers goes; an edited one stays") + XCTAssertEqual(left.workouts, [7000]) + } + + /// The same rule from the other side: deleting the canonical strap's data while a re-added strap is + /// registered takes only the canonical strap's cells. + func testDeletingTheCanonicalStrapClearsOnlyItsCellsFromASharedNamespace() throws { + let dbq = try makeDB() + let store = DeviceRegistryStore(dbQueue: dbq) + try store.add(PairedDevice(id: "whoop-new", brand: "WHOOP", model: "WHOOP 5.0 / MG", sourceKind: .liveBLE, + capabilities: [.hr], status: .active, addedAt: 1, lastSeenAt: 1)) + try seedSharedNamespace(dbq) + + try store.deleteAllData(deviceId: "my-whoop") + + let left = try canonicalCells(dbq) + XCTAssertEqual(left.days, ["A", "L", "M"]) + XCTAssertEqual(left.series, ["1970-01-01:healthspan_model", "A:sleep_performance", "A:spo2_candidate", + "A:vo2max_est", "L:sleep_performance", "M:sleep_performance", "M:spo2_candidate"]) + XCTAssertEqual(left.sleeps, [1000, 3000, 5000]) + XCTAssertEqual(left.workouts, [1000]) + } + // Regression guard (audit finding): every table with a `deviceId` column MUST appear in // `deviceScopedTables`, or `deleteAllData` silently leaves that device's rows behind — a privacy // defect for a delete-means-gone app. Enumerate the live schema and fail if any deviceId-keyed table diff --git a/docs/review/DECISIONS.md b/docs/review/DECISIONS.md index 735f589342..c7d918eaaf 100644 --- a/docs/review/DECISIONS.md +++ b/docs/review/DECISIONS.md @@ -242,7 +242,8 @@ Separate the routing token from the partition id, and replace the three strap-se pure resolver of registry rows and the active id (worn-strap set and owner precedence) that settles archived straps one way. `Repository`'s unions, `resolveDayOwner`, the day-cycle candidates, `selfHealEditedStages`, `restageFromRaw`, `rescoreManualWorkouts` and the Health bridge call it. Per-day -attribution of computed rows closes W07-002. The single-id readers are fixed as bugs first; the resolver +attribution of computed rows replaces the provenance-based delete that closed W07-002 (which cannot +reach days scored before provenance existed). The single-id readers are fixed as bugs first; the resolver is the Phase 5 refactor (with AD-1). ### AD-8 diff --git a/docs/review/workstreams/W07-app-data.md b/docs/review/workstreams/W07-app-data.md index 8ff92006fb..2f57d584a4 100644 --- a/docs/review/workstreams/W07-app-data.md +++ b/docs/review/workstreams/W07-app-data.md @@ -61,7 +61,7 @@ the two biggest hot spots). | ID | Sev | Status | Finding | Location | Evidence | PR | |---|---|---|---|---|---|---| | W07-001 | S3 | Reported | `"my-whoop"` literal at 193 sites, against the `AGENTS.md` rule that reads thread the active strap id | `MetricCatalog.swift` 43, `TodayView.swift` 21, `Repository.swift` 16 | Pattern scan in `BASELINE.md` Phase 2 review (2026-10-03): re-scoped. 191 code and comment lines carry the exact token; most code sites are routing tokens that Repository maps to the active and canonical union. The real active-id violators are single-id readers: `selfHealEditedStages` (W07-023), `rescoreManualWorkouts` (W07-028), `resolveDayOwner` (W07-024), `HealthKitBridge` (W07-031), and the engine's fixed write target (W07-002). See AD-7 | -| W07-002 | S2 | Reported | Deleting a re-added strap's data (`whoop-`) leaves the scores computed from it, because the engine writes every computed row under the canonical `my-whoop-noop`; deleting `my-whoop` clears that canonical sibling, including days scored from another strap | `IntelligenceEngine.swift` `deviceId`; `DeviceRegistryStore.swift` `deleteAllData` | Remainder of W02-005: with more than one registered device the canonical computed namespace is shared, so `deleteAllData` now leaves it in place rather than deleting another device's days (the V2 regression of a617df30). Deleting a device in a multi-device install still leaves the scores computed from it on screen. Needs per-day attribution (`dayOwnership` records overrides only; provenance covers only some metrics). AD-7 | | +| W07-002 | S2 | Fixed | Deleting a re-added strap's data (`whoop-`) leaves the scores computed from it, because the engine writes every computed row under the canonical `my-whoop-noop`; deleting `my-whoop` clears that canonical sibling, including days scored from another strap | `IntelligenceEngine.swift` `deviceId`; `DeviceRegistryStore.swift` `deleteAllData` | Remainder of W02-005: with more than one registered device the canonical computed namespace is shared, so `deleteAllData` left it in place, and the scores computed from a deleted strap stayed on screen. AD-7. Phase 2 re-check: code unchanged on `review/phase-2`. V0: `DeviceRegistryStoreTests.testDeletingAReAddedStrapClearsTheCanonicalCellsAttributedToIt` and `…TheCanonicalStrapClearsOnlyItsCellsFromASharedNamespace` failed (nothing left the shared namespace). Fix: `deleteAttributedCanonicalRows` runs first in the delete transaction whenever the canonical namespace is not the device's alone. `scoreInputProvenance` already attributes each scored day's recovery, strain and Rest-point cells to the day owner, so each cell naming the device goes; a day's row goes when no recovery or strain cell names another source, and with it the day's unattributed cells (stale values that lost their provenance, Healthspan and Fitness Age points); cells naming another source or an estimator stay. Sleeps and detected workouts go when only the device's heart rate covers them; edited nights stay. V1: both tests pass; four mutations (no mixed-day guard, no other-source heart-rate guard, edited nights not spared, no whole-day sweep) each fail them; WhoopStore 556 pass. V3 (copy of the 2026-09-30 backup, a second strap given 7 days' provenance, one of them mixed, and the heart rate under 3 nights): 6 day rows, 266 series cells, 134 provenance cells and 3 nights removed, all on those 7 days; the mixed day kept its row and lost only the 17 cells naming the strap; the other strap's heart rate untouched. Limits: a day scored before provenance (v29) has no cells and stays; a mixed day keeps the deleted strap's columns in its row; a day another strap also covers comes back under it on the next pass inside the window; nothing re-scores after the delete (W07-017). V2 due | | | W07-003 | S3 | Reported | A stored sleep session that re-detection no longer produces is never removed unless it overlaps a fresh one: sleep has no delete-reinsert reconcile (the #899 heal only collapses overlaps, as its own comment says), so a once-detected session survives every later pass | `IntelligenceEngine.swift` sleep write and #899 heal; `MetricsCache.swift` `upsertSleepSessions` | V0 (2026-09-27 export, replay harness on 11.9.12): 1 of 10 stored sessions, a short daytime one with no user edit, is found by neither V2 nor V3 over its day's full data, nor at 8 window cuts from 5 min to 10 h after its end, nor with the band, gravity or R-R stream removed, nor with band and gravity trimmed at its start; the band never scored SLEEP inside it. What produced it is not reconstructed. It stays out of the day's sleep total, but every reader of `sleepSession` sees it | | | W07-004 | S3 | Reported | Post-offload re-scores that run in the background cost 27.6–79.5 s of CPU each (52–488 s elapsed), every 10 minutes while the strap offloads; foreground passes cost 1.9–26.8 s | `IntelligenceEngine` post-offload re-score | V0 (2026-09-27 log, 11.9.12): 13 `re-score: cost` lines, 5 backgrounded; one backgrounded pass's `steps` phase took 591 s of wall time at a 16 % CPU share. Battery cost not measured Phase 2 review (2026-10-03, W07-022): the `cpu=` figures above are process-wide (`RUSAGE_SELF` over the pass), not the pass's own cost | | W07-005 | S4 | Verified | Every re-score reprints its per-day diagnostic lines (sleep, resp, hrv and rhr for about 10 days each, plus workout detect and effort), mostly unchanged since the last pass: after W06-118 they are the largest share of a strap log and limit its ring to about 2 h | `IntelligenceEngine.swift` per-day `diagnosticSink` lines; `RepeatedDayLineFilter.swift` | Found by W06-108's V2 review. V0 (2026-09-27 log, current session, 64 min): 12 re-scores, 960 `day=` lines. Correction: not all ungated. `dayOwner` (`.universal`, on whenever any Test Centre mode is) is 120 of them and `detectedBout` (`.workouts`) 12 more; the untagged per-day lines are 840, 70 a pass, and 733 are byte-identical to the previous pass. V0 test: `testAnUnchangedPassWithholdsItsDayLinesAndCountsThem` fails on the unfixed code (pass 2 with `dayCache reused=2/2` reprints every per-day line). Fixed (change-only, not a Test Centre gate: these lines are the proof of what was scored in a report sent without Test Centre): the untagged per-day lines go through `emitDayLine` → `RepeatedDayLineFilter`, keyed by the text through `day=YYYY-MM-DD` plus the occurrence in the pass. A pass withholds a line identical to the last print on its key and ends with `re-score: N per-day line(s) unchanged since their last print, not repeated (oldest print HH:mm:ss)`. An unchanged line prints again once its last print is 1 h old: at this log's density with W06-118 and this fix (about 1,800 lines an hour) the durable tail's 2,000 lines span about 67 min and the live ring about 2.8 h, so each scored day keeps a copy in both; a generation (1,000 lines) cannot be promised one, and its job is the stop. Readers checked: `CaptureAccumulator` (the Test Centre "K of N" row) and `CaptureCompleteness` (the bundle's INCOMPLETE guard) count distinct `sleep day=` days or presence, and both scan `live.exportableLogText()`, the whole ring plus generations rather than the lines since a mode started, so the hourly copy keeps every scored day where they look. Lines are compared before `redactPii` (counts and day keys only). V1: `RepeatedDayLineTests` (10) pass; `StrandTests` (2,069, 2 skipped) passes; `Strand` and `NOOPiOS` (no watch) build. Mutations: printing every line fails the 2 engine tests; dropping the per-line age check fails `testALineWhoseHourEndsDuringAPassPrints`; dropping the occurrence index fails 7. Oracle (the Swift filter over the log's 12 passes): 840 → 171 printed + 11 summaries; the first pass prints 70 and the hour refresh 68. Left, recorded here: `dayOwner` still prints 10 lines a pass with any Test Centre mode on (so in W06-108's V3 run), and per-pass constants (`sleep SKIPPED`, `Dedup(#899) … removed 0`, `detectedBout`) repeat too; the next lever if the 2 h target is missed. V2 (independent subagent) held items 1, 2, 4 and 5 (the sink cannot drop an admitted line; no early return between `beginPass` and the summary; no other path prints these lines; no reader expects one per pass) and reproduced the oracle; it found W07-006 … W07-009, and W07-006 … W07-008 are fixed on the same branch. After them the oracle is unchanged: 840 → 171 + 11 summaries, none naming a vanished line. V3 due: the W06-108 strap run's log V3 (2026-09-27 evening export, 11.9.13): 18 re-score passes in 82 min printed 366 per-day lines and 16 summaries where the old code would have printed 1,422 (79 a pass). The launch pass and the hourly refresh printed all 79; each of the other 16 printed 13 and withheld 68 under one summary. Second V3 (2026-09-28 export, 11.9.13): passes print the withheld count ("77 per-day line(s) unchanged since their last print, not repeated" on the pass after a launch's first). Batch check: a day and a night on 11.9.13. | | @@ -113,3 +113,4 @@ the two biggest hot spots). - 2026-09-30 — W07-016 fixed on `review/w07-launch-seed` (stacked on `review/strap-run-2026-09-30`): the launch seed's refresh and re-score are logged as a launch seed, not a completed sync. - 2026-10-03 — Phase 2 started; W7 claimed for the Phase 2 review workflow (passes 1, 2, 3 and 5 in two slices: orchestration; resolution and partition). W07-002 (S2) is fixed first. - 2026-10-03 — Passes 1, 2, 3 and 5 done in workflow run `wf_b77f01a5-1aa` (two reviewers: orchestration; resolution and partition; one adversary). W07-017 … W07-031 recorded, with W3's single-offset and Rest-consistency findings merged into W07-018 and W07-019. P5 refuted none; it moved W7b-8 (a restore cannot reproduce derived rows when the exporter changed an engine toggle; the toggles are deliberately outside the whitelist) to AD-8 evidence rather than a row. W07-002 confirmed unchanged on `review/phase-2`. Carried rows: W07-001 re-scoped, W07-015's cause named, W07-004's CPU figures corrected. Fix history grouped by cause (196 commits touching `IntelligenceEngine.swift`): pass cost, single-flight, cache-key misses, day boundaries, device partition, write-reconcile asymmetry, score methodology, log truth; the open residue of each is a row here. Not covered: StrandTests runs, the replay diff, restore then re-score, the background CPU premium (needs Instruments). Next: W07-002 (S2) first, then W07-023 (S2). +- 2026-10-03 — W07-002 fixed on `review/w07-fixes` (off `review/phase-2`): provenance-attributed delete from the shared canonical namespace. V2 due. From 3594b2e0cf61c49940b09430329cd3279c900002 Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 3 Oct 2026 17:28:59 +0200 Subject: [PATCH 02/13] fix(store): the attributed delete probes heart rate by key, not by index scan (W07-032) W07-002's sleep and workout delete asked whether any other source had heart rate inside each canonical session with a deviceId != predicate, which cannot use the (deviceId, ts) key: SQLite scanned the whole heart-rate index once per session, inside the delete's write transaction. On 115 days of 1 Hz rows that was 25 to 53 s with offload inserts and every store read waiting behind it. The other heart-rate sources are now read once by a skip scan over the key, and each session is probed with key lookups. --- .../WhoopStore/DeviceRegistryStore.swift | 55 ++++++++++++++----- docs/review/workstreams/W07-app-data.md | 6 +- 2 files changed, 46 insertions(+), 15 deletions(-) diff --git a/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift b/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift index bfa867682b..3d41564859 100644 --- a/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift +++ b/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift @@ -234,8 +234,9 @@ public struct DeviceRegistryStore: Sendable { /// mixed day keeps its row. A cell sourced from an estimator (weekly VO₂max) never names a device, and /// a day scored before provenance existed has no cells, so both stay. /// - /// Sleeps and detected workouts carry no day key, so they are attributed by heart rate: a session goes - /// when the device has heart rate inside it and no other source does. Nights the user edited or added + /// Sleeps and workouts under the canonical id (detected ones; user workouts live under the strap's own + /// id) carry no day key, so they are attributed by heart rate: a session goes when the device has heart + /// rate inside it and no other source does. Nights the user edited or added /// stay, as in the exclusive case. Must run before the device's heart rate is deleted. static func deleteAttributedCanonicalRows(_ db: Database, of deviceId: String) throws { let canonical = canonicalComputedId @@ -269,18 +270,44 @@ public struct DeviceRegistryStore: Sendable { try db.execute(sql: "DELETE FROM scoreInputProvenance WHERE deviceId = ? AND sourceId = ?", arguments: [canonical, deviceId]) - let coveredOnlyByDevice = """ - EXISTS(SELECT 1 FROM hrSample h WHERE h.deviceId = :device AND h.ts BETWEEN s.startTs AND s.endTs) - AND NOT EXISTS(SELECT 1 FROM hrSample h WHERE h.deviceId != :device AND h.deviceId NOT LIKE '%' || :suffix - AND h.ts BETWEEN s.startTs AND s.endTs) - """ - let args: StatementArguments = ["canonical": canonical, "device": deviceId, "suffix": computedSuffix] - try db.execute(sql: """ - DELETE FROM sleepSession AS s WHERE s.deviceId = :canonical AND s.userEdited = 0 AND \(coveredOnlyByDevice) - """, arguments: args) - try db.execute(sql: """ - DELETE FROM workout AS s WHERE s.deviceId = :canonical AND \(coveredOnlyByDevice) - """, arguments: args) + // Each session is probed with key lookups on (deviceId, ts): one for the device, one per other heart-rate + // source. A `deviceId != ?` predicate cannot use the key and scanned the whole heart-rate index per + // session, 25–53 s on 115 days of 1 Hz rows inside this write transaction (W07-002 V2). + let otherSources = try heartRateSourceIds(db).filter { $0 != deviceId && !$0.hasSuffix(computedSuffix) } + func hasHeartRate(_ id: String, _ start: Int, _ end: Int) throws -> Bool { + try Bool.fetchOne(db, sql: "SELECT EXISTS(SELECT 1 FROM hrSample WHERE deviceId = ? AND ts BETWEEN ? AND ?)", + arguments: [id, start, end]) ?? false + } + func coveredOnlyByDevice(_ start: Int, _ end: Int) throws -> Bool { + guard try hasHeartRate(deviceId, start, end) else { return false } + return try !otherSources.contains { try hasHeartRate($0, start, end) } + } + for row in try Row.fetchAll(db, sql: "SELECT startTs, endTs FROM sleepSession WHERE deviceId = ? AND userEdited = 0", + arguments: [canonical]) { + let start: Int = row["startTs"], end: Int = row["endTs"] + guard try coveredOnlyByDevice(start, end) else { continue } + try db.execute(sql: "DELETE FROM sleepSession WHERE deviceId = ? AND startTs = ?", arguments: [canonical, start]) + } + for row in try Row.fetchAll(db, sql: "SELECT startTs, endTs, sport FROM workout WHERE deviceId = ?", + arguments: [canonical]) { + let start: Int = row["startTs"], end: Int = row["endTs"], sport: String = row["sport"] + guard try coveredOnlyByDevice(start, end) else { continue } + try db.execute(sql: "DELETE FROM workout WHERE deviceId = ? AND startTs = ? AND sport = ?", + arguments: [canonical, start, sport]) + } + } + + /// The distinct `deviceId`s holding heart rate, read by skipping through the (deviceId, ts) key one id at a + /// time rather than scanning every row. + static func heartRateSourceIds(_ db: Database) throws -> [String] { + try String.fetchAll(db, sql: """ + WITH RECURSIVE ids(id) AS ( + SELECT MIN(deviceId) FROM hrSample + UNION ALL + SELECT (SELECT MIN(deviceId) FROM hrSample WHERE deviceId > ids.id) FROM ids WHERE ids.id IS NOT NULL + ) + SELECT id FROM ids WHERE id IS NOT NULL + """) } /// #771: re-point the ACTIVE Oura device from its CoreBluetooth-UUID id (`activeId`, e.g. diff --git a/docs/review/workstreams/W07-app-data.md b/docs/review/workstreams/W07-app-data.md index 2f57d584a4..ebd5b09ea8 100644 --- a/docs/review/workstreams/W07-app-data.md +++ b/docs/review/workstreams/W07-app-data.md @@ -61,7 +61,7 @@ the two biggest hot spots). | ID | Sev | Status | Finding | Location | Evidence | PR | |---|---|---|---|---|---|---| | W07-001 | S3 | Reported | `"my-whoop"` literal at 193 sites, against the `AGENTS.md` rule that reads thread the active strap id | `MetricCatalog.swift` 43, `TodayView.swift` 21, `Repository.swift` 16 | Pattern scan in `BASELINE.md` Phase 2 review (2026-10-03): re-scoped. 191 code and comment lines carry the exact token; most code sites are routing tokens that Repository maps to the active and canonical union. The real active-id violators are single-id readers: `selfHealEditedStages` (W07-023), `rescoreManualWorkouts` (W07-028), `resolveDayOwner` (W07-024), `HealthKitBridge` (W07-031), and the engine's fixed write target (W07-002). See AD-7 | -| W07-002 | S2 | Fixed | Deleting a re-added strap's data (`whoop-`) leaves the scores computed from it, because the engine writes every computed row under the canonical `my-whoop-noop`; deleting `my-whoop` clears that canonical sibling, including days scored from another strap | `IntelligenceEngine.swift` `deviceId`; `DeviceRegistryStore.swift` `deleteAllData` | Remainder of W02-005: with more than one registered device the canonical computed namespace is shared, so `deleteAllData` left it in place, and the scores computed from a deleted strap stayed on screen. AD-7. Phase 2 re-check: code unchanged on `review/phase-2`. V0: `DeviceRegistryStoreTests.testDeletingAReAddedStrapClearsTheCanonicalCellsAttributedToIt` and `…TheCanonicalStrapClearsOnlyItsCellsFromASharedNamespace` failed (nothing left the shared namespace). Fix: `deleteAttributedCanonicalRows` runs first in the delete transaction whenever the canonical namespace is not the device's alone. `scoreInputProvenance` already attributes each scored day's recovery, strain and Rest-point cells to the day owner, so each cell naming the device goes; a day's row goes when no recovery or strain cell names another source, and with it the day's unattributed cells (stale values that lost their provenance, Healthspan and Fitness Age points); cells naming another source or an estimator stay. Sleeps and detected workouts go when only the device's heart rate covers them; edited nights stay. V1: both tests pass; four mutations (no mixed-day guard, no other-source heart-rate guard, edited nights not spared, no whole-day sweep) each fail them; WhoopStore 556 pass. V3 (copy of the 2026-09-30 backup, a second strap given 7 days' provenance, one of them mixed, and the heart rate under 3 nights): 6 day rows, 266 series cells, 134 provenance cells and 3 nights removed, all on those 7 days; the mixed day kept its row and lost only the 17 cells naming the strap; the other strap's heart rate untouched. Limits: a day scored before provenance (v29) has no cells and stays; a mixed day keeps the deleted strap's columns in its row; a day another strap also covers comes back under it on the next pass inside the window; nothing re-scores after the delete (W07-017). V2 due | | +| W07-002 | S2 | Fixed | Deleting a re-added strap's data (`whoop-`) leaves the scores computed from it, because the engine writes every computed row under the canonical `my-whoop-noop`; deleting `my-whoop` clears that canonical sibling, including days scored from another strap | `IntelligenceEngine.swift` `deviceId`; `DeviceRegistryStore.swift` `deleteAllData` | Remainder of W02-005: with more than one registered device the canonical computed namespace is shared, so `deleteAllData` left it in place, and the scores computed from a deleted strap stayed on screen. AD-7. Phase 2 re-check: code unchanged on `review/phase-2`. V0: `DeviceRegistryStoreTests.testDeletingAReAddedStrapClearsTheCanonicalCellsAttributedToIt` and `…TheCanonicalStrapClearsOnlyItsCellsFromASharedNamespace` failed (nothing left the shared namespace). Fix: `deleteAttributedCanonicalRows` runs first in the delete transaction whenever the canonical namespace is not the device's alone. `scoreInputProvenance` already attributes each scored day's recovery, strain and Rest-point cells to the day owner, so each cell naming the device goes; a day's row goes when no recovery or strain cell names another source, and with it the day's unattributed cells (stale values that lost their provenance, Healthspan and Fitness Age points); cells naming another source or an estimator stay. Sleeps and detected workouts go when only the device's heart rate covers them; edited nights stay. V1: both tests pass; four mutations (no mixed-day guard, no other-source heart-rate guard, edited nights not spared, no whole-day sweep) each fail them; WhoopStore 556 pass. V3 (copy of the 2026-09-30 backup, a second strap given 7 days' provenance, one of them mixed, and the heart rate under 3 nights): 6 day rows, 266 series cells, 134 provenance cells and 3 nights removed, all on those 7 days; the mixed day kept its row and lost only the 17 cells naming the strap; the other strap's heart rate untouched. Limits: a day scored before provenance (v29) has no cells and stays; a mixed day keeps the deleted strap's columns in its row; a day another strap also covers comes back under it on the next pass inside the window; nothing re-scores after the delete (W07-017). V2 (independent subagent, 2026-10-03): holds with caveats; no realistic over-deletion (canonical strap with another registered, Apple Health, activity file, estimator cells, legacy snapshots, naps and edited nights all probed). Caveats: the canonical-id workout delete has no source filter, so a legacy manual row under `my-whoop-noop` covered only by the device's heart rate goes too (the exclusive path does the same); `vo2max_est` stays on a day the device owned alone; a day whose recovery came from a pre-provenance snapshot goes with it. Its findings are W07-032 (fixed), W07-033 and W07-034 | | | W07-003 | S3 | Reported | A stored sleep session that re-detection no longer produces is never removed unless it overlaps a fresh one: sleep has no delete-reinsert reconcile (the #899 heal only collapses overlaps, as its own comment says), so a once-detected session survives every later pass | `IntelligenceEngine.swift` sleep write and #899 heal; `MetricsCache.swift` `upsertSleepSessions` | V0 (2026-09-27 export, replay harness on 11.9.12): 1 of 10 stored sessions, a short daytime one with no user edit, is found by neither V2 nor V3 over its day's full data, nor at 8 window cuts from 5 min to 10 h after its end, nor with the band, gravity or R-R stream removed, nor with band and gravity trimmed at its start; the band never scored SLEEP inside it. What produced it is not reconstructed. It stays out of the day's sleep total, but every reader of `sleepSession` sees it | | | W07-004 | S3 | Reported | Post-offload re-scores that run in the background cost 27.6–79.5 s of CPU each (52–488 s elapsed), every 10 minutes while the strap offloads; foreground passes cost 1.9–26.8 s | `IntelligenceEngine` post-offload re-score | V0 (2026-09-27 log, 11.9.12): 13 `re-score: cost` lines, 5 backgrounded; one backgrounded pass's `steps` phase took 591 s of wall time at a 16 % CPU share. Battery cost not measured Phase 2 review (2026-10-03, W07-022): the `cpu=` figures above are process-wide (`RUSAGE_SELF` over the pass), not the pass's own cost | | W07-005 | S4 | Verified | Every re-score reprints its per-day diagnostic lines (sleep, resp, hrv and rhr for about 10 days each, plus workout detect and effort), mostly unchanged since the last pass: after W06-118 they are the largest share of a strap log and limit its ring to about 2 h | `IntelligenceEngine.swift` per-day `diagnosticSink` lines; `RepeatedDayLineFilter.swift` | Found by W06-108's V2 review. V0 (2026-09-27 log, current session, 64 min): 12 re-scores, 960 `day=` lines. Correction: not all ungated. `dayOwner` (`.universal`, on whenever any Test Centre mode is) is 120 of them and `detectedBout` (`.workouts`) 12 more; the untagged per-day lines are 840, 70 a pass, and 733 are byte-identical to the previous pass. V0 test: `testAnUnchangedPassWithholdsItsDayLinesAndCountsThem` fails on the unfixed code (pass 2 with `dayCache reused=2/2` reprints every per-day line). Fixed (change-only, not a Test Centre gate: these lines are the proof of what was scored in a report sent without Test Centre): the untagged per-day lines go through `emitDayLine` → `RepeatedDayLineFilter`, keyed by the text through `day=YYYY-MM-DD` plus the occurrence in the pass. A pass withholds a line identical to the last print on its key and ends with `re-score: N per-day line(s) unchanged since their last print, not repeated (oldest print HH:mm:ss)`. An unchanged line prints again once its last print is 1 h old: at this log's density with W06-118 and this fix (about 1,800 lines an hour) the durable tail's 2,000 lines span about 67 min and the live ring about 2.8 h, so each scored day keeps a copy in both; a generation (1,000 lines) cannot be promised one, and its job is the stop. Readers checked: `CaptureAccumulator` (the Test Centre "K of N" row) and `CaptureCompleteness` (the bundle's INCOMPLETE guard) count distinct `sleep day=` days or presence, and both scan `live.exportableLogText()`, the whole ring plus generations rather than the lines since a mode started, so the hourly copy keeps every scored day where they look. Lines are compared before `redactPii` (counts and day keys only). V1: `RepeatedDayLineTests` (10) pass; `StrandTests` (2,069, 2 skipped) passes; `Strand` and `NOOPiOS` (no watch) build. Mutations: printing every line fails the 2 engine tests; dropping the per-line age check fails `testALineWhoseHourEndsDuringAPassPrints`; dropping the occurrence index fails 7. Oracle (the Swift filter over the log's 12 passes): 840 → 171 printed + 11 summaries; the first pass prints 70 and the hour refresh 68. Left, recorded here: `dayOwner` still prints 10 lines a pass with any Test Centre mode on (so in W06-108's V3 run), and per-pass constants (`sleep SKIPPED`, `Dedup(#899) … removed 0`, `detectedBout`) repeat too; the next lever if the 2 h target is missed. V2 (independent subagent) held items 1, 2, 4 and 5 (the sink cannot drop an admitted line; no early return between `beginPass` and the summary; no other path prints these lines; no reader expects one per pass) and reproduced the oracle; it found W07-006 … W07-009, and W07-006 … W07-008 are fixed on the same branch. After them the oracle is unchanged: 840 → 171 + 11 summaries, none naming a vanished line. V3 due: the W06-108 strap run's log V3 (2026-09-27 evening export, 11.9.13): 18 re-score passes in 82 min printed 366 per-day lines and 16 summaries where the old code would have printed 1,422 (79 a pass). The launch pass and the hourly refresh printed all 79; each of the other 16 printed 13 and withheld 68 under one summary. Second V3 (2026-09-28 export, 11.9.13): passes print the withheld count ("77 per-day line(s) unchanged since their last print, not repeated" on the pass after a launch's first). Batch check: a day and a night on 11.9.13. | | @@ -91,6 +91,9 @@ the two biggest hot spots). | W07-029 | S3 | Reported | The per-day scan cache's config signature omits weight and height, which feed the day's calorie estimate and bout kcal, so an edit is not applied to cached nights until relaunch, while the code comment says profile changes drop the cache | `IntelligenceEngine.dayCacheConfig`; `WorkoutDetector.estimateDayEnergy` | Phase 2 workflow `wf_b77f01a5-1aa` (W7b-7); P5 confirmed with caveat: under the default sleep-onset day cycle, established days' calories are recomputed each pass; midnight-mode installs, days before `firstWakeDay` and bout kcal keep the stale value. V0 plan: extend `DayCacheConfigFieldTests` | | | W07-030 | S3 | Reported | Editing or deleting a sleep night older than 21 days never reaches that day's stored scores: SleepView re-scores with the default 21-day `analyzeRecent()`, so the day's daily sleep totals and Rest point keep the old night, contradicting the view's comment | `SleepView` save, delete, add nap, undo; `IntelligenceEngine.analyzeRecent(maxDays:)` | Phase 2 workflow `wf_b77f01a5-1aa` (P5 missed finding); the Sleep tab browses up to 4,000 days with no age guard. V0 plan: `SleepEditOutsideWindowTests` | | | W07-031 | S3 | Reported | HealthKitBridge reads strap heart rate, the open-night frontier and manual workouts from the fixed `my-whoop`, so after a strap re-add no strap heart rate reaches Apple Health again and every recent night is held for the full open-night hold | `StrandiOSApp` (`noopDeviceId: model.deviceId`); `HealthKitBridge` `writeHeartRate`, `hrFingerprint`, workouts | Phase 2 workflow `wf_b77f01a5-1aa` (P5 missed finding), medium confidence; iOS code (W10), recorded here as AD-7 evidence. V0 plan: extract the source-id choice into a pure static in StrandImport and pin it with an active id of `whoop-x` | | +| W07-032 | S3 | Fixed | W07-002's attributed sleep and workout delete scanned the whole heart-rate index once per canonical session (`deviceId != ?` cannot use the (deviceId, ts) key), inside the delete's write transaction, so offload inserts and every store read waited behind it | `DeviceRegistryStore.deleteAttributedCanonicalRows` | Found by W07-002's V2 (query plan `SCAN h`; 25 to 53 s on 115 days of 1 Hz heart rate). Fix: the other heart-rate sources are read once by a skip scan over the key (`heartRateSourceIds`), then each session is probed with key lookups. V1: `DeviceRegistryStoreTests` pass; on a copy of the 2026-09-30 backup plus 115 synthetic days at 1 Hz (10.9 M heart-rate rows, 130 canonical nights) deleting `my-whoop` with a second strap registered took 9.6 s in all, most of it deleting 9.3 M raw rows, against 48 s for this shape before, and deleting the second strap 1.2 s; the same nights survive as before | | +| W07-033 | S3 | Reported | A scoring pass that is running when a strap's data is deleted writes that strap's scores back: `persistComputedScores` and `upsertSleepSessions` write what the pass read before the delete, and nothing pauses or cancels the engine around Delete or Forget; the next pass evicts the day rows but the series cells (W07-021) and sleeps (W07-003) stay | `DevicesView` delete and forget actions; `IntelligenceEngine.analyzeRecent` persist | Found by W07-002's V2 by reading; not reproduced. Same window as W07-017 (Apple Health and the single-strap delete). V0 plan: an engine test that deletes between scan and persist and asserts no provenance names the device | | +| W07-034 | S4 | Reported | `adoptSerialIdentity` re-keys every `deviceId` column but not `scoreInputProvenance.sourceId`, so canonical cells keep naming the provisional id, whose registry row is removed; a later delete of the serial id leaves those days' scores | `DeviceRegistryStore.adoptSerialIdentity` | Found by W07-002's V2 (scratch test: after adoption 2 cells name the old id; after deleting the serial id one canonical day row stays). Oura serial adoption (#771) only, for days scored before adoption | | ## Log @@ -114,3 +117,4 @@ the two biggest hot spots). - 2026-10-03 — Phase 2 started; W7 claimed for the Phase 2 review workflow (passes 1, 2, 3 and 5 in two slices: orchestration; resolution and partition). W07-002 (S2) is fixed first. - 2026-10-03 — Passes 1, 2, 3 and 5 done in workflow run `wf_b77f01a5-1aa` (two reviewers: orchestration; resolution and partition; one adversary). W07-017 … W07-031 recorded, with W3's single-offset and Rest-consistency findings merged into W07-018 and W07-019. P5 refuted none; it moved W7b-8 (a restore cannot reproduce derived rows when the exporter changed an engine toggle; the toggles are deliberately outside the whitelist) to AD-8 evidence rather than a row. W07-002 confirmed unchanged on `review/phase-2`. Carried rows: W07-001 re-scoped, W07-015's cause named, W07-004's CPU figures corrected. Fix history grouped by cause (196 commits touching `IntelligenceEngine.swift`): pass cost, single-flight, cache-key misses, day boundaries, device partition, write-reconcile asymmetry, score methodology, log truth; the open residue of each is a row here. Not covered: StrandTests runs, the replay diff, restore then re-score, the background CPU premium (needs Instruments). Next: W07-002 (S2) first, then W07-023 (S2). - 2026-10-03 — W07-002 fixed on `review/w07-fixes` (off `review/phase-2`): provenance-attributed delete from the shared canonical namespace. V2 due. +- 2026-10-03 — V2 of W07-002 held with caveats; W07-032 (the delete's per-session index scan) fixed on `review/w07-fixes`, W07-033 and W07-034 recorded. From 24e865e59e52d5844b09410ec486d5a89a480713 Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 3 Oct 2026 17:31:45 +0200 Subject: [PATCH 03/13] fix(app): sleep edits reach scoring on a re-added strap (W07-023) On an install whose active strap id is not my-whoop (any remove and re-add, #814), Repository's computed sibling is whoop--noop, but the engine banks every detected night, and so every edit of one, under the canonical my-whoop-noop. selfHealEditedStages read only the active sibling, found no edited nights, and the day's totals, Rest and Charge kept the detected bounds. It now reads every id computedReadIds unions, returns one row per night (the active strap's copy winning), and writes each re-staging back to the namespace its row came from. --- Strand/Data/Repository.swift | 26 +++++++---- StrandTests/SleepEditReAddedStrapTests.swift | 47 ++++++++++++++++++++ docs/review/DECISIONS.md | 4 +- docs/review/workstreams/W07-app-data.md | 3 +- 4 files changed, 69 insertions(+), 11 deletions(-) create mode 100644 StrandTests/SleepEditReAddedStrapTests.swift diff --git a/Strand/Data/Repository.swift b/Strand/Data/Repository.swift index 704ddcd8b0..ec431dc903 100644 --- a/Strand/Data/Repository.swift +++ b/Strand/Data/Repository.swift @@ -1803,29 +1803,37 @@ final class Repository: ObservableObject { /// bed/wake correction. Idempotent: a night already staged from raw re-derives to the same JSON /// (equality-skip, no write); a night edited-too-early heals the moment its raw arrives; a true /// imported night (raw never dense) is left untouched (`restageFromRaw` returns nil). Reads/writes the - /// COMPUTED source , the same one `analyzeRecent` reads edited rows from. Returns the (possibly - /// refreshed) edited rows so the caller recomputes daily aggregates from the corrected stages. + /// COMPUTED sources, every one `computedReadIds` unions: the engine banks detected nights, and so their + /// edits, under the canonical sibling even on a re-added strap whose own sibling is the active one + /// (W07-023). A night edited under both is returned once, the active strap's copy winning, and each + /// heal is written back to the namespace its row came from. Returns the (possibly refreshed) edited + /// rows, oldest first, so the caller recomputes daily aggregates from the corrected stages. func selfHealEditedStages(from windowStart: Int, to windowEnd: Int) async -> [CachedSleepSession] { guard let store = await ensureStore() else { return [] } - func editedRows() async -> [CachedSleepSession] { - ((try? await store.sleepSessions(deviceId: computedDeviceId, from: windowStart, - to: windowEnd, limit: 100_000)) ?? []) - .filter { $0.userEdited } + let ids = computedReadIds + func editedRows() async -> [(id: String, row: CachedSleepSession)] { + var byStart: [Int: (id: String, row: CachedSleepSession)] = [:] + for id in ids { + let rows = ((try? await store.sleepSessions(deviceId: id, from: windowStart, + to: windowEnd, limit: 100_000)) ?? []) + for row in rows where row.userEdited && byStart[row.startTs] == nil { byStart[row.startTs] = (id, row) } + } + return byStart.values.sorted { $0.row.startTs < $1.row.startTs } } let edited = await editedRows() guard !edited.isEmpty else { return [] } var healed = false - for row in edited { + for (id, row) in edited { // Re-derive over the LOCKED corrected window (effective onset → wake). Skip when the raw // isn't dense yet, or when the result already matches what's stored (steady state , no write). guard let newJSON = await restageFromRaw(start: row.effectiveStartTs, end: row.endTs), newJSON != row.stagesJSON else { continue } - let n = (try? await store.updateSleepStages(deviceId: computedDeviceId, + let n = (try? await store.updateSleepStages(deviceId: id, detectedStartTs: row.startTs, stagesJSON: newJSON)) ?? 0 if n > 0 { healed = true } } - return healed ? await editedRows() : edited + return (healed ? await editedRows() : edited).map(\.row) } // MARK: - Metric explorer reads (generic substrate) diff --git a/StrandTests/SleepEditReAddedStrapTests.swift b/StrandTests/SleepEditReAddedStrapTests.swift new file mode 100644 index 0000000000..69674f76f2 --- /dev/null +++ b/StrandTests/SleepEditReAddedStrapTests.swift @@ -0,0 +1,47 @@ +import XCTest +import WhoopStore +@testable import Strand + +/// W07-023: after a strap is removed and re-added the registry's active id is `whoop-`, so +/// `Repository.computedDeviceId` is `whoop--noop`. The engine still banks every detected night under +/// the canonical `my-whoop-noop`, and an edit of one lands there, so reading edited nights from the active +/// sibling alone found none: the day's totals and Rest kept the detected bounds. +@MainActor +final class SleepEditReAddedStrapTests: XCTestCase { + + func testEditedNightUnderTheCanonicalSiblingReachesScoringOnAReAddedStrap() async throws { + let store = try await WhoopStore.inMemory() + let edited = CachedSleepSession(startTs: 1_000_000, endTs: 1_028_800, efficiency: 0.9, restingHr: 52, + avgHrv: 60, stagesJSON: nil, userEdited: true) + _ = try await store.upsertSleepSessions([edited], deviceId: "my-whoop-noop") + + let repo = Repository(deviceId: "my-whoop") + repo.setStoreForTesting(store) + _ = repo.adoptActiveDeviceId("whoop-x") + + let rows = await repo.selfHealEditedStages(from: 900_000, to: 1_100_000) + XCTAssertEqual(rows.map(\.startTs), [1_000_000]) + } + + /// The active strap's own edited nights are still read, and one night edited under both siblings is + /// returned once, the active strap's copy winning, as every other computed read does. + func testActiveSiblingWinsADuplicatedEditedNight() async throws { + let store = try await WhoopStore.inMemory() + let canonical = CachedSleepSession(startTs: 1_000_000, endTs: 1_028_800, efficiency: 0.8, restingHr: 52, + avgHrv: 60, stagesJSON: nil, userEdited: true) + let active = CachedSleepSession(startTs: 1_000_000, endTs: 1_030_000, efficiency: 0.9, restingHr: 52, + avgHrv: 60, stagesJSON: nil, userEdited: true) + let nap = CachedSleepSession(startTs: 1_050_000, endTs: 1_052_000, efficiency: 0.9, restingHr: 60, + avgHrv: nil, stagesJSON: nil, userEdited: true) + _ = try await store.upsertSleepSessions([canonical], deviceId: "my-whoop-noop") + _ = try await store.upsertSleepSessions([active, nap], deviceId: "whoop-x-noop") + + let repo = Repository(deviceId: "my-whoop") + repo.setStoreForTesting(store) + _ = repo.adoptActiveDeviceId("whoop-x") + + let rows = await repo.selfHealEditedStages(from: 900_000, to: 1_100_000) + XCTAssertEqual(rows.map(\.startTs), [1_000_000, 1_050_000]) + XCTAssertEqual(rows.first?.endTs, 1_030_000, "the active strap's copy wins") + } +} diff --git a/docs/review/DECISIONS.md b/docs/review/DECISIONS.md index c7d918eaaf..32469ebd85 100644 --- a/docs/review/DECISIONS.md +++ b/docs/review/DECISIONS.md @@ -235,7 +235,9 @@ registry; how `DeviceFamily.forRegistryDevice(model:brand:)` interacts with the (non-archived only, canonical fallback); the same pass's day-cycle candidates include archived straps. - Single-id readers that break on a re-added strap: `selfHealEditedStages` (W07-023, S2), `rescoreManualWorkouts` (W07-028), `restageFromRaw`, `resolveDayOwner` on archived straps (W07-024), - `HealthKitBridge` (W07-031), and the deletion side (W07-002, S2). + `HealthKitBridge` (W07-031), and the deletion side (W07-002, S2). W07-023's fix unions the computed + ids for edited nights, but `restageFromRaw` still reads raw under the active id only: one more reader for + the resolver. **Verdict.** Amend. Keep the canonical computed namespace, since moving it would orphan history. Separate the routing token from the partition id, and replace the three strap-set definitions with one diff --git a/docs/review/workstreams/W07-app-data.md b/docs/review/workstreams/W07-app-data.md index ebd5b09ea8..139f977e8c 100644 --- a/docs/review/workstreams/W07-app-data.md +++ b/docs/review/workstreams/W07-app-data.md @@ -82,7 +82,7 @@ the two biggest hot spots). | W07-020 | S3 | Reported | A failed store write or read in a pass is swallowed and the pass completes: the watermark advances and the re-score debt is settled whether `persistedScores` landed or not (the comment says a failed run cannot advance it), and a failed HR read becomes an empty day whose stored row is evicted | `IntelligenceEngine.swift` watermark store, `markRescoreCompleted`, `SlidingStreamWindow` failed read, stale eviction; `DayCycleIntelligenceIntegration` catch | Phase 2 workflow `wf_b77f01a5-1aa` (W7a-4); P5 confirmed with caveat: W02-008 narrowed the BUSY trigger (now >5 s); disk full, IO errors and the W02-004 post-restore window remain; transient until the next offload. AD-14 evidence. V0 plan: `AnalyzeRecentWriteFailureTests` (an abort trigger on dailyMetric) | | | W07-021 | S3 | Reported | The stale-day reconcile deletes an evicted day's computed dailyMetric row but keeps every metricSeries point it carried (Rest, zone minutes, onset, SpO₂ candidate, HRV over-count, RHR shadow), so Today's Rest and the Healthspan drivers keep showing a day whose score was evicted | `IntelligenceEngine.swift` stale eviction; `staleRestPointDays` | Phase 2 workflow `wf_b77f01a5-1aa` (W7a-5); P5 confirmed: `staleRestPointDays` only names days still persisted. Distinct from W07-002 and survives its fix. V0 plan: `StaleDayMetricSeriesTests` | | | W07-022 | S4 | Reported | The re-score cost line's `cpu=` is process-wide (`RUSAGE_SELF` over the pass's span) and the per-phase tally starts `prep` after the per-day cache-key fingerprints, so a warm pass prints `prep=0ms` and W07-004 and W07-015 quote process CPU as pass cost | `RescoreBackgroundScheduler.processCPUSeconds`, `passCostLogLine`; `IntelligenceEngine` fingerprints before `tPrep0` | Phase 2 workflow `wf_b77f01a5-1aa` (W7a-6); P5 confirmed with caveat: the code docs say both things; the log text and the two review rows do not. 21 per-day fingerprints at the 2026-09-30 size take about 2.8 s of CPU. V0 plan: `AnalyzeRecentCostTallyTests` (warm pass reports a non-zero cache-key phase) | | -| W07-023 | S2 | Reported | On an install whose active strap id is not `my-whoop` (any re-added strap, #814), sleep edits never reach scoring: `selfHealEditedStages` reads only the active strap's computed sibling while every detected night, and so every edit of one, lives under `my-whoop-noop`; the day's totals and Rest keep the detected bounds | `Repository.selfHealEditedStages`, `restageFromRaw`; `IntelligenceEngine` edited-rows consumers | Phase 2 workflow `wf_b77f01a5-1aa` (W7b-1); P5 confirmed: the edited row itself is protected by the upsert; only scoring is wrong. `restageFromRaw` reading raw under the active id only is a sibling defect. V0 plan: `SleepEditReAddedStrapTests` | | +| W07-023 | S2 | Fixed | On an install whose active strap id is not `my-whoop` (any re-added strap, #814), sleep edits never reach scoring: `selfHealEditedStages` reads only the active strap's computed sibling while every detected night, and so every edit of one, lives under `my-whoop-noop`; the day's totals and Rest keep the detected bounds | `Repository.selfHealEditedStages`, `restageFromRaw`; `IntelligenceEngine` edited-rows consumers | Phase 2 workflow `wf_b77f01a5-1aa` (W7b-1); P5 confirmed: the edited row itself is protected by the upsert; only scoring is wrong. `restageFromRaw` reading raw under the active id only is a sibling defect. V0 plan: `SleepEditReAddedStrapTests`. V0: `SleepEditReAddedStrapTests.testEditedNightUnderTheCanonicalSiblingReachesScoringOnAReAddedStrap` failed (no edited rows on a re-added strap). Fix: `selfHealEditedStages` reads edited nights from every id `computedReadIds` unions, returns one row per start (the active strap's copy winning, oldest first), and writes each heal to the namespace its row came from. V1: both tests pass; reading only the active sibling fails the first. Not changed: `restageFromRaw` reads raw under the active id only, so a canonical night recorded by the old strap is not re-staged (it returns nil and the stored stages stay) | | | W07-024 | S3 | Reported | Scoring drops the days of an archived non-canonical strap (`resolveDayOwner` filters archived devices and falls back to `my-whoop`), and the stale-day eviction then deletes their stored dailyMetric rows, while the read side (#1730) keeps that strap's raw on screen | `IntelligenceEngine.resolveDayOwner`, HR gate, stale eviction; `Repository.rawPhysiologyReadIds` | Phase 2 workflow `wf_b77f01a5-1aa` (W7b-2); P5 confirmed with caveat (S2→S3): reachable only with a non-canonical strap archived; the evicted day's series points survive (W07-021). Three strap-set definitions in one pass (AD-7). V0 plan: `ResolveDayOwnerArchivedTests` then an engine eviction test | | | W07-025 | S3 | Reported | Every completed sync, and since W07-016 every launch, republishes the dashboard caches over 120 days, so Trends 1Y/ALL, Explore daily-column series, catalog dots and the Data Sources freshness counts show only 120 days until a pass that scores something restores 4,000 | `AppModel.refreshAfterCompletedBackfill`; `Repository.refresh(days:)`; `TrendsView` | Phase 2 workflow `wf_b77f01a5-1aa` (W7b-3); P5 confirmed: a launch with no new raw stays on 120 days; the owner's logs show no skips, but any install with over 120 days of history (e.g. a WHOOP CSV import) shows it. V0 plan: `Refresh120WindowTests` | | | W07-026 | S3 | Reported | One metric under the `my-whoop` source has two resolvers with different source precedence (`exploreSeries`: imported > computed > published daily column, no Apple fallback, bounded by the last refresh; `resolvedSeries`: per candidate metricSeries then that candidate's daily column, Apple last), and screens mix them: the Metric Explorer draws one beside the other's sources, Compare draws the second, and Today's Rest value and badge come from different ones | `Repository.exploreSeries`, `resolvedSeries`, `sourceCandidates`; `MetricExplorerView`; `CompareView`; `TodayView` | Phase 2 workflow `wf_b77f01a5-1aa` (W7b-4); P5 confirmed: model twin reproduces three disagreeing states (Apple-only day, edited night over an import, the window). Same shape W3 fixed for SpO₂. V0 plan: `ExploreResolvedAgreementTests` | | @@ -118,3 +118,4 @@ the two biggest hot spots). - 2026-10-03 — Passes 1, 2, 3 and 5 done in workflow run `wf_b77f01a5-1aa` (two reviewers: orchestration; resolution and partition; one adversary). W07-017 … W07-031 recorded, with W3's single-offset and Rest-consistency findings merged into W07-018 and W07-019. P5 refuted none; it moved W7b-8 (a restore cannot reproduce derived rows when the exporter changed an engine toggle; the toggles are deliberately outside the whitelist) to AD-8 evidence rather than a row. W07-002 confirmed unchanged on `review/phase-2`. Carried rows: W07-001 re-scoped, W07-015's cause named, W07-004's CPU figures corrected. Fix history grouped by cause (196 commits touching `IntelligenceEngine.swift`): pass cost, single-flight, cache-key misses, day boundaries, device partition, write-reconcile asymmetry, score methodology, log truth; the open residue of each is a row here. Not covered: StrandTests runs, the replay diff, restore then re-score, the background CPU premium (needs Instruments). Next: W07-002 (S2) first, then W07-023 (S2). - 2026-10-03 — W07-002 fixed on `review/w07-fixes` (off `review/phase-2`): provenance-attributed delete from the shared canonical namespace. V2 due. - 2026-10-03 — V2 of W07-002 held with caveats; W07-032 (the delete's per-session index scan) fixed on `review/w07-fixes`, W07-033 and W07-034 recorded. +- 2026-10-03 — W07-023 fixed on `review/w07-fixes`. From b942b714cc0593152ba1ffbbb4c7484cc4fde473 Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 3 Oct 2026 21:44:35 +0200 Subject: [PATCH 04/13] fix(store): a deleted strap's mixed-day row goes with it, and a legacy snapshot it supplied is cleared (W07-035) W07-002 kept a mixed day's whole dailyMetric row whenever any recovery or strain cell named another source. A row whose strain the deleted strap scored, beside a recovery a legacy snapshot kept from an older source, therefore kept the deleted strap's strain, sleep, HRV and resting HR, and lost its source badge as its provenance went. The row now belongs to the source its strain cell names (the pass that scored it), else its recovery cell. The owner's delete removes the row; when the deleted strap supplied only the legacy snapshot on another source's row, just the snapshot's columns are cleared. --- .../WhoopStore/DeviceRegistryStore.swift | 52 +++++++++++++------ .../DeviceRegistryStoreTests.swift | 21 ++++++-- docs/review/workstreams/W07-app-data.md | 2 + 3 files changed, 53 insertions(+), 22 deletions(-) diff --git a/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift b/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift index 3d41564859..955e50a2c6 100644 --- a/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift +++ b/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift @@ -230,8 +230,9 @@ public struct DeviceRegistryStore: Sendable { /// /// Scored days are attributed per cell by `scoreInputProvenance`, which names the source of each /// day's recovery, strain and Rest-point series. Each series cell naming the device goes; a day's - /// `dailyMetric` row goes only when no recovery or strain cell of that day names another source, so a - /// mixed day keeps its row. A cell sourced from an estimator (weekly VO₂max) never names a device, and + /// `dailyMetric` row goes when the device owns it (its strain cell, else its recovery cell, names the + /// device), and when the device supplied only a legacy snapshot's recovery on another source's row, just + /// the snapshot's columns are cleared (W07-035). A cell sourced from an estimator (weekly VO₂max) never names a device, and /// a day scored before provenance existed has no cells, so both stay. /// /// Sleeps and workouts under the canonical id (detected ones; user workouts live under the strap's own @@ -251,21 +252,38 @@ public struct DeviceRegistryStore: Sendable { arguments: [canonical, day, key]) } for day in days { - let otherSource = try Bool.fetchOne(db, sql: """ - SELECT EXISTS(SELECT 1 FROM scoreInputProvenance - WHERE deviceId = ? AND day = ? AND key IN ('recovery', 'strain') AND sourceId != ?) - """, arguments: [canonical, day, deviceId]) ?? true - guard !otherSource else { continue } - try db.execute(sql: "DELETE FROM dailyMetric WHERE deviceId = ? AND day = ?", arguments: [canonical, day]) - // The day was the device's alone, so its other computed cells were derived from it too: values a - // later pass stopped producing (which lose their provenance but keep their value) and the - // Healthspan and Fitness Age points, which are never attributed. A cell another source is - // named for stays. - try db.execute(sql: """ - DELETE FROM metricSeries AS m WHERE m.deviceId = ? AND m.day = ? - AND NOT EXISTS(SELECT 1 FROM scoreInputProvenance p - WHERE p.deviceId = m.deviceId AND p.day = m.day AND p.key = m.key AND p.sourceId != ?) - """, arguments: [canonical, day, deviceId]) + // The row's owner is the source its strain cell names, the pass that scored it; its recovery cell + // can instead name an older source whose score a legacy snapshot kept (W07-035). Without a strain + // cell the recovery cell is the owner. + let cell = { (key: String) in + try String.fetchOne(db, sql: """ + SELECT sourceId FROM scoreInputProvenance WHERE deviceId = ? AND day = ? AND key = ? + """, arguments: [canonical, day, key]) + } + let strainSource = try cell("strain"), recoverySource = try cell("recovery") + // A day with neither cell came in through a Rest-point cell naming the device, and a pass names + // one owner for every cell of a day. + if (strainSource ?? recoverySource ?? deviceId) == deviceId { + try db.execute(sql: "DELETE FROM dailyMetric WHERE deviceId = ? AND day = ?", arguments: [canonical, day]) + // The day was the device's, so its other computed cells were derived from it too: values a + // later pass stopped producing (which lose their provenance but keep their value) and the + // Healthspan and Fitness Age points, which are never attributed. A cell another source is + // named for stays. + try db.execute(sql: """ + DELETE FROM metricSeries AS m WHERE m.deviceId = ? AND m.day = ? + AND NOT EXISTS(SELECT 1 FROM scoreInputProvenance p + WHERE p.deviceId = m.deviceId AND p.day = m.day AND p.key = m.key AND p.sourceId != ?) + """, arguments: [canonical, day, deviceId]) + try db.execute(sql: """ + DELETE FROM scoreInputProvenance WHERE deviceId = ? AND day = ? AND key IN ('recovery', 'strain') + """, arguments: [canonical, day]) + } else if recoverySource == deviceId { + // Another source owns the row; only the legacy snapshot's columns came from the device. + try db.execute(sql: """ + UPDATE dailyMetric SET recovery = NULL, avgHrv = NULL, respRateBpm = NULL, avgSdnn = NULL + WHERE deviceId = ? AND day = ? + """, arguments: [canonical, day]) + } } try db.execute(sql: "DELETE FROM scoreInputProvenance WHERE deviceId = ? AND sourceId = ?", arguments: [canonical, deviceId]) diff --git a/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift b/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift index 3348a6c9e0..2eb2113246 100644 --- a/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift +++ b/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift @@ -250,6 +250,10 @@ final class DeviceRegistryStoreTests: XCTestCase { /// are attributed by whose heart rate covers them. private func seedSharedNamespace(_ dbq: DatabaseQueue) throws { try dbq.write { db in + // Day R: whoop-new scored a night but no Charge, so only a Rest-point cell names it. + try db.execute(sql: "INSERT INTO dailyMetric (deviceId, day, totalSleepMin) VALUES ('my-whoop-noop', 'R', 400)") + try db.execute(sql: "INSERT INTO metricSeries (deviceId, day, key, value) VALUES ('my-whoop-noop', 'R', 'sleep_performance', 70)") + try db.execute(sql: "INSERT INTO scoreInputProvenance (deviceId, day, key, sourceId) VALUES ('my-whoop-noop', 'R', 'sleep_performance', 'whoop-new')") for day in ["A", "B", "M", "L"] { try db.execute(sql: "INSERT INTO dailyMetric (deviceId, day, recovery, strain) VALUES ('my-whoop-noop', ?, 60, 10)", arguments: [day]) @@ -312,10 +316,11 @@ final class DeviceRegistryStoreTests: XCTestCase { try store.deleteAllData(deviceId: "whoop-new") let left = try canonicalCells(dbq) - XCTAssertEqual(left.days, ["B", "L", "M"], "day A was whoop-new's alone; a mixed day keeps its row") + XCTAssertEqual(left.days, ["B", "L"], + "day A was whoop-new's alone; day M's row is whoop-new's too (its strain cell), only its legacy recovery is not (W07-035)") XCTAssertEqual(left.series, ["1970-01-01:healthspan_model", "A:vo2max_est", "B:sleep_performance", - "B:spo2_candidate", "L:sleep_performance", "M:spo2_candidate"], - "day A's unattributed cell goes with the day; a mixed day keeps its unattributed cell") + "B:spo2_candidate", "L:sleep_performance"], + "the unattributed cells of the days whoop-new owned go with them") XCTAssertEqual(left.sleeps, [3000, 5000, 7000], "a night only whoop-new's heart rate covers goes; an edited one stays") XCTAssertEqual(left.workouts, [7000]) } @@ -332,9 +337,15 @@ final class DeviceRegistryStoreTests: XCTestCase { try store.deleteAllData(deviceId: "my-whoop") let left = try canonicalCells(dbq) - XCTAssertEqual(left.days, ["A", "L", "M"]) + XCTAssertEqual(left.days, ["A", "L", "M", "R"]) + let m = try dbq.read { db in + try Row.fetchOne(db, sql: "SELECT recovery, strain FROM dailyMetric WHERE deviceId = 'my-whoop-noop' AND day = 'M'") + } + XCTAssertNil(m?["recovery"] as Double?, "day M's recovery was a legacy snapshot of my-whoop's (W07-035)") + XCTAssertEqual(m?["strain"] as Double?, 10, "day M's own scores are whoop-new's and stay") XCTAssertEqual(left.series, ["1970-01-01:healthspan_model", "A:sleep_performance", "A:spo2_candidate", - "A:vo2max_est", "L:sleep_performance", "M:sleep_performance", "M:spo2_candidate"]) + "A:vo2max_est", "L:sleep_performance", "M:sleep_performance", "M:spo2_candidate", + "R:sleep_performance"]) XCTAssertEqual(left.sleeps, [1000, 3000, 5000]) XCTAssertEqual(left.workouts, [1000]) } diff --git a/docs/review/workstreams/W07-app-data.md b/docs/review/workstreams/W07-app-data.md index 139f977e8c..9a900ce2c1 100644 --- a/docs/review/workstreams/W07-app-data.md +++ b/docs/review/workstreams/W07-app-data.md @@ -94,6 +94,7 @@ the two biggest hot spots). | W07-032 | S3 | Fixed | W07-002's attributed sleep and workout delete scanned the whole heart-rate index once per canonical session (`deviceId != ?` cannot use the (deviceId, ts) key), inside the delete's write transaction, so offload inserts and every store read waited behind it | `DeviceRegistryStore.deleteAttributedCanonicalRows` | Found by W07-002's V2 (query plan `SCAN h`; 25 to 53 s on 115 days of 1 Hz heart rate). Fix: the other heart-rate sources are read once by a skip scan over the key (`heartRateSourceIds`), then each session is probed with key lookups. V1: `DeviceRegistryStoreTests` pass; on a copy of the 2026-09-30 backup plus 115 synthetic days at 1 Hz (10.9 M heart-rate rows, 130 canonical nights) deleting `my-whoop` with a second strap registered took 9.6 s in all, most of it deleting 9.3 M raw rows, against 48 s for this shape before, and deleting the second strap 1.2 s; the same nights survive as before | | | W07-033 | S3 | Reported | A scoring pass that is running when a strap's data is deleted writes that strap's scores back: `persistComputedScores` and `upsertSleepSessions` write what the pass read before the delete, and nothing pauses or cancels the engine around Delete or Forget; the next pass evicts the day rows but the series cells (W07-021) and sleeps (W07-003) stay | `DevicesView` delete and forget actions; `IntelligenceEngine.analyzeRecent` persist | Found by W07-002's V2 by reading; not reproduced. Same window as W07-017 (Apple Health and the single-strap delete). V0 plan: an engine test that deletes between scan and persist and asserts no provenance names the device | | | W07-034 | S4 | Reported | `adoptSerialIdentity` re-keys every `deviceId` column but not `scoreInputProvenance.sourceId`, so canonical cells keep naming the provisional id, whose registry row is removed; a later delete of the serial id leaves those days' scores | `DeviceRegistryStore.adoptSerialIdentity` | Found by W07-002's V2 (scratch test: after adoption 2 cells name the old id; after deleting the serial id one canonical day row stays). Oura serial adoption (#771) only, for days scored before adoption | | +| W07-035 | S3 | Fixed | W07-002's delete kept a mixed day's whole `dailyMetric` row: the deleted strap's strain and the sleep, HRV and resting-HR columns computed from it stayed on screen, while its provenance cells were removed, so the row lost its source badge; and deleting the strap that supplied only a legacy snapshot's recovery left that recovery | `DeviceRegistryStore.deleteAttributedCanonicalRows` | Found by the 2026-10-03 `/code-review xhigh` of `review/w07-fixes` (W07-002's row recorded it as a limit). V0: the delete tests, extended to assert day M's row and its recovery, failed. Fix: the row belongs to the source its strain cell names (the pass that scored it), else its recovery cell, else the device whose Rest-point cell brought the day in; the owner's delete removes the row, its unattributed cells and its recovery and strain provenance; when the device supplied only the legacy snapshot on another source's row, just the snapshot's columns (recovery, HRV, respiration, SDNN) are cleared. V1: both tests pass with day M and a no-Charge day R; the old guard fails them | | ## Log @@ -119,3 +120,4 @@ the two biggest hot spots). - 2026-10-03 — W07-002 fixed on `review/w07-fixes` (off `review/phase-2`): provenance-attributed delete from the shared canonical namespace. V2 due. - 2026-10-03 — V2 of W07-002 held with caveats; W07-032 (the delete's per-session index scan) fixed on `review/w07-fixes`, W07-033 and W07-034 recorded. - 2026-10-03 — W07-023 fixed on `review/w07-fixes`. +- 2026-10-03 — `/code-review xhigh` of `review/w07-fixes`: 13 findings, recorded as W07-035 … W07-047 as they are worked; W07-035 fixed. From 64c58785ed0376f3cc230f012e5a13cf4bac8fed Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 3 Oct 2026 21:45:07 +0200 Subject: [PATCH 05/13] fix(store): a deleted strap's weekly VO2max goes with its Fitness Age (W07-036) On a day only the deleted strap supplied, the sweep removed fitness_age but kept vo2max_est, because the VO2max cell's provenance names its estimator rather than a device. Both come from one Fitness Age computation over the same week, so the strap's VO2max stayed on screen beside a missing Fitness Age and kept feeding Body Age. The sweep now treats the estimator-named cell as unattributed. --- .../Sources/WhoopStore/DeviceRegistryStore.swift | 11 +++++++---- .../WhoopStoreTests/DeviceRegistryStoreTests.swift | 4 ++-- docs/review/workstreams/W07-app-data.md | 2 ++ 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift b/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift index 955e50a2c6..913795551a 100644 --- a/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift +++ b/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift @@ -232,8 +232,9 @@ public struct DeviceRegistryStore: Sendable { /// day's recovery, strain and Rest-point series. Each series cell naming the device goes; a day's /// `dailyMetric` row goes when the device owns it (its strain cell, else its recovery cell, names the /// device), and when the device supplied only a legacy snapshot's recovery on another source's row, just - /// the snapshot's columns are cleared (W07-035). A cell sourced from an estimator (weekly VO₂max) never names a device, and - /// a day scored before provenance existed has no cells, so both stay. + /// the snapshot's columns are cleared (W07-035). The weekly VO₂max cell names its + /// estimator, not a device, so it goes with the day it was computed on (W07-036); a day scored before + /// provenance existed has no cells and stays. /// /// Sleeps and workouts under the canonical id (detected ones; user workouts live under the strap's own /// id) carry no day key, so they are attributed by heart rate: a session goes when the device has heart @@ -268,11 +269,13 @@ public struct DeviceRegistryStore: Sendable { // The day was the device's, so its other computed cells were derived from it too: values a // later pass stopped producing (which lose their provenance but keep their value) and the // Healthspan and Fitness Age points, which are never attributed. A cell another source is - // named for stays. + // named for stays. The weekly VO2max cell names its estimator, not a source; it comes from the + // same Fitness Age computation over the same week, so it goes with it (W07-036). try db.execute(sql: """ DELETE FROM metricSeries AS m WHERE m.deviceId = ? AND m.day = ? AND NOT EXISTS(SELECT 1 FROM scoreInputProvenance p - WHERE p.deviceId = m.deviceId AND p.day = m.day AND p.key = m.key AND p.sourceId != ?) + WHERE p.deviceId = m.deviceId AND p.day = m.day AND p.key = m.key + AND p.sourceId != ? AND p.key != 'vo2max_est') """, arguments: [canonical, day, deviceId]) try db.execute(sql: """ DELETE FROM scoreInputProvenance WHERE deviceId = ? AND day = ? AND key IN ('recovery', 'strain') diff --git a/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift b/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift index 2eb2113246..9ca2938b43 100644 --- a/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift +++ b/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift @@ -318,9 +318,9 @@ final class DeviceRegistryStoreTests: XCTestCase { let left = try canonicalCells(dbq) XCTAssertEqual(left.days, ["B", "L"], "day A was whoop-new's alone; day M's row is whoop-new's too (its strain cell), only its legacy recovery is not (W07-035)") - XCTAssertEqual(left.series, ["1970-01-01:healthspan_model", "A:vo2max_est", "B:sleep_performance", + XCTAssertEqual(left.series, ["1970-01-01:healthspan_model", "B:sleep_performance", "B:spo2_candidate", "L:sleep_performance"], - "the unattributed cells of the days whoop-new owned go with them") + "the unattributed cells of the days whoop-new owned go with them, and the weekly VO2max with Fitness Age (W07-036)") XCTAssertEqual(left.sleeps, [3000, 5000, 7000], "a night only whoop-new's heart rate covers goes; an edited one stays") XCTAssertEqual(left.workouts, [7000]) } diff --git a/docs/review/workstreams/W07-app-data.md b/docs/review/workstreams/W07-app-data.md index 9a900ce2c1..2de2c35537 100644 --- a/docs/review/workstreams/W07-app-data.md +++ b/docs/review/workstreams/W07-app-data.md @@ -95,6 +95,7 @@ the two biggest hot spots). | W07-033 | S3 | Reported | A scoring pass that is running when a strap's data is deleted writes that strap's scores back: `persistComputedScores` and `upsertSleepSessions` write what the pass read before the delete, and nothing pauses or cancels the engine around Delete or Forget; the next pass evicts the day rows but the series cells (W07-021) and sleeps (W07-003) stay | `DevicesView` delete and forget actions; `IntelligenceEngine.analyzeRecent` persist | Found by W07-002's V2 by reading; not reproduced. Same window as W07-017 (Apple Health and the single-strap delete). V0 plan: an engine test that deletes between scan and persist and asserts no provenance names the device | | | W07-034 | S4 | Reported | `adoptSerialIdentity` re-keys every `deviceId` column but not `scoreInputProvenance.sourceId`, so canonical cells keep naming the provisional id, whose registry row is removed; a later delete of the serial id leaves those days' scores | `DeviceRegistryStore.adoptSerialIdentity` | Found by W07-002's V2 (scratch test: after adoption 2 cells name the old id; after deleting the serial id one canonical day row stays). Oura serial adoption (#771) only, for days scored before adoption | | | W07-035 | S3 | Fixed | W07-002's delete kept a mixed day's whole `dailyMetric` row: the deleted strap's strain and the sleep, HRV and resting-HR columns computed from it stayed on screen, while its provenance cells were removed, so the row lost its source badge; and deleting the strap that supplied only a legacy snapshot's recovery left that recovery | `DeviceRegistryStore.deleteAttributedCanonicalRows` | Found by the 2026-10-03 `/code-review xhigh` of `review/w07-fixes` (W07-002's row recorded it as a limit). V0: the delete tests, extended to assert day M's row and its recovery, failed. Fix: the row belongs to the source its strain cell names (the pass that scored it), else its recovery cell, else the device whose Rest-point cell brought the day in; the owner's delete removes the row, its unattributed cells and its recovery and strain provenance; when the device supplied only the legacy snapshot on another source's row, just the snapshot's columns (recovery, HRV, respiration, SDNN) are cleared. V1: both tests pass with day M and a no-Charge day R; the old guard fails them | | +| W07-036 | S4 | Fixed | On a day only the deleted strap supplied, W07-002's sweep removed `fitness_age` but kept `vo2max_est`, because the VO₂max cell's provenance names its estimator (`nes`/`uth`), not a device; both come from one Fitness Age computation over the same week, so the strap's VO₂max stayed beside a missing Fitness Age and kept feeding Body Age | `DeviceRegistryStore.deleteAttributedCanonicalRows` day sweep | Found by the 2026-10-03 `/code-review xhigh` (the V2 review noted the cell stays). V0: the delete test, asserting day A's VO₂max goes, failed. Fix: the sweep treats an estimator-named VO₂max cell as unattributed, so it goes with the day the device owned. V1: passes; the old condition fails it | | ## Log @@ -121,3 +122,4 @@ the two biggest hot spots). - 2026-10-03 — V2 of W07-002 held with caveats; W07-032 (the delete's per-session index scan) fixed on `review/w07-fixes`, W07-033 and W07-034 recorded. - 2026-10-03 — W07-023 fixed on `review/w07-fixes`. - 2026-10-03 — `/code-review xhigh` of `review/w07-fixes`: 13 findings, recorded as W07-035 … W07-047 as they are worked; W07-035 fixed. +- 2026-10-03 — W07-036 fixed. From c68ea96ca80cc1e6c24e8f4079ffe9c67c1f507b Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 3 Oct 2026 21:45:46 +0200 Subject: [PATCH 06/13] fix(store): deleting a strap's data spares a workout the user entered under the canonical id (W07-037) The attributed workout delete selected every workout under my-whoop-noop with no source filter, so a legacy workout the user had entered there, covered only by the deleted strap's heart rate, went with the detected bouts while edited nights were spared. Only detected bouts (sport "detected" or a computed source) are attributed now; a manual row stays. --- .../Sources/WhoopStore/DeviceRegistryStore.swift | 11 +++++++---- .../WhoopStoreTests/DeviceRegistryStoreTests.swift | 9 ++++++--- docs/review/workstreams/W07-app-data.md | 2 ++ 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift b/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift index 913795551a..d1d28014d1 100644 --- a/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift +++ b/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift @@ -236,8 +236,7 @@ public struct DeviceRegistryStore: Sendable { /// estimator, not a device, so it goes with the day it was computed on (W07-036); a day scored before /// provenance existed has no cells and stays. /// - /// Sleeps and workouts under the canonical id (detected ones; user workouts live under the strap's own - /// id) carry no day key, so they are attributed by heart rate: a session goes when the device has heart + /// Sleeps and detected workouts under the canonical id carry no day key, so they are attributed by heart rate: a session goes when the device has heart /// rate inside it and no other source does. Nights the user edited or added /// stay, as in the exclusive case. Must run before the device's heart rate is deleted. static func deleteAttributedCanonicalRows(_ db: Database, of deviceId: String) throws { @@ -309,8 +308,12 @@ public struct DeviceRegistryStore: Sendable { guard try coveredOnlyByDevice(start, end) else { continue } try db.execute(sql: "DELETE FROM sleepSession WHERE deviceId = ? AND startTs = ?", arguments: [canonical, start]) } - for row in try Row.fetchAll(db, sql: "SELECT startTs, endTs, sport FROM workout WHERE deviceId = ?", - arguments: [canonical]) { + // Detected bouts only: banked with sport "detected" or a computed source. A workout the user entered + // (`source = 'manual'`) is user data even under the canonical id, as an edited night is (W07-037). + for row in try Row.fetchAll(db, sql: """ + SELECT startTs, endTs, sport FROM workout + WHERE deviceId = ? AND source != 'manual' AND (sport = 'detected' OR source LIKE '%' || ?) + """, arguments: [canonical, computedSuffix]) { let start: Int = row["startTs"], end: Int = row["endTs"], sport: String = row["sport"] guard try coveredOnlyByDevice(start, end) else { continue } try db.execute(sql: "DELETE FROM workout WHERE deviceId = ? AND startTs = ? AND sport = ?", diff --git a/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift b/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift index 9ca2938b43..de2508f263 100644 --- a/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift +++ b/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift @@ -286,10 +286,13 @@ final class DeviceRegistryStoreTests: XCTestCase { try db.execute(sql: "INSERT INTO sleepSession (deviceId, startTs, endTs, userEdited) VALUES ('my-whoop-noop', ?, ?, ?)", arguments: [start, end, edited]) } + // Detected bouts as they are banked (sport "detected", a computed source), and a legacy manual + // workout under the canonical id that only whoop-new's heart rate covers (W07-037): user data. for (start, end) in [(1000, 2000), (7000, 8000)] { - try db.execute(sql: "INSERT INTO workout (deviceId, startTs, endTs, sport, source) VALUES ('my-whoop-noop', ?, ?, 'Running', 'detected')", + try db.execute(sql: "INSERT INTO workout (deviceId, startTs, endTs, sport, source) VALUES ('my-whoop-noop', ?, ?, 'detected', 'my-whoop-noop')", arguments: [start, end]) } + try db.execute(sql: "INSERT INTO workout (deviceId, startTs, endTs, sport, source) VALUES ('my-whoop-noop', 1100, 1900, 'Running', 'manual')") } } @@ -322,7 +325,7 @@ final class DeviceRegistryStoreTests: XCTestCase { "B:spo2_candidate", "L:sleep_performance"], "the unattributed cells of the days whoop-new owned go with them, and the weekly VO2max with Fitness Age (W07-036)") XCTAssertEqual(left.sleeps, [3000, 5000, 7000], "a night only whoop-new's heart rate covers goes; an edited one stays") - XCTAssertEqual(left.workouts, [7000]) + XCTAssertEqual(left.workouts, [1100, 7000], "a manual workout is user data and stays (W07-037)") } /// The same rule from the other side: deleting the canonical strap's data while a re-added strap is @@ -347,7 +350,7 @@ final class DeviceRegistryStoreTests: XCTestCase { "A:vo2max_est", "L:sleep_performance", "M:sleep_performance", "M:spo2_candidate", "R:sleep_performance"]) XCTAssertEqual(left.sleeps, [1000, 3000, 5000]) - XCTAssertEqual(left.workouts, [1000]) + XCTAssertEqual(left.workouts, [1000, 1100]) } // Regression guard (audit finding): every table with a `deviceId` column MUST appear in diff --git a/docs/review/workstreams/W07-app-data.md b/docs/review/workstreams/W07-app-data.md index 2de2c35537..f3fd229e47 100644 --- a/docs/review/workstreams/W07-app-data.md +++ b/docs/review/workstreams/W07-app-data.md @@ -96,6 +96,7 @@ the two biggest hot spots). | W07-034 | S4 | Reported | `adoptSerialIdentity` re-keys every `deviceId` column but not `scoreInputProvenance.sourceId`, so canonical cells keep naming the provisional id, whose registry row is removed; a later delete of the serial id leaves those days' scores | `DeviceRegistryStore.adoptSerialIdentity` | Found by W07-002's V2 (scratch test: after adoption 2 cells name the old id; after deleting the serial id one canonical day row stays). Oura serial adoption (#771) only, for days scored before adoption | | | W07-035 | S3 | Fixed | W07-002's delete kept a mixed day's whole `dailyMetric` row: the deleted strap's strain and the sleep, HRV and resting-HR columns computed from it stayed on screen, while its provenance cells were removed, so the row lost its source badge; and deleting the strap that supplied only a legacy snapshot's recovery left that recovery | `DeviceRegistryStore.deleteAttributedCanonicalRows` | Found by the 2026-10-03 `/code-review xhigh` of `review/w07-fixes` (W07-002's row recorded it as a limit). V0: the delete tests, extended to assert day M's row and its recovery, failed. Fix: the row belongs to the source its strain cell names (the pass that scored it), else its recovery cell, else the device whose Rest-point cell brought the day in; the owner's delete removes the row, its unattributed cells and its recovery and strain provenance; when the device supplied only the legacy snapshot on another source's row, just the snapshot's columns (recovery, HRV, respiration, SDNN) are cleared. V1: both tests pass with day M and a no-Charge day R; the old guard fails them | | | W07-036 | S4 | Fixed | On a day only the deleted strap supplied, W07-002's sweep removed `fitness_age` but kept `vo2max_est`, because the VO₂max cell's provenance names its estimator (`nes`/`uth`), not a device; both come from one Fitness Age computation over the same week, so the strap's VO₂max stayed beside a missing Fitness Age and kept feeding Body Age | `DeviceRegistryStore.deleteAttributedCanonicalRows` day sweep | Found by the 2026-10-03 `/code-review xhigh` (the V2 review noted the cell stays). V0: the delete test, asserting day A's VO₂max goes, failed. Fix: the sweep treats an estimator-named VO₂max cell as unattributed, so it goes with the day the device owned. V1: passes; the old condition fails it | | +| W07-037 | S3 | Fixed | W07-002's attributed workout delete selected every workout under `my-whoop-noop` with no source filter, so a legacy workout the user entered under the canonical id, covered only by the deleted strap's heart rate, was deleted as if it were a detected bout, while edited nights were spared | `DeviceRegistryStore.deleteAttributedCanonicalRows` workout loop | Found by the 2026-10-03 `/code-review xhigh` (the V2 review noted the missing filter). V0: the delete test with a manual workout under the canonical id failed (deleted). Fix: only detected bouts are attributed, rows banked with sport `detected` or a computed source; `source = 'manual'` is user data. The fixture's detected rows now carry the shape the engine banks. V1: passes; without the filter it fails. Not changed: the exclusive-sibling path (a device's own `-noop` id) still clears every workout there | | ## Log @@ -123,3 +124,4 @@ the two biggest hot spots). - 2026-10-03 — W07-023 fixed on `review/w07-fixes`. - 2026-10-03 — `/code-review xhigh` of `review/w07-fixes`: 13 findings, recorded as W07-035 … W07-047 as they are worked; W07-035 fixed. - 2026-10-03 — W07-036 fixed. +- 2026-10-03 — W07-037 fixed. From 356d57655c7280cca2e0157608583645ecb1700a Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 3 Oct 2026 21:46:52 +0200 Subject: [PATCH 07/13] fix(store): a deleted strap's night is not kept for one stray sample of another source (W07-038) The attributed delete kept a canonical night or detected workout when any other source had a single heart-rate sample inside it, so a sparse second source (a chest strap, an imported activity file) protected every overlapping session the deleted strap recorded. A session now goes when the device's heart rate inside it outnumbers every other source's; an even split stays. --- .../WhoopStore/DeviceRegistryStore.swift | 21 ++++++++++++------- .../DeviceRegistryStoreTests.swift | 12 ++++++++--- docs/review/workstreams/W07-app-data.md | 2 ++ 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift b/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift index d1d28014d1..0d2d92b8a0 100644 --- a/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift +++ b/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift @@ -236,9 +236,10 @@ public struct DeviceRegistryStore: Sendable { /// estimator, not a device, so it goes with the day it was computed on (W07-036); a day scored before /// provenance existed has no cells and stays. /// - /// Sleeps and detected workouts under the canonical id carry no day key, so they are attributed by heart rate: a session goes when the device has heart - /// rate inside it and no other source does. Nights the user edited or added - /// stay, as in the exclusive case. Must run before the device's heart rate is deleted. + /// Sleeps and detected workouts under the canonical id carry no day key, so they are attributed by heart + /// rate: a session goes when the device's heart rate inside it outnumbers every other source's (W07-038). + /// Nights the user edited or added, and workouts the user entered, stay. Must run before the device's + /// heart rate is deleted. static func deleteAttributedCanonicalRows(_ db: Database, of deviceId: String) throws { let canonical = canonicalComputedId let cells = try Row.fetchAll(db, sql: """ @@ -294,13 +295,17 @@ public struct DeviceRegistryStore: Sendable { // source. A `deviceId != ?` predicate cannot use the key and scanned the whole heart-rate index per // session, 25–53 s on 115 days of 1 Hz rows inside this write transaction (W07-002 V2). let otherSources = try heartRateSourceIds(db).filter { $0 != deviceId && !$0.hasSuffix(computedSuffix) } - func hasHeartRate(_ id: String, _ start: Int, _ end: Int) throws -> Bool { - try Bool.fetchOne(db, sql: "SELECT EXISTS(SELECT 1 FROM hrSample WHERE deviceId = ? AND ts BETWEEN ? AND ?)", - arguments: [id, start, end]) ?? false + func heartRateCount(_ id: String, _ start: Int, _ end: Int) throws -> Int { + try Int.fetchOne(db, sql: "SELECT COUNT(*) FROM hrSample WHERE deviceId = ? AND ts BETWEEN ? AND ?", + arguments: [id, start, end]) ?? 0 } + // The device recorded the session when its heart rate outnumbers every other source's inside it. A + // single stray sample of another source (a chest strap, an activity file) no longer protects a night + // the device recorded (W07-038); an even split stays, the safe side. func coveredOnlyByDevice(_ start: Int, _ end: Int) throws -> Bool { - guard try hasHeartRate(deviceId, start, end) else { return false } - return try !otherSources.contains { try hasHeartRate($0, start, end) } + let own = try heartRateCount(deviceId, start, end) + guard own > 0 else { return false } + return try !otherSources.contains { try heartRateCount($0, start, end) >= own } } for row in try Row.fetchAll(db, sql: "SELECT startTs, endTs FROM sleepSession WHERE deviceId = ? AND userEdited = 0", arguments: [canonical]) { diff --git a/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift b/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift index de2508f263..80a7a5464b 100644 --- a/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift +++ b/Packages/WhoopStore/Tests/WhoopStoreTests/DeviceRegistryStoreTests.swift @@ -282,7 +282,12 @@ final class DeviceRegistryStoreTests: XCTestCase { for (dev, ts) in [("whoop-new", 1500), ("whoop-new", 3500), ("my-whoop", 3600), ("whoop-new", 5500), ("my-whoop", 7500)] { try db.execute(sql: "INSERT INTO hrSample (deviceId, ts, bpm) VALUES (?, ?, 60)", arguments: [dev, ts]) } - for (start, end, edited) in [(1000, 2000, 0), (3000, 4000, 0), (5000, 6000, 1), (7000, 8000, 0)] { + // A night whoop-new recorded, with one stray my-whoop sample inside it (W07-038). + for ts in stride(from: 9100, through: 9900, by: 90) { + try db.execute(sql: "INSERT INTO hrSample (deviceId, ts, bpm) VALUES ('whoop-new', ?, 60)", arguments: [ts]) + } + try db.execute(sql: "INSERT INTO hrSample (deviceId, ts, bpm) VALUES ('my-whoop', 9500, 60)") + for (start, end, edited) in [(1000, 2000, 0), (3000, 4000, 0), (5000, 6000, 1), (7000, 8000, 0), (9000, 10000, 0)] { try db.execute(sql: "INSERT INTO sleepSession (deviceId, startTs, endTs, userEdited) VALUES ('my-whoop-noop', ?, ?, ?)", arguments: [start, end, edited]) } @@ -324,7 +329,8 @@ final class DeviceRegistryStoreTests: XCTestCase { XCTAssertEqual(left.series, ["1970-01-01:healthspan_model", "B:sleep_performance", "B:spo2_candidate", "L:sleep_performance"], "the unattributed cells of the days whoop-new owned go with them, and the weekly VO2max with Fitness Age (W07-036)") - XCTAssertEqual(left.sleeps, [3000, 5000, 7000], "a night only whoop-new's heart rate covers goes; an edited one stays") + XCTAssertEqual(left.sleeps, [3000, 5000, 7000], + "a night mostly whoop-new's heart rate covers goes, a stray sample of another source notwithstanding (W07-038); an even split and an edited night stay") XCTAssertEqual(left.workouts, [1100, 7000], "a manual workout is user data and stays (W07-037)") } @@ -349,7 +355,7 @@ final class DeviceRegistryStoreTests: XCTestCase { XCTAssertEqual(left.series, ["1970-01-01:healthspan_model", "A:sleep_performance", "A:spo2_candidate", "A:vo2max_est", "L:sleep_performance", "M:sleep_performance", "M:spo2_candidate", "R:sleep_performance"]) - XCTAssertEqual(left.sleeps, [1000, 3000, 5000]) + XCTAssertEqual(left.sleeps, [1000, 3000, 5000, 9000]) XCTAssertEqual(left.workouts, [1000, 1100]) } diff --git a/docs/review/workstreams/W07-app-data.md b/docs/review/workstreams/W07-app-data.md index f3fd229e47..ada317e8e4 100644 --- a/docs/review/workstreams/W07-app-data.md +++ b/docs/review/workstreams/W07-app-data.md @@ -97,6 +97,7 @@ the two biggest hot spots). | W07-035 | S3 | Fixed | W07-002's delete kept a mixed day's whole `dailyMetric` row: the deleted strap's strain and the sleep, HRV and resting-HR columns computed from it stayed on screen, while its provenance cells were removed, so the row lost its source badge; and deleting the strap that supplied only a legacy snapshot's recovery left that recovery | `DeviceRegistryStore.deleteAttributedCanonicalRows` | Found by the 2026-10-03 `/code-review xhigh` of `review/w07-fixes` (W07-002's row recorded it as a limit). V0: the delete tests, extended to assert day M's row and its recovery, failed. Fix: the row belongs to the source its strain cell names (the pass that scored it), else its recovery cell, else the device whose Rest-point cell brought the day in; the owner's delete removes the row, its unattributed cells and its recovery and strain provenance; when the device supplied only the legacy snapshot on another source's row, just the snapshot's columns (recovery, HRV, respiration, SDNN) are cleared. V1: both tests pass with day M and a no-Charge day R; the old guard fails them | | | W07-036 | S4 | Fixed | On a day only the deleted strap supplied, W07-002's sweep removed `fitness_age` but kept `vo2max_est`, because the VO₂max cell's provenance names its estimator (`nes`/`uth`), not a device; both come from one Fitness Age computation over the same week, so the strap's VO₂max stayed beside a missing Fitness Age and kept feeding Body Age | `DeviceRegistryStore.deleteAttributedCanonicalRows` day sweep | Found by the 2026-10-03 `/code-review xhigh` (the V2 review noted the cell stays). V0: the delete test, asserting day A's VO₂max goes, failed. Fix: the sweep treats an estimator-named VO₂max cell as unattributed, so it goes with the day the device owned. V1: passes; the old condition fails it | | | W07-037 | S3 | Fixed | W07-002's attributed workout delete selected every workout under `my-whoop-noop` with no source filter, so a legacy workout the user entered under the canonical id, covered only by the deleted strap's heart rate, was deleted as if it were a detected bout, while edited nights were spared | `DeviceRegistryStore.deleteAttributedCanonicalRows` workout loop | Found by the 2026-10-03 `/code-review xhigh` (the V2 review noted the missing filter). V0: the delete test with a manual workout under the canonical id failed (deleted). Fix: only detected bouts are attributed, rows banked with sport `detected` or a computed source; `source = 'manual'` is user data. The fixture's detected rows now carry the shape the engine banks. V1: passes; without the filter it fails. Not changed: the exclusive-sibling path (a device's own `-noop` id) still clears every workout there | | +| W07-038 | S3 | Fixed | W07-002 kept a canonical night or detected workout whenever any other source had even one heart-rate sample inside it, so a sparse second source (a BLE chest strap, an imported activity file) protected every overlapping session the deleted strap recorded, and no re-score can evict them once its raw is gone | `DeviceRegistryStore.deleteAttributedCanonicalRows` session attribution | Found by the 2026-10-03 `/code-review xhigh`. V0: the delete test with a night of the strap's samples and one stray sample of the other strap failed (kept). Fix: a session goes when the device's heart rate inside it outnumbers every other source's (key-range counts); an even split stays. V1: passes; the any-sample rule fails it. Cost on the 10.9 M-row synthetic store: deleting the second strap 1.6 s (1.2 s with the existence probe), the same 100 nights left | | ## Log @@ -125,3 +126,4 @@ the two biggest hot spots). - 2026-10-03 — `/code-review xhigh` of `review/w07-fixes`: 13 findings, recorded as W07-035 … W07-047 as they are worked; W07-035 fixed. - 2026-10-03 — W07-036 fixed. - 2026-10-03 — W07-037 fixed. +- 2026-10-03 — W07-038 fixed. From c231b4c2873f39d0e88552e093c6cf2220761099 Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 3 Oct 2026 21:47:28 +0200 Subject: [PATCH 08/13] refactor(store): one spelling of the canonical id in the attributed delete (W07-040) W07-002 added canonicalComputedId as "my-whoop" + suffix beside a bare "my-whoop" in exclusiveComputedSibling, and tested "the canonical namespace is this device's alone" as exclusiveSibling != canonicalComputedId. One canonicalDeviceId constant now feeds both, and the condition has a name. No behaviour change; the delete tests pass unchanged. W07-039 (the repeated provenance delete) is kept on purpose. --- .../Sources/WhoopStore/DeviceRegistryStore.swift | 13 +++++++++---- docs/review/workstreams/W07-app-data.md | 3 +++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift b/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift index 0d2d92b8a0..073d2c2ead 100644 --- a/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift +++ b/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift @@ -189,7 +189,10 @@ public struct DeviceRegistryStore: Sendable { public func deleteAllData(deviceId: String) throws { try dbQueue.write { db in let exclusiveSibling = try Self.exclusiveComputedSibling(db, of: deviceId) - if exclusiveSibling != Self.canonicalComputedId { + // The canonical namespace is cleared whole only when it is this device's alone; otherwise only + // what is attributed to the device leaves it. + let canonicalIsDevicesAlone = exclusiveSibling == Self.canonicalComputedId + if !canonicalIsDevicesAlone { try Self.deleteAttributedCanonicalRows(db, of: deviceId) } for table in Self.deviceScopedTables { @@ -217,14 +220,16 @@ public struct DeviceRegistryStore: Sendable { static func exclusiveComputedSibling(_ db: Database, of deviceId: String) throws -> String? { guard !deviceId.hasSuffix(computedSuffix) else { return nil } let sibling = deviceId + computedSuffix - guard deviceId == "my-whoop" else { return sibling } + guard deviceId == canonicalDeviceId else { return sibling } let others = try Int.fetchOne(db, sql: "SELECT COUNT(*) FROM pairedDevice WHERE id != ?", arguments: [deviceId]) ?? 0 return others == 0 ? sibling : nil } - /// The computed namespace the engine writes every scored day under, whichever source supplied it. - static let canonicalComputedId = "my-whoop" + computedSuffix + /// The canonical strap id, and the computed namespace the engine writes every scored day under, + /// whichever source supplied it. One spelling, so the two cannot drift (W07-040). + static let canonicalDeviceId = "my-whoop" + static let canonicalComputedId = canonicalDeviceId + computedSuffix /// Removes from the shared canonical namespace what was computed from `deviceId` (W07-002). /// diff --git a/docs/review/workstreams/W07-app-data.md b/docs/review/workstreams/W07-app-data.md index ada317e8e4..5437dbe341 100644 --- a/docs/review/workstreams/W07-app-data.md +++ b/docs/review/workstreams/W07-app-data.md @@ -98,6 +98,8 @@ the two biggest hot spots). | W07-036 | S4 | Fixed | On a day only the deleted strap supplied, W07-002's sweep removed `fitness_age` but kept `vo2max_est`, because the VO₂max cell's provenance names its estimator (`nes`/`uth`), not a device; both come from one Fitness Age computation over the same week, so the strap's VO₂max stayed beside a missing Fitness Age and kept feeding Body Age | `DeviceRegistryStore.deleteAttributedCanonicalRows` day sweep | Found by the 2026-10-03 `/code-review xhigh` (the V2 review noted the cell stays). V0: the delete test, asserting day A's VO₂max goes, failed. Fix: the sweep treats an estimator-named VO₂max cell as unattributed, so it goes with the day the device owned. V1: passes; the old condition fails it | | | W07-037 | S3 | Fixed | W07-002's attributed workout delete selected every workout under `my-whoop-noop` with no source filter, so a legacy workout the user entered under the canonical id, covered only by the deleted strap's heart rate, was deleted as if it were a detected bout, while edited nights were spared | `DeviceRegistryStore.deleteAttributedCanonicalRows` workout loop | Found by the 2026-10-03 `/code-review xhigh` (the V2 review noted the missing filter). V0: the delete test with a manual workout under the canonical id failed (deleted). Fix: only detected bouts are attributed, rows banked with sport `detected` or a computed source; `source = 'manual'` is user data. The fixture's detected rows now carry the shape the engine banks. V1: passes; without the filter it fails. Not changed: the exclusive-sibling path (a device's own `-noop` id) still clears every workout there | | | W07-038 | S3 | Fixed | W07-002 kept a canonical night or detected workout whenever any other source had even one heart-rate sample inside it, so a sparse second source (a BLE chest strap, an imported activity file) protected every overlapping session the deleted strap recorded, and no re-score can evict them once its raw is gone | `DeviceRegistryStore.deleteAttributedCanonicalRows` session attribution | Found by the 2026-10-03 `/code-review xhigh`. V0: the delete test with a night of the strap's samples and one stray sample of the other strap failed (kept). Fix: a session goes when the device's heart rate inside it outnumbers every other source's (key-range counts); an even split stays. V1: passes; the any-sample rule fails it. Cost on the 10.9 M-row synthetic store: deleting the second strap 1.6 s (1.2 s with the existence probe), the same 100 nights left | | +| W07-039 | S4 | Not a bug | `deleteAttributedCanonicalRows` deletes the device's canonical provenance cells, which `deleteAllData`'s unconditional `DELETE FROM scoreInputProvenance WHERE sourceId = ?` repeats in the same transaction, and deletes series cells one statement per cell | `DeviceRegistryStore.deleteAttributedCanonicalRows` | Found by the 2026-10-03 `/code-review xhigh`. Kept deliberately: the helper's own delete keeps it correct if it is ever called without `deleteAllData`, and the cost is one indexed statement; the per-cell deletes are a handful per deleted day, inside a transaction measured at 1.6 s on 10.9 M heart-rate rows, almost all of it the raw delete | | +| W07-040 | S4 | Fixed | W07-002 added `canonicalComputedId` as `"my-whoop" + suffix` beside a bare `"my-whoop"` in `exclusiveComputedSibling`, and expressed "the canonical namespace is this device's alone" as `exclusiveSibling != canonicalComputedId`; if the canonical id moves (AD-7), one spelling can be missed | `DeviceRegistryStore` | Found by the 2026-10-03 `/code-review xhigh`. Fix: one `canonicalDeviceId` constant, the computed id derived from it, and the condition named (`canonicalIsDevicesAlone`). Cleanup of code this branch added, no behaviour change: the delete tests pass unchanged. The file's older capability-decode literal is AD-7's | | ## Log @@ -127,3 +129,4 @@ the two biggest hot spots). - 2026-10-03 — W07-036 fixed. - 2026-10-03 — W07-037 fixed. - 2026-10-03 — W07-038 fixed. +- 2026-10-03 — W07-039 not a bug; W07-040 fixed. From dde7538d8be7716c3a06f717a20d2d34a4fe096d Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 3 Oct 2026 21:50:22 +0200 Subject: [PATCH 09/13] fix(app): edited nights are read, healed and re-staged across every registered strap (W07-041, W07-042, W07-043) Three gaps W07-023 left in one function, fixed together: - Edited nights came from the active and canonical siblings only, while the Sleep tab shows every registered strap's computed nights; two re-adds later a nap under the first re-added strap was shown but never scored. The heal now reads through rawComputedReadIds, the Sleep tab's resolver, active first. - Only the copy scoring takes was healed, so a second edited copy the Sleep tab still shows went stale. Every copy is healed now. - restageFromRaw read raw under the active strap only, so a night the old strap recorded was never re-staged and a partial active read could overwrite it. The raw now comes from the strap with the most gravity inside the night. --- Strand/Data/Repository.swift | 38 ++++++--- StrandTests/SleepEditReAddedStrapTests.swift | 84 +++++++++++++++++++- docs/review/workstreams/W07-app-data.md | 4 + 3 files changed, 115 insertions(+), 11 deletions(-) diff --git a/Strand/Data/Repository.swift b/Strand/Data/Repository.swift index ec431dc903..543f3b2574 100644 --- a/Strand/Data/Repository.swift +++ b/Strand/Data/Repository.swift @@ -1761,8 +1761,18 @@ final class Repository: ObservableObject { private func restageFromRaw(start: Int, end: Int) async -> String? { guard let store = await ensureStore() else { return nil } let lo = start - 3_600, hi = end + 3_600 - let grav = (try? await store.gravitySamples(deviceId: deviceId, from: lo, to: hi, limit: 200_000)) ?? [] - let inWindowGravity = grav.lazy.filter { $0.ts >= start && $0.ts <= end }.count + // W07-043: the night's raw lives under the strap that recorded it, which after a re-add is not the + // active one. Read from the registered strap with the most gravity inside the window (the active one + // first on a tie), so a canonical night is re-staged from its own raw and never from another strap's + // partial raw. + var deviceId = self.deviceId + var grav: [GravitySample] = [] + var inWindowGravity = 0 + for id in rawPhysiologyReadIds(store: store) { + let g = (try? await store.gravitySamples(deviceId: id, from: lo, to: hi, limit: 200_000)) ?? [] + let n = g.lazy.filter { $0.ts >= start && $0.ts <= end }.count + if n > inWindowGravity { deviceId = id; grav = g; inWindowGravity = n } + } let windowSeconds = max(1, end - start) guard inWindowGravity >= max(20, windowSeconds / 120) else { return nil } let hr = (try? await store.hrSamples(deviceId: deviceId, from: lo, to: hi, limit: 200_000)) ?? [] @@ -1810,20 +1820,28 @@ final class Repository: ObservableObject { /// rows, oldest first, so the caller recomputes daily aggregates from the corrected stages. func selfHealEditedStages(from windowStart: Int, to windowEnd: Int) async -> [CachedSleepSession] { guard let store = await ensureStore() else { return [] } - let ids = computedReadIds - func editedRows() async -> [(id: String, row: CachedSleepSession)] { - var byStart: [Int: (id: String, row: CachedSleepSession)] = [:] + // Every registered strap's computed sibling, active first: the set the Sleep tab shows nights from + // (W07-041). Two re-adds later a nap logged under the first re-added strap's sibling is still shown. + let ids = rawComputedReadIds(store: store) + /// Every edited row, in read order, and the one per start scoring takes (the first read, so the + /// active strap's copy wins). + func editedRows() async -> (all: [(id: String, row: CachedSleepSession)], scored: [CachedSleepSession]) { + var all: [(id: String, row: CachedSleepSession)] = [] for id in ids { let rows = ((try? await store.sleepSessions(deviceId: id, from: windowStart, to: windowEnd, limit: 100_000)) ?? []) - for row in rows where row.userEdited && byStart[row.startTs] == nil { byStart[row.startTs] = (id, row) } + all += rows.filter(\.userEdited).map { (id, $0) } } - return byStart.values.sorted { $0.row.startTs < $1.row.startTs } + var byStart: [Int: CachedSleepSession] = [:] + for (_, row) in all where byStart[row.startTs] == nil { byStart[row.startTs] = row } + return (all, byStart.values.sorted { $0.startTs < $1.startTs }) } let edited = await editedRows() - guard !edited.isEmpty else { return [] } + guard !edited.all.isEmpty else { return [] } var healed = false - for (id, row) in edited { + // Every copy is healed, not only the one scoring takes, so a copy the Sleep tab still shows does not + // go stale (W07-042). + for (id, row) in edited.all { // Re-derive over the LOCKED corrected window (effective onset → wake). Skip when the raw // isn't dense yet, or when the result already matches what's stored (steady state , no write). guard let newJSON = await restageFromRaw(start: row.effectiveStartTs, end: row.endTs), @@ -1833,7 +1851,7 @@ final class Repository: ObservableObject { stagesJSON: newJSON)) ?? 0 if n > 0 { healed = true } } - return (healed ? await editedRows() : edited).map(\.row) + return (healed ? await editedRows() : edited).scored } // MARK: - Metric explorer reads (generic substrate) diff --git a/StrandTests/SleepEditReAddedStrapTests.swift b/StrandTests/SleepEditReAddedStrapTests.swift index 69674f76f2..f11f4b803c 100644 --- a/StrandTests/SleepEditReAddedStrapTests.swift +++ b/StrandTests/SleepEditReAddedStrapTests.swift @@ -1,5 +1,7 @@ import XCTest import WhoopStore +import WhoopProtocol +import StrandAnalytics @testable import Strand /// W07-023: after a strap is removed and re-added the registry's active id is `whoop-`, so @@ -24,7 +26,8 @@ final class SleepEditReAddedStrapTests: XCTestCase { } /// The active strap's own edited nights are still read, and one night edited under both siblings is - /// returned once, the active strap's copy winning, as every other computed read does. + /// returned once to scoring, the active strap's copy winning (the Sleep tab's union keeps both copies, + /// which is the display's question, not scoring's). func testActiveSiblingWinsADuplicatedEditedNight() async throws { let store = try await WhoopStore.inMemory() let canonical = CachedSleepSession(startTs: 1_000_000, endTs: 1_028_800, efficiency: 0.8, restingHr: 52, @@ -44,4 +47,83 @@ final class SleepEditReAddedStrapTests: XCTestCase { XCTAssertEqual(rows.map(\.startTs), [1_000_000, 1_050_000]) XCTAssertEqual(rows.first?.endTs, 1_030_000, "the active strap's copy wins") } + + private func registry(_ store: WhoopStore, _ ids: [(String, DeviceStatus)]) throws { + let reg = DeviceRegistryStore(dbQueue: store.registryWriter) + for (id, status) in ids { + try reg.add(PairedDevice(id: id, brand: "WHOOP", model: "WHOOP 5.0 / MG", sourceKind: .liveBLE, + capabilities: [.hr, .sleep], status: status, addedAt: 1, lastSeenAt: 1)) + } + } + + /// Dense still gravity and calm HR over [start, end], one sample a minute. + private func night(_ start: Int, _ end: Int) -> Streams { + let ts = Array(stride(from: start, through: end, by: 60)) + return Streams(hr: ts.map { HRSample(ts: $0, bpm: 50) }, + gravity: ts.map { GravitySample(ts: $0, x: 0, y: 0, z: 1) }) + } + + /// W07-041: two re-adds later, a nap logged under the first re-added strap's sibling is still a night the + /// Sleep tab shows (it reads every registered strap's computed sibling), so scoring must read it too. + func testAnEditedNightUnderAnEarlierReAddedStrapReachesScoring() async throws { + let store = try await WhoopStore.inMemory() + try registry(store, [("whoop-old", .archived), ("whoop-new", .active)]) + let nap = CachedSleepSession(startTs: 1_000_000, endTs: 1_005_400, efficiency: 0.9, restingHr: 55, + avgHrv: nil, stagesJSON: nil, userEdited: true) + _ = try await store.upsertSleepSessions([nap], deviceId: "whoop-old-noop") + let repo = Repository(deviceId: "my-whoop") + repo.setStoreForTesting(store) + _ = repo.adoptActiveDeviceId("whoop-new") + + let rows = await repo.selfHealEditedStages(from: 900_000, to: 1_100_000) + XCTAssertEqual(rows.map(\.startTs), [1_000_000]) + } + + /// W07-042: a night edited under two siblings is healed in both, so the copy that does not win scoring + /// does not go stale while the Sleep tab still shows it. + func testEveryEditedCopyIsHealed() async throws { + let store = try await WhoopStore.inMemory() + try registry(store, [("whoop-x", .active)]) + let start = 1_000_000, end = 1_028_800 + _ = try await store.insert(night(start - 3_600, end + 3_600), deviceId: "whoop-x") + for id in ["my-whoop-noop", "whoop-x-noop"] { + _ = try await store.upsertSleepSessions([CachedSleepSession(startTs: start, endTs: end, efficiency: 0.9, + restingHr: 50, avgHrv: nil, stagesJSON: nil, + userEdited: true)], deviceId: id) + } + let repo = Repository(deviceId: "my-whoop") + repo.setStoreForTesting(store) + _ = repo.adoptActiveDeviceId("whoop-x") + + _ = await repo.selfHealEditedStages(from: start - 100, to: end + 100) + for id in ["my-whoop-noop", "whoop-x-noop"] { + let row = try await store.sleepSessions(deviceId: id, from: start - 100, to: end + 100, limit: 10).first + XCTAssertNotNil(row?.stagesJSON, "the copy under \(id) was not healed") + } + } + + /// W07-043: a canonical night the old strap recorded is re-staged from the old strap's raw, not left + /// alone because the active strap holds none (and never overwritten from the active strap's sparse raw). + func testACanonicalNightIsRestagedFromTheStrapThatRecordedIt() async throws { + let store = try await WhoopStore.inMemory() + try registry(store, [("whoop-x", .active)]) + let start = 1_000_000, end = 1_028_800 + _ = try await store.insert(night(start - 3_600, end + 3_600), deviceId: "my-whoop") + _ = try await store.insert(Streams(hr: [HRSample(ts: start + 600, bpm: 90)], + gravity: stride(from: start, through: start + 30 * 120, by: 120) + .map { GravitySample(ts: $0, x: 0.3, y: 0, z: 0.9) }), + deviceId: "whoop-x") + _ = try await store.upsertSleepSessions([CachedSleepSession(startTs: start, endTs: end, efficiency: 0.9, + restingHr: 50, avgHrv: nil, stagesJSON: nil, + userEdited: true)], deviceId: "my-whoop-noop") + let repo = Repository(deviceId: "my-whoop") + repo.setStoreForTesting(store) + _ = repo.adoptActiveDeviceId("whoop-x") + + let rows = await repo.selfHealEditedStages(from: start - 100, to: end + 100) + let stages = AnalyticsEngine.decodeStages(rows.first?.stagesJSON) + XCTAssertFalse(stages.isEmpty, "the night was not re-staged from the strap that recorded it") + XCTAssertFalse(stages.contains { $0.stage == "wake" && $0.end - $0.start > 3_600 }, + "the night was staged from the active strap's sparse, moving raw") + } } diff --git a/docs/review/workstreams/W07-app-data.md b/docs/review/workstreams/W07-app-data.md index 5437dbe341..100f878fb6 100644 --- a/docs/review/workstreams/W07-app-data.md +++ b/docs/review/workstreams/W07-app-data.md @@ -100,6 +100,9 @@ the two biggest hot spots). | W07-038 | S3 | Fixed | W07-002 kept a canonical night or detected workout whenever any other source had even one heart-rate sample inside it, so a sparse second source (a BLE chest strap, an imported activity file) protected every overlapping session the deleted strap recorded, and no re-score can evict them once its raw is gone | `DeviceRegistryStore.deleteAttributedCanonicalRows` session attribution | Found by the 2026-10-03 `/code-review xhigh`. V0: the delete test with a night of the strap's samples and one stray sample of the other strap failed (kept). Fix: a session goes when the device's heart rate inside it outnumbers every other source's (key-range counts); an even split stays. V1: passes; the any-sample rule fails it. Cost on the 10.9 M-row synthetic store: deleting the second strap 1.6 s (1.2 s with the existence probe), the same 100 nights left | | | W07-039 | S4 | Not a bug | `deleteAttributedCanonicalRows` deletes the device's canonical provenance cells, which `deleteAllData`'s unconditional `DELETE FROM scoreInputProvenance WHERE sourceId = ?` repeats in the same transaction, and deletes series cells one statement per cell | `DeviceRegistryStore.deleteAttributedCanonicalRows` | Found by the 2026-10-03 `/code-review xhigh`. Kept deliberately: the helper's own delete keeps it correct if it is ever called without `deleteAllData`, and the cost is one indexed statement; the per-cell deletes are a handful per deleted day, inside a transaction measured at 1.6 s on 10.9 M heart-rate rows, almost all of it the raw delete | | | W07-040 | S4 | Fixed | W07-002 added `canonicalComputedId` as `"my-whoop" + suffix` beside a bare `"my-whoop"` in `exclusiveComputedSibling`, and expressed "the canonical namespace is this device's alone" as `exclusiveSibling != canonicalComputedId`; if the canonical id moves (AD-7), one spelling can be missed | `DeviceRegistryStore` | Found by the 2026-10-03 `/code-review xhigh`. Fix: one `canonicalDeviceId` constant, the computed id derived from it, and the condition named (`canonicalIsDevicesAlone`). Cleanup of code this branch added, no behaviour change: the delete tests pass unchanged. The file's older capability-decode literal is AD-7's | | +| W07-041 | S3 | Fixed | W07-023 read edited nights from `computedReadIds` (active and canonical only), while the Sleep tab shows computed nights from every registered strap's sibling (`rawComputedReadIds`): two re-adds later, a nap logged under the first re-added strap's sibling is shown but never reaches its day's totals, Rest or Charge | `Repository.selfHealEditedStages` | Found by the 2026-10-03 `/code-review xhigh`. V0: `SleepEditReAddedStrapTests.testAnEditedNightUnderAnEarlierReAddedStrapReachesScoring` failed (no rows). Fix: the heal reads through `rawComputedReadIds`, the Sleep tab's resolver, active first. V1: passes (commit shared with W07-042 and W07-043; one function) | | +| W07-042 | S4 | Fixed | W07-023 kept one edited row per start for scoring and healed only that one, so a second edited copy of the night under another sibling, which the Sleep tab's union still shows, went stale; its test comment also claimed this matched every other computed read, which de-duplicate by start and end | `Repository.selfHealEditedStages`; `SleepEditReAddedStrapTests` | Found by the 2026-10-03 `/code-review xhigh`. V0: `testEveryEditedCopyIsHealed` failed (the canonical copy kept no stages). Fix: every edited copy is healed; scoring still takes one per start, the active copy winning; the comment now says the Sleep tab keeps both copies and that is the display's question. V1: passes | | +| W07-043 | S3 | Fixed | `restageFromRaw` read the night's raw only under the active strap id, so after a re-add a canonical night the old strap recorded was never re-staged, and with overlapping straps the active strap's partial raw could pass the density gate and overwrite the night's stages with another strap's data | `Repository.restageFromRaw` | Found by the 2026-10-03 `/code-review xhigh` (W07-023's row recorded it as not changed; AD-7 evidence). V0: `testACanonicalNightIsRestagedFromTheStrapThatRecordedIt` failed. Fix: the raw is read from the registered strap with the most gravity inside the window (active first on a tie); the edit path shares it. V1: passes. AD-7's resolver remains the structural answer | | ## Log @@ -130,3 +133,4 @@ the two biggest hot spots). - 2026-10-03 — W07-037 fixed. - 2026-10-03 — W07-038 fixed. - 2026-10-03 — W07-039 not a bug; W07-040 fixed. +- 2026-10-03 — W07-041, W07-042 and W07-043 fixed in one commit (one function). From 5d29140e8529f30dd1d86e695cf1a04b2255eb1a Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 3 Oct 2026 21:52:16 +0200 Subject: [PATCH 10/13] fix(app): lastSyncedAt can be written only through the two attributed writers (W07-045) W07-016 documented that seedLastSynced and stampCompletedSync are the only writers of lastSyncedAt, each setting its origin first, but the property kept a public setter, so a direct write would leave the origin stale. The setter is private now; SyncChipStateTests stamps through stampCompletedSync. --- Strand/BLE/LiveState.swift | 5 ++++- StrandTests/SyncChipStateTests.swift | 10 +++++----- docs/review/workstreams/W07-app-data.md | 2 ++ 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/Strand/BLE/LiveState.swift b/Strand/BLE/LiveState.swift index b450cfeeda..4797c97092 100644 --- a/Strand/BLE/LiveState.swift +++ b/Strand/BLE/LiveState.swift @@ -533,7 +533,10 @@ public final class LiveState: ObservableObject { /// Wall time (unix seconds) of the last successfully-completed offload (a sync, even if nothing new /// came — i.e. caught up). Drives the sync tile + the staleness nudge. - @Published public var lastSyncedAt: TimeInterval? + /// + /// The setter is private (W07-045): `seedLastSynced` and `stampCompletedSync` are the only writers, so + /// `lastSyncedAtOrigin` always names the writer of the value an observer is handed. + @Published public private(set) var lastSyncedAt: TimeInterval? /// Which writer set `lastSyncedAt` last (W07-016). Written only by the two methods below, each before /// the value, so an observer of `$lastSyncedAt` reads the origin of the value it was handed. private(set) var lastSyncedAtOrigin: LastSyncOrigin? diff --git a/StrandTests/SyncChipStateTests.swift b/StrandTests/SyncChipStateTests.swift index 39b88a935a..3238e28f13 100644 --- a/StrandTests/SyncChipStateTests.swift +++ b/StrandTests/SyncChipStateTests.swift @@ -20,7 +20,7 @@ final class SyncChipStateTests: XCTestCase { func testLastSyncedAt_isSyncedWithAgeText() { let live = LiveState() - live.lastSyncedAt = Date().timeIntervalSince1970 - 65 + live.stampCompletedSync(at: Date().timeIntervalSince1970 - 65) XCTAssertEqual(SyncChipState.resolve(live: live), .synced(agoText: "1m")) } @@ -30,7 +30,7 @@ final class SyncChipStateTests: XCTestCase { /// "<1m" is the fix and this pins it. Twin of the Android `lastSyncedUnderAMinute_usesSubMinuteToken`. func testLastSyncedUnderAMinute_usesSubMinuteToken() { let live = LiveState() - live.lastSyncedAt = Date().timeIntervalSince1970 - 5 + live.stampCompletedSync(at: Date().timeIntervalSince1970 - 5) XCTAssertEqual(SyncChipState.resolve(live: live), .synced(agoText: "<1m")) } @@ -49,13 +49,13 @@ final class SyncChipStateTests: XCTestCase { let live = LiveState() live.backfilling = true live.syncChunksThisSession = 2 - live.lastSyncedAt = Date().timeIntervalSince1970 - 5 + live.stampCompletedSync(at: Date().timeIntervalSince1970 - 5) XCTAssertEqual(SyncChipState.resolve(live: live), .syncing(chunks: 2, pagesBehind: nil)) } func testLastSyncedAt_takesPriorityOverHistorySyncExperimental() { let live = LiveState() - live.lastSyncedAt = Date().timeIntervalSince1970 - 5 + live.stampCompletedSync(at: Date().timeIntervalSince1970 - 5) live.historySyncExperimental = true if case .synced = SyncChipState.resolve(live: live) { // expected @@ -101,7 +101,7 @@ final class SyncChipStateTests: XCTestCase { func testNotBackfillingIgnoresTheBacklog() { let live = LiveState() live.backfilling = false - live.lastSyncedAt = Date().timeIntervalSince1970 - 65 + live.stampCompletedSync(at: Date().timeIntervalSince1970 - 65) live.pagesBehindAtConnect = 120 XCTAssertEqual(SyncChipState.resolve(live: live), .synced(agoText: "1m")) } diff --git a/docs/review/workstreams/W07-app-data.md b/docs/review/workstreams/W07-app-data.md index 100f878fb6..8b46eba5c9 100644 --- a/docs/review/workstreams/W07-app-data.md +++ b/docs/review/workstreams/W07-app-data.md @@ -103,6 +103,7 @@ the two biggest hot spots). | W07-041 | S3 | Fixed | W07-023 read edited nights from `computedReadIds` (active and canonical only), while the Sleep tab shows computed nights from every registered strap's sibling (`rawComputedReadIds`): two re-adds later, a nap logged under the first re-added strap's sibling is shown but never reaches its day's totals, Rest or Charge | `Repository.selfHealEditedStages` | Found by the 2026-10-03 `/code-review xhigh`. V0: `SleepEditReAddedStrapTests.testAnEditedNightUnderAnEarlierReAddedStrapReachesScoring` failed (no rows). Fix: the heal reads through `rawComputedReadIds`, the Sleep tab's resolver, active first. V1: passes (commit shared with W07-042 and W07-043; one function) | | | W07-042 | S4 | Fixed | W07-023 kept one edited row per start for scoring and healed only that one, so a second edited copy of the night under another sibling, which the Sleep tab's union still shows, went stale; its test comment also claimed this matched every other computed read, which de-duplicate by start and end | `Repository.selfHealEditedStages`; `SleepEditReAddedStrapTests` | Found by the 2026-10-03 `/code-review xhigh`. V0: `testEveryEditedCopyIsHealed` failed (the canonical copy kept no stages). Fix: every edited copy is healed; scoring still takes one per start, the active copy winning; the comment now says the Sleep tab keeps both copies and that is the display's question. V1: passes | | | W07-043 | S3 | Fixed | `restageFromRaw` read the night's raw only under the active strap id, so after a re-add a canonical night the old strap recorded was never re-staged, and with overlapping straps the active strap's partial raw could pass the density gate and overwrite the night's stages with another strap's data | `Repository.restageFromRaw` | Found by the 2026-10-03 `/code-review xhigh` (W07-023's row recorded it as not changed; AD-7 evidence). V0: `testACanonicalNightIsRestagedFromTheStrapThatRecordedIt` failed. Fix: the raw is read from the registered strap with the most gravity inside the window (active first on a tie); the edit path shares it. V1: passes. AD-7's resolver remains the structural answer | | +| W07-045 | S4 | Fixed | W07-016 documented that `lastSyncedAt` is written only by `seedLastSynced` and `stampCompletedSync`, each setting its origin first, but the property kept a public setter, so any direct write would leave the origin stale and the refresh would be logged under the previous writer's origin | `LiveState.lastSyncedAt` | Found by the 2026-10-03 `/code-review xhigh`. V0 by reading: no app code writes it directly; `SyncChipStateTests` did, at five sites. Fix: `public private(set)`; the tests stamp through `stampCompletedSync`. V1: the app compiles, `SyncChipStateTests` and `LastSyncAttributionTests` pass | | ## Log @@ -134,3 +135,4 @@ the two biggest hot spots). - 2026-10-03 — W07-038 fixed. - 2026-10-03 — W07-039 not a bug; W07-040 fixed. - 2026-10-03 — W07-041, W07-042 and W07-043 fixed in one commit (one function). +- 2026-10-03 — W07-045 fixed. From 752bd87fa63efc95d398710a5ccda7b40a05c4a2 Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 3 Oct 2026 21:53:12 +0200 Subject: [PATCH 11/13] fix(app): a last-sync change with no recorded writer is not logged as a completed sync (W07-044) W07-016's sink defaulted an unrecorded origin to completedSync, so a value whose writer it could not name would be logged as a completed sync with trigger=post-offload, the claim W07-016 removed. Such a value is now reported as unattributed, with its own neutral line and trigger. --- Strand/App/AppModel.swift | 2 +- Strand/BLE/LastSyncAttribution.swift | 11 +++++++++++ StrandTests/LastSyncAttributionTests.swift | 8 ++++++++ docs/review/workstreams/W07-app-data.md | 2 ++ 4 files changed, 22 insertions(+), 1 deletion(-) diff --git a/Strand/App/AppModel.swift b/Strand/App/AppModel.swift index e0d7548fa4..7f9be8fdaa 100644 --- a/Strand/App/AppModel.swift +++ b/Strand/App/AppModel.swift @@ -376,7 +376,7 @@ final class AppModel: ObservableObject { // W07-016: the launch seed of the persisted time is an emission too. Read the origin here, // after the debounce: if a HISTORY_COMPLETE landed inside it, a sync did complete. guard let self else { return } - let origin = self.live.lastSyncedAtOrigin ?? .completedSync + let origin = LastSyncOrigin.observed(self.live.lastSyncedAtOrigin) Task { [weak self] in await self?.refreshAfterCompletedBackfill(origin: origin) } } .store(in: &hrCancellables) diff --git a/Strand/BLE/LastSyncAttribution.swift b/Strand/BLE/LastSyncAttribution.swift index c3fa706009..b97f130e46 100644 --- a/Strand/BLE/LastSyncAttribution.swift +++ b/Strand/BLE/LastSyncAttribution.swift @@ -84,6 +84,14 @@ enum LastSyncOrigin: Equatable { case persisted /// Stamped by a HISTORY_COMPLETE in this session. case completedSync + /// A value whose writer the observer cannot name (W07-044). Unreachable while the two writers above are + /// the only ones, which `LiveState` enforces; if it is ever reached, the log says so instead of + /// claiming a sync. + case unattributed + + /// The origin an observer of `lastSyncedAt` reports for the value it was handed: the recorded writer, + /// or `unattributed` when none is recorded. Never a completed sync by default (W07-044). + static func observed(_ recorded: LastSyncOrigin?) -> LastSyncOrigin { recorded ?? .unattributed } /// The strap-log line for the dashboard refresh this origin causes. The completed-sync wording is /// unchanged, since existing logs and their readers know it. @@ -93,6 +101,8 @@ enum LastSyncOrigin: Equatable { return "Launch: refreshing dashboard cache from the persisted last-sync time (no sync this session yet)" case .completedSync: return "Backfill: refreshing dashboard cache from completed sync" + case .unattributed: + return "Refreshing dashboard cache after a last-sync change this session cannot attribute" } } @@ -102,6 +112,7 @@ enum LastSyncOrigin: Equatable { switch self { case .persisted: return "launch-seed" case .completedSync: return nil + case .unattributed: return "last-sync-unattributed" } } } diff --git a/StrandTests/LastSyncAttributionTests.swift b/StrandTests/LastSyncAttributionTests.swift index 2ce2a25397..1bc39f4c58 100644 --- a/StrandTests/LastSyncAttributionTests.swift +++ b/StrandTests/LastSyncAttributionTests.swift @@ -133,4 +133,12 @@ final class LastSyncAttributionTests: XCTestCase { XCTAssertFalse(LastSyncOrigin.persisted.refreshLogLine.contains("completed sync")) XCTAssertEqual(LastSyncOrigin.persisted.rescoreTriggerLabel, "launch-seed") } + + /// W07-044: a value with no recorded writer is reported as unattributed, never as a completed sync. + func testAnUnrecordedOriginIsNotLoggedAsACompletedSync() { + XCTAssertEqual(LastSyncOrigin.observed(nil), .unattributed) + XCTAssertEqual(LastSyncOrigin.observed(.completedSync), .completedSync) + XCTAssertFalse(LastSyncOrigin.unattributed.refreshLogLine.contains("completed sync")) + XCTAssertEqual(LastSyncOrigin.unattributed.rescoreTriggerLabel, "last-sync-unattributed") + } } diff --git a/docs/review/workstreams/W07-app-data.md b/docs/review/workstreams/W07-app-data.md index 8b46eba5c9..56fde1194f 100644 --- a/docs/review/workstreams/W07-app-data.md +++ b/docs/review/workstreams/W07-app-data.md @@ -104,6 +104,7 @@ the two biggest hot spots). | W07-042 | S4 | Fixed | W07-023 kept one edited row per start for scoring and healed only that one, so a second edited copy of the night under another sibling, which the Sleep tab's union still shows, went stale; its test comment also claimed this matched every other computed read, which de-duplicate by start and end | `Repository.selfHealEditedStages`; `SleepEditReAddedStrapTests` | Found by the 2026-10-03 `/code-review xhigh`. V0: `testEveryEditedCopyIsHealed` failed (the canonical copy kept no stages). Fix: every edited copy is healed; scoring still takes one per start, the active copy winning; the comment now says the Sleep tab keeps both copies and that is the display's question. V1: passes | | | W07-043 | S3 | Fixed | `restageFromRaw` read the night's raw only under the active strap id, so after a re-add a canonical night the old strap recorded was never re-staged, and with overlapping straps the active strap's partial raw could pass the density gate and overwrite the night's stages with another strap's data | `Repository.restageFromRaw` | Found by the 2026-10-03 `/code-review xhigh` (W07-023's row recorded it as not changed; AD-7 evidence). V0: `testACanonicalNightIsRestagedFromTheStrapThatRecordedIt` failed. Fix: the raw is read from the registered strap with the most gravity inside the window (active first on a tie); the edit path shares it. V1: passes. AD-7's resolver remains the structural answer | | | W07-045 | S4 | Fixed | W07-016 documented that `lastSyncedAt` is written only by `seedLastSynced` and `stampCompletedSync`, each setting its origin first, but the property kept a public setter, so any direct write would leave the origin stale and the refresh would be logged under the previous writer's origin | `LiveState.lastSyncedAt` | Found by the 2026-10-03 `/code-review xhigh`. V0 by reading: no app code writes it directly; `SyncChipStateTests` did, at five sites. Fix: `public private(set)`; the tests stamp through `stampCompletedSync`. V1: the app compiles, `SyncChipStateTests` and `LastSyncAttributionTests` pass | | +| W07-044 | S4 | Fixed | W07-016's sink took an unrecorded origin as `.completedSync`, so a value whose writer it could not name would be logged as a completed sync with `trigger=post-offload`, the defect W07-016 removed, through the fallback (AGENTS.md: a diagnostic may only assert what it can attribute) | `AppModel` `lastSyncedAt` sink | Found by the 2026-10-03 `/code-review xhigh`. V0 by reading (`?? .completedSync`). Fix: `LastSyncOrigin.unattributed` with its own neutral line and `trigger=last-sync-unattributed`; the sink reports `LastSyncOrigin.observed(_:)`. Unreachable while W07-045's private setter holds. V1: `testAnUnrecordedOriginIsNotLoggedAsACompletedSync` | | ## Log @@ -136,3 +137,4 @@ the two biggest hot spots). - 2026-10-03 — W07-039 not a bug; W07-040 fixed. - 2026-10-03 — W07-041, W07-042 and W07-043 fixed in one commit (one function). - 2026-10-03 — W07-045 fixed. +- 2026-10-03 — W07-044 fixed. From ac7eb51ed8310b863f41e8602a4bdfaa0039aeb6 Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 3 Oct 2026 21:53:58 +0200 Subject: [PATCH 12/13] fix(app): the seeded refresh's log line does not claim a launch (W07-046) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The persisted last-sync time is seeded from bootstrapStore, which beginBackfill retries on a later tick when the first open fails, for example on a relaunch while the phone is locked. The "Launch:" line and trigger=launch-seed could then appear after Connected, asserting a launch the code did not see. The line now reads "Seed: …" and the trigger is stored-seed. --- Strand/BLE/LastSyncAttribution.swift | 9 ++++++--- StrandTests/LastSyncAttributionTests.swift | 4 +++- docs/review/workstreams/W07-app-data.md | 2 ++ 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/Strand/BLE/LastSyncAttribution.swift b/Strand/BLE/LastSyncAttribution.swift index b97f130e46..50b273c353 100644 --- a/Strand/BLE/LastSyncAttribution.swift +++ b/Strand/BLE/LastSyncAttribution.swift @@ -80,7 +80,9 @@ enum LastSyncAttribution { /// The work is the same either way, and on a background launch the seeded pass is the only one the /// launch gets, so the pass is kept. What differs is what the log may claim about it. enum LastSyncOrigin: Equatable { - /// Read back from defaults at launch. No sync has completed in this session. + /// Read back from defaults when the store is bootstrapped: at launch, or later when a first open failed + /// (a relaunch while the phone is locked) and a later tick retried it. No sync has completed in this + /// session. case persisted /// Stamped by a HISTORY_COMPLETE in this session. case completedSync @@ -98,7 +100,8 @@ enum LastSyncOrigin: Equatable { var refreshLogLine: String { switch self { case .persisted: - return "Launch: refreshing dashboard cache from the persisted last-sync time (no sync this session yet)" + // W07-046: not "Launch:", since a retried bootstrap seeds after the strap has connected. + return "Seed: refreshing dashboard cache from the stored last-sync time (no sync completed this session yet)" case .completedSync: return "Backfill: refreshing dashboard cache from completed sync" case .unattributed: @@ -110,7 +113,7 @@ enum LastSyncOrigin: Equatable { /// `post-offload`, as it did before this type existed. var rescoreTriggerLabel: String? { switch self { - case .persisted: return "launch-seed" + case .persisted: return "stored-seed" case .completedSync: return nil case .unattributed: return "last-sync-unattributed" } diff --git a/StrandTests/LastSyncAttributionTests.swift b/StrandTests/LastSyncAttributionTests.swift index 1bc39f4c58..8c5c40da34 100644 --- a/StrandTests/LastSyncAttributionTests.swift +++ b/StrandTests/LastSyncAttributionTests.swift @@ -131,7 +131,9 @@ final class LastSyncAttributionTests: XCTestCase { "Backfill: refreshing dashboard cache from completed sync") XCTAssertNil(LastSyncOrigin.completedSync.rescoreTriggerLabel) XCTAssertFalse(LastSyncOrigin.persisted.refreshLogLine.contains("completed sync")) - XCTAssertEqual(LastSyncOrigin.persisted.rescoreTriggerLabel, "launch-seed") + XCTAssertEqual(LastSyncOrigin.persisted.rescoreTriggerLabel, "stored-seed") + XCTAssertFalse(LastSyncOrigin.persisted.refreshLogLine.hasPrefix("Launch"), + "a retried bootstrap seeds after launch (W07-046)") } /// W07-044: a value with no recorded writer is reported as unattributed, never as a completed sync. diff --git a/docs/review/workstreams/W07-app-data.md b/docs/review/workstreams/W07-app-data.md index 56fde1194f..5501d3bb07 100644 --- a/docs/review/workstreams/W07-app-data.md +++ b/docs/review/workstreams/W07-app-data.md @@ -105,6 +105,7 @@ the two biggest hot spots). | W07-043 | S3 | Fixed | `restageFromRaw` read the night's raw only under the active strap id, so after a re-add a canonical night the old strap recorded was never re-staged, and with overlapping straps the active strap's partial raw could pass the density gate and overwrite the night's stages with another strap's data | `Repository.restageFromRaw` | Found by the 2026-10-03 `/code-review xhigh` (W07-023's row recorded it as not changed; AD-7 evidence). V0: `testACanonicalNightIsRestagedFromTheStrapThatRecordedIt` failed. Fix: the raw is read from the registered strap with the most gravity inside the window (active first on a tie); the edit path shares it. V1: passes. AD-7's resolver remains the structural answer | | | W07-045 | S4 | Fixed | W07-016 documented that `lastSyncedAt` is written only by `seedLastSynced` and `stampCompletedSync`, each setting its origin first, but the property kept a public setter, so any direct write would leave the origin stale and the refresh would be logged under the previous writer's origin | `LiveState.lastSyncedAt` | Found by the 2026-10-03 `/code-review xhigh`. V0 by reading: no app code writes it directly; `SyncChipStateTests` did, at five sites. Fix: `public private(set)`; the tests stamp through `stampCompletedSync`. V1: the app compiles, `SyncChipStateTests` and `LastSyncAttributionTests` pass | | | W07-044 | S4 | Fixed | W07-016's sink took an unrecorded origin as `.completedSync`, so a value whose writer it could not name would be logged as a completed sync with `trigger=post-offload`, the defect W07-016 removed, through the fallback (AGENTS.md: a diagnostic may only assert what it can attribute) | `AppModel` `lastSyncedAt` sink | Found by the 2026-10-03 `/code-review xhigh`. V0 by reading (`?? .completedSync`). Fix: `LastSyncOrigin.unattributed` with its own neutral line and `trigger=last-sync-unattributed`; the sink reports `LastSyncOrigin.observed(_:)`. Unreachable while W07-045's private setter holds. V1: `testAnUnrecordedOriginIsNotLoggedAsACompletedSync` | | +| W07-046 | S4 | Fixed | W07-016's seeded refresh was logged `Launch: …` with `trigger=launch-seed`, but the seed runs from `bootstrapStore`, which `beginBackfill` retries on a later tick when the first open fails (a relaunch while the phone is locked), so the line could appear after `Connected`, long after launch, asserting a launch it did not see | `LastSyncOrigin.persisted` line and trigger | Found by the 2026-10-03 `/code-review xhigh`. V0 by reading (`BLEManager` seeds inside `bootstrapStore`, retried from `beginBackfill`). Fix: `Seed: refreshing dashboard cache from the stored last-sync time (no sync completed this session yet)` and `trigger=stored-seed`. W07-016's strap check now reads: the seed line, and `post-offload` only after a HISTORY_COMPLETE. V1: `LastSyncAttributionTests` | | ## Log @@ -138,3 +139,4 @@ the two biggest hot spots). - 2026-10-03 — W07-041, W07-042 and W07-043 fixed in one commit (one function). - 2026-10-03 — W07-045 fixed. - 2026-10-03 — W07-044 fixed. +- 2026-10-03 — W07-046 fixed (W07-016's seed line no longer claims a launch). From 1f9d4af1dc101f5c87caf36a945868f0edb01458 Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 3 Oct 2026 21:55:43 +0200 Subject: [PATCH 13/13] test(app): the last-sync attribution test drives the app's own refresh chain (W07-047) W07-016's test subscribed to $lastSyncedAt directly, not to the chain the app runs (drop first, de-duplicate, 2-second debounce, origin read after it), so a change that broke the chain would still pass. The chain is now AppModel.lastSyncRefreshes(_:debounce:), which the app sinks and the test drives: a seed followed by a HISTORY_COMPLETE settles once as a completed sync, a seed alone as the stored seed. --- Strand/App/AppModel.swift | 27 ++++++++++++++-------- StrandTests/LastSyncAttributionTests.swift | 23 ++++++++++++++++++ docs/review/workstreams/W07-app-data.md | 2 ++ 3 files changed, 42 insertions(+), 10 deletions(-) diff --git a/Strand/App/AppModel.swift b/Strand/App/AppModel.swift index 7f9be8fdaa..0b5b109209 100644 --- a/Strand/App/AppModel.swift +++ b/Strand/App/AppModel.swift @@ -367,16 +367,8 @@ final class AppModel: ObservableObject { // trailing edge, so the dashboard still refreshes with the newly-synced data , freshness is kept, // we just stop re-doing it dozens of times mid-download. removeDuplicates() still drops a slice that // stamped an identical second; the trailing refresh after a real change is never dropped. - live.$lastSyncedAt - .dropFirst() - .compactMap { $0 } - .removeDuplicates() - .debounce(for: .seconds(2), scheduler: DispatchQueue.main) - .sink { [weak self] _ in - // W07-016: the launch seed of the persisted time is an emission too. Read the origin here, - // after the debounce: if a HISTORY_COMPLETE landed inside it, a sync did complete. - guard let self else { return } - let origin = LastSyncOrigin.observed(self.live.lastSyncedAtOrigin) + Self.lastSyncRefreshes(live, debounce: .seconds(2)) + .sink { [weak self] origin in Task { [weak self] in await self?.refreshAfterCompletedBackfill(origin: origin) } } .store(in: &hrCancellables) @@ -695,6 +687,21 @@ final class AppModel: ObservableObject { #endif } + /// The dashboard refreshes `lastSyncedAt` drives, one per settled change, each with the origin of the + /// value it settled on. The chain the app runs, extracted so the tests drive it rather than a copy + /// (W07-047). W07-016: the seed of the stored time is an emission too, and the origin is read after the + /// debounce, so a HISTORY_COMPLETE landing inside it makes the refresh a completed sync's. + static func lastSyncRefreshes(_ live: LiveState, + debounce: DispatchQueue.SchedulerTimeType.Stride) -> AnyPublisher { + live.$lastSyncedAt + .dropFirst() + .compactMap { $0 } + .removeDuplicates() + .debounce(for: debounce, scheduler: DispatchQueue.main) + .map { [weak live] _ in LastSyncOrigin.observed(live?.lastSyncedAtOrigin) } + .eraseToAnyPublisher() + } + private func refreshAfterCompletedBackfill(origin: LastSyncOrigin) async { live.append(log: origin.refreshLogLine) await repo.refresh(days: 120) diff --git a/StrandTests/LastSyncAttributionTests.swift b/StrandTests/LastSyncAttributionTests.swift index 8c5c40da34..68ae441de8 100644 --- a/StrandTests/LastSyncAttributionTests.swift +++ b/StrandTests/LastSyncAttributionTests.swift @@ -136,6 +136,29 @@ final class LastSyncAttributionTests: XCTestCase { "a retried bootstrap seeds after launch (W07-046)") } + /// W07-047: the app's own chain, not a bare subscriber. A seed that a HISTORY_COMPLETE follows inside the + /// debounce settles once, as a completed sync; a seed alone settles as the stored seed. + @MainActor + func testTheAppsRefreshChainSettlesOnTheLastWritersOrigin() { + let live = LiveState() + var seen: [LastSyncOrigin] = [] + let done = expectation(description: "settled") + let c = AppModel.lastSyncRefreshes(live, debounce: .milliseconds(50)).sink { seen.append($0); done.fulfill() } + live.seedLastSynced(1_790_000_000) + live.stampCompletedSync(at: 1_790_000_900) + wait(for: [done], timeout: 2) + XCTAssertEqual(seen, [.completedSync]) + + let live2 = LiveState() + var seen2: [LastSyncOrigin] = [] + let done2 = expectation(description: "settled alone") + let c2 = AppModel.lastSyncRefreshes(live2, debounce: .milliseconds(50)).sink { seen2.append($0); done2.fulfill() } + live2.seedLastSynced(1_790_000_000) + wait(for: [done2], timeout: 2) + XCTAssertEqual(seen2, [.persisted]) + c.cancel(); c2.cancel() + } + /// W07-044: a value with no recorded writer is reported as unattributed, never as a completed sync. func testAnUnrecordedOriginIsNotLoggedAsACompletedSync() { XCTAssertEqual(LastSyncOrigin.observed(nil), .unattributed) diff --git a/docs/review/workstreams/W07-app-data.md b/docs/review/workstreams/W07-app-data.md index 5501d3bb07..1dd0af480a 100644 --- a/docs/review/workstreams/W07-app-data.md +++ b/docs/review/workstreams/W07-app-data.md @@ -106,6 +106,7 @@ the two biggest hot spots). | W07-045 | S4 | Fixed | W07-016 documented that `lastSyncedAt` is written only by `seedLastSynced` and `stampCompletedSync`, each setting its origin first, but the property kept a public setter, so any direct write would leave the origin stale and the refresh would be logged under the previous writer's origin | `LiveState.lastSyncedAt` | Found by the 2026-10-03 `/code-review xhigh`. V0 by reading: no app code writes it directly; `SyncChipStateTests` did, at five sites. Fix: `public private(set)`; the tests stamp through `stampCompletedSync`. V1: the app compiles, `SyncChipStateTests` and `LastSyncAttributionTests` pass | | | W07-044 | S4 | Fixed | W07-016's sink took an unrecorded origin as `.completedSync`, so a value whose writer it could not name would be logged as a completed sync with `trigger=post-offload`, the defect W07-016 removed, through the fallback (AGENTS.md: a diagnostic may only assert what it can attribute) | `AppModel` `lastSyncedAt` sink | Found by the 2026-10-03 `/code-review xhigh`. V0 by reading (`?? .completedSync`). Fix: `LastSyncOrigin.unattributed` with its own neutral line and `trigger=last-sync-unattributed`; the sink reports `LastSyncOrigin.observed(_:)`. Unreachable while W07-045's private setter holds. V1: `testAnUnrecordedOriginIsNotLoggedAsACompletedSync` | | | W07-046 | S4 | Fixed | W07-016's seeded refresh was logged `Launch: …` with `trigger=launch-seed`, but the seed runs from `bootstrapStore`, which `beginBackfill` retries on a later tick when the first open fails (a relaunch while the phone is locked), so the line could appear after `Connected`, long after launch, asserting a launch it did not see | `LastSyncOrigin.persisted` line and trigger | Found by the 2026-10-03 `/code-review xhigh`. V0 by reading (`BLEManager` seeds inside `bootstrapStore`, retried from `beginBackfill`). Fix: `Seed: refreshing dashboard cache from the stored last-sync time (no sync completed this session yet)` and `trigger=stored-seed`. W07-016's strap check now reads: the seed line, and `post-offload` only after a HISTORY_COMPLETE. V1: `LastSyncAttributionTests` | | +| W07-047 | S4 | Fixed | W07-016's subscriber test watched a bare `$lastSyncedAt.sink`, not the app's chain (drop first, de-duplicate, 2-second debounce, origin read after it), so a change that broke the chain, a seed coalesced with a HISTORY_COMPLETE labelled wrongly, would pass (AGENTS.md: a gate must be able to fail on the change that caused it) | `AppModel` `lastSyncedAt` sink; `LastSyncAttributionTests` | Found by the 2026-10-03 `/code-review xhigh`. Fix: the chain is `AppModel.lastSyncRefreshes(_:debounce:)`, which the app sinks and `testTheAppsRefreshChainSettlesOnTheLastWritersOrigin` drives (a seed followed by a HISTORY_COMPLETE settles once as a completed sync; a seed alone as the stored seed). V1: passes; with the debounce removed it fails. Not catchable by this test: reading the origin before the debounce, since the last value's origin is the same either way | | ## Log @@ -140,3 +141,4 @@ the two biggest hot spots). - 2026-10-03 — W07-045 fixed. - 2026-10-03 — W07-044 fixed. - 2026-10-03 — W07-046 fixed (W07-016's seed line no longer claims a launch). +- 2026-10-03 — W07-047 fixed. All 13 findings of the code review worked: W07-035 … W07-038, W07-040 … W07-047 fixed, W07-039 not a bug.