Skip to content

Add failing test to demonstrate delete-then-reinsert issue #418 - #575

Open
jsutula wants to merge 7 commits into
pointfreeco:mainfrom
jsutula:delete-reinsert-known-issue-test
Open

jsutula wants to merge 7 commits into
pointfreeco:mainfrom
jsutula:delete-reinsert-known-issue-test

Conversation

@jsutula

@jsutula jsutula commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Refs #418.

Two small stacked commits extracted from #421:

1. Dedupe by ID in MockSyncEngineState add(pendingRecordZoneChanges:) and add(pendingDatabaseChanges:)

According to Apple docs for CKSyncEngine.State.add(pendingRecordZoneChanges:), for a given CKRecord.ID, adding a delete after a save discards the save, and adding a save after a delete discards the delete.

Note
The order in which you apply record zone changes is important.
For example:

  • If you add .saveRecord(recordA) then .deleteRecord(recordA), the sync engine discards the save and sends only the delete change.
  • If you add .deleteRecord(recordA) then .saveRecord(recordA), the sync engine discards the delete and sends only the save change.

In other words, pending save and delete changes are deduplicated by CKRecord.ID, retaining the last change added. The same is true for CKSyncEngine.State.add(pendingDatabaseChanges:), but deduped on CKRecordZone.ID instead of CKRecord.ID.

Currently MockSyncEngineState only deduplicates fully identical changes (ID + type(save/delete)) in its add methods. This commit changes its implementation to match CKSyncEngine.State behavior and dedupe on ID alone. This is accomplished by switching its internal pending changes storage:

  • OrderedSet<CKSyncEngine.PendingRecordZoneChange> -> OrderedDictionary<CKRecord.ID, CKSyncEngine.PendingRecordZoneChange>
  • OrderedSet<CKSyncEngine.PendingDatabaseChange> -> OrderedDictionary<CKRecordZone.ID, CKSyncEngine.PendingDatabaseChange>.

2. Add failing test to demonstrate delete-then-reinsert issue #418

(failing assertions wrapped in withKnownIssue)

In order to be meaningful, this new test depends on the MockSyncEngineState changes in the first commit. Without the mock changes, a fix for #418 that queues a save after the delete would result in MockSyncEngineState holding both the pending save and the pending delete (since there's no deduping on ID). With both present, only the delete is handled, and the result would be an unexpectedly empty CloudKit and still-failing assertions, even though the real CKSyncEngine would discard the delete and send only the save.

Comment on lines +191 to +193
if changes[id] == change {
changes.removeValue(forKey: id)
}

@jsutula jsutula Nov 5, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As compared to the add methods which now dedupe on ID, remove must only remove fully matching changes (ID + type). This behavior needs to be preserved. Since storage is no longer a Set, we can't use the convenient subtract method anymore and must manually check for full change equality before removing.

@mbrandonw

Copy link
Copy Markdown
Member

Hey @jsutula, thanks for the PR! I rebased this on all the work we merged today, and things are looking good. Will either merge this soon, or may spend some time investigating a fix.

@mbrandonw

Copy link
Copy Markdown
Member

I just pushed 3aec4e3 that now fixes the delete+reinsert bug. Could you give it a test and let me know if it works for you?

@jsutula

jsutula commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor Author

Great, looks like a clean fix. To test, I rebased the original live repro setup on top of these changes - jsutula@5271521.

Result: data loss no longer occurs on every other update.

Before fix (data loss every other update) After fix (no data loss)
Screen.Recording.2026-11-05.at.6.09.43.PM.mov
Screen.Recording.2026-11-05.at.6.11.41.PM.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants