From 8c8f22850efa9e540687538453a6888402f7e769 Mon Sep 17 00:00:00 2001 From: rzen Date: Thu, 9 Jul 2026 08:35:16 -0400 Subject: [PATCH] Merge watch workout pushes per log instead of whole-document MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ingestFromWatch arbitrated by whole-document updatedAt, so concurrent edits to the same workout on both devices (phone edits exercise A while the watch completes exercise B) lost one side wholesale — the newer snapshot replaced the other (H1). Reconcile per log instead. WorkoutMergePlanner (pure, deterministic) unions logs by id, resolves each by newest per-log updatedAt, and applies phone-authored deletion tombstones so an absent log is never ambiguous between "deleted on the phone" and "just added on the watch". Edits to different exercises now commute — delivery order and offline gaps stop mattering. A stale/duplicate push merges back to exactly the cached doc, so ingest re-pushes authoritative state rather than writing. The per-log updatedAt scaffolding shipped (unused) in schema v4; it's now stamped by transition(to:) on status flips and a new touch() at the content-only edit sites (order, notes, machine settings, adjusted entries, new logs) on both phone and watch. deletedLogIDs is new: additive on the wire and cache, phone-authored (deleteLog), pruned after a 30-day grace. Because an older build rewriting a file would strip the tombstones and resurrect a deleted exercise, WorkoutDocument schema bumps 4->5 (forward gate quarantines old builds) and the cache bumps 5->6. recomputeStatusFromLogs takes an injectable now: so the merge recomputes status/end deterministically. WorkoutMergePlannerTests pins the decision table (commute, per-log newer-wins, legacy-nil, watch-add, tombstone honored/resurrect/union/prune, status recompute, no-op re-push); WorkoutDocumentMapperTests gains the deletedLogIDs round-trip. --- CHANGELOG.md | 2 + SCHEMA.md | 7 +- Shared/Model/Documents.swift | 31 ++- Shared/Model/Entities.swift | 5 + Shared/Model/Mappers.swift | 4 +- .../Persistence/WorkoutsModelContainer.swift | 3 +- .../Views/WorkoutLogListView.swift | 3 +- Workouts/Sync/SyncEngine.swift | 40 ++-- Workouts/Sync/WorkoutMergePlanner.swift | 81 ++++++++ .../WorkoutLogs/ExerciseProgressView.swift | 1 + Workouts/Views/WorkoutLogs/ExerciseView.swift | 1 + .../Views/WorkoutLogs/NotesEditView.swift | 1 + Workouts/Views/WorkoutLogs/PlanEditView.swift | 1 + .../WorkoutLogs/WorkoutLogListView.swift | 10 +- .../WorkoutDocumentMapperTests.swift | 4 +- WorkoutsTests/WorkoutMergePlannerTests.swift | 193 ++++++++++++++++++ 16 files changed, 350 insertions(+), 37 deletions(-) create mode 100644 Workouts/Sync/WorkoutMergePlanner.swift create mode 100644 WorkoutsTests/WorkoutMergePlannerTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index ee50fa6..51a68ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ **July 2026** +Editing the same workout on your iPhone and Apple Watch at the same time no longer loses one device's changes. + The watch workout view now closes reliably when you end a workout on your iPhone, instead of the app reappearing on every wrist raise. Watch and iPhone set timers no longer drift apart when the watch sleeps through the end of a rest. diff --git a/SCHEMA.md b/SCHEMA.md index a387903..d1ebec6 100644 --- a/SCHEMA.md +++ b/SCHEMA.md @@ -54,11 +54,11 @@ SwiftData cache entity (`Shared/Model/Entities.swift`) kept in sync by | `durationSeconds` | Int | | Total seconds; 0 when not a timed exercise | | `machineSettings` | [MachineSetting] | ✓ | Ordered machine comfort settings; nil → not a machine exercise, empty → machine exercise with nothing recorded. Doubles as the machine-based flag | -## WorkoutDocument — `currentSchemaVersion: 4` +## WorkoutDocument — `currentSchemaVersion: 5` | Property | Type | Optional | Notes | |---|---|---|---| -| `schemaVersion` | Int | | Bumped 1→2 when `metrics` was added (captured HR data is irreplaceable, so older apps must quarantine, not strip). Bumped 2→3 when the derived `completed` flag was removed from `WorkoutLogDocument` and `machineSettings` was added — dropping a required field means older apps can't decode new files, so they must quarantine. Bumped 3→4 when `WorkoutLogDocument.weight` went Int → Double and per-set actuals (`setEntries`) were added — a fractional weight fails an older decode, and a rewrite would strip the irreplaceable actuals | +| `schemaVersion` | Int | | Bumped 1→2 when `metrics` was added (captured HR data is irreplaceable, so older apps must quarantine, not strip). Bumped 2→3 when the derived `completed` flag was removed from `WorkoutLogDocument` and `machineSettings` was added — dropping a required field means older apps can't decode new files, so they must quarantine. Bumped 3→4 when `WorkoutLogDocument.weight` went Int → Double and per-set actuals (`setEntries`) were added — a fractional weight fails an older decode, and a rewrite would strip the irreplaceable actuals. Bumped 4→5 when per-log `updatedAt` began being written and `deletedLogIDs` was added for the per-log merge — a rewrite by an older app would strip the deletion tombstones and resurrect a deleted exercise, so older apps must quarantine | | `id` | String | | ULID (chronological — drives month bucketing and sorting) | | `splitID` | String | ✓ | Denormalized reference; no live relationship | | `splitName` | String | ✓ | Denormalized snapshot of the split's name | @@ -69,6 +69,7 @@ SwiftData cache entity (`Shared/Model/Entities.swift`) kept in sync by | `updatedAt` | Date | | | | `logs` | [WorkoutLogDocument] | | Embedded aggregate members | | `metrics` | WorkoutMetrics | ✓ | Nil until the workout completes and a writer fills it in | +| `deletedLogIDs` | [String: Date] | ✓ | Per-log deletion tombstones (`logID → when deleted`), phone-authored. Disambiguate an absent log during the per-log merge so a stale watch push can't resurrect a removed exercise; pruned after a 30-day grace. Added in schema v5 (`WorkoutMergePlanner`) | ## WorkoutLogDocument (embedded in WorkoutDocument) @@ -90,7 +91,7 @@ SwiftData cache entity (`Shared/Model/Entities.swift`) kept in sync by | `startedAt` | Date | ✓ | First move to `.inProgress`. *Not schema-bumped* | | `completedAt` | Date | ✓ | Last move to `.completed`. *Not schema-bumped* | | `setEntries` | [SetEntry] | ✓ | Per-set actuals in set order; nil → legacy file / nothing recorded. Appended as sets complete (pre-filled from the plan); `transition(to:)` fills the missing tail on `.completed` and clears on `.notStarted`; `.skipped` keeps partials. Added in schema v4 | -| `updatedAt` | Date | ✓ | Reserved for the future per-log merge (H1); nothing writes it yet. Added in schema v4 | +| `updatedAt` | Date | ✓ | Per-log modification time driving the per-log merge (`WorkoutMergePlanner`); stamped by `transition(to:)` on status flips and `touch()` at content-only edit sites. Nil in files written before v5. Added (unused) in schema v4, written since v5 | ## MachineSetting (embedded in ExerciseDocument and WorkoutLogDocument) diff --git a/Shared/Model/Documents.swift b/Shared/Model/Documents.swift index 3a4ad25..6715055 100644 --- a/Shared/Model/Documents.swift +++ b/Shared/Model/Documents.swift @@ -95,6 +95,14 @@ struct WorkoutDocument: Codable, Sendable, Equatable, Identifiable { /// watch and the phone from both writing the same workout to Health. var metrics: WorkoutMetrics? + /// Per-log deletion tombstones — `logID → when deleted`. Authored only by the phone + /// (`WorkoutLogListView.deleteLog`, the sole log-removal site). They disambiguate an + /// absent log during the per-log merge: without them, a log the watch still carries + /// (having missed a phone-side delete) is indistinguishable from one the watch just + /// added. `nil`/absent in files written before the merge shipped; pruned after a grace + /// period. See `WorkoutMergePlanner`. + var deletedLogIDs: [String: Date]? = nil + // Bumped 1→2 when `metrics` was added: the captured HR/calorie data is // irreplaceable, so the forward-gate must quarantine these files on older apps // rather than let them strip `metrics` on rewrite. @@ -107,7 +115,11 @@ struct WorkoutDocument: Codable, Sendable, Equatable, Identifiable { // into `Int` fails, and one rewriting the file would strip `setEntries` — // the recorded actuals are irreplaceable (same rationale as the v2 `metrics` // bump), so older apps must quarantine. - static let currentSchemaVersion = 4 + // Bumped 4→5 when per-log `updatedAt` began being written and `deletedLogIDs` + // was added for the per-log merge: an older app rewriting the file would strip + // the deletion tombstones and resurrect a deleted exercise (irreplaceable + // intent, same rationale as v2/v4), so older apps must quarantine. + static let currentSchemaVersion = 5 var relativePath: String { Self.relativePath(id: id, start: start) } @@ -127,7 +139,7 @@ extension WorkoutDocument { /// source of the status-from-logs rule — every screen that mutates logs calls it, /// so an ended workout (remaining exercises skipped) stays finished no matter which /// screen the next edit comes from. - mutating func recomputeStatusFromLogs() { + mutating func recomputeStatusFromLogs(now: Date = Date()) { let statuses: [WorkoutStatus] = logs.map { WorkoutStatus(rawValue: $0.status) ?? .notStarted } let isResolved: (WorkoutStatus) -> Bool = { $0 == .completed || $0 == .skipped } @@ -138,10 +150,11 @@ extension WorkoutDocument { // Only stamp the finish time on the *transition* into completed. A workout // that was already completed (with an `end`) keeps that original end, so // editing a finished workout — which re-runs this recompute with all logs - // still resolved — doesn't jump its end forward to now. + // still resolved — doesn't jump its end forward to now. `now` is injectable + // so the pure merge planner can recompute deterministically. let wasCompletedWithEnd = status == WorkoutStatus.completed.rawValue && end != nil status = WorkoutStatus.completed.rawValue - if !wasCompletedWithEnd { end = Date() } + if !wasCompletedWithEnd { end = now } } else if anyStarted { status = WorkoutStatus.inProgress.rawValue end = nil @@ -220,6 +233,12 @@ extension WorkoutLogDocument { ) } + /// Stamp the per-log modification time for a content-only edit — one that changes a + /// log's fields (order, notes, machine settings, an adjusted entry) without flipping its + /// status. Status flips stamp `updatedAt` inside `transition(to:)`; this is the hook the + /// other edit sites call so the merge can order every kind of per-log change. + mutating func touch(_ now: Date = Date()) { updatedAt = now } + /// Move to a new status, keeping the started/completed timestamps consistent. /// Every screen that flips a log's status (phone and watch) goes through here, /// so the timestamps can't drift per-call-site. Completion is derived from @@ -227,6 +246,10 @@ extension WorkoutLogDocument { /// there's no separate flag to keep in sync. mutating func transition(to newStatus: WorkoutStatus) { status = newStatus.rawValue + // Stamp the per-log modification time on every status flip — the merge orders + // concurrent edits by it. Content-only edits (order, notes, machine settings, + // adjusted entries) stamp `updatedAt` at their own call sites. + updatedAt = Date() switch newStatus { case .notStarted: // A full reset — the run never happened, so recorded actuals go too. diff --git a/Shared/Model/Entities.swift b/Shared/Model/Entities.swift index 982d729..3684838 100644 --- a/Shared/Model/Entities.swift +++ b/Shared/Model/Entities.swift @@ -144,6 +144,11 @@ final class Workout { var metricSourceRaw: String? var metricRecordedAt: Date? + /// Mirrors the document's per-log deletion tombstones (`logID → when deleted`), so the + /// phone re-derives them into any `WorkoutDocument` it rebuilds and the merge stays + /// deletion-safe across cache round-trips. Phone-authored; see `WorkoutMergePlanner`. + var deletedLogIDs: [String: Date]? + @Relationship(deleteRule: .cascade, inverse: \WorkoutLog.workout) var logs: [WorkoutLog] = [] diff --git a/Shared/Model/Mappers.swift b/Shared/Model/Mappers.swift index c7af71b..3376bb8 100644 --- a/Shared/Model/Mappers.swift +++ b/Shared/Model/Mappers.swift @@ -49,7 +49,8 @@ extension WorkoutDocument { splitName: workout.splitName, start: workout.start, end: workout.end, status: workout.statusRaw, createdAt: workout.createdAt, updatedAt: workout.updatedAt, logs: workout.logsArray.map(WorkoutLogDocument.init(from:)), - metrics: workout.metrics) + metrics: workout.metrics, + deletedLogIDs: workout.deletedLogIDs) } } @@ -145,6 +146,7 @@ enum CacheMapper { workout.updatedAt = doc.updatedAt workout.jsonRelativePath = relativePath workout.metrics = doc.metrics + workout.deletedLogIDs = doc.deletedLogIDs let existing = Dictionary(workout.logs.map { ($0.id, $0) }, uniquingKeysWith: { a, _ in a }) var keep = Set() diff --git a/Shared/Persistence/WorkoutsModelContainer.swift b/Shared/Persistence/WorkoutsModelContainer.swift index b82a6a4..b677d5b 100644 --- a/Shared/Persistence/WorkoutsModelContainer.swift +++ b/Shared/Persistence/WorkoutsModelContainer.swift @@ -12,7 +12,8 @@ enum WorkoutsModelContainer { /// the `completed` column from `WorkoutLog`; added `machineSettings` to both. /// 5: `weight` went Int → Double on `Exercise` and `WorkoutLog`; added /// `setEntries` and `logUpdatedAt` to `WorkoutLog`. - static let currentSchemaVersion = 5 + /// 6: added `deletedLogIDs` to `Workout` (per-log merge deletion tombstones). + static let currentSchemaVersion = 6 private static let schemaVersionKey = "Workouts.persistenceSchemaVersion" private static let identityTokenKey = "Workouts.iCloudIdentityToken" diff --git a/Workouts Watch App/Views/WorkoutLogListView.swift b/Workouts Watch App/Views/WorkoutLogListView.swift index 7266963..b06636e 100644 --- a/Workouts Watch App/Views/WorkoutLogListView.swift +++ b/Workouts Watch App/Views/WorkoutLogListView.swift @@ -147,7 +147,7 @@ struct WorkoutLogListView: View { } private func addExercise(_ exercise: Exercise) { - let newLog = WorkoutLogDocument( + var newLog = WorkoutLogDocument( id: ULID.make(), exerciseName: exercise.name, order: doc.logs.count, @@ -161,6 +161,7 @@ struct WorkoutLogListView: View { notes: nil, date: doc.start ) + newLog.touch() // a fresh log carries a definite creation time for the merge doc.logs.append(newLog) doc.updatedAt = Date() bridge.update(workout: doc) diff --git a/Workouts/Sync/SyncEngine.swift b/Workouts/Sync/SyncEngine.swift index d2d3e5b..9752126 100644 --- a/Workouts/Sync/SyncEngine.swift +++ b/Workouts/Sync/SyncEngine.swift @@ -307,35 +307,27 @@ final class SyncEngine { onCacheChanged?() return } - // `updatedAt` intake gate: the watch mixes `sendMessage` with a queued - // `transferUserInfo` fallback, which are unordered — a failed-then-queued - // older edit can arrive after a newer one. Accept only what's strictly - // newer than the cache; the cache mirrors every accepted write - // immediately, so it's always at least as new as any pending slot. - // Strictly older means the watch is behind — re-push authoritative state - // so it corrects. Equal is the duplicate/echo case — ignore silently. - // - // KNOWN LIMIT (deliberate, revisit if users report vanished sets): this - // gate arbitrates *timestamps*, not *content*. A doc edited on a stale - // watch snapshot carries a fresh stamp and replaces the whole workout — - // concurrent edits inside one push round-trip (or a disconnection) lose - // one side wholesale. The watch-side absorb (WorkoutLogListView's - // onChange re-seed) narrows the stale window to one push latency; the - // durable fix is per-log merge: give WorkoutLogDocument its own optional - // `updatedAt` (same additive pattern as startedAt/completedAt), merge - // incoming docs log-by-log (newer log wins per id) instead of replacing, - // and carry log add/remove as explicit intents so an absent log is never - // ambiguous between "deleted" and "not seen yet". That makes edits to - // different logs commute, so delivery order and offline gaps stop - // mattering entirely. + // Per-log merge (H1's durable fix). The watch mixes `sendMessage` with a queued + // `transferUserInfo` fallback, which are unordered, and either device can edit the + // same workout at once. Rather than arbitrate by whole-document `updatedAt` — which + // loses one side's edit wholesale — reconcile the incoming doc against the cache + // log-by-log (`WorkoutMergePlanner`): newer per-log stamp wins, deletion tombstones + // resolve absent logs, so edits to different exercises commute regardless of delivery + // order. When the merge changes nothing (a stale or duplicate push), re-push + // authoritative state so the watch corrects; otherwise persist the merged document + // (which echoes back to the watch via `onCacheChanged`). if let cached = CacheMapper.fetchWorkout(id: doc.id, in: context) { - if doc.updatedAt < cached.updatedAt { + let cachedDoc = WorkoutDocument(from: cached) + let merged = WorkoutMergePlanner.merge(incoming: doc, cached: cachedDoc) + guard merged != cachedDoc else { onCacheChanged?() return } - if doc.updatedAt == cached.updatedAt { return } + await save(workout: merged) + } else { + // First time we've seen this workout — nothing to merge against. + await save(workout: doc) } - await save(workout: doc) } // MARK: - Public CRUD (mirror-first: cache now, file via the write queue) diff --git a/Workouts/Sync/WorkoutMergePlanner.swift b/Workouts/Sync/WorkoutMergePlanner.swift new file mode 100644 index 0000000..4042c1b --- /dev/null +++ b/Workouts/Sync/WorkoutMergePlanner.swift @@ -0,0 +1,81 @@ +// +// WorkoutMergePlanner.swift +// Workouts +// +// Copyright 2025 Rouslan Zenetl. All Rights Reserved. +// + +import Foundation + +/// Pure, deterministic per-log merge of a watch-pushed `WorkoutDocument` against the phone's +/// cached copy — the durable fix for H1, where `ingestFromWatch` arbitrated by *whole-document* +/// `updatedAt` and so lost one side's edit whenever the two devices changed the same workout at +/// once. Merging per log makes edits to *different* exercises commute: neither side is clobbered +/// just because its overall stamp is newer, so delivery order and offline gaps stop mattering. +/// +/// Log existence is reconciled by id against explicit deletion tombstones (`deletedLogIDs`, +/// phone-authored — the watch never deletes a log), so an absent log is never ambiguous between +/// "deleted on the phone" and "just added on the watch". Runs phone-side only (in +/// `SyncEngine.ingestFromWatch`); the merged document then flows through the normal +/// `save(workout:)` path and is echoed authoritatively back to the watch. +enum WorkoutMergePlanner { + /// Deletion tombstones older than this are pruned. A device offline longer than the grace + /// may resurrect a since-deleted log — the same bounded risk as the whole-workout stub grace. + static let tombstoneGrace: TimeInterval = 30 * 24 * 60 * 60 + + /// The instant a log was last changed, for ordering concurrent edits: its own `updatedAt` + /// when present, else the best lifecycle timestamp, else `.distantPast` — so a legacy log + /// carrying no stamp always loses to a stamped edit and to any tombstone. + static func modTime(_ log: WorkoutLogDocument) -> Date { + log.updatedAt ?? log.completedAt ?? log.startedAt ?? .distantPast + } + + /// Merge `incoming` (from the watch) into `cached` (the phone's current copy). Log content + /// is resolved per id (newer `modTime` wins); deletion tombstones from both sides union and + /// drop a log only when the deletion is at-or-after the log's own edit — a strictly-newer + /// edit resurrects the log and clears its stale tombstone. Workout-level scalars come from + /// whichever document is newer overall; status and `end` are recomputed from the merged logs. + static func merge(incoming: WorkoutDocument, cached: WorkoutDocument, now: Date = Date()) -> WorkoutDocument { + // 1. Union deletion tombstones — newest stamp per id wins. + var tombstones = cached.deletedLogIDs ?? [:] + for (id, when) in incoming.deletedLogIDs ?? [:] { + tombstones[id] = tombstones[id].map { max($0, when) } ?? when + } + + // 2. Per-id log union — newer modTime wins; equal stamps keep the cached (phone) copy. + var logsByID: [String: WorkoutLogDocument] = [:] + for log in cached.logs { logsByID[log.id] = log } + for log in incoming.logs { + guard let have = logsByID[log.id] else { logsByID[log.id] = log; continue } + if modTime(log) > modTime(have) { logsByID[log.id] = log } + } + + // 3. Apply tombstones: a deletion at-or-after the log's own edit drops it; a strictly + // newer edit resurrects the log and clears the now-stale tombstone. + for (id, deletedAt) in tombstones { + guard let log = logsByID[id] else { continue } + if modTime(log) > deletedAt { + tombstones.removeValue(forKey: id) + } else { + logsByID.removeValue(forKey: id) + } + } + + // 4. Prune tombstones past the grace period. + tombstones = tombstones.filter { now.timeIntervalSince($0.value) < tombstoneGrace } + + // 5. Base scalars (splitID/name, start, metrics, …) on the newer document overall — it + // carries a just-attached metrics summary or a rename — then override the reconciled + // parts. Ties prefer the cached copy (the phone is the sole writer). Sort is + // id-tie-broken so the result is deterministic even when two logs share an `order`. + var merged = incoming.updatedAt > cached.updatedAt ? incoming : cached + merged.logs = logsByID.values.sorted { + $0.order != $1.order ? $0.order < $1.order : $0.id < $1.id + } + merged.deletedLogIDs = tombstones.isEmpty ? nil : tombstones + merged.recomputeStatusFromLogs(now: now) + merged.updatedAt = max(incoming.updatedAt, cached.updatedAt) + merged.schemaVersion = WorkoutDocument.currentSchemaVersion + return merged + } +} diff --git a/Workouts/Views/WorkoutLogs/ExerciseProgressView.swift b/Workouts/Views/WorkoutLogs/ExerciseProgressView.swift index abe259b..772ddac 100644 --- a/Workouts/Views/WorkoutLogs/ExerciseProgressView.swift +++ b/Workouts/Views/WorkoutLogs/ExerciseProgressView.swift @@ -605,6 +605,7 @@ struct ExerciseProgressView: View { guard let i = doc.logs.firstIndex(where: { $0.id == logID }), let last = doc.logs[i].setEntries?.indices.last else { return } doc.logs[i].setEntries?[last] = entry + doc.logs[i].touch() doc.updatedAt = Date() onChange() } diff --git a/Workouts/Views/WorkoutLogs/ExerciseView.swift b/Workouts/Views/WorkoutLogs/ExerciseView.swift index 7ee17a6..e1e8417 100644 --- a/Workouts/Views/WorkoutLogs/ExerciseView.swift +++ b/Workouts/Views/WorkoutLogs/ExerciseView.swift @@ -191,6 +191,7 @@ struct ExerciseView: View { private func applyMachineSettings(_ settings: [MachineSetting]) { guard let i = doc.logs.firstIndex(where: { $0.id == logID }) else { return } doc.logs[i].machineSettings = settings + doc.logs[i].touch() doc.updatedAt = Date() let snapshot = doc let exerciseName = doc.logs[i].exerciseName diff --git a/Workouts/Views/WorkoutLogs/NotesEditView.swift b/Workouts/Views/WorkoutLogs/NotesEditView.swift index 770ec40..01b36ed 100644 --- a/Workouts/Views/WorkoutLogs/NotesEditView.swift +++ b/Workouts/Views/WorkoutLogs/NotesEditView.swift @@ -51,6 +51,7 @@ struct NotesEditView: View { var doc = WorkoutDocument(from: workout) guard let i = doc.logs.firstIndex(where: { $0.id == logID }) else { return } doc.logs[i].notes = notesText + doc.logs[i].touch() doc.updatedAt = Date() let snapshot = doc Task { await sync.save(workout: snapshot) } diff --git a/Workouts/Views/WorkoutLogs/PlanEditView.swift b/Workouts/Views/WorkoutLogs/PlanEditView.swift index eb6c203..a63995b 100644 --- a/Workouts/Views/WorkoutLogs/PlanEditView.swift +++ b/Workouts/Views/WorkoutLogs/PlanEditView.swift @@ -158,6 +158,7 @@ struct PlanEditView: View { doc.logs[i].weight = Double(weight) doc.logs[i].durationSeconds = totalSeconds doc.logs[i].loadType = selectedLoadType.rawValue + doc.logs[i].touch() doc.updatedAt = Date() let exerciseName = doc.logs[i].exerciseName let workoutDoc = doc diff --git a/Workouts/Views/WorkoutLogs/WorkoutLogListView.swift b/Workouts/Views/WorkoutLogs/WorkoutLogListView.swift index 490b532..bee6fa9 100644 --- a/Workouts/Views/WorkoutLogs/WorkoutLogListView.swift +++ b/Workouts/Views/WorkoutLogs/WorkoutLogListView.swift @@ -294,6 +294,9 @@ struct WorkoutLogListView: View { private func deleteLog(_ log: WorkoutLogDocument) { withAnimation { doc.logs.removeAll { $0.id == log.id } + // Tombstone the deletion so a stale watch push still carrying this log can't + // resurrect it — the per-log merge drops a tombstoned id (see WorkoutMergePlanner). + doc.deletedLogIDs = (doc.deletedLogIDs ?? [:]).merging([log.id: Date()]) { _, new in new } save() } } @@ -302,8 +305,9 @@ struct WorkoutLogListView: View { var logs = sortedLogs logs.move(fromOffsets: source, toOffset: destination) for (index, log) in logs.enumerated() { - if let i = doc.logs.firstIndex(where: { $0.id == log.id }) { + if let i = doc.logs.firstIndex(where: { $0.id == log.id }), doc.logs[i].order != index { doc.logs[i].order = index + doc.logs[i].touch() } } save() @@ -333,7 +337,8 @@ struct WorkoutLogListView: View { } doc.end = nil - let newLog = WorkoutLogDocument(planFrom: plan, order: doc.logs.count, date: now) + var newLog = WorkoutLogDocument(planFrom: plan, order: doc.logs.count, date: now) + newLog.touch(now) // a fresh log carries a definite creation time for the merge doc.logs.append(newLog) save() @@ -389,6 +394,7 @@ struct WorkoutLogListView: View { private func applyMachineSettings(_ settings: [MachineSetting], toLogID id: String) { guard let i = doc.logs.firstIndex(where: { $0.id == id }) else { return } doc.logs[i].machineSettings = settings + doc.logs[i].touch() doc.recomputeStatusFromLogs() doc.updatedAt = Date() diff --git a/WorkoutsTests/WorkoutDocumentMapperTests.swift b/WorkoutsTests/WorkoutDocumentMapperTests.swift index 86d6011..2576c08 100644 --- a/WorkoutsTests/WorkoutDocumentMapperTests.swift +++ b/WorkoutsTests/WorkoutDocumentMapperTests.swift @@ -85,7 +85,8 @@ struct WorkoutDocumentMapperTests { MachineSetting(name: "Back Rest", value: "45°")], startedAt: Self.startedAt), ], - metrics: fullMetrics() + metrics: fullMetrics(), + deletedLogIDs: ["LOG-DELETED": Self.completedAt] ) CacheMapper.upsertWorkout(original, relativePath: original.relativePath, into: context) @@ -120,6 +121,7 @@ struct WorkoutDocumentMapperTests { #expect(rebuilt.metrics == fullMetrics()) #expect(rebuilt.metrics?.hrZoneSeconds == [10, 20, 30, 40, 50]) #expect(rebuilt.metrics?.healthKitWorkoutUUID == "HK-UUID-123") + #expect(rebuilt.deletedLogIDs == ["LOG-DELETED": Self.completedAt]) // merge tombstones survive } /// A workout with no metrics yet (the common in-progress / just-started case) must diff --git a/WorkoutsTests/WorkoutMergePlannerTests.swift b/WorkoutsTests/WorkoutMergePlannerTests.swift new file mode 100644 index 0000000..5a84aab --- /dev/null +++ b/WorkoutsTests/WorkoutMergePlannerTests.swift @@ -0,0 +1,193 @@ +import Foundation +import Testing +@testable import Workouts + +/// Pins the per-log merge (`WorkoutMergePlanner.merge`) that replaced `ingestFromWatch`'s +/// whole-document `updatedAt` gate — the durable fix for H1. The decisive properties: edits to +/// *different* logs commute (neither side is clobbered), the same log resolves by newest per-log +/// stamp, a phone-authored deletion tombstone stops a stale watch push from resurrecting a +/// removed exercise (unless a strictly-newer edit supersedes the deletion), and a stale/duplicate +/// push merges back to exactly the cached document (so ingest re-pushes rather than writes). +struct WorkoutMergePlannerTests { + private static let t0 = Date(timeIntervalSince1970: 1_700_000_000) + private static func at(_ seconds: TimeInterval) -> Date { t0.addingTimeInterval(seconds) } + + private func log(_ id: String, order: Int = 0, status: WorkoutStatus = .inProgress, + reps: Int = 10, updatedAt: Date? = nil, + startedAt: Date? = nil, completedAt: Date? = nil) -> WorkoutLogDocument { + WorkoutLogDocument( + id: id, exerciseName: "Ex-\(id)", order: order, sets: 3, reps: reps, weight: 100, + loadType: LoadType.weight.rawValue, durationSeconds: 0, currentStateIndex: 0, + status: status.rawValue, notes: nil, date: Self.t0, machineSettings: nil, + startedAt: startedAt, completedAt: completedAt, setEntries: nil, updatedAt: updatedAt) + } + + private func workout(updatedAt: Date, status: WorkoutStatus = .inProgress, + logs: [WorkoutLogDocument], deletedLogIDs: [String: Date]? = nil, + metrics: WorkoutMetrics? = nil, end: Date? = nil) -> WorkoutDocument { + WorkoutDocument( + schemaVersion: WorkoutDocument.currentSchemaVersion, id: "W", splitID: nil, + splitName: nil, start: Self.t0, end: end, status: status.rawValue, + createdAt: Self.t0, updatedAt: updatedAt, logs: logs, metrics: metrics, + deletedLogIDs: deletedLogIDs) + } + + private func merged(_ incoming: WorkoutDocument, _ cached: WorkoutDocument) -> WorkoutDocument { + WorkoutMergePlanner.merge(incoming: incoming, cached: cached, now: Self.at(1000)) + } + + private func reps(_ doc: WorkoutDocument, _ id: String) -> Int? { + doc.logs.first { $0.id == id }?.reps + } + + // MARK: - Commuting edits to different logs (the H1 case) + + @Test func concurrentEditsToDifferentLogsBothSurvive() { + // Phone advanced A; watch advanced B — each from the shared base, unaware of the other. + let phone = workout(updatedAt: Self.at(10), + logs: [log("A", order: 0, reps: 12, updatedAt: Self.at(10)), + log("B", order: 1, reps: 10, updatedAt: Self.at(5))]) + let watch = workout(updatedAt: Self.at(11), + logs: [log("A", order: 0, reps: 10, updatedAt: Self.at(5)), + log("B", order: 1, reps: 15, updatedAt: Self.at(11))]) + + let result = merged(watch, phone) + #expect(reps(result, "A") == 12) // phone's A edit kept + #expect(reps(result, "B") == 15) // watch's B edit kept + #expect(result.logs.count == 2) + } + + @Test func mergeIsOrderIndependentForLogContent() { + let phone = workout(updatedAt: Self.at(10), + logs: [log("A", order: 0, reps: 12, updatedAt: Self.at(10)), + log("B", order: 1, reps: 10, updatedAt: Self.at(5))]) + let watch = workout(updatedAt: Self.at(11), + logs: [log("A", order: 0, reps: 10, updatedAt: Self.at(5)), + log("B", order: 1, reps: 15, updatedAt: Self.at(11))]) + + let a = merged(watch, phone) + let b = merged(phone, watch) + // The winning log versions are the same regardless of which document is "incoming". + #expect(reps(a, "A") == reps(b, "A")) + #expect(reps(a, "B") == reps(b, "B")) + } + + // MARK: - Same log: newest per-log stamp wins + + @Test func sameLogNewerStampWins() { + let cached = workout(updatedAt: Self.at(10), logs: [log("A", reps: 12, updatedAt: Self.at(10))]) + let incoming = workout(updatedAt: Self.at(20), logs: [log("A", reps: 20, updatedAt: Self.at(20))]) + #expect(reps(merged(incoming, cached), "A") == 20) + } + + @Test func sameLogOlderStampLoses() { + let cached = workout(updatedAt: Self.at(20), logs: [log("A", reps: 20, updatedAt: Self.at(20))]) + let incoming = workout(updatedAt: Self.at(10), logs: [log("A", reps: 12, updatedAt: Self.at(10))]) + #expect(reps(merged(incoming, cached), "A") == 20) + } + + @Test func legacyNilStampIsOldest() { + // A legacy log with no per-log stamp must lose to any stamped edit. + let cached = workout(updatedAt: Self.at(1), logs: [log("A", reps: 99, updatedAt: nil)]) + let incoming = workout(updatedAt: Self.at(5), logs: [log("A", reps: 20, updatedAt: Self.at(5))]) + #expect(reps(merged(incoming, cached), "A") == 20) + } + + // MARK: - Additions + + @Test func watchAddedLogIsKept() { + let cached = workout(updatedAt: Self.at(10), logs: [log("A", order: 0, updatedAt: Self.at(10))]) + let incoming = workout(updatedAt: Self.at(11), + logs: [log("A", order: 0, updatedAt: Self.at(10)), + log("C", order: 1, updatedAt: Self.at(11))]) + let result = merged(incoming, cached) + #expect(Set(result.logs.map(\.id)) == ["A", "C"]) + } + + // MARK: - Deletion tombstones + + @Test func tombstonedLogDoesNotResurrect() { + // Phone deleted B (tombstone at t=10); the watch, unaware, still pushes B (edited earlier). + let cached = workout(updatedAt: Self.at(10), logs: [log("A", updatedAt: Self.at(10))], + deletedLogIDs: ["B": Self.at(10)]) + let incoming = workout(updatedAt: Self.at(11), + logs: [log("A", updatedAt: Self.at(6)), + log("B", updatedAt: Self.at(5))]) + let result = merged(incoming, cached) + #expect(Set(result.logs.map(\.id)) == ["A"]) // B stays deleted + #expect(result.deletedLogIDs?["B"] == Self.at(10)) // tombstone retained (within grace) + } + + @Test func newerEditResurrectsAndClearsTombstone() { + // The watch edited B *after* the phone's deletion — the newer action wins (LWW per log), + // so B comes back and its now-stale tombstone is dropped. + let cached = workout(updatedAt: Self.at(10), logs: [log("A", updatedAt: Self.at(10))], + deletedLogIDs: ["B": Self.at(10)]) + let incoming = workout(updatedAt: Self.at(21), + logs: [log("A", updatedAt: Self.at(6)), + log("B", updatedAt: Self.at(20))]) + let result = merged(incoming, cached) + #expect(Set(result.logs.map(\.id)) == ["A", "B"]) + #expect(result.deletedLogIDs?["B"] == nil) + } + + @Test func tombstonesUnionAcrossSides() { + let cached = workout(updatedAt: Self.at(10), logs: [log("A", updatedAt: Self.at(10))], + deletedLogIDs: ["B": Self.at(10)]) + let incoming = workout(updatedAt: Self.at(11), logs: [log("A", updatedAt: Self.at(6))], + deletedLogIDs: ["C": Self.at(11)]) + let result = merged(incoming, cached) + #expect(result.deletedLogIDs?["B"] == Self.at(10)) + #expect(result.deletedLogIDs?["C"] == Self.at(11)) + } + + @Test func staleTombstonesArePruned() { + // A tombstone older than the grace period (now = t0+1000; grace = 30d) is dropped. + let old = Self.at(1000 - WorkoutMergePlanner.tombstoneGrace - 1) + let cached = workout(updatedAt: Self.at(10), logs: [log("A", updatedAt: Self.at(10))], + deletedLogIDs: ["B": old]) + let result = merged(cached, cached) + #expect(result.deletedLogIDs == nil) + } + + // MARK: - Workout-level resolution + + @Test func statusRecomputedFromMergedLogs() { + // Phone: A done, B in progress → inProgress. Watch completes B (newer) → merged is all + // resolved, so status recomputes to completed with the injected end time. + let cached = workout(updatedAt: Self.at(10), status: .inProgress, + logs: [log("A", status: .completed, updatedAt: Self.at(8)), + log("B", status: .inProgress, updatedAt: Self.at(9))]) + let incoming = workout(updatedAt: Self.at(20), status: .inProgress, + logs: [log("A", status: .completed, updatedAt: Self.at(8)), + log("B", status: .completed, updatedAt: Self.at(20))]) + let result = merged(incoming, cached) + #expect(result.status == WorkoutStatus.completed.rawValue) + #expect(result.end == Self.at(1000)) // recompute used the injected `now` + } + + @Test func newerDocumentSuppliesWorkoutScalars() { + // The just-finished watch push carries the metrics summary; the merge must keep it. + let m = WorkoutMetrics(activeEnergyKcal: 200, avgHeartRate: 130, maxHeartRate: 160, + minHeartRate: 70, totalVolume: 3000, hrZoneSeconds: nil, + healthKitWorkoutUUID: "HK-1", source: .watch, recordedAt: Self.at(20)) + let cached = workout(updatedAt: Self.at(10), logs: [log("A", updatedAt: Self.at(10))]) + let incoming = workout(updatedAt: Self.at(20), logs: [log("A", updatedAt: Self.at(10))], + metrics: m) + #expect(merged(incoming, cached).metrics == m) + } + + // MARK: - The re-push (no-op) case ingest relies on + + @Test func stalePushMergesBackToCached() { + // A behind/duplicate watch push adds nothing newer → merge == cached, so ingest re-pushes + // authoritative state instead of writing. + let cached = workout(updatedAt: Self.at(20), + logs: [log("A", order: 0, reps: 20, updatedAt: Self.at(20)), + log("B", order: 1, reps: 15, updatedAt: Self.at(18))]) + let stale = workout(updatedAt: Self.at(9), + logs: [log("A", order: 0, reps: 10, updatedAt: Self.at(5)), + log("B", order: 1, reps: 10, updatedAt: Self.at(5))]) + #expect(merged(stale, cached) == cached) + } +}