While this PR addresses the issue with shared CKRecord instances, I’ve since observed another limitation of the current implementation.

It's common to invoke the modification callback at a later stage, possibly after further modifications to the same record in the database. This is useful for testing fetch-before-retry and retry-before-fetch merge conflict scenarios. However, the callback captures records at modification time, so records reported in the fetch can be stale, carrying older change tags and content (assuming mocked change tags).

This deviates from CloudKit’s behavior, which always delivers the latest record versions. It doesn’t cause issues in the current implementation since the upsert path doesn’t rely on change tag comparison, but it falls short when detecting conflicts based on change tags (needed for customizable conflict resolution).

I believe the more correct approach is to read the latest state from the database at callback invocation time (for both saves and deletes):

// In PrimaryKeyedTable.modifyRecords(…) in CloudKitTestHelpers.swift
return ModifyRecordsCallback {
  await syncEngine.parentSyncEngine.handleEvent(
    .fetchedRecordZoneChanges(
      modifications: saveResults.compactMap { recordID, result in
        guard case .success = result else { return nil }
        // NB: Read the latest version from the database in case the record has been
        //     modified between the original save and when this callback is invoked.
        return try? syncEngine.database.record(for: recordID)
      },
      deletions: deleteResults.compactMap { recordID, result in
        guard case .success = result else { return nil }
        // NB: Skip if the record has been re-saved between the original delete and
        //     when this callback is invoked.
        guard (try? syncEngine.database.record(for: recordID)) == nil else { return nil }
        guard let record = recordsToDeleteByID[recordID] else { return nil }
        return (recordID, record.recordType)
      }
    ),
    syncEngine: syncEngine
  )
  return (saveResults, deleteResults)
}

Should I amend this PR or submit it separately after the initial conflict resolution PRs have been reviewed?

Should I amend this PR or submit it separately after the initial conflict resolution PRs have been reviewed?

Hi @lukaskubanek, considering that #507 now subsumes this PR, how about a new PR for the behavior you describe above?

how about a new PR for the behavior you describe above?

Sounds good. I’ll open a separate PR for the staleness issue in the callbacks once #507 gets merged. I’ve opened the separate PR #510 for this issue.

One note on context: this fix really pays off together with mocked CKRecord.recordChangeTag, which I have lined up as a follow-up to #411. So #411 would be the most helpful one to look at next, as all other work I have sitting in my fork is based on it.

Closing this PR as superseded by #507.