fix(sync): bound a CloudKit record name so a long database path cannot crash the app

CHANGELOG.md

@@ -37,6 +37,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Cell editor opening off screen when `Tab` wrapped onto a row below the visible ones.
 - Cell cursor left on the old column after `Tab` carried the editor to the next one.
 - Every data grid switching to its accessibility layout after one `Tab` press, with no assistive app attached.
+- Crash loop on every launch after resizing a column on a database with a long file path, with iCloud sync on. (#2575)
 - SSH Agent auth prompting for a private key passphrase instead of reporting that the agent was never reached. (#2583)
 - "SSH password rejected" on an SSH connection that has no password, when the server offers no keyboard-interactive.
 

Packages/TableProCore/Sources/TableProSyncTransport/SyncRecordType.swift

@@ -1,3 +1,4 @@
+import CryptoKit
 import Foundation
 
 public enum SyncRecordType: String, CaseIterable, Sendable {
@@ -25,10 +26,23 @@ public enum SyncRecordType: String, CaseIterable, Sendable {
         }
     }
 
+    /// A name longer than `SyncRecordName.maximumLength` carries a digest of the identifier in
+    /// place of the identifier. `CKRecord.ID(recordName:)` raises `CKException` past that length,
+    /// and an Objective-C exception raised inside a Swift task leaves the concurrency runtime
+    /// unwound, which crashes the app seconds later from an unrelated call site. A settings
+    /// category embeds a database name, so on SQLite it embeds a percent-encoded file path and
+    /// has no bound at all (#2575).
+    ///
+    /// Every name that already fits is returned unchanged, so records that reached iCloud keep
+    /// their identity. A name that did not fit could never be written, so nothing is orphaned.
     public func recordName(for id: String) -> String {
-        recordNamePrefix + id
+        let name = recordNamePrefix + id
+        guard (name as NSString).length > SyncRecordName.maximumLength else { return name }
+        return recordNamePrefix + SyncRecordName.digestPrefix + SyncRecordName.digest(of: id)
     }
 
+    /// The identifier is only recoverable when `recordName(for:)` did not shorten it, so the push
+    /// path resolves a saved record through the identifiers it sent rather than through this.
     public static func parse(recordName: String) -> (type: SyncRecordType, id: String)? {
         for type in longestPrefixFirst where recordName.hasPrefix(type.recordNamePrefix) {
             return (type, String(recordName.dropFirst(type.recordNamePrefix.count)))
@@ -39,3 +53,20 @@ public enum SyncRecordType: String, CaseIterable, Sendable {
     private static let longestPrefixFirst: [SyncRecordType] = allCases
         .sorted { $0.recordNamePrefix.count > $1.recordNamePrefix.count }
 }
+
+/// CloudKit's own limit on a record name, and how a name that would exceed it is shortened.
+public enum SyncRecordName {
+    /// Measured against the CloudKit framework: 255 UTF-16 code units pass and 256 raise, and the
+    /// count is of UTF-16 units rather than characters or bytes (250 two-byte characters pass at
+    /// 500 UTF-8 bytes; 128 emoji raise at 256 UTF-16 units). `scripts/check-cloudkit-record-name-limit.sh`
+    /// re-measures it.
+    public static let maximumLength = 255
+
+    /// Names the shortening in the CloudKit dashboard, and keeps a digest from colliding with an
+    /// identifier that is genuinely 64 hexadecimal characters, such as a favorite's sync id.
+    public static let digestPrefix = "sha256-"
+
+    public static func digest(of id: String) -> String {
+        SHA256.hash(data: Data(id.utf8)).map { String(format: "%02x", $0) }.joined()
+    }
+}

Packages/TableProCore/Tests/TableProSyncTests/SyncRecordNameConstructionTests.swift

@@ -0,0 +1,76 @@
+import Foundation
+import Testing
+
+@Suite("Record names are only built where CloudKit's length limit is enforced")
+struct SyncRecordNameConstructionTests {
+    private static let repositoryRoot = URL(fileURLWithPath: #filePath)
+        .deletingLastPathComponent()
+        .deletingLastPathComponent()
+        .deletingLastPathComponent()
+        .deletingLastPathComponent()
+        .deletingLastPathComponent()
+
+    private static let sourceRoots = [
+        "TablePro",
+        "TableProMobile/TableProMobile",
+        "Packages/TableProCore/Sources"
+    ]
+
+    private static let permittedPaths: Set<String> = [
+        "TablePro/Core/Sync/SyncRecordMapper.swift",
+        "Packages/TableProCore/Sources/TableProSync/SyncRecordMapper.swift"
+    ]
+
+    @Test("Every source root the check covers is where the check expects it")
+    func sourceRootsExist() {
+        for path in Self.sourceRoots {
+            let url = Self.repositoryRoot.appendingPathComponent(path)
+            #expect(FileManager.default.fileExists(atPath: url.path), """
+            \(path) has moved, so this check would pass vacuously. Update sourceRoots.
+            """)
+        }
+    }
+
+    @Test("Every mapper the check permits is where the check expects it")
+    func permittedMappersExist() {
+        for path in Self.permittedPaths {
+            let url = Self.repositoryRoot.appendingPathComponent(path)
+            #expect(FileManager.default.fileExists(atPath: url.path), """
+            \(path) has moved. Update permittedPaths.
+            """)
+        }
+    }
+
+    @Test("No shipping source constructs a CKRecord.ID outside the mappers")
+    func recordIdsComeFromTheMappers() {
+        var offenders: [String] = []
+
+        for root in Self.sourceRoots {
+            let rootURL = Self.repositoryRoot.appendingPathComponent(root)
+            guard let files = FileManager.default.enumerator(
+                at: rootURL,
+                includingPropertiesForKeys: nil
+            ) else { continue }
+
+            for case let url as URL in files where url.pathExtension == "swift" {
+                let path = url.path.replacingOccurrences(of: Self.repositoryRoot.path + "/", with: "")
+                guard !Self.permittedPaths.contains(path),
+                      let source = try? String(contentsOf: url, encoding: .utf8) else { continue }
+
+                for (offset, line) in source.components(separatedBy: .newlines).enumerated() {
+                    let code = line.trimmingCharacters(in: .whitespaces)
+                    guard !code.hasPrefix("//"), code.contains("CKRecord.ID(recordName:") else { continue }
+                    offenders.append("\(path):\(offset + 1): \(code)")
+                }
+            }
+        }
+
+        #expect(offenders.isEmpty, """
+        CKRecord.ID(recordName:) raises CKException past 255 UTF-16 code units, and an \
+        Objective-C exception raised inside a Swift task crashes the app from an unrelated call \
+        site seconds later. SyncRecordType.recordName(for:) is the only thing that bounds the \
+        name, so go through SyncRecordMapper.recordID(type:id:in:).
+        \(offenders.joined(separator: "\n"))
+        """)
+    }
+}

Packages/TableProCore/Tests/TableProSyncTests/SyncRecordTypeTests.swift

@@ -62,4 +62,63 @@ struct SyncRecordTypeTests {
         let prefixes = SyncRecordType.allCases.map(\.recordNamePrefix)
         #expect(Set(prefixes).count == prefixes.count)
     }
+
+    @Test("A name that already fits is returned unchanged", arguments: SyncRecordType.allCases)
+    func namesThatFitAreUnchanged(_ type: SyncRecordType) {
+        let id = String(repeating: "a", count: SyncRecordName.maximumLength - type.recordNamePrefix.count)
+        let name = type.recordName(for: id)
+        #expect(name == type.recordNamePrefix + id)
+        #expect((name as NSString).length == SyncRecordName.maximumLength)
+    }
+
+    @Test("A name one unit too long is shortened", arguments: SyncRecordType.allCases)
+    func namesPastTheLimitAreShortened(_ type: SyncRecordType) {
+        let id = String(repeating: "a", count: SyncRecordName.maximumLength - type.recordNamePrefix.count + 1)
+        let name = type.recordName(for: id)
+        #expect((name as NSString).length <= SyncRecordName.maximumLength)
+        #expect(name.hasPrefix(type.recordNamePrefix + SyncRecordName.digestPrefix))
+    }
+
+    /// The limit CloudKit enforces counts UTF-16 code units, so a name of 128 emoji is over it at
+    /// 128 characters. `scripts/check-cloudkit-record-name-limit.sh` measures that.
+    @Test("The limit counts UTF-16 code units, not characters")
+    func theLimitCountsUTF16CodeUnits() {
+        let id = String(repeating: "😀", count: 200)
+        #expect(id.count < SyncRecordName.maximumLength)
+        let name = SyncRecordType.settings.recordName(for: id)
+        #expect((name as NSString).length <= SyncRecordName.maximumLength)
+        #expect(name.hasPrefix("Settings_" + SyncRecordName.digestPrefix))
+    }
+
+    @Test("Shortening is stable, so two devices agree on the record")
+    func shorteningIsDeterministic() {
+        let id = String(repeating: "path/to/database.sqlite", count: 40)
+        #expect(SyncRecordType.settings.recordName(for: id) == SyncRecordType.settings.recordName(for: id))
+        #expect(
+            SyncRecordType.settings.recordName(for: id) == "Settings_sha256-"
+                + SyncRecordName.digest(of: id)
+        )
+    }
+
+    @Test("Two long identifiers do not collapse onto one record")
+    func distinctLongIdentifiersStayDistinct() {
+        let base = String(repeating: "a", count: 300)
+        #expect(SyncRecordType.settings.recordName(for: base) != SyncRecordType.settings.recordName(for: base + "b"))
+    }
+
+    /// The column layout category that produced #2575: a connection UUID, a percent-encoded
+    /// SQLite file path, an empty schema and a table name.
+    @Test("A long SQLite path produces a name CloudKit accepts")
+    func aLongSQLitePathFits() {
+        let path = "/Users/example/projects/acme/api/.wrangler/state/v3/d1"
+            + "/miniflare-D1DatabaseObject/"
+            + String(repeating: "f", count: 64) + ".sqlite"
+        let parts = [UUID().uuidString, path, "", "d1_migrations"]
+            .map { $0.addingPercentEncoding(withAllowedCharacters: .alphanumerics) ?? $0 }
+        let category = "columnLayout." + parts.joined(separator: ".")
+
+        #expect((("Settings_" + category) as NSString).length > SyncRecordName.maximumLength)
+        #expect((SyncRecordType.settings.recordName(for: category) as NSString).length
+            <= SyncRecordName.maximumLength)
+    }
 }

TablePro/Core/Sync/SyncCoordinator.swift

@@ -391,19 +391,20 @@ final class SyncCoordinator {
 
         guard !recordsToSave.isEmpty || !uniqueDeletions.isEmpty else { return }
 
+        let identities = SyncRecordMapper.identities(for: pushedLocalIds(), in: zoneID)
         let outcome = try await engine.push(records: recordsToSave, deletions: uniqueDeletions)
 
         recordCache.store(Array(outcome.savedRecords.values))
         recordCache.remove(Array(outcome.deletedRecordIDs))
 
         for recordID in outcome.savedRecords.keys {
-            guard let parsed = SyncRecordMapper.parse(recordName: recordID.recordName) else { continue }
-            changeTracker.clearDirty(parsed.type, id: parsed.id)
+            guard let identity = identities[recordID] else { continue }
+            changeTracker.clearDirty(identity.type, id: identity.id)
         }
 
         for recordID in outcome.deletedRecordIDs {
-            guard let parsed = SyncRecordMapper.parse(recordName: recordID.recordName) else { continue }
-            metadataStorage.removeTombstone(parsed.id, type: parsed.type)
+            guard let identity = identities[recordID] else { continue }
+            metadataStorage.removeTombstone(identity.id, type: identity.type)
         }
 
         let savedCount = outcome.savedRecords.count
@@ -417,6 +418,20 @@ final class SyncCoordinator {
         throw SyncError.pushRejected(count: outcome.failures.count, detail: firstFailure.message)
     }
 
+    /// Every local identifier this push can have sent. `SyncChangeTracker` is not isolated to this
+    /// actor, so the sets can move under an await; a record whose identifier is missing from the
+    /// snapshot is left dirty and pushed again rather than cleared against the wrong entry.
+    private func pushedLocalIds() -> [SyncRecordType: Set<String>] {
+        var localIds: [SyncRecordType: Set<String>] = [:]
+        for type in SyncRecordType.allCases {
+            let ids = changeTracker.dirtyRecords(for: type)
+                .union(metadataStorage.tombstones(for: type).map(\.id))
+            guard !ids.isEmpty else { continue }
+            localIds[type] = ids
+        }
+        return localIds
+    }
+
     // MARK: - Pull
 
     nonisolated static func isTokenExpired(_ error: Error) -> Bool {

TablePro/Core/Sync/SyncRecordIdentity.swift

@@ -0,0 +1,14 @@
+//
+//  SyncRecordIdentity.swift
+//  TablePro
+//
+
+import Foundation
+import TableProSyncTransport
+
+/// The local identifier behind a record the push sent, kept because a CloudKit record name is an
+/// identity rather than an encoding of that identifier.
+struct SyncRecordIdentity: Hashable, Sendable {
+    let type: SyncRecordType
+    let id: String
+}

TablePro/Core/Sync/SyncRecordMapper.swift

@@ -45,6 +45,23 @@ struct SyncRecordMapper {
         SyncRecordType.parse(recordName: recordName)
     }
 
+    /// Maps every record the push is about to send back onto the local identifier it was built
+    /// from. `SyncRecordType.recordName(for:)` shortens an identifier that would take the name
+    /// past CloudKit's limit, so a saved record cannot be read back through `parse(recordName:)`
+    /// without clearing the wrong dirty entry and pushing the same record on every sync forever.
+    static func identities(
+        for localIds: [SyncRecordType: Set<String>],
+        in zone: CKRecordZone.ID
+    ) -> [CKRecord.ID: SyncRecordIdentity] {
+        var identities: [CKRecord.ID: SyncRecordIdentity] = [:]
+        for (type, ids) in localIds {
+            for id in ids {
+                identities[recordID(type: type, id: id, in: zone)] = SyncRecordIdentity(type: type, id: id)
+            }
+        }
+        return identities
+    }
+
     // MARK: - Connection
 
     static func toCKRecord(

TableProTests/Core/Storage/ColumnLayoutSyncTests.swift

@@ -59,4 +59,25 @@ struct ColumnLayoutSyncTests {
     func categoryPrefix() {
         #expect(FileColumnLayoutPersister.syncCategory(for: "abc").hasPrefix(FileColumnLayoutPersister.syncCategoryPrefix))
     }
+
+    /// A SQLite database name is a file path, and the storage key percent-encodes every character
+    /// that is not alphanumeric, so a wrangler path takes the record name past what CloudKit
+    /// accepts. `CKRecord.ID(recordName:)` raised there, and the app crashed seconds later from an
+    /// unrelated call site, on every launch (#2575).
+    @Test("A long SQLite path still yields a record name CloudKit accepts")
+    func longSQLitePathYieldsAcceptableRecordName() {
+        let path = "/Users/example/projects/acme/api/.wrangler/state/v3/d1"
+            + "/miniflare-D1DatabaseObject/" + String(repeating: "f", count: 64) + ".sqlite"
+        let tableKey = ColumnLayoutTableKey(
+            connectionId: UUID(),
+            databaseName: path,
+            schemaName: nil,
+            tableName: "d1_migrations"
+        )
+        let category = FileColumnLayoutPersister.syncCategory(for: tableKey.storageKey)
+
+        #expect((("Settings_" + category) as NSString).length > SyncRecordName.maximumLength)
+        #expect((SyncRecordType.settings.recordName(for: category) as NSString).length
+            <= SyncRecordName.maximumLength)
+    }
 }

TableProTests/Core/Sync/SyncRecordIdentityTests.swift

@@ -0,0 +1,77 @@
+//
+//  SyncRecordIdentityTests.swift
+//  TableProTests
+//
+
+import CloudKit
+import Foundation
+@testable import TablePro
+import TableProSyncTransport
+import Testing
+
+@Suite("Push identities survive a shortened record name")
+@MainActor
+struct SyncRecordIdentityTests {
+    private static let zone = CKRecordZone.ID(zoneName: "TableProZone", ownerName: CKCurrentUserDefaultName)
+
+    private static func longColumnLayoutCategory() -> String {
+        let path = "/Users/example/projects/acme/api/.wrangler/state/v3/d1"
+            + "/miniflare-D1DatabaseObject/" + String(repeating: "f", count: 64) + ".sqlite"
+        let key = ColumnLayoutTableKey(
+            connectionId: UUID(),
+            databaseName: path,
+            schemaName: nil,
+            tableName: "d1_migrations"
+        )
+        return FileColumnLayoutPersister.syncCategory(for: key.storageKey)
+    }
+
+    @Test("A category too long for a record name still resolves back to itself")
+    func longCategoryResolvesBack() {
+        let category = Self.longColumnLayoutCategory()
+        let identities = SyncRecordMapper.identities(for: [.settings: [category]], in: Self.zone)
+        let recordID = SyncRecordMapper.recordID(type: .settings, id: category, in: Self.zone)
+
+        #expect(identities[recordID] == SyncRecordIdentity(type: .settings, id: category))
+    }
+
+    /// The record name carries a digest once it is shortened, so the identifier the push needs
+    /// back is not in it. Reading it out of the name clears the wrong dirty entry, which leaves
+    /// the real one dirty and pushes the same record on every sync forever.
+    @Test("The shortened record name no longer carries the category")
+    func aShortenedNameDoesNotCarryTheCategory() {
+        let category = Self.longColumnLayoutCategory()
+        #expect((("Settings_" + category) as NSString).length > SyncRecordName.maximumLength, """
+        The fixture no longer exceeds CloudKit's limit, so this check would pass vacuously.
+        """)
+
+        let recordID = SyncRecordMapper.recordID(type: .settings, id: category, in: Self.zone)
+        let parsed = SyncRecordMapper.parse(recordName: recordID.recordName)
+
+        #expect((recordID.recordName as NSString).length <= SyncRecordName.maximumLength)
+        #expect(parsed?.type == .settings)
+        #expect(parsed?.id != category)
+    }
+
+    @Test("A short identifier resolves back without being shortened")
+    func shortIdentifierResolvesBack() {
+        let id = UUID().uuidString
+        let identities = SyncRecordMapper.identities(for: [.connection: [id]], in: Self.zone)
+        let recordID = SyncRecordMapper.recordID(type: .connection, id: id, in: Self.zone)
+
+        #expect(recordID.recordName == "Connection_" + id)
+        #expect(identities[recordID] == SyncRecordIdentity(type: .connection, id: id))
+    }
+
+    @Test("Identities keep every type apart")
+    func identitiesKeepTypesApart() {
+        let id = UUID().uuidString
+        let identities = SyncRecordMapper.identities(
+            for: [.connection: [id], .group: [id], .tag: [id]],
+            in: Self.zone
+        )
+
+        #expect(identities.count == 3)
+        #expect(identities[SyncRecordMapper.recordID(type: .group, id: id, in: Self.zone)]?.type == .group)
+    }
+}