diff --git a/apple/Packages/TableProCore/Sources/TableProSync/SyncRecordMapper.swift b/apple/Packages/TableProCore/Sources/TableProSync/SyncRecordMapper.swift index 0bc37cca24..3faae1906d 100644 --- a/apple/Packages/TableProCore/Sources/TableProSync/SyncRecordMapper.swift +++ b/apple/Packages/TableProCore/Sources/TableProSync/SyncRecordMapper.swift @@ -9,7 +9,12 @@ import TableProSyncTransport public enum SyncRecordMapper { private static let logger = Logger(subsystem: "com.TablePro", category: "SyncRecordMapper") - private static let encoder = JSONEncoder() + // Two encodes of one dictionary can order its keys differently, and an edit compares bytes. + private static let encoder: JSONEncoder = { + let encoder = JSONEncoder() + encoder.outputFormatting = .sortedKeys + return encoder + }() private static let decoder = JSONDecoder() private static let schemaVersion: Int64 = 1 @@ -27,6 +32,12 @@ public enum SyncRecordMapper { SyncRecordType.parse(recordName: recordName) } + private static func record(type: SyncRecordType, id: String, in zone: CKRecordZone.ID, base: CKRecord?) -> CKRecord { + let recordID = recordID(type: type, id: id, in: zone) + if let base, base.recordID == recordID { return base } + return CKRecord(recordType: type.rawValue, recordID: recordID) + } + // iOS wrote hex to `colorTag` before both apps converged on `color`. private static func color(from fields: SyncRecordFields) -> ConnectionColor { if let stored = fields[.color] as? String { @@ -40,7 +51,7 @@ public enum SyncRecordMapper { // MARK: - Connection -> CKRecord - // Under `.changedKeys` only a named key reaches the server, so every field `updateRecord` would clear is named here too. + // Only a named key reaches the server, so every field `updateRecord` would clear is named here too. public static func toRecord(_ connection: DatabaseConnection, zoneID: CKRecordZone.ID) -> CKRecord { let id = recordID(type: .connection, id: connection.id.uuidString, in: zoneID) let record = CKRecord(recordType: SyncRecordType.connection.rawValue, recordID: id) @@ -345,7 +356,7 @@ public enum SyncRecordMapper { let record = CKRecord(recordType: SyncRecordType.group.rawValue, recordID: id) // Built whole from the local group, so an empty parent or icon is the user's choice and has - // to reach the server, which keeps any key a `.changedKeys` push leaves unnamed. + // to reach the server, which keeps any key a push leaves unnamed. let fields = record.fields(ConnectionGroupSyncField.self, absentValues: .clear) fields[.groupId] = group.id.uuidString fields[.name] = group.name @@ -447,12 +458,11 @@ public enum SyncRecordMapper { // MARK: - Saved Query -> CKRecord - // Built whole from the local query, so an emptied keyword, folder or connection is named and cleared on the server. - public static func toRecord(_ query: SavedQuery, zoneID: CKRecordZone.ID) -> CKRecord { - let id = recordID(type: .favorite, id: query.id.uuidString, in: zoneID) - let record = CKRecord(recordType: SyncRecordType.favorite.rawValue, recordID: id) + // A record built whole names an emptied keyword, folder or connection, so the server clears it. + public static func toRecord(_ query: SavedQuery, zoneID: CKRecordZone.ID, base: CKRecord? = nil) -> CKRecord { + let record = record(type: .favorite, id: query.id.uuidString, in: zoneID, base: base) - let fields = record.fields(SQLFavoriteSyncField.self, absentValues: .clear) + let fields = record.fields(SQLFavoriteSyncField.self, absentValues: record === base ? .leave : .clear) fields[.favoriteId] = query.id.uuidString fields[.name] = query.name fields[.query] = query.query @@ -496,11 +506,10 @@ public enum SyncRecordMapper { // MARK: - Saved Query Folder -> CKRecord - public static func toRecord(_ folder: SavedQueryFolder, zoneID: CKRecordZone.ID) -> CKRecord { - let id = recordID(type: .favoriteFolder, id: folder.id.uuidString, in: zoneID) - let record = CKRecord(recordType: SyncRecordType.favoriteFolder.rawValue, recordID: id) + public static func toRecord(_ folder: SavedQueryFolder, zoneID: CKRecordZone.ID, base: CKRecord? = nil) -> CKRecord { + let record = record(type: .favoriteFolder, id: folder.id.uuidString, in: zoneID, base: base) - let fields = record.fields(SQLFavoriteFolderSyncField.self, absentValues: .clear) + let fields = record.fields(SQLFavoriteFolderSyncField.self, absentValues: record === base ? .leave : .clear) fields[.folderId] = folder.id.uuidString fields[.name] = folder.name fields[.parentId] = folder.parentId?.uuidString @@ -536,4 +545,76 @@ public enum SyncRecordMapper { updatedAt: fields[.updatedAt] as? Date ) } + + // MARK: - Edits + + // What a push of each local copy changes on the cached server record. The base is read and written + // again here, so a value only the Mac spells differently is not part of the edit. + public static func edit(_ connection: DatabaseConnection, base: CKRecord?, zoneID: CKRecordZone.ID) -> SyncRecordEdit { + edit(ConnectionSyncField.self, base: base) { + updateRecord($0, with: connection) + } build: { + toRecord(connection, zoneID: zoneID) + } reread: { + toConnection($0).map { toRecord($0, zoneID: zoneID) } + } + } + + public static func edit(_ group: ConnectionGroup, base: CKRecord?, zoneID: CKRecordZone.ID) -> SyncRecordEdit { + edit(ConnectionGroupSyncField.self, base: base) { + updateRecord($0, with: group) + } build: { + toRecord(group, zoneID: zoneID) + } reread: { + toGroup($0).map { toRecord($0, zoneID: zoneID) } + } + } + + public static func edit(_ tag: ConnectionTag, base: CKRecord?, zoneID: CKRecordZone.ID) -> SyncRecordEdit { + edit(ConnectionTagSyncField.self, base: base) { + updateRecord($0, with: tag) + } build: { + toRecord(tag, zoneID: zoneID) + } reread: { + toTag($0).map { toRecord($0, zoneID: zoneID) } + } + } + + public static func edit(_ query: SavedQuery, base: CKRecord?, zoneID: CKRecordZone.ID) -> SyncRecordEdit { + edit(SQLFavoriteSyncField.self, base: base) { + _ = toRecord(query, zoneID: $0.recordID.zoneID, base: $0) + } build: { + toRecord(query, zoneID: zoneID) + } reread: { + toSavedQuery($0).map { toRecord($0, zoneID: zoneID) } + } + } + + public static func edit(_ folder: SavedQueryFolder, base: CKRecord?, zoneID: CKRecordZone.ID) -> SyncRecordEdit { + edit(SQLFavoriteFolderSyncField.self, base: base) { + _ = toRecord(folder, zoneID: $0.recordID.zoneID, base: $0) + } build: { + toRecord(folder, zoneID: zoneID) + } reread: { + toSavedQueryFolder($0).map { toRecord($0, zoneID: zoneID) } + } + } + + // Written into a copy, so `base` stays what the server last held. + private static func edit( + _ field: Field.Type, + base: CKRecord?, + update: (CKRecord) -> Void, + build: () -> CKRecord, + reread: (CKRecord) -> CKRecord? + ) -> SyncRecordEdit { + let record: CKRecord + if let copy = base?.copy() as? CKRecord { + update(copy) + record = copy + } else { + record = build() + } + return SyncRecordEdit(field, record: record, base: base, baseAsWritten: base.flatMap(reread)) + } } diff --git a/apple/Packages/TableProCore/Sources/TableProSyncTransport/CloudKitSyncEngine.swift b/apple/Packages/TableProCore/Sources/TableProSyncTransport/CloudKitSyncEngine.swift index a700ef3636..d9992388dd 100644 --- a/apple/Packages/TableProCore/Sources/TableProSyncTransport/CloudKitSyncEngine.swift +++ b/apple/Packages/TableProCore/Sources/TableProSyncTransport/CloudKitSyncEngine.swift @@ -163,12 +163,7 @@ public actor CloudKitSyncEngine { private func pushBatch(records: [CKRecord], deletions: [CKRecord.ID]) async throws -> PushOutcome { guard let database else { throw Self.unavailable } return try await withRetry { - let operation = CKModifyRecordsOperation( - recordsToSave: records, - recordIDsToDelete: deletions - ) - operation.savePolicy = .changedKeys - operation.isAtomic = false + let operation = Self.modifyOperation(saving: records, deleting: deletions) return try await withCheckedThrowingContinuation { continuation in var outcome = PushOutcome() @@ -210,8 +205,26 @@ public actor CloudKitSyncEngine { } } + // A record built on a cached copy that is no longer the server's is refused with + // `serverRecordChanged` and the server's copy, so the caller merges its edit onto that copy. + static func modifyOperation(saving records: [CKRecord], deleting deletions: [CKRecord.ID]) -> CKModifyRecordsOperation { + let operation = CKModifyRecordsOperation(recordsToSave: records, recordIDsToDelete: deletions) + operation.savePolicy = .ifServerRecordUnchanged + operation.isAtomic = false + return operation + } + // MARK: - Pull + public func fetch(recordIDs: [CKRecord.ID]) async throws -> [CKRecord.ID: CKRecord] { + guard !recordIDs.isEmpty else { return [:] } + guard let database else { throw Self.unavailable } + let results = try await withRetry { + try await database.records(for: recordIDs) + } + return results.compactMapValues { try? $0.get() } + } + public func pull(since token: CKServerChangeToken?) async throws -> PullResult { var changedRecords: [CKRecord] = [] var deletedRecordIDs: [CKRecord.ID] = [] diff --git a/apple/Packages/TableProCore/Sources/TableProSyncTransport/PushOutcome.swift b/apple/Packages/TableProCore/Sources/TableProSyncTransport/PushOutcome.swift index 1c730498fc..b2cc723e92 100644 --- a/apple/Packages/TableProCore/Sources/TableProSyncTransport/PushOutcome.swift +++ b/apple/Packages/TableProCore/Sources/TableProSyncTransport/PushOutcome.swift @@ -44,6 +44,7 @@ public struct PushOutcome: Sendable { public private(set) var savedRecords: [CKRecord.ID: CKRecord] public private(set) var deletedRecordIDs: Set public private(set) var failures: [CKRecord.ID: SyncItemFailure] + public private(set) var rebasedRecordIDs: Set = [] public init( savedRecords: [CKRecord.ID: CKRecord] = [:], @@ -55,6 +56,16 @@ public struct PushOutcome: Sendable { self.failures = failures } + // A rebased save also holds another device's change the local copy has not taken yet, so the record this + // device pushed stays the base until it does. Its old change tag gets the next push refused and merged + // onto the saved one, an edit back to an earlier value included. + public func savedBases(pushed edits: [CKRecord.ID: SyncRecordEdit]) -> [CKRecord] { + savedRecords.map { recordID, saved in + guard rebasedRecordIDs.contains(recordID), let pushed = edits[recordID]?.record else { return saved } + return pushed + } + } + public var hasFailures: Bool { !failures.isEmpty } public var isEmpty: Bool { @@ -113,10 +124,22 @@ public struct PushOutcome: Sendable { } } + public mutating func markRebased(_ recordIDs: [CKRecord.ID]) { + rebasedRecordIDs.formUnion(recordIDs) + } + + // A record sent again after a refusal can land in a later outcome than its failure. public mutating func merge(_ other: PushOutcome) { savedRecords.merge(other.savedRecords) { _, new in new } deletedRecordIDs.formUnion(other.deletedRecordIDs) failures.merge(other.failures) { _, new in new } + for recordID in other.savedRecords.keys { + failures[recordID] = nil + } + for recordID in other.deletedRecordIDs { + failures[recordID] = nil + } + rebasedRecordIDs.formUnion(other.rebasedRecordIDs) } public mutating func absorbPartialErrors(from error: CKError) { diff --git a/apple/Packages/TableProCore/Sources/TableProSyncTransport/SyncRecordCache.swift b/apple/Packages/TableProCore/Sources/TableProSyncTransport/SyncRecordCache.swift index 6632d5b838..8d50e3a23f 100644 --- a/apple/Packages/TableProCore/Sources/TableProSyncTransport/SyncRecordCache.swift +++ b/apple/Packages/TableProCore/Sources/TableProSyncTransport/SyncRecordCache.swift @@ -41,6 +41,11 @@ public final class SyncRecordCache { record(named: recordID.recordName) } + public func holdsRecord(named recordName: String) -> Bool { + migrateIfNeeded() + return FileManager.default.fileExists(atPath: fileURL(for: recordName).path) + } + // A record's name is the same in every zone, so a caller with no zone at hand can still read it. public func record(named recordName: String) -> CKRecord? { migrateIfNeeded() diff --git a/apple/Packages/TableProCore/Sources/TableProSyncTransport/SyncRecordEdit.swift b/apple/Packages/TableProCore/Sources/TableProSyncTransport/SyncRecordEdit.swift new file mode 100644 index 0000000000..d8c6adfd3a --- /dev/null +++ b/apple/Packages/TableProCore/Sources/TableProSyncTransport/SyncRecordEdit.swift @@ -0,0 +1,224 @@ +import CloudKit +import Foundation + +// The fields a push changes on the server record it was built on, so the same change can be made +// again on the server's newer copy when CloudKit refuses the push. +public struct SyncRecordEdit { + public static let rebaseLimit = 2 + + public let record: CKRecord + public let editedKeys: Set + public let hasBase: Bool + private let apply: (CKRecord) -> Void + private let whole: () -> CKRecord + private let settle: ((_ merged: CKRecord, _ server: CKRecord) -> Void)? + + // `base` is an untouched copy of what `record` was built on, compared value by value: an archived + // or copied record keeps its `changedKeys()`, so a cached record can still name keys an earlier save + // sent. `baseAsWritten` is `base` read and written again by this device, so a value this device only + // spells differently is not an edit. + public init( + _: Field.Type, + record: CKRecord, + base: CKRecord?, + baseAsWritten: CKRecord? = nil, + settle: ((_ merged: CKRecord, _ server: CKRecord) -> Void)? = nil + ) { + let pushed = record.fields(Field.self) + let base = base.flatMap { $0.recordID == record.recordID ? $0 : nil } + let edited: [Field] + if let base { + let original = base.fields(Field.self) + let written = baseAsWritten?.fields(Field.self) + edited = Field.allCases.filter { field in + !CKRecord.isEqualRecordValue(pushed[field], original[field]) + && !(written.map { CKRecord.isEqualRecordValue(pushed[field], $0[field]) } ?? false) + } + } else { + let named = Set(record.changedKeys()) + edited = Field.allCases.filter { named.contains($0.key) } + } + let values = edited.map { (field: $0, value: pushed[$0]) } + + self.record = record + self.editedKeys = Set(edited.map(\.key)) + self.hasBase = base != nil + self.apply = { target in + let fields = target.fields(Field.self, absentValues: .clear) + for (field, value) in values { + fields[field] = value + } + } + self.whole = { + let fresh = CKRecord(recordType: record.recordType, recordID: record.recordID) + let fields = fresh.fields(Field.self) + for field in Field.allCases { + fields[field] = pushed[field] + } + return fresh + } + self.settle = settle + } + + public var recordID: CKRecord.ID { record.recordID } + + // Every push stamps these, so an edit holding only them changes nothing a person made. + public var changesContent: Bool { + !editedKeys.subtracting(["modifiedAtLocal", "schemaVersion"]).isEmpty + } + + // The server no longer holds the record (deleted elsewhere, or its zone made again), so the whole + // local copy goes up as a new one, as a push built with no cached record always did. + public func recreated() -> CKRecord { + whole() + } + + // Carries the server's change tag, so CloudKit saves it only while that copy is still the latest. + public func rebased(onto server: CKRecord) -> CKRecord? { + guard server.recordID == record.recordID, server.recordType == record.recordType, + let merged = server.copy() as? CKRecord + else { return nil } + apply(merged) + settle?(merged, server) + return merged + } +} + +public enum SyncConflictRebase { + // A record still refused after the last round stays a failure, pending for the next run. `fetch` + // reads the server's copy when a refusal arrives without one. + public static func push( + records: [CKRecord], + deletions: [CKRecord.ID], + edits: [CKRecord.ID: SyncRecordEdit], + isolation: isolated (any Actor)? = #isolation, + send: (_ records: [CKRecord], _ deletions: [CKRecord.ID]) async throws -> PushOutcome, + fetch: (_ recordIDs: [CKRecord.ID]) async throws -> [CKRecord.ID: CKRecord] + ) async throws -> PushOutcome { + var outcome = try await send(records, deletions) + for _ in 0.. Data? { + guard let base = parse(base), let local = parse(local), let serverData = server, let server = parse(server) + else { return nil } + if isEqual(local, base) { return serverData } + return try? JSONSerialization.data( + withJSONObject: merge(base: base, local: local, server: server, depth: 0), + options: [.sortedKeys, .fragmentsAllowed] + ) + } + + public static func sameDocument(_ lhs: Data?, _ rhs: Data?) -> Bool { + guard let lhs = parse(lhs), let rhs = parse(rhs) else { return false } + return isEqual(lhs, rhs) + } + + private static func merge(base: Any?, local: Any, server: Any, depth: Int) -> Any { + if isEqual(local, base) { return server } + if isEqual(server, base) { return local } + guard depth < objectDepth else { return local } + if let base = base as? [String: Any], let local = local as? [String: Any], let server = server as? [String: Any] { + return mergeObjects(base: base, local: local, server: server, depth: depth) + } + if let base = base as? [Any], let local = local as? [Any], let server = server as? [Any], + let merged = mergeLists(base: base, local: local, server: server) { + return merged + } + return local + } + + private static func mergeObjects(base: [String: Any], local: [String: Any], server: [String: Any], depth: Int) -> [String: Any] { + var merged = server + for key in Set(local.keys).union(base.keys) where !isEqual(local[key], base[key]) { + guard let value = local[key] else { + merged.removeValue(forKey: key) + continue + } + merged[key] = server[key].map { merge(base: base[key], local: value, server: $0, depth: depth + 1) } ?? value + } + return merged + } + + private static func mergeLists(base: [Any], local: [Any], server: [Any]) -> [Any]? { + guard let baseItems = keyed(base), let localItems = keyed(local), let serverItems = keyed(server) else { return nil } + let baseIds = Set(baseItems.map(\.id)) + let localIds = Set(localItems.map(\.id)) + let serverIds = Set(serverItems.map(\.id)) + let baseById = Dictionary(uniqueKeysWithValues: baseItems.map { ($0.id, $0.item) }) + let localById = Dictionary(uniqueKeysWithValues: localItems.map { ($0.id, $0.item) }) + + var merged: [Any] = serverItems.compactMap { entry in + guard !(baseIds.contains(entry.id) && !localIds.contains(entry.id)) else { return nil } + guard let mine = localById[entry.id], !isEqual(mine, baseById[entry.id]) else { return entry.item } + return mine + } + // Added here, or changed here while another device removed it. + for entry in localItems where !serverIds.contains(entry.id) && !isEqual(entry.item, baseById[entry.id]) { + merged.append(entry.item) + } + return merged + } + + private static func keyed(_ list: [Any]) -> [(id: String, item: Any)]? { + var seen: Set = [] + var keyed: [(id: String, item: Any)] = [] + for item in list { + guard let object = item as? [String: Any], let id = object["id"].map({ "\($0)" }), seen.insert(id).inserted + else { return nil } + keyed.append((id, item)) + } + return keyed + } + + private static func parse(_ data: Data?) -> Any? { + data.flatMap { try? JSONSerialization.jsonObject(with: $0, options: [.fragmentsAllowed]) } + } + + private static func isEqual(_ lhs: Any?, _ rhs: Any?) -> Bool { + switch (lhs, rhs) { + case (nil, nil): + return true + case (let lhs as NSObject, let rhs as NSObject): + return lhs.isEqual(rhs) + default: + return false + } + } +} diff --git a/apple/Packages/TableProCore/Sources/TableProSyncTransport/SyncSchemaField.swift b/apple/Packages/TableProCore/Sources/TableProSyncTransport/SyncSchemaField.swift index 6a3e2d58d0..6baa036795 100644 --- a/apple/Packages/TableProCore/Sources/TableProSyncTransport/SyncSchemaField.swift +++ b/apple/Packages/TableProCore/Sources/TableProSyncTransport/SyncSchemaField.swift @@ -32,20 +32,12 @@ public extension SyncSchemaField where Self: RawRepresentable, Self.RawValue == } public struct SyncRecordFields { - /// What writing nil means for a key the record does not already hold. - /// - /// `CKModifyRecordsOperation.savePolicy` is `.changedKeys`, so the server keeps whatever it has - /// for any key the pushed record never names, and a key is named only once something assigns to - /// it. The two answers are therefore genuinely different pushes, and which one is right depends - /// on what the record being built is. + // What nil means for a key the record does not hold. A save sends only the keys the record names + // (WWDC 2014, session 231), so an unnamed key keeps the server's value. public enum AbsentValueWrite: Sendable { - /// Say nothing about the key, so the server keeps its value. Correct for a mapper that - /// merges into the record the server last gave us, whose job is to leave alone what it was - /// not asked about. + // For a mapper merging into the server's copy, which leaves alone what it was not asked about. case leave - /// Name the key with no value, so the server clears it. Correct for a mapper that builds - /// the whole record from the local model, where a field with no value is a value: it is the - /// user having emptied it. + // For a mapper building the whole record from the local model, where an empty field was emptied. case clear } diff --git a/apple/Packages/TableProCore/Tests/TableProSyncTests/SyncRecordEditTests.swift b/apple/Packages/TableProCore/Tests/TableProSyncTests/SyncRecordEditTests.swift new file mode 100644 index 0000000000..33ccdc7ecb --- /dev/null +++ b/apple/Packages/TableProCore/Tests/TableProSyncTests/SyncRecordEditTests.swift @@ -0,0 +1,432 @@ +import CloudKit +import Foundation +import Testing + +@testable import TableProSyncTransport + +struct SyncRecordEditTests { + private let zoneID = CKRecordZone.ID(zoneName: "TestZone", ownerName: CKCurrentUserDefaultName) + + private func serverRecord(_ values: [ConnectionSyncField: Any]) -> CKRecord { + let record = CKRecord( + recordType: SyncRecordType.connection.rawValue, + recordID: CKRecord.ID(recordName: SyncRecordType.connection.recordName(for: "a"), zoneID: zoneID) + ) + let fields = record.fields(ConnectionSyncField.self) + for (field, value) in values { + fields[field] = value + } + return record + } + + private func copy(_ record: CKRecord) throws -> CKRecord { + try #require(record.copy() as? CKRecord) + } + + private func value(_ field: ConnectionSyncField, in record: CKRecord) -> Any? { + record.fields(ConnectionSyncField.self)[field] + } + + @Test("An edit holds only the fields the push changed on its base") + func editHoldsChangedFields() throws { + let base = serverRecord([.name: "Production", .port: Int64(5_432), .host: "db"]) + let pushed = try copy(base) + pushed.fields(ConnectionSyncField.self)[.port] = Int64(6_543) + + let edit = SyncRecordEdit(ConnectionSyncField.self, record: pushed, base: base) + + #expect(edit.editedKeys == [ConnectionSyncField.port.key]) + } + + @Test("A rebased edit keeps the fields another device changed on the newer copy") + func rebaseKeepsServerFields() throws { + let base = serverRecord([.name: "Production", .port: Int64(5_432)]) + let pushed = try copy(base) + pushed.fields(ConnectionSyncField.self)[.port] = Int64(6_543) + let server = try copy(base) + server.fields(ConnectionSyncField.self)[.name] = "Renamed elsewhere" + + let merged = try #require(SyncRecordEdit(ConnectionSyncField.self, record: pushed, base: base).rebased(onto: server)) + + #expect(value(.name, in: merged) as? String == "Renamed elsewhere") + #expect(value(.port, in: merged) as? Int64 == 6_543) + } + + @Test("A value this device only spells differently is not part of the edit") + func respelledValueIsNotAnEdit() throws { + let base = serverRecord([.name: "Production", .sshConfigJson: Data(#"{"b":1,"a":2}"#.utf8)]) + let asWritten = serverRecord([.name: "Production", .sshConfigJson: Data(#"{"a":2,"b":1}"#.utf8)]) + let pushed = try copy(base) + pushed.fields(ConnectionSyncField.self)[.sshConfigJson] = Data(#"{"a":2,"b":1}"#.utf8) + pushed.fields(ConnectionSyncField.self)[.name] = "Renamed here" + let server = try copy(base) + let newer = Data(#"{"a":3,"b":1}"#.utf8) + server.fields(ConnectionSyncField.self)[.sshConfigJson] = newer + + let edit = SyncRecordEdit(ConnectionSyncField.self, record: pushed, base: base, baseAsWritten: asWritten) + let merged = try #require(edit.rebased(onto: server)) + + #expect(edit.editedKeys == [ConnectionSyncField.name.key]) + #expect(value(.sshConfigJson, in: merged) as? Data == newer) + #expect(value(.name, in: merged) as? String == "Renamed here") + } + + @Test("A field the edit emptied is emptied on the newer copy") + func clearedFieldStaysCleared() throws { + let base = serverRecord([.name: "Production", .aiRules: "Be careful"]) + let pushed = try copy(base) + pushed.fields(ConnectionSyncField.self)[.aiRules] = nil + let server = try copy(base) + server.fields(ConnectionSyncField.self)[.aiRules] = "Be very careful" + + let merged = try #require(SyncRecordEdit(ConnectionSyncField.self, record: pushed, base: base).rebased(onto: server)) + + #expect(value(.aiRules, in: merged) == nil) + #expect(Set(merged.changedKeys()).contains(ConnectionSyncField.aiRules.key)) + } + + @Test("A key an earlier save left in changedKeys is not part of the edit") + func staleChangedKeysAreIgnored() throws { + let base = serverRecord([.name: "Production", .port: Int64(5_432)]) + #expect(Set(base.changedKeys()).contains(ConnectionSyncField.name.key)) + let pushed = try copy(base) + pushed.fields(ConnectionSyncField.self)[.port] = Int64(6_543) + + #expect(SyncRecordEdit(ConnectionSyncField.self, record: pushed, base: base).editedKeys == [ConnectionSyncField.port.key]) + } + + @Test("A record built with no base sends every field it names") + func freshRecordSendsEveryNamedField() { + let fresh = serverRecord([.name: "Production", .port: Int64(5_432)]) + + let edit = SyncRecordEdit(ConnectionSyncField.self, record: fresh, base: nil) + + #expect(edit.editedKeys == [ConnectionSyncField.name.key, ConnectionSyncField.port.key]) + } + + @Test("The settle step sees the merged record and the server's copy") + func settleAdjustsTheMergedRecord() throws { + let base = serverRecord([.name: "Production"]) + let pushed = try copy(base) + pushed.fields(ConnectionSyncField.self)[.name] = "Mine" + let server = try copy(base) + server.fields(ConnectionSyncField.self)[.name] = "Theirs" + + let edit = SyncRecordEdit(ConnectionSyncField.self, record: pushed, base: base) { merged, server in + merged.fields(ConnectionSyncField.self)[.name] = server.fields(ConnectionSyncField.self)[.name] + } + + let merged = try #require(edit.rebased(onto: server)) + #expect(value(.name, in: merged) as? String == "Theirs") + } + + @Test("An edit lands only on a copy of its own record") + func rebaseRefusesAnotherRecord() { + let pushed = serverRecord([.name: "Production"]) + let other = CKRecord( + recordType: SyncRecordType.connection.rawValue, + recordID: CKRecord.ID(recordName: SyncRecordType.connection.recordName(for: "b"), zoneID: zoneID) + ) + + #expect(SyncRecordEdit(ConnectionSyncField.self, record: pushed, base: nil).rebased(onto: other) == nil) + } + + @Test("Saves are refused when the server's copy moved on") + func savesAreConditional() { + let operation = CloudKitSyncEngine.modifyOperation(saving: [], deleting: []) + + #expect(operation.savePolicy == .ifServerRecordUnchanged) + #expect(operation.isAtomic == false) + } + + // MARK: - Rebase loop + + private func conflict(_ server: CKRecord) -> SyncItemFailure { + SyncItemFailure(code: .serverRecordChanged, serverRecord: server, clientRecord: nil, message: "changed") + } + + @Test("A refused push is merged onto the server's copy and sent again") + func refusedPushIsRebased() async throws { + let base = serverRecord([.name: "Production", .port: Int64(5_432)]) + let pushed = try copy(base) + pushed.fields(ConnectionSyncField.self)[.port] = Int64(6_543) + let server = try copy(base) + server.fields(ConnectionSyncField.self)[.name] = "Renamed elsewhere" + let edit = SyncRecordEdit(ConnectionSyncField.self, record: pushed, base: base) + var sent: [[CKRecord]] = [] + let failure = conflict(server) + + let outcome = try await SyncConflictRebase.push( + records: [pushed], + deletions: [], + edits: [edit.recordID: edit] + ) { records, _ in + sent.append(records) + guard sent.count > 1 else { return PushOutcome(failures: [edit.recordID: failure]) } + return PushOutcome(savedRecords: Dictionary(uniqueKeysWithValues: records.map { ($0.recordID, $0) })) + } fetch: { _ in [:] } + + #expect(sent.count == 2) + let retried = try #require(sent.last?.first) + #expect(value(.name, in: retried) as? String == "Renamed elsewhere") + #expect(value(.port, in: retried) as? Int64 == 6_543) + #expect(outcome.didSave(edit.recordID)) + #expect(outcome.hasFailures == false) + #expect(outcome.rebasedRecordIDs == [edit.recordID]) + #expect(outcome.savedBases(pushed: [edit.recordID: edit]).first === pushed) + } + + @Test("A record refused in every round stays pending") + func rebaseStopsAtTheLimit() async throws { + let base = serverRecord([.name: "Production"]) + let pushed = try copy(base) + pushed.fields(ConnectionSyncField.self)[.name] = "Mine" + let edit = SyncRecordEdit(ConnectionSyncField.self, record: pushed, base: base) + let failure = conflict(try copy(base)) + var sends = 0 + + let outcome = try await SyncConflictRebase.push( + records: [pushed], + deletions: [], + edits: [edit.recordID: edit] + ) { _, _ in + sends += 1 + return PushOutcome(failures: [edit.recordID: failure]) + } fetch: { _ in [:] } + + #expect(sends == 1 + SyncRecordEdit.rebaseLimit) + #expect(outcome.failures[edit.recordID]?.isConflict == true) + } + + // MARK: - Settings options + + private func json(_ text: String) -> Data { + Data(text.utf8) + } + + private func parsed(_ data: Data?) throws -> Any { + let data = try #require(data) + return try JSONSerialization.jsonObject(with: data) + } + + private func options(_ data: Data?) throws -> NSDictionary { + let document = try parsed(data) as? NSDictionary + return try #require(document) + } + + @Test("Two devices changing different options of one category keep both") + func optionsChangedOnEachSideAreKept() throws { + let merged = SyncJSONMerge.document( + base: json(#"{"rowHeight":20,"zebra":false,"font":"Menlo"}"#), + local: json(#"{"font":"Menlo","rowHeight":24,"zebra":false}"#), + server: json(#"{"rowHeight":20,"zebra":true,"font":"Menlo"}"#) + ) + + #expect(try options(merged) == ["rowHeight": 24, "zebra": true, "font": "Menlo"]) + } + + @Test("An option removed here is removed from the server's object") + func removedOptionIsRemoved() throws { + let merged = SyncJSONMerge.document( + base: json(#"{"a":1,"b":2}"#), + local: json(#"{"a":1}"#), + server: json(#"{"a":3,"b":2}"#) + ) + + #expect(try options(merged) == ["a": 3]) + } + + @Test("Values both sides changed, outside a keyed list, take this device's") + func bothChangedTakesLocal() throws { + let merged = SyncJSONMerge.document(base: json("[1]"), local: json("[1,2]"), server: json("[3]")) + + let list = try parsed(merged) as? [Int] + #expect(list == [1, 2]) + #expect(SyncJSONMerge.document(base: nil, local: json(#"{"a":1}"#), server: json(#"{"a":2}"#)) == nil) + } + + @Test("A document this device only spelled differently leaves the server's untouched") + func respelledDocumentKeepsTheServers() { + let server = json(#"[{"id":"1","name":"a"},{"id":"2","name":"b"}]"#) + + let merged = SyncJSONMerge.document( + base: json(#"[{"name":"a","id":"1"}]"#), + local: json(#"[{"id":"1","name":"a"}]"#), + server: server + ) + + #expect(merged == server) + } + + @Test("Items with an id added on each side are both kept, and one removed here is removed") + func keyedListsMergeByItem() throws { + let merged = SyncJSONMerge.document( + base: json(#"[{"id":"1","name":"a"},{"id":"2","name":"b"}]"#), + local: json(#"[{"id":"1","name":"a2"},{"id":"3","name":"c"}]"#), + server: json(#"[{"id":"1","name":"a"},{"id":"2","name":"b"},{"id":"4","name":"d"}]"#) + ) + + let items = try parsed(merged) as? [[String: String]] + #expect(items == [["id": "1", "name": "a2"], ["id": "4", "name": "d"], ["id": "3", "name": "c"]]) + } + + @Test("An item changed here and removed elsewhere stays, as any value both sides changed") + func itemEditedHereAndRemovedElsewhereStays() throws { + let merged = SyncJSONMerge.document( + base: json(#"[{"id":"1","name":"a"},{"id":"2","name":"b"}]"#), + local: json(#"[{"id":"1","name":"a2"},{"id":"2","name":"b"}]"#), + server: json(#"[{"id":"2","name":"b"}]"#) + ) + + let items = try parsed(merged) as? [[String: String]] + #expect(items == [["id": "2", "name": "b"], ["id": "1", "name": "a2"]]) + } + + @Test("Options nested in an object merge one by one") + func nestedObjectsMerge() throws { + let merged = SyncJSONMerge.document( + base: json(#"{"shortcuts":{"x":1,"y":1}}"#), + local: json(#"{"shortcuts":{"x":2,"y":1}}"#), + server: json(#"{"shortcuts":{"x":1,"y":3}}"#) + ) + + #expect(try options(merged) == ["shortcuts": ["x": 2, "y": 3]]) + } + + @Test("A shortcut both sides rebound is one option, so neither side's keys mix into a third") + func shortcutIsOneOption() throws { + let merged = SyncJSONMerge.document( + base: json(#"{"shortcuts":{"copy":{"key":"a","modifiers":["cmd"]},"paste":{"key":"v","modifiers":["cmd"]}}}"#), + local: json(#"{"shortcuts":{"copy":{"key":"a","modifiers":["cmd","shift"]},"paste":{"key":"v","modifiers":["cmd"]}}}"#), + server: json(#"{"shortcuts":{"copy":{"key":"b","modifiers":["cmd"]},"paste":{"key":"p","modifiers":["cmd"]}}}"#) + ) + + #expect(try options(merged) == [ + "shortcuts": [ + "copy": ["key": "a", "modifiers": ["cmd", "shift"]], + "paste": ["key": "p", "modifiers": ["cmd"]] + ] + ]) + } + + @Test("An item both sides changed is taken whole from this device") + func itemChangedOnBothSidesIsTakenWhole() throws { + let merged = SyncJSONMerge.document( + base: json(#"[{"id":"1","name":"a","prompt":"p"}]"#), + local: json(#"[{"id":"1","name":"a2","prompt":"p"}]"#), + server: json(#"[{"id":"1","name":"a","prompt":"p3"}]"#) + ) + + let items = try parsed(merged) as? [[String: String]] + #expect(items == [["id": "1", "name": "a2", "prompt": "p"]]) + } + + @Test("Two spellings of one document are the same document") + func respellingIsTheSameDocument() { + #expect(SyncJSONMerge.sameDocument(json(#"{"a":1,"b":[2]}"#), json(#"{"b":[2],"a":1}"#))) + #expect(!SyncJSONMerge.sameDocument(json(#"{"a":1}"#), json(#"{"a":2}"#))) + #expect(!SyncJSONMerge.sameDocument(nil, json(#"{"a":1}"#))) + } + + @Test("An edit of only the stamps every push writes changes no content") + func stampsAreNotContent() throws { + let base = serverRecord([.name: "Production", .modifiedAtLocal: Date(timeIntervalSince1970: 1)]) + let pushed = try copy(base) + pushed.fields(ConnectionSyncField.self)[.modifiedAtLocal] = Date(timeIntervalSince1970: 2) + + #expect(SyncRecordEdit(ConnectionSyncField.self, record: pushed, base: base).changesContent == false) + pushed.fields(ConnectionSyncField.self)[.name] = "Renamed" + #expect(SyncRecordEdit(ConnectionSyncField.self, record: pushed, base: base).changesContent) + } + + @Test("A record deleted elsewhere goes up again whole, as a new record") + func missingRecordIsRecreated() async throws { + let base = serverRecord([.name: "Production", .port: Int64(5_432)]) + let pushed = try copy(base) + pushed.fields(ConnectionSyncField.self)[.port] = Int64(6_543) + let edit = SyncRecordEdit(ConnectionSyncField.self, record: pushed, base: base) + let missing = SyncItemFailure(code: .unknownItem, serverRecord: nil, clientRecord: nil, message: "Record not found") + var sent: [[CKRecord]] = [] + + let outcome = try await SyncConflictRebase.push(records: [pushed], deletions: [], edits: [edit.recordID: edit]) { records, _ in + sent.append(records) + guard sent.count > 1 else { return PushOutcome(failures: [edit.recordID: missing]) } + return PushOutcome(savedRecords: Dictionary(uniqueKeysWithValues: records.map { ($0.recordID, $0) })) + } fetch: { _ in [:] } + + let recreated = try #require(sent.last?.first) + #expect(recreated !== pushed) + #expect(value(.name, in: recreated) as? String == "Production") + #expect(value(.port, in: recreated) as? Int64 == 6_543) + #expect(outcome.didSave(edit.recordID)) + #expect(outcome.rebasedRecordIDs.isEmpty) + } + + @Test("A refusal that names no server copy reads it and merges onto it") + func refusalWithoutCopyFetchesIt() async throws { + let base = serverRecord([.name: "Production", .port: Int64(5_432)]) + let pushed = try copy(base) + pushed.fields(ConnectionSyncField.self)[.port] = Int64(6_543) + let server = try copy(base) + server.fields(ConnectionSyncField.self)[.name] = "Renamed elsewhere" + let edit = SyncRecordEdit(ConnectionSyncField.self, record: pushed, base: base) + let refusal = SyncItemFailure(code: .serverRecordChanged, serverRecord: nil, clientRecord: nil, message: "changed") + var sent: [[CKRecord]] = [] + + let outcome = try await SyncConflictRebase.push(records: [pushed], deletions: [], edits: [edit.recordID: edit]) { records, _ in + sent.append(records) + guard sent.count > 1 else { return PushOutcome(failures: [edit.recordID: refusal]) } + return PushOutcome(savedRecords: Dictionary(uniqueKeysWithValues: records.map { ($0.recordID, $0) })) + } fetch: { recordIDs in + Dictionary(uniqueKeysWithValues: recordIDs.map { ($0, server) }) + } + + let retried = try #require(sent.last?.first) + #expect(value(.name, in: retried) as? String == "Renamed elsewhere") + #expect(value(.port, in: retried) as? Int64 == 6_543) + #expect(outcome.didSave(edit.recordID)) + } + + @Test("A merge round that throws keeps what the first round saved and the refusal still pending") + func throwingRoundKeepsEarlierOutcome() async throws { + let base = serverRecord([.name: "Production"]) + let pushed = try copy(base) + pushed.fields(ConnectionSyncField.self)[.name] = "Mine" + let edit = SyncRecordEdit(ConnectionSyncField.self, record: pushed, base: base) + let saved = CKRecord( + recordType: SyncRecordType.connection.rawValue, + recordID: CKRecord.ID(recordName: SyncRecordType.connection.recordName(for: "b"), zoneID: zoneID) + ) + let failure = conflict(try copy(base)) + var sends = 0 + + do { + _ = try await SyncConflictRebase.push( + records: [pushed, saved], + deletions: [], + edits: [edit.recordID: edit] + ) { _, _ in + sends += 1 + guard sends == 1 else { throw CKError(.networkFailure) } + return PushOutcome(savedRecords: [saved.recordID: saved], failures: [edit.recordID: failure]) + } fetch: { _ in [:] } + Issue.record("Expected the merge round to throw") + } catch let interruption as SyncPushInterruption { + #expect(interruption.completed.didSave(saved.recordID)) + #expect(interruption.completed.failures[edit.recordID]?.isConflict == true) + #expect(interruption.completed.savedBases(pushed: [edit.recordID: edit]).map(\.recordID) == [saved.recordID]) + #expect(SyncFailure(interruption) == .offline) + } + } + + @Test("A later save clears the failure an earlier push reported for the same record") + func laterSaveClearsEarlierFailure() { + let record = serverRecord([.name: "Production"]) + var outcome = PushOutcome(failures: [record.recordID: conflict(record)]) + + outcome.merge(PushOutcome(savedRecords: [record.recordID: record])) + + #expect(outcome.didSave(record.recordID)) + #expect(outcome.hasFailures == false) + } +} diff --git a/apple/TablePro/Core/Storage/CustomSlashCommandStorage.swift b/apple/TablePro/Core/Storage/CustomSlashCommandStorage.swift index 8cf0aa7df7..9a20765697 100644 --- a/apple/TablePro/Core/Storage/CustomSlashCommandStorage.swift +++ b/apple/TablePro/Core/Storage/CustomSlashCommandStorage.swift @@ -26,7 +26,7 @@ enum CustomSlashCommandError: LocalizedError, Equatable { final class CustomSlashCommandStorage: ObservableObject { static let shared = CustomSlashCommandStorage() - static let syncCategory = "customSlashCommands" + nonisolated static let syncCategory = "customSlashCommands" nonisolated private static let logger = Logger(subsystem: "com.TablePro", category: "CustomSlashCommandStorage") private static let defaultsKey = "ai.customSlashCommands.v1" @@ -47,6 +47,21 @@ final class CustomSlashCommandStorage: ObservableObject { persist(markDirty: false) } + // Two Macs can each add a command under one name before they sync, and a name picks one command, so a later one takes a free name. + nonisolated static func renamingCollisions(_ commands: [CustomSlashCommand]) -> [CustomSlashCommand] { + var kept: [CustomSlashCommand] = [] + for command in commands { + var named = command + var suffix = 2 + while kept.contains(where: { $0.name.caseInsensitiveCompare(named.name) == .orderedSame }) { + named.name = "\(command.name)-\(suffix)" + suffix += 1 + } + kept.append(named) + } + return kept + } + func isDuplicate(_ name: String, excluding id: UUID? = nil) -> Bool { commands.contains { existing in if let id, existing.id == id { return false } diff --git a/apple/TablePro/Core/Sync/Extensions/SyncCoordinator+PushCollection.swift b/apple/TablePro/Core/Sync/Extensions/SyncCoordinator+PushCollection.swift index 7e41cb96a5..f0050d139c 100644 --- a/apple/TablePro/Core/Sync/Extensions/SyncCoordinator+PushCollection.swift +++ b/apple/TablePro/Core/Sync/Extensions/SyncCoordinator+PushCollection.swift @@ -4,24 +4,32 @@ import os import TableProSyncTransport struct SyncPushBatch { - var records: [CKRecord] = [] + var edits: [SyncRecordEdit] = [] var deletions: [CKRecord.ID] = [] var supersededTombstones: Set = [] + var records: [CKRecord] { + edits.map(\.record) + } + + var editsByRecordID: [CKRecord.ID: SyncRecordEdit] { + Dictionary(edits.map { ($0.recordID, $0) }, uniquingKeysWith: { _, last in last }) + } + var uniqueDeletions: [CKRecord.ID] { Array(Set(deletions)) } } enum SyncPushDisposition { - case push(CKRecord) + case push(SyncRecordEdit) case hold case drop } private struct BuiltRecord { let id: String - let record: CKRecord + let edit: SyncRecordEdit } private struct CollectedRecords { @@ -35,8 +43,8 @@ private struct CollectedRecords { mutating func add(_ id: String, _ disposition: SyncPushDisposition) { switch disposition { - case .push(let record): - built.append(BuiltRecord(id: id, record: record)) + case .push(let edit): + built.append(BuiltRecord(id: id, edit: edit)) case .hold: heldIds.insert(id) case .drop: @@ -53,6 +61,24 @@ extension SyncCoordinator { profiles.refreshingLinkedProfile(connection) } + // Built on a copy of the cached record, so the edit compares against the untouched one. + private func edit( + _ field: Field.Type, + _ type: SyncRecordType, + id: String, + in zoneID: CKRecordZone.ID, + build: (CKRecord?) -> CKRecord, + reread: (CKRecord) -> CKRecord? = { _ in nil } + ) -> SyncRecordEdit { + let base = recordCache.record(for: SyncRecordMapper.recordID(type: type, id: id, in: zoneID)) + return SyncRecordEdit( + field, + record: build(base?.copy() as? CKRecord), + base: base, + baseAsWritten: base.flatMap(reread) + ) + } + func collectPushBatch( snapshot: SyncEditSnapshot, boundary: SyncBoundary, @@ -70,13 +96,12 @@ extension SyncCoordinator { } disposition: { (connection: DatabaseConnection) in guard connection.participatesInSync else { return .drop } let recordID = SyncRecordMapper.recordID(type: .connection, id: connection.id.uuidString, in: zoneID) - let base = recordCache.record(for: recordID) - let synced = SyncRecordMapper.keepingForeignRoute( - of: Self.syncedTunnel(of: connection, profiles: profiles), - in: base, + return .push(SyncRecordMapper.connectionEdit( + Self.syncedTunnel(of: connection, profiles: profiles), + in: zoneID, + base: recordCache.record(for: recordID), publisher: routePublisher - ) - return .push(SyncRecordMapper.toCKRecord(synced, in: zoneID, base: base)) + )) } } @@ -86,8 +111,11 @@ extension SyncCoordinator { let groups = groupStorage.loadGroups() return groupStorage.storeIsUnreadable ? nil : groups } disposition: { (group: ConnectionGroup) in - let recordID = SyncRecordMapper.recordID(type: .group, id: group.id.uuidString, in: zoneID) - return .push(SyncRecordMapper.toCKRecord(group, in: zoneID, base: recordCache.record(for: recordID))) + .push(edit(ConnectionGroupSyncField.self, .group, id: group.id.uuidString, in: zoneID) { base in + SyncRecordMapper.toCKRecord(group, in: zoneID, base: base) + } reread: { base in + SyncRecordMapper.toGroup(base).map { SyncRecordMapper.toCKRecord($0, in: zoneID) } + }) } } @@ -96,7 +124,13 @@ extension SyncCoordinator { await collectRecords(of: .tag, snapshot: snapshot, boundary: boundary, into: &batch, zoneID: zoneID) { let tags = tagStorage.loadTags() return tagStorage.storeIsUnreadable ? nil : tags - } disposition: { (tag: ConnectionTag) in .push(SyncRecordMapper.toCKRecord(tag, in: zoneID)) } + } disposition: { (tag: ConnectionTag) in + .push(edit(ConnectionTagSyncField.self, .tag, id: tag.id.uuidString, in: zoneID) { base in + SyncRecordMapper.toCKRecord(tag, in: zoneID, base: base) + } reread: { base in + SyncRecordMapper.toTag(base).map { SyncRecordMapper.toCKRecord($0, in: zoneID) } + }) + } } if boundary.includes(.sshProfile) { @@ -104,7 +138,13 @@ extension SyncCoordinator { await collectRecords(of: .sshProfile, snapshot: snapshot, boundary: boundary, into: &batch, zoneID: zoneID) { let profiles = storage.loadProfiles() return storage.lastLoadFailed ? nil : profiles - } disposition: { (profile: SSHProfile) in .push(SyncRecordMapper.toCKRecord(profile, in: zoneID)) } + } disposition: { (profile: SSHProfile) in + .push(edit(SSHProfileSyncField.self, .sshProfile, id: profile.id.uuidString, in: zoneID) { base in + SyncRecordMapper.toCKRecord(profile, in: zoneID, base: base) + } reread: { base in + (try? SyncRecordMapper.toSSHProfile(base)).map { SyncRecordMapper.toCKRecord($0, in: zoneID) } + }) + } } if boundary.includes(.credentialProfile) { @@ -114,7 +154,13 @@ extension SyncCoordinator { ) { let profiles = storage.loadProfiles() return storage.lastLoadFailed ? nil : profiles - } disposition: { (profile: CredentialProfile) in .push(SyncRecordMapper.toCKRecord(profile, in: zoneID)) } + } disposition: { (profile: CredentialProfile) in + .push(edit(CredentialProfileSyncField.self, .credentialProfile, id: profile.id.uuidString, in: zoneID) { base in + SyncRecordMapper.toCKRecord(profile, in: zoneID, base: base) + } reread: { base in + (try? SyncRecordMapper.toCredentialProfile(base)).map { SyncRecordMapper.toCKRecord($0, in: zoneID) } + }) + } } if boundary.includes(.settings) { @@ -190,7 +236,7 @@ extension SyncCoordinator { return } let pushable = Set(collected.built.map(\.id)) - batch.records.append(contentsOf: collected.built.map(\.record)) + batch.edits.append(contentsOf: collected.built.map(\.edit)) discardUnpushable(type, dirtyIds: dirtyIds.subtracting(collected.heldIds), pushable: pushable) let superseded = appendTombstones(of: type, sparing: pushable, boundary: boundary, to: &batch, zoneID: zoneID) batch.supersededTombstones.formUnion(superseded.map { SyncRecordIdentity(type: type, id: $0) }) @@ -246,7 +292,12 @@ extension SyncCoordinator { let owner = SyncBoundary.owner(ofRecordId: category, type: .settings) guard boundary.includes(.settings, owner: owner) else { return .hold } guard let data = settingsData(for: category) else { return .drop } - return .push(SyncRecordMapper.toCKRecord(category: category, settingsData: data, in: zoneID)) + return .push(SyncRecordMapper.settingsEdit( + category: category, + settingsData: data, + in: zoneID, + base: recordCache.record(for: SyncRecordMapper.recordID(type: .settings, id: category, in: zoneID)) + )) } append(.settings, dirtyIds: dirtyIds, collected: collected, boundary: boundary, into: &batch, zoneID: zoneID) } @@ -268,11 +319,11 @@ extension SyncCoordinator { ) { let dirtyIds = snapshot.dirtyIds(for: .tableFavorite) let favorites = dirtyIds.isEmpty ? [] : services.favoriteTablesStorage.loadFavorites() - let collected = Self.collectTableFavorites(favorites, dirtyIds: dirtyIds, boundary: boundary, zoneID: zoneID) + let collected = collectTableFavorites(favorites, dirtyIds: dirtyIds, boundary: boundary, zoneID: zoneID) append(.tableFavorite, dirtyIds: dirtyIds, collected: collected, boundary: boundary, into: &batch, zoneID: zoneID) } - private static func collectTableFavorites( + private func collectTableFavorites( _ favorites: Set, dirtyIds: Set, boundary: SyncBoundary, @@ -288,7 +339,9 @@ extension SyncCoordinator { let admitted = boundary.includes(.tableFavorite, owner: entry.connectionId) for recordId in recordIds where dirtyIds.contains(recordId) { collected.add(recordId, admitted ? .push( - SyncRecordMapper.toCKRecord(favoriteEntry: entry, recordId: recordId, in: zoneID) + edit(FavoriteTableSyncField.self, .tableFavorite, id: recordId, in: zoneID) { base in + SyncRecordMapper.toCKRecord(favoriteEntry: entry, recordId: recordId, in: zoneID, base: base) + } ) : .hold) } } @@ -309,7 +362,13 @@ extension SyncCoordinator { id: FavoriteDatabasesStorage.syncId(for:) ) { entry in guard boundary.includes(.favoriteDatabase, owner: entry.connectionId) else { return .hold } - return .push(SyncRecordMapper.toCKRecord(favoriteDatabase: entry, in: zoneID)) + return .push(edit( + FavoriteDatabaseSyncField.self, .favoriteDatabase, id: FavoriteDatabasesStorage.syncId(for: entry), in: zoneID + ) { base in + SyncRecordMapper.toCKRecord(favoriteDatabase: entry, in: zoneID, base: base) + } reread: { base in + (try? SyncRecordMapper.favoriteDatabase(from: base)).map { SyncRecordMapper.toCKRecord(favoriteDatabase: $0, in: zoneID) } + }) } append(.favoriteDatabase, dirtyIds: dirtyIds, collected: collected, boundary: boundary, into: &batch, zoneID: zoneID) } @@ -329,7 +388,11 @@ extension SyncCoordinator { id: { $0.id.uuidString } ) { folder in guard boundary.includes(.tableFolder, owner: folder.scope.connectionId) else { return .hold } - return .push(SyncRecordMapper.toCKRecord(tableFolder: folder, in: zoneID)) + return .push(edit(TableFolderSyncField.self, .tableFolder, id: folder.id.uuidString, in: zoneID) { base in + SyncRecordMapper.toCKRecord(tableFolder: folder, in: zoneID, base: base) + } reread: { base in + (try? SyncRecordMapper.tableFolder(from: base)).map { SyncRecordMapper.toCKRecord(tableFolder: $0, in: zoneID) } + }) } append(.tableFolder, dirtyIds: dirtyIds, collected: collected, boundary: boundary, into: &batch, zoneID: zoneID) } @@ -349,7 +412,11 @@ extension SyncCoordinator { id: \.syncId ) { item in guard boundary.includes(.tableFolderItem, owner: item.scope.connectionId) else { return .hold } - return .push(SyncRecordMapper.toCKRecord(tableFolderItem: item, in: zoneID)) + return .push(edit(TableFolderItemSyncField.self, .tableFolderItem, id: item.syncId, in: zoneID) { base in + SyncRecordMapper.toCKRecord(tableFolderItem: item, in: zoneID, base: base) + } reread: { base in + (try? SyncRecordMapper.tableFolderItem(from: base)).map { SyncRecordMapper.toCKRecord(tableFolderItem: $0, in: zoneID) } + }) } append(.tableFolderItem, dirtyIds: dirtyIds, collected: collected, boundary: boundary, into: &batch, zoneID: zoneID) } @@ -366,7 +433,11 @@ extension SyncCoordinator { await manager.favoritesForSync() } disposition: { (favorite: SQLFavorite) in guard boundary.includes(.favorite, owner: favorite.connectionId) else { return .hold } - return .push(SyncRecordMapper.toCKRecord(sqlFavorite: favorite, in: zoneID)) + return .push(edit(SQLFavoriteSyncField.self, .favorite, id: favorite.id.uuidString, in: zoneID) { base in + SyncRecordMapper.toCKRecord(sqlFavorite: favorite, in: zoneID, base: base) + } reread: { base in + (try? SyncRecordMapper.sqlFavorite(from: base)).map { SyncRecordMapper.toCKRecord(sqlFavorite: $0, in: zoneID) } + }) } } @@ -377,7 +448,13 @@ extension SyncCoordinator { await manager.foldersForSync() } disposition: { (folder: SQLFavoriteFolder) in guard boundary.includes(.favoriteFolder, owner: folder.connectionId) else { return .hold } - return .push(SyncRecordMapper.toCKRecord(sqlFavoriteFolder: folder, in: zoneID)) + return .push(edit(SQLFavoriteFolderSyncField.self, .favoriteFolder, id: folder.id.uuidString, in: zoneID) { base in + SyncRecordMapper.toCKRecord(sqlFavoriteFolder: folder, in: zoneID, base: base) + } reread: { base in + (try? SyncRecordMapper.sqlFavoriteFolder(from: base)).map { + SyncRecordMapper.toCKRecord(sqlFavoriteFolder: $0, in: zoneID) + } + }) } } } diff --git a/apple/TablePro/Core/Sync/Extensions/SyncCoordinator+Settings.swift b/apple/TablePro/Core/Sync/Extensions/SyncCoordinator+Settings.swift index 8a4b5e97dc..ef318b30cb 100644 --- a/apple/TablePro/Core/Sync/Extensions/SyncCoordinator+Settings.swift +++ b/apple/TablePro/Core/Sync/Extensions/SyncCoordinator+Settings.swift @@ -8,6 +8,7 @@ extension SyncCoordinator { func settingsData(for category: String) -> Data? { let storage = services.appSettingsStorage let encoder = JSONEncoder() + encoder.outputFormatting = .sortedKeys do { switch category { diff --git a/apple/TablePro/Core/Sync/SyncCoordinator.swift b/apple/TablePro/Core/Sync/SyncCoordinator.swift index 4712fc18ac..cc99975f9c 100644 --- a/apple/TablePro/Core/Sync/SyncCoordinator.swift +++ b/apple/TablePro/Core/Sync/SyncCoordinator.swift @@ -580,7 +580,16 @@ final class SyncCoordinator: ObservableObject { var outcome: PushOutcome var interruption: Error? do { - outcome = try await transport.push(records: batch.records, deletions: deletions) + let transport = transport + outcome = try await SyncConflictRebase.push( + records: batch.records, + deletions: deletions, + edits: batch.editsByRecordID + ) { + try await transport.push(records: $0, deletions: $1) + } fetch: { + try await transport.fetch(recordIDs: $0) + } } catch let interrupted as SyncPushInterruption { outcome = interrupted.completed interruption = interrupted.cause @@ -590,7 +599,7 @@ final class SyncCoordinator: ObservableObject { } outcome.acceptMissingDeletions(of: deletions) - recordCache.store(Array(outcome.savedRecords.values)) + recordCache.store(outcome.savedBases(pushed: batch.editsByRecordID)) recordCache.remove(Array(outcome.deletedRecordIDs)) let savedRecords = settleSavedRecords(outcome, batch: batch, identities: identities, snapshot: snapshot) @@ -720,14 +729,20 @@ final class SyncCoordinator: ObservableObject { let deletedRecordIDs = result.deletedRecordIDs.filter { recordID in echoGuard?.withholdsDeletion(recordID, tracker: changeTracker) != true } + let pendingEdits = await recordNamesWithPendingEdits() + let admitted = PullResult( + changedRecords: result.changedRecords.filter { !pendingEdits.contains($0.recordID.recordName) }, + deletedRecordIDs: result.deletedRecordIDs, + newToken: result.newToken + ) let storesPersisted = applyRemoteChanges( - result, + admitted, deletedRecordIDs: deletedRecordIDs, settings: settings, echoGuard: echoGuard ) let favoritesOutcome = await services.sqlFavoriteManager.applyRemote( - remoteSQLFavoriteBatch(from: result, deletedRecordIDs: deletedRecordIDs, settings: settings), + remoteSQLFavoriteBatch(from: admitted, deletedRecordIDs: deletedRecordIDs, settings: settings), echoGuard: echoGuard ) @@ -745,8 +760,10 @@ final class SyncCoordinator: ObservableObject { metadataStorage.saveToken(newToken) } - recordCache.store(result.changedRecords.filter { record in + // Only what the local copy took: a record of a category this Mac does not sync stays out until a full fetch applies it. + recordCache.store(admitted.changedRecords.filter { record in echoGuard?.withholds(record.recordID, tracker: changeTracker) != true + && SyncRecordType(rawValue: record.recordType).map { settings.syncs($0) } == true }) recordCache.remove(result.deletedRecordIDs) @@ -756,6 +773,29 @@ final class SyncCoordinator: ObservableObject { return true } + // Left alone by a pull, a copy with an edit waiting to go up keeps it, and its push is refused and merges + // the edit onto the newer record. Built the way the push builds it, so only a real change counts; a + // connection merges on pull instead. + private func recordNamesWithPendingEdits() async -> Set { + let snapshot = changeTracker.editSnapshot() + guard !snapshot.dirty.isEmpty else { return [] } + let merged = SyncEchoGuard.typesMergedOnPull.map(\.rawValue) + let batch = await collectPushBatch( + snapshot: snapshot, + boundary: syncBoundary(settings: services.appSettingsStorage.loadSync()), + zoneID: await transport.currentZoneID + ) + var names = Set(batch.edits.lazy + .filter { $0.hasBase && $0.changesContent && !merged.contains($0.record.recordType) } + .map(\.recordID.recordName)) + // An edit made while the batch was read is newer than anything it saw. + for identity in changeTracker.editSnapshot().dirty + where !SyncEchoGuard.typesMergedOnPull.contains(identity.type) && changeTracker.hasEdit(identity, since: snapshot) { + names.insert(identity.type.recordName(for: identity.id)) + } + return names + } + // 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. @@ -907,25 +947,13 @@ final class SyncCoordinator: ObservableObject { @discardableResult private func mergeLocalEdits(into remoteRecord: CKRecord, localConnection: DatabaseConnection) -> DatabaseConnection? { guard let base = recordCache.record(for: remoteRecord.recordID) else { return nil } - let zoneID = remoteRecord.recordID.zoneID - - let local = SyncRecordMapper.keepingForeignRoute( - of: localConnection, - in: base, + let edit = SyncRecordMapper.connectionEdit( + localConnection, + in: remoteRecord.recordID.zoneID, + base: base, publisher: services.connectionStorage.sshRoutePublisherId ) - let localRecord = SyncRecordMapper.toCKRecord(local, in: zoneID) - // Written again the way this Mac writes, so iOS's spelling of a value this Mac never changed is not an edit. - let baseAsWritten = (try? SyncRecordMapper.toConnection(base)).map { SyncRecordMapper.toCKRecord($0, in: zoneID) } ?? base - guard let merged = remoteRecord.copy() as? CKRecord else { return nil } - - let localFields = localRecord.fields(ConnectionSyncField.self) - let baseFields = baseAsWritten.fields(ConnectionSyncField.self) - let mergedFields = merged.fields(ConnectionSyncField.self) - for field in ConnectionSyncField.allCases where field != .modifiedAtLocal { - guard !CKRecord.isEqualRecordValue(localFields[field], baseFields[field]) else { continue } - mergedFields[field] = localFields[field] - } + guard let merged = edit.rebased(onto: remoteRecord) else { return nil } do { return try SyncRecordMapper.toConnection(merged) diff --git a/apple/TablePro/Core/Sync/SyncRecordMapper.swift b/apple/TablePro/Core/Sync/SyncRecordMapper.swift index 1f454b547b..4d26ed9ae3 100644 --- a/apple/TablePro/Core/Sync/SyncRecordMapper.swift +++ b/apple/TablePro/Core/Sync/SyncRecordMapper.swift @@ -90,8 +90,8 @@ struct SyncRecordMapper { // MARK: - Connection - // Optional fields are written even when empty: under `.changedKeys` a key the record never names - // keeps the server's value, so an emptied group, tag or AI rule came back on the next pull. + // Optional fields are written even when empty: a key the record never names keeps the server's + // value, so an emptied group, tag or AI rule came back on the next pull. static func toCKRecord( _ connection: DatabaseConnection, in zone: CKRecordZone.ID, @@ -310,6 +310,41 @@ struct SyncRecordMapper { return kept } + // `base` is left untouched, so the edit is what the push changes on the server's copy, and the + // base read and written again keeps a value only iOS spells differently out of the edit. + static func connectionEdit( + _ connection: DatabaseConnection, + in zone: CKRecordZone.ID, + base: CKRecord?, + publisher: UUID + ) -> SyncRecordEdit { + let kept = keepingForeignRoute(of: connection, in: base, publisher: publisher) + let record = toCKRecord(kept, in: zone, base: base?.copy() as? CKRecord) + let baseAsWritten = base.flatMap { try? toConnection($0) }.map { toCKRecord($0, in: zone) } + return SyncRecordEdit(ConnectionSyncField.self, record: record, base: base, baseAsWritten: baseAsWritten) { merged, server in + settleRoute(in: merged, server: server, publisher: publisher) + } + } + + // An edit judged its route against an older copy, so the route lands on the server's by the rules a dial uses. + static func settleRoute(in merged: CKRecord, server: CKRecord, publisher: UUID) { + let mergedFields = merged.fields(ConnectionSyncField.self) + let serverFields = server.fields(ConnectionSyncField.self) + let current = sshRoute(in: serverFields) + guard rebasedRoute(wanted: sshRoute(in: mergedFields), server: current, publisher: publisher) == current else { return } + mergedFields[.sshRouteJson] = serverFields[.sshRouteJson] + } + + static func rebasedRoute( + wanted: PublishedSSHRoute?, + server: PublishedSSHRoute?, + publisher: UUID + ) -> PublishedSSHRoute? { + guard let server else { return wanted } + guard let wanted else { return server.publisher == publisher ? nil : server } + return server.leadsToSameHosts(as: wanted) ? server : wanted + } + /// A policy a newer release added reads as nil on this Mac. Writing that nil back would clear /// the other device's choice the next time this Mac saves any other change to the connection. private static func unrecognizedAIPolicy(in fields: SyncRecordFields) -> String? { @@ -336,9 +371,7 @@ struct SyncRecordMapper { // MARK: - Connection Group - /// Writes `parentId` and `iconName` whether or not they hold anything, for the reason the - /// connection mapper gives: a group dragged to the top level, or set back to the folder, has - /// to clear the server's value or the next pull restores it. + // `parentId` and `iconName` are written even when empty, or a group moved to the top level keeps its old parent. static func toCKRecord(_ group: ConnectionGroup, in zone: CKRecordZone.ID, base: CKRecord? = nil) -> CKRecord { let record = record(type: .group, id: group.id.uuidString, in: zone, base: base) @@ -381,9 +414,8 @@ struct SyncRecordMapper { // MARK: - Connection Tag - static func toCKRecord(_ tag: ConnectionTag, in zone: CKRecordZone.ID) -> CKRecord { - let recordID = recordID(type: .tag, id: tag.id.uuidString, in: zone) - let record = CKRecord(recordType: SyncRecordType.tag.rawValue, recordID: recordID) + static func toCKRecord(_ tag: ConnectionTag, in zone: CKRecordZone.ID, base: CKRecord? = nil) -> CKRecord { + let record = record(type: .tag, id: tag.id.uuidString, in: zone, base: base) let fields = record.fields(ConnectionTagSyncField.self) fields[.tagId] = tag.id.uuidString @@ -422,10 +454,10 @@ struct SyncRecordMapper { static func toCKRecord( category: String, settingsData: Data, - in zone: CKRecordZone.ID + in zone: CKRecordZone.ID, + base: CKRecord? = nil ) -> CKRecord { - let recordID = recordID(type: .settings, id: category, in: zone) - let record = CKRecord(recordType: SyncRecordType.settings.rawValue, recordID: recordID) + let record = record(type: .settings, id: category, in: zone, base: base) let fields = record.fields(AppSettingsSyncField.self) fields[.category] = category @@ -436,6 +468,44 @@ struct SyncRecordMapper { return record } + static func settingsEdit( + category: String, + settingsData: Data, + in zone: CKRecordZone.ID, + base: CKRecord? + ) -> SyncRecordEdit { + let record = toCKRecord(category: category, settingsData: settingsData, in: zone, base: base?.copy() as? CKRecord) + let baseData = base.flatMap(Self.settingsData(from:)) + // A base an older build wrote in another key order holds the same options, which is no edit. + let baseAsWritten = SyncJSONMerge.sameDocument(baseData, settingsData) ? record : nil + let mergesOptions = mergesOptions(of: category) + return SyncRecordEdit(AppSettingsSyncField.self, record: record, base: base, baseAsWritten: baseAsWritten) { merged, server in + guard mergesOptions, let document = SyncJSONMerge.document( + base: baseData, + local: settingsData, + server: Self.settingsData(from: server) + ) else { return } + merged.fields(AppSettingsSyncField.self)[.settingsJson] = category == CustomSlashCommandStorage.syncCategory + ? Self.renamingCommandCollisions(in: document) + : document + } + } + + // Only where options stand alone: a shortcut chord has to stay unique, the active AI provider has to be one + // of the providers, and a theme is one design, replaced whole on pull too. Those go up whole. + private static func mergesOptions(of category: String) -> Bool { + category != AppSettingsCategory.keyboard + && category != AppSettingsCategory.ai + && ThemeCatalog.themeId(forSyncCategory: category) == nil + } + + private static func renamingCommandCollisions(in document: Data) -> Data { + guard let commands = try? decoder.decode([CustomSlashCommand].self, from: document), + let renamed = try? encoder.encode(CustomSlashCommandStorage.renamingCollisions(commands)) + else { return document } + return renamed + } + static func settingsCategory(from record: CKRecord) -> String? { record.fields(AppSettingsSyncField.self)[.category] as? String } @@ -453,10 +523,10 @@ struct SyncRecordMapper { static func toCKRecord( favoriteEntry entry: FavoriteTablesStorage.FavoriteEntry, recordId favoriteId: String, - in zone: CKRecordZone.ID + in zone: CKRecordZone.ID, + base: CKRecord? = nil ) -> CKRecord { - let recordID = recordID(type: .tableFavorite, id: favoriteId, in: zone) - let record = CKRecord(recordType: SyncRecordType.tableFavorite.rawValue, recordID: recordID) + let record = record(type: .tableFavorite, id: favoriteId, in: zone, base: base) let fields = record.fields(FavoriteTableSyncField.self) fields[.connectionId] = entry.connectionId.uuidString @@ -494,10 +564,12 @@ struct SyncRecordMapper { // MARK: - Favorite Database - static func toCKRecord(favoriteDatabase entry: FavoriteDatabaseEntry, in zone: CKRecordZone.ID) -> CKRecord { - let favoriteId = FavoriteDatabasesStorage.syncId(for: entry) - let recordID = recordID(type: .favoriteDatabase, id: favoriteId, in: zone) - let record = CKRecord(recordType: SyncRecordType.favoriteDatabase.rawValue, recordID: recordID) + static func toCKRecord( + favoriteDatabase entry: FavoriteDatabaseEntry, + in zone: CKRecordZone.ID, + base: CKRecord? = nil + ) -> CKRecord { + let record = record(type: .favoriteDatabase, id: FavoriteDatabasesStorage.syncId(for: entry), in: zone, base: base) let fields = record.fields(FavoriteDatabaseSyncField.self) fields[.connectionId] = entry.connectionId.uuidString @@ -531,18 +603,11 @@ struct SyncRecordMapper { // MARK: - SQL Favorite - /// Writes the optional fields whether or not they hold anything. - /// - /// `CKModifyRecordsOperation.savePolicy` is `.changedKeys`, so a key the pushed record never - /// names keeps whatever the server has, and writing a field only when it was set therefore - /// meant a field the user emptied was never emptied anywhere else: marking a query Global - /// cleared `connectionId` locally, pushed a record that said nothing about it, and the next - /// pull put the old connection back. `absentValues: .clear` is what names the key, measured as - /// putting it in `changedKeys()` and leaving it out of `allKeys()` on a record built here. - static func toCKRecord(sqlFavorite favorite: SQLFavorite, in zone: CKRecordZone.ID) -> CKRecord { - let record = record(type: .favorite, id: favorite.id.uuidString, in: zone, base: nil) - - let fields = record.fields(SQLFavoriteSyncField.self, absentValues: .clear) + // Optional fields are written even when empty, or a query marked Global kept its old connection (#3045). + static func toCKRecord(sqlFavorite favorite: SQLFavorite, in zone: CKRecordZone.ID, base: CKRecord? = nil) -> CKRecord { + let record = record(type: .favorite, id: favorite.id.uuidString, in: zone, base: base) + + let fields = record.fields(SQLFavoriteSyncField.self, absentValues: record === base ? .leave : .clear) fields[.favoriteId] = favorite.id.uuidString fields[.name] = favorite.name fields[.query] = favorite.query @@ -584,12 +649,15 @@ struct SyncRecordMapper { // MARK: - SQL Favorite Folder - /// Writes the optional fields whether or not they hold anything, for the reason - /// `toCKRecord(sqlFavorite:in:)` gives. - static func toCKRecord(sqlFavoriteFolder folder: SQLFavoriteFolder, in zone: CKRecordZone.ID) -> CKRecord { - let record = record(type: .favoriteFolder, id: folder.id.uuidString, in: zone, base: nil) + // Optional fields are written even when empty, as for a query. + static func toCKRecord( + sqlFavoriteFolder folder: SQLFavoriteFolder, + in zone: CKRecordZone.ID, + base: CKRecord? = nil + ) -> CKRecord { + let record = record(type: .favoriteFolder, id: folder.id.uuidString, in: zone, base: base) - let fields = record.fields(SQLFavoriteFolderSyncField.self, absentValues: .clear) + let fields = record.fields(SQLFavoriteFolderSyncField.self, absentValues: record === base ? .leave : .clear) fields[.folderId] = folder.id.uuidString fields[.name] = folder.name fields[.parentId] = folder.parentId?.uuidString @@ -624,12 +692,11 @@ struct SyncRecordMapper { // MARK: - Table Folder - /// Writes `schema` whether or not it holds anything, for the reason `toCKRecord(sqlFavorite:in:)` - /// gives: a folder moved to a database with no schema must clear the one it had. - static func toCKRecord(tableFolder folder: TableFolder, in zone: CKRecordZone.ID) -> CKRecord { - let record = record(type: .tableFolder, id: folder.id.uuidString, in: zone, base: nil) + // `schema` is written even when empty, or a folder moved to a database with no schema keeps the old one. + static func toCKRecord(tableFolder folder: TableFolder, in zone: CKRecordZone.ID, base: CKRecord? = nil) -> CKRecord { + let record = record(type: .tableFolder, id: folder.id.uuidString, in: zone, base: base) - let fields = record.fields(TableFolderSyncField.self, absentValues: .clear) + let fields = record.fields(TableFolderSyncField.self, absentValues: record === base ? .leave : .clear) fields[.folderId] = folder.id.uuidString fields[.connectionId] = folder.scope.connectionId.uuidString fields[.database] = folder.scope.database @@ -666,10 +733,10 @@ struct SyncRecordMapper { ) } - static func toCKRecord(tableFolderItem item: TableFolderItem, in zone: CKRecordZone.ID) -> CKRecord { - let record = record(type: .tableFolderItem, id: item.syncId, in: zone, base: nil) + static func toCKRecord(tableFolderItem item: TableFolderItem, in zone: CKRecordZone.ID, base: CKRecord? = nil) -> CKRecord { + let record = record(type: .tableFolderItem, id: item.syncId, in: zone, base: base) - let fields = record.fields(TableFolderItemSyncField.self, absentValues: .clear) + let fields = record.fields(TableFolderItemSyncField.self, absentValues: record === base ? .leave : .clear) fields[.connectionId] = item.scope.connectionId.uuidString fields[.database] = item.scope.database fields[.schema] = item.scope.schema @@ -711,11 +778,10 @@ struct SyncRecordMapper { // MARK: - SSH Profile - static func toCKRecord(_ profile: SSHProfile, in zone: CKRecordZone.ID) -> CKRecord { - let recordID = recordID(type: .sshProfile, id: profile.id.uuidString, in: zone) - let record = CKRecord(recordType: SyncRecordType.sshProfile.rawValue, recordID: recordID) + static func toCKRecord(_ profile: SSHProfile, in zone: CKRecordZone.ID, base: CKRecord? = nil) -> CKRecord { + let record = record(type: .sshProfile, id: profile.id.uuidString, in: zone, base: base) - let fields = record.fields(SSHProfileSyncField.self, absentValues: .clear) + let fields = record.fields(SSHProfileSyncField.self, absentValues: record === base ? .leave : .clear) fields[.profileId] = profile.id.uuidString fields[.name] = profile.name fields[.host] = profile.host @@ -796,14 +862,11 @@ struct SyncRecordMapper { // MARK: - Credential Profile - /// The password never goes in, and neither does the payload of a `.source` mode: it can name a - /// shell command, and one that arrived over iCloud is a command this Mac never agreed to run. - /// A source-backed profile therefore travels as `prompt`. - static func toCKRecord(_ profile: CredentialProfile, in zone: CKRecordZone.ID) -> CKRecord { - let recordID = recordID(type: .credentialProfile, id: profile.id.uuidString, in: zone) - let record = CKRecord(recordType: SyncRecordType.credentialProfile.rawValue, recordID: recordID) + // No password, and no `.source` payload: it can name a shell command this Mac never agreed to run. + static func toCKRecord(_ profile: CredentialProfile, in zone: CKRecordZone.ID, base: CKRecord? = nil) -> CKRecord { + let record = record(type: .credentialProfile, id: profile.id.uuidString, in: zone, base: base) - let fields = record.fields(CredentialProfileSyncField.self, absentValues: .clear) + let fields = record.fields(CredentialProfileSyncField.self, absentValues: record === base ? .leave : .clear) fields[.profileId] = profile.id.uuidString fields[.name] = profile.name fields[.username] = profile.username diff --git a/apple/TablePro/Core/Sync/SyncTransport.swift b/apple/TablePro/Core/Sync/SyncTransport.swift index 981faad1a7..0192c05266 100644 --- a/apple/TablePro/Core/Sync/SyncTransport.swift +++ b/apple/TablePro/Core/Sync/SyncTransport.swift @@ -9,6 +9,7 @@ protocol SyncTransport: Sendable { func ensureZoneExists() async throws func push(records: [CKRecord], deletions: [CKRecord.ID]) async throws -> PushOutcome func pull(since token: CKServerChangeToken?) async throws -> PullResult + func fetch(recordIDs: [CKRecord.ID]) async throws -> [CKRecord.ID: CKRecord] } extension CloudKitSyncEngine: SyncTransport {} diff --git a/apple/TableProMobile/TableProMobile/Sync/IOSSyncCoordinator.swift b/apple/TableProMobile/TableProMobile/Sync/IOSSyncCoordinator.swift index e946eca161..9420079f9f 100644 --- a/apple/TableProMobile/TableProMobile/Sync/IOSSyncCoordinator.swift +++ b/apple/TableProMobile/TableProMobile/Sync/IOSSyncCoordinator.swift @@ -373,6 +373,7 @@ final class IOSSyncCoordinator { metadata.zoneState = .confirmed let changes = PullChanges(pulled) + let keptEdits = pendingEditRecordNames() let connCount = changes.changedConnections.count let groupCount = changes.changedGroups.count let tagCount = changes.changedTags.count @@ -390,18 +391,19 @@ final class IOSSyncCoordinator { if applied == .saved, let newToken = pulled.newToken { metadata.saveToken(newToken) } - // The connection writes landed, and a dirty connection pushed over a stale change tag would conflict. - recordCache.store(pulled.changedRecords) + cacheRecords(pulled.changedRecords, takenBy: applied, keptEdits: keptEdits) recordCache.remove(pulled.deletedRecordIDs) - settleLegacySSLModes() + settleLegacySSLModes(pulled: pulled.changedRecords) let heldDownload: SyncStepFailure? = applied == .saved ? nil : .pullNotSaved guard admission == .full else { return RunResult(admission: admission, failure: heldDownload) } - guard let pushFailure = await push(using: transport) else { - return RunResult(admission: admission, failure: heldDownload) + let pushed = await push(using: transport) + let download = heldDownload ?? (pushed.adoptedMergedSaves ? nil : .pullNotSaved) + guard let pushFailure = pushed.failure else { + return RunResult(admission: admission, failure: download) } let upload = await refined(pushFailure, using: transport) - return RunResult(admission: admission, failure: SyncStepFailure.decisive(upload: upload, download: heldDownload)) + return RunResult(admission: admission, failure: SyncStepFailure.decisive(upload: upload, download: download)) } private func fetchChanges(using transport: any IOSSyncTransport) async throws -> PullResult { @@ -602,16 +604,17 @@ final class IOSSyncCoordinator { // MARK: - Apply - // A copy an older iOS build stored lost the Mac's Preferred, which only the cached server record still holds: after a - // pull that record is the server's current one, and nothing fresher exists once the cache is about to go. - private func settleLegacySSLModes() { + // A copy an older iOS build stored lost the Mac's Preferred, which only the server's record still holds: the one this + // pull brought, else the cached one, and nothing fresher exists once the cache is about to go. + private func settleLegacySSLModes(pulled: [CKRecord] = []) { + let latest = Dictionary(pulled.map { ($0.recordID.recordName, $0) }, uniquingKeysWith: { _, last in last }) guard var connections = getCurrentState?()?.connections else { return } var changed = false for index in connections.indices { guard let local = connections[index].sslConfiguration, local.isLegacyRequire else { continue } let recordName = SyncRecordType.connection.recordName(for: connections[index].id.uuidString) // Only Preferred or Required is in question, so certificate paths edited on either side do not matter. - guard let cached = recordCache.record(named: recordName), + guard let cached = latest[recordName] ?? recordCache.record(named: recordName), let server = SyncRecordMapper.serverSSL(in: cached), server.mode == local.mode else { continue } connections[index].sslConfiguration?.settleLegacyRequire(asMacPreferred: server.isMacPreferred) @@ -687,44 +690,60 @@ final class IOSSyncCoordinator { // MARK: - Push + private struct PushResult { + var failure: SyncStepFailure? + // False when a merged save could not be written here, which holds the run like a pull that was not saved. + var adoptedMergedSaves = true + } + /// Whatever went through is settled even when a later batch threw, so a saved record is not /// sent again and a cached record does not stand in with a stale change tag. - private func push(using transport: any IOSSyncTransport) async -> SyncStepFailure? { + private func push(using transport: any IOSSyncTransport) async -> PushResult { let zoneID = await transport.currentZoneID - guard let state = getCurrentState?() else { return nil } + guard let state = getCurrentState?() else { return PushResult() } let generationsAtPush = editGenerations let batch = pushBatch(from: state, savedQueries: getSavedQueries?(), in: zoneID) - guard !batch.records.isEmpty || !batch.deletions.isEmpty else { return nil } + guard !batch.edits.isEmpty || !batch.deletions.isEmpty else { return PushResult() } + let edits = Dictionary(batch.edits.map { ($0.recordID, $0) }, uniquingKeysWith: { _, last in last }) var outcome: PushOutcome var interruption: (any Error)? do { - outcome = try await transport.push(records: batch.records, deletions: batch.deletions) + outcome = try await SyncConflictRebase.push( + records: batch.edits.map(\.record), + deletions: batch.deletions, + edits: edits + ) { + try await transport.push(records: $0, deletions: $1) + } fetch: { + try await transport.fetch(recordIDs: $0) + } } catch let interrupted as SyncPushInterruption { outcome = interrupted.completed interruption = interrupted.cause } catch { Self.logger.error("Push failed: \(error.localizedDescription, privacy: .private)") - return SyncStepFailure(error) + return PushResult(failure: SyncStepFailure(error)) } outcome.acceptMissingDeletions(of: batch.deletions) - settle(outcome, generationsAtPush: generationsAtPush) + let adopted = settle(outcome, edits: edits, generationsAtPush: generationsAtPush) let itemFailure = SyncStepFailure(outcome) if let interruption { Self.logger.error("Push stopped part way: \(interruption.localizedDescription, privacy: .private)") /// A blocker an earlier batch hit explains the interruption too; anything less does not. - return itemFailure.flatMap { $0.failure.stopsUpload ? $0 : nil } ?? SyncStepFailure(interruption) + let failure = itemFailure.flatMap { $0.failure.stopsUpload ? $0 : nil } ?? SyncStepFailure(interruption) + return PushResult(failure: failure, adoptedMergedSaves: adopted) } if let itemFailure { let pending = outcome.failures.count Self.logger.error("Push left \(pending, privacy: .public) items pending: \(String(describing: itemFailure.failure), privacy: .public)") } - return itemFailure + return PushResult(failure: itemFailure, adoptedMergedSaves: adopted) } - private func settle(_ outcome: PushOutcome, generationsAtPush: [EditKey: Int]) { - recordCache.store(Array(outcome.savedRecords.values)) + private func settle(_ outcome: PushOutcome, edits: [CKRecord.ID: SyncRecordEdit], generationsAtPush: [EditKey: Int]) -> Bool { + recordCache.store(outcome.savedBases(pushed: edits)) recordCache.remove(Array(outcome.deletedRecordIDs)) for recordID in outcome.savedRecords.keys { @@ -739,53 +758,79 @@ final class IOSSyncCoordinator { guard let parsed = SyncRecordMapper.parse(recordName: recordID.recordName) else { continue } metadata.removeTombstone(parsed.id, type: parsed.type) } + + return adoptRebasedSaves(outcome) + } + + // A rebased save holds another device's change too, which the local copy takes as from a pull. + private func adoptRebasedSaves(_ outcome: PushOutcome) -> Bool { + let rebased = outcome.savedRecords.filter { outcome.rebasedRecordIDs.contains($0.key) }.map(\.value) + guard !rebased.isEmpty else { return true } + let keptEdits = pendingEditRecordNames() + let pulled = PullResult(changedRecords: rebased, deletedRecordIDs: [], newToken: nil) + let applied = applyRemoteChanges(PullChanges(pulled)) + guard applied != .notSaved else { + Self.logger.error("A merged save was not applied here, so the next pull brings it") + return false + } + cacheRecords(rebased, takenBy: applied, keptEdits: keptEdits) + return applied == .saved + } + + // The cache holds only what the local copy holds: a copy that kept its edit keeps the record the edit + // was made on, so its push is refused and merged instead of restating fields it never changed. + private func cacheRecords(_ records: [CKRecord], takenBy applied: RemoteApply, keptEdits: Set) { + let savedQueryTypes = [SyncRecordType.favorite, .favoriteFolder].map(\.rawValue) + recordCache.store(records.filter { record in + !keptEdits.contains(record.recordID.recordName) + && (applied == .saved || !savedQueryTypes.contains(record.recordType)) + }) + } + + // With no cached record there is no base to keep, and the pulled one serves as well as any. + private func pendingEditRecordNames() -> Set { + var names: Set = [] + for type in [SyncRecordType.connection, .group, .tag, .favorite, .favoriteFolder] { + for id in metadata.dirtyIds(for: type).union(tombstonedIds(for: type)) { + let name = type.recordName(for: id) + if recordCache.holdsRecord(named: name) { + names.insert(name) + } + } + } + return names } private func pushBatch( from state: LibraryState, savedQueries: SavedQueryContents?, in zoneID: CKRecordZone.ID - ) -> (records: [CKRecord], deletions: [CKRecord.ID]) { - var records: [CKRecord] = [] + ) -> (edits: [SyncRecordEdit], deletions: [CKRecord.ID]) { + var edits: [SyncRecordEdit] = [] var deletions: [CKRecord.ID] = [] let dirtyConnIDs = metadata.dirtyIds(for: .connection) for connection in state.connections where connection.participatesInSync && dirtyConnIDs.contains(connection.id.uuidString) { - let recordID = SyncRecordMapper.recordID(type: .connection, id: connection.id.uuidString, in: zoneID) - if let existing = recordCache.record(for: recordID) { - SyncRecordMapper.updateRecord(existing, with: connection) - records.append(existing) - } else { - records.append(SyncRecordMapper.toRecord(connection, zoneID: zoneID)) - } + let base = cachedRecord(.connection, id: connection.id.uuidString, in: zoneID) + edits.append(SyncRecordMapper.edit(connection, base: base, zoneID: zoneID)) } let dirtyGroupIDs = metadata.dirtyIds(for: .group) for group in state.groups where dirtyGroupIDs.contains(group.id.uuidString) { - let recordID = SyncRecordMapper.recordID(type: .group, id: group.id.uuidString, in: zoneID) - if let existing = recordCache.record(for: recordID) { - SyncRecordMapper.updateRecord(existing, with: group) - records.append(existing) - } else { - records.append(SyncRecordMapper.toRecord(group, zoneID: zoneID)) - } + let base = cachedRecord(.group, id: group.id.uuidString, in: zoneID) + edits.append(SyncRecordMapper.edit(group, base: base, zoneID: zoneID)) } let dirtyTagIDs = metadata.dirtyIds(for: .tag) for tag in state.tags where dirtyTagIDs.contains(tag.id.uuidString) { - let recordID = SyncRecordMapper.recordID(type: .tag, id: tag.id.uuidString, in: zoneID) - if let existing = recordCache.record(for: recordID) { - SyncRecordMapper.updateRecord(existing, with: tag) - records.append(existing) - } else { - records.append(SyncRecordMapper.toRecord(tag, zoneID: zoneID)) - } + let base = cachedRecord(.tag, id: tag.id.uuidString, in: zoneID) + edits.append(SyncRecordMapper.edit(tag, base: base, zoneID: zoneID)) } var deletedTypes: [SyncRecordType] = [.connection, .group, .tag] if let savedQueries { - records += savedQueryRecords(savedQueries, connections: state.connections, in: zoneID) + edits += savedQueryEdits(savedQueries, connections: state.connections, in: zoneID) deletedTypes += [.favorite, .favoriteFolder] } @@ -795,24 +840,27 @@ final class IOSSyncCoordinator { } } - return (records, deletions) + return (edits, deletions) + } + + private func cachedRecord(_ type: SyncRecordType, id: String, in zoneID: CKRecordZone.ID) -> CKRecord? { + recordCache.record(for: SyncRecordMapper.recordID(type: type, id: id, in: zoneID)) } - // Built fresh, not from the cache: `.changedKeys` clears a keyword, folder or connection only when the key is named. - private func savedQueryRecords( + private func savedQueryEdits( _ contents: SavedQueryContents, connections: [DatabaseConnection], in zoneID: CKRecordZone.ID - ) -> [CKRecord] { + ) -> [SyncRecordEdit] { let syncs = Self.ownerSyncs(in: connections) let dirtyFolderIds = metadata.dirtyIds(for: .favoriteFolder) let dirtyQueryIds = metadata.dirtyIds(for: .favorite) let folders = contents.folders .filter { dirtyFolderIds.contains($0.id.uuidString) && syncs($0.connectionId) } - .map { SyncRecordMapper.toRecord($0, zoneID: zoneID) } + .map { SyncRecordMapper.edit($0, base: cachedRecord(.favoriteFolder, id: $0.id.uuidString, in: zoneID), zoneID: zoneID) } let queries = contents.queries .filter { dirtyQueryIds.contains($0.id.uuidString) && syncs($0.connectionId) } - .map { SyncRecordMapper.toRecord($0, zoneID: zoneID) } + .map { SyncRecordMapper.edit($0, base: cachedRecord(.favorite, id: $0.id.uuidString, in: zoneID), zoneID: zoneID) } return folders + queries } diff --git a/apple/TableProMobile/TableProMobile/Sync/IOSSyncTransport.swift b/apple/TableProMobile/TableProMobile/Sync/IOSSyncTransport.swift index d3fc8aab4b..13e86f88b2 100644 --- a/apple/TableProMobile/TableProMobile/Sync/IOSSyncTransport.swift +++ b/apple/TableProMobile/TableProMobile/Sync/IOSSyncTransport.swift @@ -9,6 +9,7 @@ nonisolated protocol IOSSyncTransport: Sendable { func ensureZoneExists() async throws func pull(since token: CKServerChangeToken?) async throws -> PullResult func push(records: [CKRecord], deletions: [CKRecord.ID]) async throws -> PushOutcome + func fetch(recordIDs: [CKRecord.ID]) async throws -> [CKRecord.ID: CKRecord] } extension CloudKitSyncEngine: IOSSyncTransport {} diff --git a/apple/TableProMobile/TableProMobileTests/Mocks/RecordingSyncTransport.swift b/apple/TableProMobile/TableProMobileTests/Mocks/RecordingSyncTransport.swift index 413b50afe8..fa68820f00 100644 --- a/apple/TableProMobile/TableProMobileTests/Mocks/RecordingSyncTransport.swift +++ b/apple/TableProMobile/TableProMobileTests/Mocks/RecordingSyncTransport.swift @@ -43,6 +43,10 @@ actor RecordingSyncTransport: IOSSyncTransport { return PullResult(changedRecords: remoteRecords, deletedRecordIDs: remoteDeletions, newToken: newToken) } + func fetch(recordIDs: [CKRecord.ID]) async throws -> [CKRecord.ID: CKRecord] { + [:] + } + func push(records: [CKRecord], deletions: [CKRecord.ID]) async throws -> PushOutcome { pushedRecords.append(contentsOf: records) pushedDeletions.append(contentsOf: deletions) diff --git a/apple/TableProMobile/TableProMobileTests/Mocks/UnreachableSyncTransport.swift b/apple/TableProMobile/TableProMobileTests/Mocks/UnreachableSyncTransport.swift index 371d6c9dd3..69ff123e1f 100644 --- a/apple/TableProMobile/TableProMobileTests/Mocks/UnreachableSyncTransport.swift +++ b/apple/TableProMobile/TableProMobileTests/Mocks/UnreachableSyncTransport.swift @@ -26,4 +26,8 @@ struct UnreachableSyncTransport: IOSSyncTransport { func push(records: [CKRecord], deletions: [CKRecord.ID]) async throws -> PushOutcome { PushOutcome(savedRecords: [:], deletedRecordIDs: []) } + + func fetch(recordIDs: [CKRecord.ID]) async throws -> [CKRecord.ID: CKRecord] { + throw CKError(.notAuthenticated) + } } diff --git a/apple/TableProMobile/TableProMobileTests/Sync/IOSSyncCoordinatorTests.swift b/apple/TableProMobile/TableProMobileTests/Sync/IOSSyncCoordinatorTests.swift index a26f3c8767..801413e2e2 100644 --- a/apple/TableProMobile/TableProMobileTests/Sync/IOSSyncCoordinatorTests.swift +++ b/apple/TableProMobile/TableProMobileTests/Sync/IOSSyncCoordinatorTests.swift @@ -44,6 +44,7 @@ private actor FakeSyncTransport: IOSSyncTransport { private var pullErrors: [any Error] = [] private var interruption: (savedCount: Int, cause: CKError)? private var newToken: CKServerChangeToken? + private var newerServerCopies: [CKRecord.ID: CKRecord] = [:] private(set) var pushedRecords: [CKRecord] = [] private(set) var pushedDeletions: [CKRecord.ID] = [] private(set) var pushCount = 0 @@ -104,6 +105,15 @@ private actor FakeSyncTransport: IOSSyncTransport { newToken = token } + // The next save of that record is refused as CloudKit refuses one built on an older copy. + func holdNewerCopy(_ server: CKRecord) { + newerServerCopies[server.recordID] = server + } + + func fetch(recordIDs: [CKRecord.ID]) async throws -> [CKRecord.ID: CKRecord] { + [:] + } + func accountStatus() async throws -> CKAccountStatus { let status = accountStatuses.first ?? .available if accountStatuses.count > 1 { @@ -148,15 +158,26 @@ private actor FakeSyncTransport: IOSSyncTransport { return PushOutcome(failures: Dictionary(uniqueKeysWithValues: ids.map { ($0, itemFailure) })) } let missing = deletions.filter { recordsTheServerNeverHad.contains($0.recordName) } + var refused: [CKRecord.ID: SyncItemFailure] = [:] + for record in records { + guard let server = newerServerCopies.removeValue(forKey: record.recordID) else { continue } + refused[record.recordID] = SyncItemFailure( + code: .serverRecordChanged, + serverRecord: server, + clientRecord: record, + message: "Server record changed" + ) + } + let saved = records.filter { refused[$0.recordID] == nil } return PushOutcome( - savedRecords: Dictionary(uniqueKeysWithValues: records.map { ($0.recordID, $0) }), + savedRecords: Dictionary(uniqueKeysWithValues: saved.map { ($0.recordID, $0) }), deletedRecordIDs: Set(deletions).subtracting(missing), - failures: Dictionary(uniqueKeysWithValues: missing.map { recordID in + failures: refused.merging(missing.map { recordID in ( recordID, SyncItemFailure(code: .unknownItem, serverRecord: nil, clientRecord: nil, message: "Record not found") ) - }) + }, uniquingKeysWith: { first, _ in first }) ) } } @@ -1415,6 +1436,121 @@ struct IOSSyncCoordinatorTests { #expect(await transport.pushedDeletions.count == 2) } + private func copy(_ record: CKRecord) throws -> CKRecord { + try #require(record.copy() as? CKRecord) + } + + @Test("A copy with an edit waiting to go up sends only that edit, keeping what the Mac changed") + func dirtyCopyKeepsTheMacsChange() async throws { + let box = LibraryStateBox() + let local = DatabaseConnection(name: "Production", type: .postgresql, host: "db", port: 5_432) + box.connections = [local] + let cached = SyncRecordMapper.toRecord(local, zoneID: zoneID) + SyncRecordCache(directory: cacheDirectory, defaults: nil).store([cached]) + let server = try copy(cached) + server.fields(ConnectionSyncField.self)[.name] = "Production (Mac)" + let transport = FakeSyncTransport(remoteRecords: [server], box: box) + await transport.holdNewerCopy(server) + let coordinator = makeCoordinator(box: box, transport: transport) + box.connections[0].port = 6_543 + coordinator.markDirty(local.id) + + await coordinator.sync() + + let pushed = try #require(await transport.pushedRecords.last { $0.recordID == cached.recordID }) + let sent = try #require(SyncRecordMapper.toConnection(pushed)) + #expect(sent.name == "Production (Mac)") + #expect(sent.port == 6_543) + let stored = try #require(box.connections.first) + #expect(stored.name == "Production (Mac)") + #expect(stored.port == 6_543) + #expect(coordinator.status.error == nil) + } + + @Test("A field set back to its old value while a merged save is in flight reaches iCloud on the next sync") + func undoDuringMergedSaveIsPushed() async throws { + let box = LibraryStateBox() + let local = DatabaseConnection(name: "Production", type: .postgresql, host: "db", port: 5_432) + box.connections = [local] + let cached = SyncRecordMapper.toRecord(local, zoneID: zoneID) + SyncRecordCache(directory: cacheDirectory, defaults: nil).store([cached]) + let server = try copy(cached) + server.fields(ConnectionSyncField.self)[.name] = "Production (Mac)" + let transport = FakeSyncTransport(remoteRecords: [server], box: box) + await transport.holdNewerCopy(server) + let coordinator = makeCoordinator(box: box, transport: transport) + box.connections[0].port = 6_543 + coordinator.markDirty(local.id) + var undone = false + box.duringPush = { + guard !undone else { return } + undone = true + box.connections[0].port = 5_432 + coordinator.markDirty(local.id) + } + + await coordinator.sync() + #expect(box.connections.first?.port == 5_432) + + let merged = try #require(await transport.pushedRecords.last { $0.recordID == cached.recordID }) + await transport.holdNewerCopy(merged) + await coordinator.sync() + + let pushed = try #require(await transport.pushedRecords.last { $0.recordID == cached.recordID }) + let sent = try #require(SyncRecordMapper.toConnection(pushed)) + #expect(sent.name == "Production (Mac)") + #expect(sent.port == 5_432) + #expect(box.connections.first?.port == 5_432) + #expect(box.connections.first?.name == "Production (Mac)") + } + + @Test("A merged save this device could not store holds the run like a pull that was not saved") + func unstoredMergedSaveIsReported() async throws { + let box = LibraryStateBox() + let local = DatabaseConnection(name: "Production", type: .postgresql, host: "db", port: 5_432) + box.connections = [local] + let cached = SyncRecordMapper.toRecord(local, zoneID: zoneID) + SyncRecordCache(directory: cacheDirectory, defaults: nil).store([cached]) + let server = try copy(cached) + server.fields(ConnectionSyncField.self)[.name] = "Production (Mac)" + let transport = FakeSyncTransport(remoteRecords: [server], box: box) + await transport.holdNewerCopy(server) + let coordinator = makeCoordinator(box: box, transport: transport) + box.connections[0].port = 6_543 + coordinator.markDirty(local.id) + box.duringPush = { box.saves = false } + + await coordinator.sync() + + #expect(coordinator.status == .error(.pullNotSaved)) + } + + @Test("A saved query edited here keeps the name another device gave it") + func dirtySavedQueryKeepsAnotherDevicesName() async throws { + let box = LibraryStateBox() + let query = SavedQuery(name: "Daily", query: "SELECT 1") + box.savedQueries = SavedQueryContents(queries: [query]) + let cached = SyncRecordMapper.toRecord(query, zoneID: zoneID) + SyncRecordCache(directory: cacheDirectory, defaults: nil).store([cached]) + let server = try copy(cached) + server.fields(SQLFavoriteSyncField.self)[.name] = "Daily report" + let transport = FakeSyncTransport(remoteRecords: [server], box: box) + await transport.holdNewerCopy(server) + let coordinator = makeCoordinator(box: box, transport: transport) + box.savedQueries?.queries[0].query = "SELECT 2" + coordinator.markDirty(savedQueries: [query.id]) + + await coordinator.sync() + + let pushed = try #require(await transport.pushedRecords.last { $0.recordID == cached.recordID }) + let sent = try #require(SyncRecordMapper.toSavedQuery(pushed)) + #expect(sent.name == "Daily report") + #expect(sent.query == "SELECT 2") + let stored = try #require(box.savedQueries?.queries.first) + #expect(stored.name == "Daily report") + #expect(stored.query == "SELECT 2") + } + private static func makeChangeToken() throws -> CKServerChangeToken { let archiver = NSKeyedArchiver(requiringSecureCoding: true) archiver.setClassName("CKServerChangeToken", for: ChangeTokenArchive.self) diff --git a/apple/TableProMobile/TableProMobileTests/Sync/SyncedWriteFailureTests.swift b/apple/TableProMobile/TableProMobileTests/Sync/SyncedWriteFailureTests.swift index c1b30e9314..cde864d935 100644 --- a/apple/TableProMobile/TableProMobileTests/Sync/SyncedWriteFailureTests.swift +++ b/apple/TableProMobile/TableProMobileTests/Sync/SyncedWriteFailureTests.swift @@ -61,7 +61,7 @@ struct SyncedWriteFailureTests { #expect(savedToken != nil) } - @Test("A pulled saved query that could not be saved holds only the token: the cache moves and connections go up") + @Test("A pulled saved query that could not be saved holds the token and stays out of the cache, and connections go up") func unsavedSavedQueryHoldsOnlyTheToken() async throws { let query = SavedQuery(name: "One", query: "SELECT 1") let record = SyncRecordMapper.toRecord(query, zoneID: zoneID) @@ -80,7 +80,7 @@ struct SyncedWriteFailureTests { let pushed = await transport.pushedRecords.compactMap(SyncRecordMapper.toConnection).map(\.id) #expect(pushed == [local.id]) let cache = SyncRecordCache(directory: fixture.root.appendingPathComponent("Cache"), defaults: nil) - #expect(cache.record(for: record.recordID) != nil) + #expect(cache.record(for: record.recordID) == nil) try setLibraryWritable(true) await state.syncCoordinator.sync() diff --git a/apple/TableProTests/Core/Sync/SyncCategoryRefetchTests.swift b/apple/TableProTests/Core/Sync/SyncCategoryRefetchTests.swift index ea96a263cc..52a9dc34cf 100644 --- a/apple/TableProTests/Core/Sync/SyncCategoryRefetchTests.swift +++ b/apple/TableProTests/Core/Sync/SyncCategoryRefetchTests.swift @@ -192,6 +192,10 @@ private actor ZoneServer: SyncTransport { return outcome } + func fetch(recordIDs: [CKRecord.ID]) async throws -> [CKRecord.ID: CKRecord] { + [:] + } + func pull(since token: CKServerChangeToken?) async throws -> PullResult { pulls.append(token == nil ? .full : .delta) duringPull() diff --git a/apple/TableProTests/Core/Sync/SyncConflictRebaseTests.swift b/apple/TableProTests/Core/Sync/SyncConflictRebaseTests.swift new file mode 100644 index 0000000000..a3fc339e00 --- /dev/null +++ b/apple/TableProTests/Core/Sync/SyncConflictRebaseTests.swift @@ -0,0 +1,415 @@ +import CloudKit +import Foundation +@testable import TablePro +@testable import TableProSSHConfig +import TableProSSHTransport +import TableProSyncTransport +import Testing + +@MainActor +private final class UndoOnce { + var pending = true +} + +@MainActor +struct SyncConflictRebaseTests { + private static let zoneID = SyncTestEnvironment.zoneID + private let environment: SyncTestEnvironment + + init() throws { + environment = try SyncTestEnvironment(label: "sync-conflict-rebase") + } + + private func copy(_ record: CKRecord) throws -> CKRecord { + try #require(record.copy() as? CKRecord) + } + + private func lastPush(of recordID: CKRecord.ID, in transport: ScriptedSyncTransport) async throws -> CKRecord { + try #require(await transport.pushedRecords.last { $0.recordID == recordID }) + } + + // The latest push of each record, as a pull after the push returns it. + nonisolated private static func latestPushes(_ records: [CKRecord]) -> PullResult { + let latest = Dictionary(records.map { ($0.recordID, $0) }, uniquingKeysWith: { _, last in last }) + return PullResult(changedRecords: Array(latest.values), deletedRecordIDs: [], newToken: nil) + } + + private static func tunnelled() -> DatabaseConnection { + DatabaseConnection(name: "Alias", sshConfig: SSHConfiguration(enabled: true, host: "db", authMethod: .sshAgent)) + } + + private static func route(of connection: DatabaseConnection, publisher: UUID, hop: String) -> PublishedSSHRoute { + PublishedSSHRoute( + publisher: publisher, + input: connection.sshConfig.publishedRouteInput, + hops: [.init(id: UUID(), alias: "bastion", host: hop, port: 22, username: "jump")], + target: .init(alias: "db", host: "10.0.0.5", port: 22, username: "deploy") + ) + } + + // This Mac published `own`, cached the record, then cleared its route; another Mac published a route since. + private func clearedOwnRouteAfterAnotherMacPublished() throws -> (cached: CKRecord, server: CKRecord, other: PublishedSSHRoute) { + var connection = Self.tunnelled() + connection.sshRoute = Self.route(of: connection, publisher: environment.connections.sshRoutePublisherId, hop: "old.example") + #expect(environment.connections.addConnection(connection)) + let stored = try #require(environment.connections.loadConnection(id: connection.id)) + let cached = SyncRecordMapper.toCKRecord(stored, in: Self.zoneID) + environment.recordCache.store([cached]) + + let other = Self.route(of: connection, publisher: UUID(), hop: "new.example") + let server = try copy(cached) + server.fields(ConnectionSyncField.self)[.sshRouteJson] = other.encoded() + #expect(environment.connections.mutateConnections(ids: [connection.id], byUser: false) { $0.sshRoute = nil }) + return (cached, server, other) + } + + @Test("A Mac clearing its own route keeps the route another Mac published after its copy") + func staleClearKeepsAnotherMacsRoute() async throws { + let (cached, server, other) = try clearedOwnRouteAfterAnotherMacPublished() + let transport = ScriptedSyncTransport(zoneID: Self.zoneID) + await transport.holdNewerCopy(server, of: cached.recordID) + + #expect(await environment.makeCoordinator(transport: transport).runSyncCycle() == nil) + + let pushed = try await lastPush(of: cached.recordID, in: transport) + #expect(pushed[ConnectionSyncField.sshRouteJson.key] as? Data == other.encoded()) + #expect(await transport.pushCount == 2) + } + + @Test("A copy whose push failed keeps another Mac's route when it merges the pull") + func pullMergeKeepsAnotherMacsRoute() async throws { + let (cached, server, other) = try clearedOwnRouteAfterAnotherMacPublished() + let transport = ScriptedSyncTransport( + zoneID: Self.zoneID, + rejecting: [cached.recordID], + pulled: { _ in PullResult(changedRecords: [server], deletedRecordIDs: [], newToken: nil) } + ) + + _ = await environment.makeCoordinator(transport: transport).runSyncCycle() + + let parsed = try #require(SyncRecordMapper.parse(recordName: cached.recordID.recordName)) + let connectionId = try #require(UUID(uuidString: parsed.id)) + #expect(environment.connections.loadConnection(id: connectionId)?.sshRoute == other) + } + + @Test("A rename from a stale copy keeps the tunnel another device changed, however that device spelled it") + func staleRenameKeepsAnotherDevicesTunnel() async throws { + let connection = DatabaseConnection( + name: "Production", + sshConfig: SSHConfiguration(enabled: true, host: "bastion", authMethod: .sshAgent) + ) + #expect(environment.connections.addConnection(connection)) + let stored = try #require(environment.connections.loadConnection(id: connection.id)) + let otherSpelling = JSONEncoder() + otherSpelling.outputFormatting = .prettyPrinted + let cached = SyncRecordMapper.toCKRecord(stored, in: Self.zoneID) + cached.fields(ConnectionSyncField.self)[.sshConfigJson] = try otherSpelling.encode(stored.sshConfig) + environment.recordCache.store([cached]) + + var moved = stored.sshConfig + moved.host = "bastion-eu" + let server = try copy(cached) + server.fields(ConnectionSyncField.self)[.sshConfigJson] = try otherSpelling.encode(moved) + #expect(environment.connections.mutateConnections(ids: [connection.id]) { $0.name = "Production EU" }) + let transport = ScriptedSyncTransport(zoneID: Self.zoneID) + await transport.holdNewerCopy(server, of: cached.recordID) + + #expect(await environment.makeCoordinator(transport: transport).runSyncCycle() == nil) + + let pushed = try SyncRecordMapper.toConnection(try await lastPush(of: cached.recordID, in: transport)) + #expect(pushed.name == "Production EU") + #expect(pushed.sshConfig.host == "bastion-eu") + } + + @Test("A tag recoloured from a stale copy keeps the name another device gave it, here and in iCloud") + func staleTagEditKeepsAnotherDevicesName() async throws { + let tag = ConnectionTag(name: "prod", color: .red) + try environment.tags.addTag(tag) + let cached = SyncRecordMapper.toCKRecord(tag, in: Self.zoneID) + environment.recordCache.store([cached]) + let server = try copy(cached) + server.fields(ConnectionTagSyncField.self)[.name] = "production" + var recoloured = tag + recoloured.color = .blue + #expect(environment.tags.saveTags([recoloured])) + let transport = ScriptedSyncTransport(zoneID: Self.zoneID, echoing: { records, _ in Self.latestPushes(records) }) + await transport.holdNewerCopy(server, of: cached.recordID) + + #expect(await environment.makeCoordinator(transport: transport).runSyncCycle() == nil) + + let pushed = try #require(SyncRecordMapper.toTag(try await lastPush(of: cached.recordID, in: transport))) + #expect(pushed.name == "production") + #expect(pushed.color == .blue) + let local = try #require(environment.tags.tag(for: tag.id)) + #expect(local.name == "production") + #expect(local.color == .blue) + } + + @Test("A refused edit no run could merge stays on this Mac and merges on the next push") + func unmergedRefusalKeepsTheEdit() async throws { + let tag = ConnectionTag(name: "prod", color: .red) + try environment.tags.addTag(tag) + let cached = SyncRecordMapper.toCKRecord(tag, in: Self.zoneID) + environment.recordCache.store([cached]) + let server = try copy(cached) + server.fields(ConnectionTagSyncField.self)[.name] = "production" + var recoloured = tag + recoloured.color = .blue + #expect(environment.tags.saveTags([recoloured])) + let transport = ScriptedSyncTransport( + zoneID: Self.zoneID, + pulled: { _ in PullResult(changedRecords: [server], deletedRecordIDs: [], newToken: nil) } + ) + await transport.holdNewerCopy(nil, of: cached.recordID) + let coordinator = environment.makeCoordinator(transport: transport) + + _ = await coordinator.runSyncCycle() + + #expect(environment.tags.tag(for: tag.id)?.color == .blue) + #expect(environment.tracker.dirtyRecords(for: .tag).contains(tag.id.uuidString)) + + await transport.holdNewerCopy(server, of: cached.recordID) + _ = await coordinator.runSyncCycle() + + let pushed = try #require(SyncRecordMapper.toTag(try await lastPush(of: cached.recordID, in: transport))) + #expect(pushed.name == "production") + #expect(pushed.color == .blue) + } + + @Test("Options changed on two Macs in one settings category are both kept") + func settingsOptionsMerge() throws { + let base = SyncRecordMapper.toCKRecord(category: "dataGrid", settingsData: Data(#"{"a":1,"b":1}"#.utf8), in: Self.zoneID) + let server = try copy(base) + server.fields(AppSettingsSyncField.self)[.settingsJson] = Data(#"{"a":2,"b":1}"#.utf8) + + let edit = SyncRecordMapper.settingsEdit( + category: "dataGrid", + settingsData: Data(#"{"a":1,"b":2}"#.utf8), + in: Self.zoneID, + base: base + ) + + let merged = try #require(edit.rebased(onto: server)) + let blob = try #require(SyncRecordMapper.settingsData(from: merged)) + let document = try JSONSerialization.jsonObject(with: blob) as? NSDictionary + #expect(document == ["a": 2, "b": 2]) + } + + @Test( + "Shortcuts, AI providers and a theme both Macs changed go up whole from this Mac, never mixed", + arguments: [AppSettingsCategory.keyboard, AppSettingsCategory.ai, ThemeCatalog.syncCategory(for: "ocean")] + ) + func wholeCategoriesAreNotMixed(category: String) throws { + let base = SyncRecordMapper.toCKRecord(category: category, settingsData: Data(#"{"a":1,"b":1}"#.utf8), in: Self.zoneID) + let server = try copy(base) + server.fields(AppSettingsSyncField.self)[.settingsJson] = Data(#"{"a":2,"b":1}"#.utf8) + let mine = Data(#"{"a":1,"b":2}"#.utf8) + + let edit = SyncRecordMapper.settingsEdit(category: category, settingsData: mine, in: Self.zoneID, base: base) + + let merged = try #require(edit.rebased(onto: server)) + #expect(SyncRecordMapper.settingsData(from: merged) == mine) + } + + @Test("A settings category an older build cached in another key order is no pending edit") + func respelledSettingsAreNoEdit() { + let base = SyncRecordMapper.toCKRecord(category: "dataGrid", settingsData: Data(#"{"b":1,"a":2}"#.utf8), in: Self.zoneID) + + let edit = SyncRecordMapper.settingsEdit( + category: "dataGrid", + settingsData: Data(#"{"a":2,"b":1}"#.utf8), + in: Self.zoneID, + base: base + ) + + #expect(edit.changesContent == false) + } + + @Test("Slash commands two Macs added under one name both stay, the later one under a free name") + func mergedSlashCommandsKeepDistinctNames() throws { + let shared = CustomSlashCommand(name: "explain", promptTemplate: "Explain {{query}}") + let mine = CustomSlashCommand(name: "summarize", promptTemplate: "Summarize mine") + let theirs = CustomSlashCommand(name: "Summarize", promptTemplate: "Summarize theirs") + let encoder = JSONEncoder() + let category = CustomSlashCommandStorage.syncCategory + let base = SyncRecordMapper.toCKRecord(category: category, settingsData: try encoder.encode([shared]), in: Self.zoneID) + let server = try copy(base) + server.fields(AppSettingsSyncField.self)[.settingsJson] = try encoder.encode([shared, theirs]) + + let edit = SyncRecordMapper.settingsEdit( + category: category, + settingsData: try encoder.encode([shared, mine]), + in: Self.zoneID, + base: base + ) + + let merged = try #require(edit.rebased(onto: server)) + let blob = try #require(SyncRecordMapper.settingsData(from: merged)) + let commands = try JSONDecoder().decode([CustomSlashCommand].self, from: blob) + #expect(commands.map(\.name) == ["explain", "Summarize", "summarize-2"]) + #expect(commands.map(\.id) == [shared.id, theirs.id, mine.id]) + } + + @Test("Command names that differ only as a lookup ignores are told apart") + func collidingNamesFollowLookup() { + let first = CustomSlashCommand(name: "Straße", promptTemplate: "one") + let second = CustomSlashCommand(name: "STRASSE", promptTemplate: "two") + #expect(first.name.caseInsensitiveCompare(second.name) == .orderedSame) + + let renamed = CustomSlashCommandStorage.renamingCollisions([first, second]) + + #expect(renamed.map(\.name) == ["Straße", "STRASSE-2"]) + } + + @Test("A tag edited while the pull reads what is pending keeps the edit") + func editDuringPendingReadIsKept() async throws { + let pending = ConnectionTag(name: "pending", color: .red) + let clean = ConnectionTag(name: "clean", color: .red) + try environment.tags.addTag(pending) + try environment.tags.addTag(clean) + environment.recordCache.store([pending, clean].map { SyncRecordMapper.toCKRecord($0, in: Self.zoneID) }) + environment.tracker.clearDirty(.tag, id: clean.id.uuidString) + let server = SyncRecordMapper.toCKRecord(ConnectionTag(id: clean.id, name: "clean elsewhere", color: .red), in: Self.zoneID) + let transport = ScriptedSyncTransport(zoneID: Self.zoneID) + let tags = environment.tags + var recoloured = clean + recoloured.color = .green + let edit = recoloured + await transport.whileZoneIsRead { + var all = tags.loadTags() + all.removeAll { $0.id == edit.id } + _ = tags.saveTags(all + [edit]) + } + let coordinator = environment.makeCoordinator(transport: transport) + + #expect(await coordinator.applyPullResult(PullResult(changedRecords: [server], deletedRecordIDs: [], newToken: nil))) + + #expect(environment.tags.tag(for: clean.id)?.color == .green) + #expect(environment.tags.tag(for: clean.id)?.name == "clean") + } + + @Test("A pulled record of a category this Mac does not sync is not cached") + func unsyncedCategoryStaysOutOfTheCache() async throws { + let tag = ConnectionTag(name: "prod", color: .red) + let record = SyncRecordMapper.toCKRecord(tag, in: Self.zoneID) + var settings = SyncSettings.default + settings.syncGroupsAndTags = false + let coordinator = environment.makeCoordinator(transport: ScriptedSyncTransport(zoneID: Self.zoneID)) + + #expect(await coordinator.applyPullResult( + PullResult(changedRecords: [record], deletedRecordIDs: [], newToken: nil), + settings: settings + )) + + #expect(environment.recordCache.record(for: record.recordID) == nil) + } + + // A tag cached as iCloud held it, recoloured here and renamed in iCloud since. + private func recolouredTag() throws -> (tag: ConnectionTag, cached: CKRecord, server: CKRecord) { + let tag = ConnectionTag(name: "prod", color: .red) + try environment.tags.addTag(tag) + let cached = SyncRecordMapper.toCKRecord(tag, in: Self.zoneID) + environment.recordCache.store([cached]) + let server = try copy(cached) + server.fields(ConnectionTagSyncField.self)[.name] = "production" + var recoloured = tag + recoloured.color = .blue + #expect(environment.tags.saveTags([recoloured])) + return (tag, cached, server) + } + + @Test("A refusal that names no server copy reads it and merges onto it in the same run") + func refusalWithoutCopyIsMerged() async throws { + let (_, cached, server) = try recolouredTag() + let transport = ScriptedSyncTransport(zoneID: Self.zoneID) + await transport.holdNewerCopy(nil, of: cached.recordID) + await transport.serveOnFetch(server) + + #expect(await environment.makeCoordinator(transport: transport).runSyncCycle() == nil) + + let pushed = try #require(SyncRecordMapper.toTag(try await lastPush(of: cached.recordID, in: transport))) + #expect(pushed.name == "production") + #expect(pushed.color == .blue) + } + + @Test("A record deleted elsewhere while this Mac edited it goes up again whole") + func recordDeletedElsewhereIsRecreated() async throws { + let (tag, cached, _) = try recolouredTag() + let transport = ScriptedSyncTransport(zoneID: Self.zoneID) + await transport.deleteElsewhere(cached.recordID) + + #expect(await environment.makeCoordinator(transport: transport).runSyncCycle() == nil) + + let pushed = try await lastPush(of: cached.recordID, in: transport) + #expect(await transport.pushCount == 2) + #expect(SyncRecordMapper.toTag(pushed) == ConnectionTag(id: tag.id, name: "prod", color: .blue)) + #expect(!environment.tracker.dirtyRecords(for: .tag).contains(tag.id.uuidString)) + } + + @Test("A record marked for upload but not changed here takes the change a pull brings") + func unchangedMarkedRecordTakesThePull() async throws { + let tag = ConnectionTag(name: "prod", color: .red) + try environment.tags.addTag(tag) + let cached = SyncRecordMapper.toCKRecord(tag, in: Self.zoneID) + environment.recordCache.store([cached]) + environment.tracker.markDirty(.tag, ids: [tag.id.uuidString]) + let server = try copy(cached) + server.fields(ConnectionTagSyncField.self)[.name] = "production" + let coordinator = environment.makeCoordinator(transport: ScriptedSyncTransport(zoneID: Self.zoneID)) + + #expect(await coordinator.applyPullResult(PullResult(changedRecords: [server], deletedRecordIDs: [], newToken: nil))) + + #expect(environment.tags.tag(for: tag.id)?.name == "production") + } + + @Test("A field set back to its old value while a merged save is in flight reaches iCloud on the next push") + func undoDuringMergedSaveIsPushed() async throws { + let (tag, cached, server) = try recolouredTag() + let tags = environment.tags + let undo = UndoOnce() + let transport = ScriptedSyncTransport( + zoneID: Self.zoneID, + duringPush: { + guard undo.pending else { return } + undo.pending = false + _ = tags.saveTags([tag]) + }, + echoing: { records, _ in Self.latestPushes(records) } + ) + await transport.holdNewerCopy(server, of: cached.recordID) + let coordinator = environment.makeCoordinator(transport: transport) + + _ = await coordinator.runSyncCycle() + #expect(environment.tags.tag(for: tag.id)?.color == .red) + + let merged = try await lastPush(of: cached.recordID, in: transport) + await transport.holdNewerCopy(merged, of: cached.recordID) + #expect(await coordinator.runSyncCycle() == nil) + + let pushed = try #require(SyncRecordMapper.toTag(try await lastPush(of: cached.recordID, in: transport))) + #expect(pushed.name == "production") + #expect(pushed.color == .red) + } + + @Test("A route lands on the server's copy by the rules a dial uses") + func routeRebaseRules() { + let connection = Self.tunnelled() + let me = UUID() + let mine = Self.route(of: connection, publisher: me, hop: "a.example") + let theirs = Self.route(of: connection, publisher: UUID(), hop: "b.example") + var theirsElsewhere = theirs + theirsElsewhere.publisher = UUID() + theirsElsewhere.hops = theirs.hops.map { hop in + var renamed = hop + renamed.alias = "jump" + renamed.id = UUID() + return renamed + } + + #expect(SyncRecordMapper.rebasedRoute(wanted: nil, server: theirs, publisher: me) == theirs) + #expect(SyncRecordMapper.rebasedRoute(wanted: nil, server: mine, publisher: me) == nil) + #expect(SyncRecordMapper.rebasedRoute(wanted: theirsElsewhere, server: theirs, publisher: me) == theirs) + #expect(SyncRecordMapper.rebasedRoute(wanted: mine, server: theirs, publisher: me) == mine) + #expect(SyncRecordMapper.rebasedRoute(wanted: mine, server: nil, publisher: me) == mine) + } +} diff --git a/apple/TableProTests/Helpers/SyncTestEnvironment.swift b/apple/TableProTests/Helpers/SyncTestEnvironment.swift index 84bb33d152..8f3c4b9bb7 100644 --- a/apple/TableProTests/Helpers/SyncTestEnvironment.swift +++ b/apple/TableProTests/Helpers/SyncTestEnvironment.swift @@ -144,7 +144,20 @@ final class SyncTestEnvironment { actor ScriptedSyncTransport: SyncTransport { static let accountId = "tests" - let currentZoneID: CKRecordZone.ID + private let zoneID: CKRecordZone.ID + private var duringZoneRead: @MainActor @Sendable () async -> Void = {} + + var currentZoneID: CKRecordZone.ID { + get async { + await duringZoneRead() + return zoneID + } + } + + // Runs at every read of the zone, the way an edit lands while the coordinator waits on the transport. + func whileZoneIsRead(_ action: @escaping @MainActor @Sendable () async -> Void) { + duringZoneRead = action + } private let rejectedRecordIDs: Set private let missingRecordIDs: Set private let interruption: (any Error)? @@ -162,6 +175,9 @@ actor ScriptedSyncTransport: SyncTransport { private(set) var accountIdValue = ScriptedSyncTransport.accountId private(set) var everyItemFails: (code: CKError.Code, retryAfter: TimeInterval?)? private(set) var pullError: (any Error)? + private var newerServerCopies: [CKRecord.ID: CKRecord?] = [:] + private var fetchableCopies: [CKRecord.ID: CKRecord] = [:] + private var missingFromServer: Set = [] func setAccountStatus(_ status: CKAccountStatus) { accountStatusValue = status @@ -179,6 +195,26 @@ actor ScriptedSyncTransport: SyncTransport { pullError = error } + // The next save of the record is refused as CloudKit refuses one built on an older copy, carrying + // `server` as the copy it holds; nil stands for a refusal that names no copy. + func holdNewerCopy(_ server: CKRecord?, of recordID: CKRecord.ID) { + newerServerCopies[recordID] = .some(server) + } + + // What a fetch by id returns, for a refusal that came without the server's copy. + func serveOnFetch(_ server: CKRecord) { + fetchableCopies[server.recordID] = server + } + + // The next save of a record built on a cached copy fails as one deleted elsewhere does. + func deleteElsewhere(_ recordID: CKRecord.ID) { + missingFromServer.insert(recordID) + } + + func fetch(recordIDs: [CKRecord.ID]) async throws -> [CKRecord.ID: CKRecord] { + fetchableCopies.filter { recordIDs.contains($0.key) } + } + init( zoneID: CKRecordZone.ID, rejecting rejectedRecordIDs: Set = [], @@ -207,7 +243,7 @@ actor ScriptedSyncTransport: SyncTransport { duringPush: @escaping @MainActor @Sendable () async -> Void = {}, echoing pulled: @escaping @Sendable (_ records: [CKRecord], _ deletions: [CKRecord.ID]) -> PullResult ) { - self.currentZoneID = zoneID + self.zoneID = zoneID self.rejectedRecordIDs = rejectedRecordIDs self.missingRecordIDs = missingRecordIDs self.interruption = interruption @@ -249,6 +285,20 @@ actor ScriptedSyncTransport: SyncTransport { return outcome } for record in records { + if missingFromServer.remove(record.recordID) != nil { + outcome.recordFailure( + SyncItemFailure(code: .unknownItem, serverRecord: nil, clientRecord: record, message: "Record not found"), + for: record.recordID + ) + continue + } + if let server = newerServerCopies.removeValue(forKey: record.recordID) { + outcome.recordFailure( + SyncItemFailure(code: .serverRecordChanged, serverRecord: server, clientRecord: record, message: "Server record changed"), + for: record.recordID + ) + continue + } guard rejectedRecordIDs.contains(record.recordID) else { outcome.recordSave(record) continue diff --git a/apple/changelog.d/3773.fixed.md b/apple/changelog.d/3773.fixed.md new file mode 100644 index 0000000000..8c32550927 --- /dev/null +++ b/apple/changelog.d/3773.fixed.md @@ -0,0 +1 @@ +Editing one connection, saved query or setting on two devices before they synced undid the other device's change.