From 881190331983779ee49444da7db11a284c3117c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20W=C3=BCrsch?= Date: Sat, 13 Jun 2026 16:42:23 +0200 Subject: [PATCH] Protect standalone Watch workouts from stale stops (#46) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Standalone Watch workouts (started on the wrist, no iPhone) were exposed two ways: - Finding 7 (Medium): the StopTimerMessage staleness check only ran when a sync service was registered, but standalone timers never register one. A stale persisted stop from an earlier iPhone-led timer therefore fell to the "no active timer" branch and handleOrphanedStop ended the LIVE standalone HKWorkoutSession — truncating HR/kcal and the saved workout. Track timer presence independently of the sync service (new isTimerPresented flag, set for both standalone and iPhone-led presentations) and route the stop through a pure routeStop(...) decision: no timer → orphaned cleanup; standalone on screen → ignore (the iPhone has no correlation for a wrist workout); iPhone-led mismatch → ignore stale; iPhone-led match → forward. - Finding 12 (Low): handleWorkoutCompleted's standalone save used try? modelContext.save() then logged success unconditionally — a save failure lost the only record with no diagnostics. Now do/catch with Logger.error, matching DefaultWorkoutLoggingService on iPhone. Extracting routeStop as a pure function makes the standalone-protection decision unit-testable without HealthKit. Adds routeStop tests (incl. the regression) and updates the existing iPhone-led stop tests to set isTimerPresented. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Presets/WatchPresetListView.swift | 2 + .../Shared/WatchWorkoutCoordinator.swift | 9 ++- .../Services/WatchMessageCoordinator.swift | 67 ++++++++++++++++--- .../WatchMessageCoordinatorTests.swift | 63 +++++++++++++++++ 4 files changed, 129 insertions(+), 12 deletions(-) diff --git a/Kraftli Timers Watch App/Features/Presets/WatchPresetListView.swift b/Kraftli Timers Watch App/Features/Presets/WatchPresetListView.swift index 74f58ef..ad59cff 100644 --- a/Kraftli Timers Watch App/Features/Presets/WatchPresetListView.swift +++ b/Kraftli Timers Watch App/Features/Presets/WatchPresetListView.swift @@ -154,9 +154,11 @@ struct WatchPresetListView: View { // Update coordinator's active sync service and correlation ID // when timer is presented/dismissed if let timer = newValue { + messageCoordinator.isTimerPresented = true messageCoordinator.activeSyncService = syncService messageCoordinator.activeCorrelationID = timer.correlationID } else { + messageCoordinator.isTimerPresented = false messageCoordinator.activeSyncService = nil messageCoordinator.activeCorrelationID = nil syncService = nil diff --git a/Kraftli Timers Watch App/Features/Timer/Shared/WatchWorkoutCoordinator.swift b/Kraftli Timers Watch App/Features/Timer/Shared/WatchWorkoutCoordinator.swift index a30918b..b3eec0a 100644 --- a/Kraftli Timers Watch App/Features/Timer/Shared/WatchWorkoutCoordinator.swift +++ b/Kraftli Timers Watch App/Features/Timer/Shared/WatchWorkoutCoordinator.swift @@ -343,7 +343,14 @@ struct WatchWorkoutCoordinator { } else { let log = config.makeWorkoutLog(healthKitUUID) modelContext.insert(log) - try? modelContext.save() + // Surface a save failure: this is the only SwiftData record of a + // standalone workout, saved just as watchOS is most likely to suspend + // the app. Swallowing the error lost the workout with no diagnostics. + do { + try modelContext.save() + } catch { + Logger.workoutLogging.error("Failed to save standalone Watch workout log: \(error.localizedDescription)") + } } Logger.healthKit.info("Watch \(config.timerKind.rawValue) workout completed, HealthKit UUID: \(healthKitUUID?.uuidString ?? "none"), displayOnly: \(config.displayOnly), avgHR: \(averageHeartRate.map { String(format: "%.0f", $0) } ?? "none")") diff --git a/Kraftli Timers Watch App/Services/WatchMessageCoordinator.swift b/Kraftli Timers Watch App/Services/WatchMessageCoordinator.swift index 5d7fbe4..85e0da9 100644 --- a/Kraftli Timers Watch App/Services/WatchMessageCoordinator.swift +++ b/Kraftli Timers Watch App/Services/WatchMessageCoordinator.swift @@ -48,6 +48,14 @@ final class WatchMessageCoordinator { /// the active timer (or with nil correlation) are applied. var activeCorrelationID: UUID? + /// Whether *any* timer view is currently on screen — set for both iPhone-led + /// and standalone (wrist-started) timers. Standalone timers register no sync + /// service and carry no correlation ID, so this flag is the only way to tell + /// "standalone timer presented" apart from "nothing presented" — without it a + /// stale iPhone-led stop would reach `handleOrphanedStop` and end a live + /// standalone workout (issue #46, finding 7). + var isTimerPresented: Bool = false + /// Reference to the workout session manager for orphaned stop handling. /// Set by the app entry point after both coordinator and manager are created. var sessionManager: WorkoutSessionManager? @@ -133,11 +141,44 @@ final class WatchMessageCoordinator { // MARK: - Stop Timer Handling + /// How an inbound `StopTimerMessage` should be dispatched. Pure routing + /// decision — see `routeStop`. + enum StopRouting: Equatable { + /// No timer view at all — end any orphaned HKWorkoutSession left running. + case orphanedCleanup + /// A standalone (wrist-started) timer is on screen — never stop it from a + /// remote message (the iPhone has no correlation for a standalone workout). + case ignoreStandalone + /// An iPhone-led timer is on screen but the stop targets a different + /// workout — stale, ignore. + case ignoreStaleMismatch + /// An iPhone-led timer is on screen and the stop matches — forward it. + case forward + } + + /// Decides how to route an inbound stop. Pure (no side effects, no HealthKit) + /// so the staleness/standalone-protection logic is unit-testable directly. + static func routeStop( + isTimerPresented: Bool, + hasSyncService: Bool, + activeCorrelationID: UUID?, + stopCorrelationID: UUID? + ) -> StopRouting { + guard isTimerPresented else { return .orphanedCleanup } + guard hasSyncService else { return .ignoreStandalone } + if stopCorrelationID == nil || stopCorrelationID == activeCorrelationID { + return .forward + } + return .ignoreStaleMismatch + } + /// Handles a reliable stop message from iPhone. /// /// This arrives via the application context (persisted delivery) and/or /// `sendMessage` (immediate delivery). Correlation ID matching prevents stale stops from - /// killing a new timer that started after the original one completed. + /// killing a new timer that started after the original one completed, and the + /// presence flag prevents a stale stop from ending a live *standalone* workout + /// (issue #46, finding 7). @MainActor private func handleStopTimerMessage(_ message: StopTimerMessage) { // Deduplicate within a short window (stop may arrive via two delivery @@ -157,18 +198,22 @@ final class WatchMessageCoordinator { stoppedCorrelationIDs.insert(stoppedID) } - if let syncService = activeSyncService { - // Timer view is showing — check correlation before forwarding - if message.correlationID == nil || message.correlationID == activeCorrelationID { - Logger.timerSync.info("StopTimerMessage matches active timer, forwarding stop") - syncService.handleControlMessage(TimerControlMessage(action: .stop)) - } else { - Logger.timerSync.info("StopTimerMessage correlationID mismatch (stop: \(message.correlationID?.uuidString ?? "nil"), active: \(self.activeCorrelationID?.uuidString ?? "nil")), ignoring stale stop") - } - } else { - // No timer view showing — handle as orphaned stop + switch Self.routeStop( + isTimerPresented: isTimerPresented, + hasSyncService: activeSyncService != nil, + activeCorrelationID: activeCorrelationID, + stopCorrelationID: message.correlationID + ) { + case .orphanedCleanup: Logger.timerSync.info("StopTimerMessage received with no active timer, cleaning up orphaned session") handleOrphanedStop() + case .ignoreStandalone: + Logger.timerSync.info("StopTimerMessage received while a standalone timer is active, ignoring (no correlation for wrist-started workouts)") + case .ignoreStaleMismatch: + Logger.timerSync.info("StopTimerMessage correlationID mismatch (stop: \(message.correlationID?.uuidString ?? "nil"), active: \(self.activeCorrelationID?.uuidString ?? "nil")), ignoring stale stop") + case .forward: + Logger.timerSync.info("StopTimerMessage matches active timer, forwarding stop") + activeSyncService?.handleControlMessage(TimerControlMessage(action: .stop)) } } diff --git a/Kraftli Timers Watch AppTests/WatchMessageCoordinatorTests.swift b/Kraftli Timers Watch AppTests/WatchMessageCoordinatorTests.swift index 296dde4..bbb2e57 100644 --- a/Kraftli Timers Watch AppTests/WatchMessageCoordinatorTests.swift +++ b/Kraftli Timers Watch AppTests/WatchMessageCoordinatorTests.swift @@ -91,6 +91,7 @@ struct WatchMessageCoordinatorTests { @Test @MainActor func twoDistinctStops_bothApply() { let coordinator = WatchMessageCoordinator() let syncService = DefaultWatchTimerSyncService() + coordinator.isTimerPresented = true coordinator.activeSyncService = syncService var stopCount = 0 @@ -115,6 +116,7 @@ struct WatchMessageCoordinatorTests { @Test @MainActor func duplicateStop_sameCorrelationID_appliesOnce() { let coordinator = WatchMessageCoordinator() let syncService = DefaultWatchTimerSyncService() + coordinator.isTimerPresented = true coordinator.activeSyncService = syncService var stopCount = 0 @@ -135,6 +137,7 @@ struct WatchMessageCoordinatorTests { @Test @MainActor func stop_mismatchedCorrelationID_isIgnored() { let coordinator = WatchMessageCoordinator() let syncService = DefaultWatchTimerSyncService() + coordinator.isTimerPresented = true coordinator.activeSyncService = syncService coordinator.activeCorrelationID = UUID() @@ -149,4 +152,64 @@ struct WatchMessageCoordinatorTests { #expect(stopCount == 0) } + + // MARK: - Stop routing (pure decision, issue #46 finding 7) + + /// The regression: a standalone timer is on screen (no sync service, no + /// correlation) and a stale iPhone-led stop arrives. It must be ignored, NOT + /// routed to orphaned cleanup — which previously ended the live HK session. + @Test func routeStop_standalonePresented_ignoresForeignStop() { + let routing = WatchMessageCoordinator.routeStop( + isTimerPresented: true, + hasSyncService: false, + activeCorrelationID: nil, + stopCorrelationID: UUID() + ) + #expect(routing == .ignoreStandalone) + } + + /// No timer on screen → end any orphaned HK session. + @Test func routeStop_noTimer_orphanedCleanup() { + let routing = WatchMessageCoordinator.routeStop( + isTimerPresented: false, + hasSyncService: false, + activeCorrelationID: nil, + stopCorrelationID: UUID() + ) + #expect(routing == .orphanedCleanup) + } + + /// iPhone-led timer on screen, stop matches the active correlation → forward. + @Test func routeStop_iPhoneLedMatching_forwards() { + let id = UUID() + let routing = WatchMessageCoordinator.routeStop( + isTimerPresented: true, + hasSyncService: true, + activeCorrelationID: id, + stopCorrelationID: id + ) + #expect(routing == .forward) + } + + /// A nil-correlation stop applies to the active iPhone-led timer. + @Test func routeStop_iPhoneLedNilCorrelation_forwards() { + let routing = WatchMessageCoordinator.routeStop( + isTimerPresented: true, + hasSyncService: true, + activeCorrelationID: UUID(), + stopCorrelationID: nil + ) + #expect(routing == .forward) + } + + /// iPhone-led timer on screen, stop targets a different workout → stale. + @Test func routeStop_iPhoneLedMismatch_stale() { + let routing = WatchMessageCoordinator.routeStop( + isTimerPresented: true, + hasSyncService: true, + activeCorrelationID: UUID(), + stopCorrelationID: UUID() + ) + #expect(routing == .ignoreStaleMismatch) + } }