diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b832ba10..b2ba0eae4 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/TablePro/Core/Storage/GroupStorage.swift b/TablePro/Core/Storage/GroupStorage.swift index 49430a56e..56e59ea14 100644 --- a/TablePro/Core/Storage/GroupStorage.swift +++ b/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. diff --git a/TablePro/Core/Storage/RemoteApplyOutcome.swift b/TablePro/Core/Storage/RemoteApplyOutcome.swift new file mode 100644 index 000000000..e6c79dbfb --- /dev/null +++ b/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 +} diff --git a/TablePro/Core/Storage/TagStorage.swift b/TablePro/Core/Storage/TagStorage.swift index dfd60edda..775e78b94 100644 --- a/TablePro/Core/Storage/TagStorage.swift +++ b/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) diff --git a/TablePro/Core/Sync/SyncCoordinator.swift b/TablePro/Core/Sync/SyncCoordinator.swift index 6f61ade44..1a88ab287 100644 --- a/TablePro/Core/Sync/SyncCoordinator.swift +++ b/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) -> Bool { + private func applyRemoteConnection(_ record: CKRecord, tombstoneIds: Set) -> 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) -> 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) -> 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) -> 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) -> RemoteApplyOutcome { + guard let remoteTag = SyncRecordMapper.toTag(record) else { return .skipped } + if tombstoneIds.contains(remoteTag.id.uuidString) { return .skipped } return services.tagStorage.applyRemoteTag(remoteTag) } diff --git a/TableProTests/Core/Storage/GroupStorageTests.swift b/TableProTests/Core/Storage/GroupStorageTests.swift index 9441335bb..efc06fb1c 100644 --- a/TableProTests/Core/Storage/GroupStorageTests.swift +++ b/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() { diff --git a/TableProTests/Core/Storage/TagStorageTests.swift b/TableProTests/Core/Storage/TagStorageTests.swift index 1963d12d6..180d25eeb 100644 --- a/TableProTests/Core/Storage/TagStorageTests.swift +++ b/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"))