fix(sync): bound a CloudKit record name so a long database path cannot crash the app - #2585
Merged
Conversation
…t crash the app Claude-Session: https://claude.ai/code/session_01KhHdFvjmq8f8cEFyx5WGiv
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2575.
Root cause
A column layout's sync category embeds the connection UUID, the database name and the table name, each percent-encoded with
.alphanumericsas the only allowed set (TableScope.storageComponent). On SQLite the database name is a file path, so every/,-,.and_becomes three characters and the category has no bound at all.SyncRecordType.recordName(for:)then concatenated it ontoSettings_and handed the result toCKRecord.ID(recordName:), which raisesCKExceptionpast CloudKit's limit.That exception unwinds a Swift async frame, which leaves the concurrency runtime inconsistent, so the app segfaults seconds to a minute later in
swift_task_isCurrentExecutorWithFlagsImplfrom a call site that varies per crash. The dirty flag is on disk, so the next launch retries and crashes again.Measured, not assumed
scripts/check-cloudkit-record-name-limit.shbinary-searches the real CloudKit framework for the limit and fails when it disagrees withSyncRecordName.maximumLength, so a future SDK change re-measures rather than trusting a transcribed number. What it and the exploratory probe found:CKException: recordName (…) is too longé(250 UTF-16 units, 500 UTF-8 bytes)héllo,テーブル""recordName can not be empty)So the limit counts UTF-16 code units, not characters and not bytes, and the documented "ASCII only" is not enforced. Non-ASCII table names were never a second crash path.
The fix
SyncRecordType.recordName(for:)is the one function both mappers call, so the bound goes there. A name that already fits is returned unchanged, byte for byte, so every record that reached iCloud keeps its identity and nothing migrates. A name that would exceed the limit carriessha256-<digest of the identifier>instead, which is deterministic, so two Macs land on the same record. Names that would have exceeded the limit cannot exist in iCloud today, because creating one crashed.Pull is unaffected:
SyncRecordMapper.settingsCategory(from:)already reads the category out of the record'scategoryfield rather than out of its name.A stuck user self-heals on update. The dirty entry is still on disk and now pushes under a bounded name.
The trap that came with it
performPushused to recover a local identifier by parsing it back out of the returned record name. With a digest that returns the hash, so the dirty flag would never clear and the same record would push on every sync forever. The record name is an identity, not an encoding of the identifier, so the push now resolves a saved record throughSyncRecordMapper.identities(for:in:), built from the dirty sets and tombstones it already read, before the push goes out. That also drops the push path's reliance onFavorite_/FavoriteFolder_/FavoriteTable_longest-prefix disambiguation.Scope of the class
AppSettingsis the only record type with an unbounded identifier.FavoriteTablesStorage.syncIdandFavoriteDatabasesStorage.syncIdalready SHA-256 their composite keys for exactly this reason; column layout is the one that did not. Everything else is a UUID. The iOS target only syncs connections, groups and tags, all UUID-keyed, and picks up the same bound through the shared transport.Built and tested
verify.sh generate,verify.sh build,verify.sh lint: PASSswift test --package-path Packages/TableProCore: full package suite greenverify.sh test SyncRecordIdentityTests ColumnLayoutSyncTests SyncChangeTrackerTests SyncScopeTests SyncCoordinatorTokenExpiryTests FavoriteDatabaseSyncTests: PASSscripts/check-cloudkit-record-name-limit.sh: PASS (declared 255, measured 255)New coverage:
SyncRecordTypeTests: a name at exactly 255 is unchanged, one unit past it is shortened, the count is UTF-16 units, shortening is deterministic, two long identifiers stay distinct, and a wrangler-length SQLite category fits.ColumnLayoutSyncTests: the realColumnLayoutTableKeyfor a long SQLite path produces a record name CloudKit accepts. Without the fix the same key measures over 255.SyncRecordIdentityTests: a shortened name no longer carries its category, and the identity map still resolves it.SyncRecordNameConstructionTests: a source scan proving nothing outside the two mappers constructs aCKRecord.ID, the same shape as the existingSyncMapperFieldAccessTestsgate.No UI change, so no screenshots and no
TableProUITestsautomation: the whole fix is in the sync layer and is covered by unit tests.Review
Codex read the diff cold. Its
reviewpass returned no findings. Itsadversarial-reviewpass returned two, both verified as pre-existing and left out of this PR:AppSettingstombstones are never pushed as deletions. Real, and it predates this change: every other record type appends its tombstones to the delete list,AppSettingsdoes not, so a cleared column layout is never removed from iCloud and a full fetch restores it. Codex's added point is fair, that an over-long record could not reach iCloud before this fix, so the gap now reaches one more group of users. It is still not a six-line addition:ColumnLayoutPersister.clearfollowed by a latersaveon the same table leaves one identifier both dirty and tombstoned, so enqueueing settings tombstones today would delete a layout the user had just saved, in the same push. Closing it means makingmarkDirtyretire a tombstone, which changes behaviour for every record type, plus inbound deletion handling that a digest name cannot resolve on its own. That is its own change with its own two-device test.