PR review: #228 — Fix T-1699: Prevent duplicate sync heartbeats

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex df586be..b9ed2ac 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -7,6 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased]  - T-1711: MCP `get_projects` now returns the exact tool error `Failed to fetch milestones: <error>` when a project-scoped milestone fetch fails, rather than a successful partial list that omits milestone metadata. The scoped service fetch preserves its injected storage error; deterministic regressions cover the failure, an empty project's omitted `milestones` field, and correctly scoped milestones for another project.+- T-1699: Sync heartbeat fetch failures no longer create duplicate `SyncHeartbeat` singleton rows. `SyncManager` now logs and skips a beat when its singleton fetch fails, then retries on the existing next timer interval; a successful empty fetch still creates the record and a successful existing fetch still updates it. The save remains best-effort and timer lifecycle is unchanged. Deterministic `SyncManagerTests` verify zero insertion, later recovery, and the existing/missing cases. - T-1675: Project-scoped milestone-name lookups now preserve the exact SwiftData fetch failure through shared assignment as `INTERNAL_ERROR` (`Failed to look up milestone: <error>`), matching JSON/MCP create, update, and scoped-query adapters. The milestone service's injected fetcher is now available in MCP test setup; deterministic regressions prove no-match and ambiguity remain distinct, cross-project unscoped filtering is untouched, and failed lookups create or mutate nothing. - T-1825: Dashboard milestone filters now live-observe local, MCP, and CloudKit milestone changes, retaining selected Done or Abandoned rows within the active project scope so they remain visible and individually deselectable. Open rows remain first and follow their displayed title order; terminal selections are appended deterministically and deduplicated by UUID. Selected rows expose the native selected accessibility trait once, while inaccessible or deleted selections still leave Clear available. Add Task remains open-only. 

diff --git a/Transit/Transit/Services/SyncManager.swift b/Transit/Transit/Services/SyncManager.swiftindex 6c966b1..4430c32 100644--- a/Transit/Transit/Services/SyncManager.swift+++ b/Transit/Transit/Services/SyncManager.swift@@ -1,5 +1,6 @@ import CloudKit import Foundation+import OSLog import SwiftData  /// Manages the CloudKit sync enabled/disabled preference.@@ -24,6 +25,7 @@ final class SyncManager {     /// Matches the @AppStorage key used in SettingsView.     private static let syncEnabledKey = "syncEnabled"     private static let cloudKitContainerID = "iCloud.me.nore.ig.Transit"+    private static let logger = Logger(subsystem: "me.nore.ig.Transit", category: "SyncManager")      private(set) var isSyncEnabled: Bool @@ -33,13 +35,24 @@ final class SyncManager {     /// whichever launch path actually creates the container.     private(set) var isCloudSyncActive: Bool +    /// Fetches the heartbeat singleton. Injectable so tests can deterministically+    /// simulate a SwiftData read failure while keeping inserts and saves real.+    typealias HeartbeatFetcher =+        (ModelContext, FetchDescriptor<SyncHeartbeat>) throws -> [SyncHeartbeat]++    private let heartbeatFetcher: HeartbeatFetcher+     /// True when the preference no longer matches the mode the live container runs in,     /// i.e. the user changed the toggle and has not relaunched yet.     var syncChangeRequiresRestart: Bool {         isSyncEnabled != isCloudSyncActive     } -    init() {+    init(+        heartbeatFetcher: @escaping HeartbeatFetcher = { context, descriptor in+            try context.fetch(descriptor)+        }+    ) {         // Default to enabled if never set         let defaults = UserDefaults.standard         if defaults.object(forKey: Self.syncEnabledKey) == nil {@@ -48,6 +61,7 @@ final class SyncManager {         let enabled = defaults.bool(forKey: Self.syncEnabledKey)         self.isSyncEnabled = enabled         self.isCloudSyncActive = enabled+        self.heartbeatFetcher = heartbeatFetcher     }      // MARK: - Public API@@ -123,12 +137,23 @@ final class SyncManager {      /// Writes a timestamp to the `SyncHeartbeat` singleton, triggering a     /// CloudKit sync cycle that pulls pending remote changes.-    private func beat(context: ModelContext) {+    func beat(context: ModelContext) {         let singletonID = SyncHeartbeat.singletonID         let descriptor = FetchDescriptor<SyncHeartbeat>(             predicate: #Predicate { $0.id == singletonID }         )-        let heartbeat = (try? context.fetch(descriptor))?.first ?? SyncHeartbeat()++        let heartbeats: [SyncHeartbeat]+        do {+            heartbeats = try heartbeatFetcher(context, descriptor)+        } catch {+            Self.logger.error(+                "Skipping sync heartbeat because the singleton fetch failed: \(error.localizedDescription)"+            )+            return+        }++        let heartbeat = heartbeats.first ?? SyncHeartbeat()         heartbeat.lastBeat = Date()         if heartbeat.modelContext == nil {             context.insert(heartbeat)

diff --git a/Transit/TransitTests/SyncManagerTests.swift b/Transit/TransitTests/SyncManagerTests.swiftindex 934613d..4fa35bf 100644--- a/Transit/TransitTests/SyncManagerTests.swift+++ b/Transit/TransitTests/SyncManagerTests.swift@@ -1,4 +1,5 @@ import Foundation+import SwiftData import Testing @testable import Transit @@ -8,7 +9,7 @@ struct SyncManagerTests {      /// Saves and restores the UserDefaults value for "syncEnabled" around each     /// test to avoid polluting shared state.-    private func withSavedDefaults(_ body: () -> Void) {+    private func withSavedDefaults(_ body: () throws -> Void) rethrows {         let key = "syncEnabled"         let previous = UserDefaults.standard.object(forKey: key)         defer {@@ -18,7 +19,7 @@ struct SyncManagerTests {                 UserDefaults.standard.removeObject(forKey: key)             }         }-        body()+        try body()     }      // MARK: - T-699 Regression: setSyncEnabled updates runtime state@@ -89,4 +90,101 @@ struct SyncManagerTests {                     "Re-enabling sync must not restart the heartbeat; a new startHeartbeat call is required")         }     }++    // MARK: - T-1699 Regression: heartbeat singleton fetch failures++    private struct FetchFailure: Swift.Error {}++    private final class HeartbeatFetchController {+        var shouldFail = false++        func fetch(+            context: ModelContext,+            descriptor: FetchDescriptor<SyncHeartbeat>+        ) throws -> [SyncHeartbeat] {+            if shouldFail {+                throw FetchFailure()+            }+            return try context.fetch(descriptor)+        }+    }++    private func heartbeatCount(in context: ModelContext) throws -> Int {+        try context.fetch(FetchDescriptor<SyncHeartbeat>()).count+    }++    @Test+    func heartbeatWithMissingSingletonInsertsAndSavesOneRecord() throws {+        let fixture = try TestModelContainer()+        let context = fixture.context+        let manager = SyncManager()++        manager.beat(context: context)++        #expect(try heartbeatCount(in: context) == 1)+        #expect(!context.hasChanges,+                "A successful missing-singleton heartbeat must save its newly inserted record")+    }++    @Test+    func heartbeatWithExistingSingletonUpdatesAndSavesWithoutInsertingAnotherRecord() throws {+        let fixture = try TestModelContainer()+        let context = fixture.context+        let existing = SyncHeartbeat()+        existing.lastBeat = .distantPast+        context.insert(existing)+        try context.save()+        let manager = SyncManager()++        manager.beat(context: context)++        #expect(try heartbeatCount(in: context) == 1)+        #expect(existing.lastBeat > .distantPast)+        #expect(!context.hasChanges,+                "A successful existing-singleton heartbeat must save its updated timestamp")+    }++    @Test+    func heartbeatFetchFailureDoesNotInsertOrSaveAndNextBeatRecovers() throws {+        let fixture = try TestModelContainer()+        let context = fixture.context+        let pendingProject = Project(name: "Pending", description: "", gitRepo: nil, colorHex: "")+        context.insert(pendingProject)+        let fetcher = HeartbeatFetchController()+        let manager = SyncManager(heartbeatFetcher: fetcher.fetch)+        fetcher.shouldFail = true++        manager.beat(context: context)++        #expect(try heartbeatCount(in: context) == 0,+                "A failed singleton fetch must not be treated as a missing record")+        #expect(context.hasChanges,+                "A failed singleton fetch must return before it can save unrelated pending changes")++        fetcher.shouldFail = false+        manager.beat(context: context)++        #expect(try heartbeatCount(in: context) == 1,+                "The next heartbeat must retry normally after a transient fetch failure")+        #expect(!context.hasChanges,+                "The recovered heartbeat should retain the normal best-effort save behavior")+    }++    @Test+    func heartbeatFetchFailureKeepsTimerScheduledForNextInterval() throws {+        try withSavedDefaults {+            UserDefaults.standard.set(true, forKey: "syncEnabled")+            let fixture = try TestModelContainer()+            let fetcher = HeartbeatFetchController()+            fetcher.shouldFail = true+            let manager = SyncManager(heartbeatFetcher: fetcher.fetch)++            manager.startHeartbeat(context: fixture.context)+            defer { manager.stopHeartbeat() }+            manager.beat(context: fixture.context)++            #expect(manager.isHeartbeatRunning,+                    "A failed beat must leave the existing timer scheduled to retry at its next interval")+        }+    } }

diff --git a/specs/bugfixes/sync-heartbeat-fetch-failure/report.md b/specs/bugfixes/sync-heartbeat-fetch-failure/report.mdnew file mode 100644index 0000000..c819e6c--- /dev/null+++ b/specs/bugfixes/sync-heartbeat-fetch-failure/report.md@@ -0,0 +1,81 @@+# Bugfix Report: Sync Heartbeat Fetch Failure++**Date:** 2026-08-05+**Status:** Fixed++## Description of the Issue++`SyncManager.beat(context:)` treated a failed SwiftData fetch for the `SyncHeartbeat` singleton exactly like an empty result. The expression `(try? context.fetch(descriptor))?.first ?? SyncHeartbeat()` created and inserted a new record whenever the fetch threw.++**Reproduction steps:**+1. Run a heartbeat against a context whose `SyncHeartbeat` fetch throws.+2. The failed optional fetch evaluates to `nil`.+3. The fallback creates and saves a new `SyncHeartbeat`, despite the store not being readable.++**Impact:** A transient fetch failure could insert a duplicate fixed-ID singleton. CloudKit-backed SwiftData cannot use `@Attribute(.unique)`, so future heartbeat runs could update an arbitrary duplicate.++## Investigation Summary++- **Symptoms examined:** A fetch error was silently collapsed into the normal missing-record path.+- **Code inspected:** `SyncManager.beat(context:)`, `SyncHeartbeat`, timer callers, existing sync tests, and the CloudKit/SwiftData persistence constraints.+- **Hypotheses tested:** The issue occurs before the best-effort save; preserving `try? context.save()` does not prevent the duplicate insertion. A deterministic injected fetch closure confirmed that an error must be handled separately from an empty result.++## Discovered Root Cause++The heartbeat's error-handling expression combined two semantically different states: a successful empty fetch (the singleton is missing) and a failed fetch (the store is unavailable). The nil-coalescing fallback then created a model for both.++**Defect type:** Logic error / swallowed storage failure.++**Why it occurred:** `try?` erased the fetch error before the code chose whether creation was valid. The code had no branch that could skip the beat after a storage-read failure.++**Contributing factors:** The singleton's fixed string ID is not a unique attribute because CloudKit does not support SwiftData unique constraints, so an accidental insert is not rejected locally.++## Resolution for the Issue++**Changes made:**+- `Transit/Transit/Services/SyncManager.swift` - adds an injected `HeartbeatFetcher` seam for deterministic tests; `beat(context:)` now catches fetch errors, logs the skipped heartbeat, and returns before insertion or save. A successful empty result still creates the singleton; a successful existing result still updates it. The existing timer loop and best-effort `try? context.save()` remain unchanged.+- `Transit/TransitTests/SyncManagerTests.swift` - adds missing-singleton, existing-singleton, and failure-then-recovery regression coverage using a controllable fetcher.++**Approach rationale:** Returning before mutation is fail-closed: it cannot add a duplicate when the singleton's existence is unknown, and the already-scheduled next timer iteration retries normally.++**Alternatives considered:**+- Continue treating a fetch failure as an empty result - rejected because it is the source of duplicate singleton records.+- Add a unique attribute to `SyncHeartbeat.id` - rejected because CloudKit-backed SwiftData does not support unique constraints.+- Stop the timer after an error - rejected because transient storage failures should recover on the next scheduled beat without changing timer lifecycle.++## Regression Test++**Test file:** `Transit/TransitTests/SyncManagerTests.swift`+**Test names:** `heartbeatWithMissingSingletonInsertsAndSavesOneRecord`, `heartbeatWithExistingSingletonUpdatesAndSavesWithoutInsertingAnotherRecord`, `heartbeatFetchFailureDoesNotInsertOrSaveAndNextBeatRecovers`, and `heartbeatFetchFailureKeepsTimerScheduledForNextInterval`++**What it verifies:** A successful empty fetch creates and saves one record, a successful existing fetch updates and saves without adding another record, an injected fetch failure inserts nothing before a later successful beat recovers normally, and a failed beat leaves the scheduled timer active for its next interval.++**Run commands:** `make test-quick`; `make lint`++## Affected Files++| File | Change |+|------|--------|+| `Transit/Transit/Services/SyncManager.swift` | Separates failed fetches from missing singleton results and logs skipped beats. |+| `Transit/TransitTests/SyncManagerTests.swift` | Adds deterministic singleton and recovery regression tests. |+| `CHANGELOG.md` | Records the fixed heartbeat-fetch behavior. |++## Verification++**Automated:**+- [x] Focused `SyncManagerTests` pass.+- [x] Full macOS unit suite passes (`make test-quick`).+- [x] Strict lint passes (`make lint`).++**Manual verification:** Not required; the failure path is covered deterministically through the injected fetch seam.++## Prevention++- Do not use `try?` where a fetch result controls whether a CloudKit-safe singleton may be inserted.+- Keep read failure, empty result, and successful result as distinct control-flow paths.+- Exercise error paths through an injected seam while retaining real mutation and save behavior in tests.++## Related++- Transit task T-1699: Sync heartbeat fetch failures can create duplicate singleton records.+- `How to allocate human-friendly sequential IDs in a CloudKit app` — generated knowledge note on CloudKit's lack of SwiftData unique attributes.