fix(connections): stop a sync acknowledging records a store refused to write

CHANGELOG.md

@@ -43,6 +43,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Every saved tag replaced by the preset list when one unreadable entry stopped the library decoding.
 - Tag created in one window missing from another until relaunch.
 - Two Macs re-uploading the whole tag library to each other after a single tag changed.
+- Connections, groups and tags from another device silently dropped when the store could not be written, and never sent again.
 - Two Macs re-uploading the whole group list to each other after a single group changed. (#1311)
 - Deleting a group that a broken sync left in a loop also deleting the group it pointed at. (#1311)
 

TablePro/Core/Storage/GroupStorage.swift

@@ -170,17 +170,17 @@ internal final class GroupStorage {
     /// re-uploads the whole list, which the other device receives and writes back, and two Macs
     /// trade the same records forever. The iOS coordinator has always had this guard.
     @discardableResult
-    internal func applyRemoteGroup(_ group: ConnectionGroup) -> Bool {
+    internal func applyRemoteGroup(_ group: ConnectionGroup) -> RemoteApplyOutcome {
         var groups = loadGroups()
 
         if let index = groups.firstIndex(where: { $0.id == group.id }) {
-            guard groups[index] != group else { return false }
+            guard groups[index] != group else { return .skipped }
             groups[index] = group
         } else {
             groups.append(group)
         }
 
-        return saveGroups(groups)
+        return saveGroups(groups) ? .applied : .failed
     }
 
     /// Root every group left on a parent cycle, reporting whether anything moved.

TablePro/Core/Storage/RemoteApplyOutcome.swift

@@ -0,0 +1,21 @@
+//
+//  RemoteApplyOutcome.swift
+//  TablePro
+//
+
+import Foundation
+
+/// What applying one record from another device did.
+///
+/// A `Bool` cannot carry this. A store that refused every write and a pull that carried nothing new
+/// both answered false, so the coordinator read a broken store as a quiet sync and committed the
+/// server token over records it had never stored. They are three answers, and only one of them
+/// means the batch has to arrive again.
+internal enum RemoteApplyOutcome: Equatable {
+    /// The record was written.
+    case applied
+    /// Nothing to do: a tombstone, or a record identical to the one already stored.
+    case skipped
+    /// The record could not be persisted, so it is not in the local store and has to be re-fetched.
+    case failed
+}

TablePro/Core/Storage/TagStorage.swift

@@ -132,17 +132,17 @@ internal final class TagStorage {
     /// every tag dirty and the push uploads every dirty tag, so writing an unchanged record
     /// re-uploads the whole library to the device it came from, which writes it back.
     @discardableResult
-    internal func applyRemoteTag(_ tag: ConnectionTag) -> Bool {
+    internal func applyRemoteTag(_ tag: ConnectionTag) -> RemoteApplyOutcome {
         var tags = loadTags()
 
         if let index = tags.firstIndex(where: { $0.id == tag.id }) {
-            guard tags[index] != tag else { return false }
+            guard tags[index] != tag else { return .skipped }
             tags[index] = tag
         } else {
             tags.append(tag)
         }
 
-        return saveTags(tags)
+        return saveTags(tags) ? .applied : .failed
     }
 
     /// Delete a custom tag (presets cannot be deleted)

TablePro/Core/Sync/SyncCoordinator.swift

@@ -461,12 +461,22 @@ final class SyncCoordinator {
     }
 
     private func applyPullResult(_ result: PullResult) {
+        let persisted = applyRemoteChanges(result)
+
+        /// The token and the cache are the record of what this device holds, so neither is
+        /// committed over a batch a store refused. Saving the token first acknowledged records that
+        /// were never written and the server never sent them again, and the cached record then
+        /// stood in as the merge base for an edit that had no local base at all. Not saving it
+        /// means the next pull replays the batch, which every apply here is written to survive.
+        guard persisted else {
+            Self.logger.error("Pull not acknowledged: a store refused to persist part of the batch")
+            return
+        }
+
         if let newToken = result.newToken {
             metadataStorage.saveToken(newToken)
         }
 
-        applyRemoteChanges(result)
-
         recordCache.store(result.changedRecords)
         recordCache.remove(result.deletedRecordIDs)
 
@@ -478,7 +488,10 @@ final class SyncCoordinator {
     // Performance: storage reads here (loadSync, loadConnections, loadGroups, etc.) run on
     // @MainActor and can block the UI on large sync batches. Consider moving to Task.detached
     // for large payloads.
-    private func applyRemoteChanges(_ result: PullResult) {
+    /// Reports whether every record that can say so was persisted. A pull that answers false must
+    /// not commit its token: the batch has to arrive again.
+    @discardableResult
+    private func applyRemoteChanges(_ result: PullResult) -> Bool {
         let settings = services.appSettingsStorage.loadSync()
 
         services.connectionStorage.invalidateCache()
@@ -490,6 +503,7 @@ final class SyncCoordinator {
 
         var actualConnectionChanges = false
         var groupsOrTagsChanged = false
+        var persistenceFailed = false
 
         let connectionTombstoneIds = Set(metadataStorage.tombstones(for: .connection).map(\.id))
         let groupTombstoneIds = Set(metadataStorage.tombstones(for: .group).map(\.id))
@@ -505,16 +519,22 @@ final class SyncCoordinator {
         for record in result.changedRecords {
             switch record.recordType {
             case SyncRecordType.connection.rawValue where settings.syncConnections:
-                if applyRemoteConnection(record, tombstoneIds: connectionTombstoneIds) {
-                    actualConnectionChanges = true
+                switch applyRemoteConnection(record, tombstoneIds: connectionTombstoneIds) {
+                case .applied: actualConnectionChanges = true
+                case .failed: persistenceFailed = true
+                case .skipped: break
                 }
             case SyncRecordType.group.rawValue where settings.syncGroupsAndTags:
-                if applyRemoteGroup(record, tombstoneIds: groupTombstoneIds) {
-                    groupsOrTagsChanged = true
+                switch applyRemoteGroup(record, tombstoneIds: groupTombstoneIds) {
+                case .applied: groupsOrTagsChanged = true
+                case .failed: persistenceFailed = true
+                case .skipped: break
                 }
             case SyncRecordType.tag.rawValue where settings.syncGroupsAndTags:
-                if applyRemoteTag(record, tombstoneIds: tagTombstoneIds) {
-                    groupsOrTagsChanged = true
+                switch applyRemoteTag(record, tombstoneIds: tagTombstoneIds) {
+                case .applied: groupsOrTagsChanged = true
+                case .failed: persistenceFailed = true
+                case .skipped: break
                 }
             case SyncRecordType.sshProfile.rawValue where settings.syncSSHProfiles:
                 applyRemoteSSHProfile(record, tombstoneIds: sshTombstoneIds)
@@ -642,6 +662,8 @@ final class SyncCoordinator {
         if actualConnectionChanges || groupsOrTagsChanged {
             services.appEvents.connectionUpdated.send(nil)
         }
+
+        return !persistenceFailed
     }
 
     @discardableResult
@@ -667,17 +689,17 @@ final class SyncCoordinator {
         }
     }
 
-    private func applyRemoteConnection(_ record: CKRecord, tombstoneIds: Set<String>) -> Bool {
+    private func applyRemoteConnection(_ record: CKRecord, tombstoneIds: Set<String>) -> RemoteApplyOutcome {
         let remoteConnection: DatabaseConnection
         do {
             remoteConnection = try SyncRecordMapper.toConnection(record)
         } catch {
             Self.logger.error("Skipping remote connection \(record.recordID.recordName, privacy: .public): \(error.localizedDescription, privacy: .public)")
-            return false
+            return .skipped
         }
 
         if tombstoneIds.contains(remoteConnection.id.uuidString) {
-            return false
+            return .skipped
         }
 
         var connections = services.connectionStorage.loadConnections()
@@ -688,7 +710,7 @@ final class SyncCoordinator {
                     into: record,
                     localConnection: connections[index]
                 ) else {
-                    return false
+                    return .skipped
                 }
                 incoming = reconciled
             }
@@ -701,23 +723,22 @@ final class SyncCoordinator {
         }
         guard services.connectionStorage.saveConnections(connections) else {
             Self.logger.error("Failed to apply remote connection update: persistence error for \(remoteConnection.id, privacy: .public)")
-            return false
+            return .failed
         }
-        return true
+        return .applied
     }
 
-    @discardableResult
-    private func applyRemoteGroup(_ record: CKRecord, tombstoneIds: Set<String>) -> Bool {
-        guard let remoteGroup = SyncRecordMapper.toGroup(record) else { return false }
-        if tombstoneIds.contains(remoteGroup.id.uuidString) { return false }
+    private func applyRemoteGroup(_ record: CKRecord, tombstoneIds: Set<String>) -> RemoteApplyOutcome {
+        guard let remoteGroup = SyncRecordMapper.toGroup(record) else { return .skipped }
+        if tombstoneIds.contains(remoteGroup.id.uuidString) { return .skipped }
 
         return services.groupStorage.applyRemoteGroup(remoteGroup)
     }
 
     @discardableResult
-    private func applyRemoteTag(_ record: CKRecord, tombstoneIds: Set<String>) -> Bool {
-        guard let remoteTag = SyncRecordMapper.toTag(record) else { return false }
-        if tombstoneIds.contains(remoteTag.id.uuidString) { return false }
+    private func applyRemoteTag(_ record: CKRecord, tombstoneIds: Set<String>) -> RemoteApplyOutcome {
+        guard let remoteTag = SyncRecordMapper.toTag(record) else { return .skipped }
+        if tombstoneIds.contains(remoteTag.id.uuidString) { return .skipped }
 
         return services.tagStorage.applyRemoteTag(remoteTag)
     }

TableProTests/Core/Storage/GroupStorageTests.swift

@@ -366,10 +366,22 @@ final class GroupStorageTests: XCTestCase {
         try storage.addGroup(group)
         tracker.clearAllDirty(.group)
 
-        XCTAssertFalse(storage.applyRemoteGroup(group))
+        XCTAssertEqual(storage.applyRemoteGroup(group), .skipped)
         XCTAssertTrue(tracker.dirtyRecords(for: .group).isEmpty)
     }
 
+    func testApplyingANewRemoteGroupReportsItWasWritten() {
+        XCTAssertEqual(storage.applyRemoteGroup(ConnectionGroup(name: "FromAnotherMac")), .applied)
+    }
+
+    /// The pull reads this to decide whether to acknowledge the batch, so a store that refused must
+    /// not answer the same as one that had nothing to do.
+    func testApplyingARemoteGroupOverAnUnreadableStoreReportsFailure() {
+        defaults.set(Data([0x00, 0x01]), forKey: "com.TablePro.groups")
+
+        XCTAssertEqual(storage.applyRemoteGroup(ConnectionGroup(name: "FromAnotherMac")), .failed)
+    }
+
     // MARK: - Unreadable Store
 
     func testAnUnreadableStoreIsLeftUntouched() {

TableProTests/Core/Storage/TagStorageTests.swift

@@ -120,10 +120,22 @@ final class TagStorageTests: XCTestCase {
         try storage.addTag(tag)
         tracker.clearAllDirty(.tag)
 
-        XCTAssertFalse(storage.applyRemoteTag(tag))
+        XCTAssertEqual(storage.applyRemoteTag(tag), .skipped)
         XCTAssertTrue(tracker.dirtyRecords(for: .tag).isEmpty)
     }
 
+    func testApplyingANewRemoteTagReportsItWasWritten() {
+        XCTAssertEqual(storage.applyRemoteTag(ConnectionTag(name: "from-another-mac")), .applied)
+    }
+
+    /// The pull reads this to decide whether to acknowledge the batch, so a store that refused must
+    /// not answer the same as one that had nothing to do.
+    func testApplyingARemoteTagOverAnUnreadableStoreReportsFailure() {
+        let storage = makeStorage(seeding: Data([0x00, 0x01]))
+
+        XCTAssertEqual(storage.applyRemoteTag(ConnectionTag(name: "from-another-mac")), .failed)
+    }
+
     func testApplyingARemoteTagAnnouncesNothing() {
         storage.applyRemoteTag(ConnectionTag(name: "from-another-mac"))