Deliver latest record state in ModifyRecordsCallback test helper

[lukaskubanek]

Motivation

The modifyRecords(…) test helper returns a ModifyRecordsCallback, which allows tests to notify the sync engine about server-side changes at a later point. This is useful for testing merge conflict scenarios that depend on ordering, such as fetch-before-retry and retry-before-fetch.

However, the callback currently captures the saved records at modification time. If the same record is modified again in the mock database before the callback is invoked, the callback replays stale record versions, so an earlier edit is delivered to the sync engine even though the mock database already holds a newer one. The deletion side has the equivalent problem, as a deferred deletion is still delivered even if the record has been re-created in the meantime. This deviates from CloudKit's behavior, where a fetch always reflects the latest server state. Production code is not involved, but tests relying on deferred callbacks currently exercise scenarios that real CloudKit can never produce.

The issue would become even more relevant with mocked CKRecord.recordChangeTag (a follow-up to #411 I have lined up), where stale change tags would break the conflict detection required for supporting customizable conflict resolution in the future.

This was originally discussed in this comment on #412, where filing it as a separate PR was suggested. I had planned to wait for #507, but the change turned out to be independent of it. It also doesn't reintroduce the shared reference issue addressed there, since record(for:) already returns copies of the stored records on main.

Implementation

The callback now reads the latest state from the mock database at invocation time instead of using the captured save results:

• For modifications, each successfully saved record is re-read via record(for:), so the sync engine receives the current server state.
• For deletions, the event is skipped if the record has been re-saved between the original delete and the callback invocation, mirroring how a real fetch would no longer report such a record as deleted.

Both behaviors are covered by regression tests that fail on main and pass with this fix.

---

Disclaimer: AI assistance (Claude Code with Fable model) was used to prepare this PR. All changes and verifications were reviewed by me.

[mbrandonw]

Hi @lukaskubanek, thanks for this!

It mostly looks good, but I have a few suggestions. I'm not able to push directly to your branch for some reason, but I think we should:

• Drop the ReturnValue generic from ModifyRecordsCallback. It's not actually used, and if we keep it we have to answer the question of what do we return? The saved/deleted results or the fresh results (as of now you have not updated it to return fresh results).
• Batch all of the record fetching into a single records(for:) so that it is all done in a single transaction instead of a bunch of transactions.

I have pushed these changes to a branch of mine: e5d1563, 8b40128. Feel free to cherry pick them or re-implement them from scratch.

[lukaskubanek]

I'm not able to push directly to your branch for some reason

Apparently, when forked to an organization account, it’s not possible for the maintainers of the upstream repo to push to PR branches. Seems to be a known, long-standing limitation. That’s a real bummer, as it complicates the workflow. I could add you as a collaborator to my fork if it simplifies things on your end. But I can also manually integrate changes from your branch. Whatever works the best for you.

I think we should [make these two changes]

Yeah, both are great additions. The ReturnValue seems to be a relic of the past. I’ve checked and it’s indeed not used at all, even in my development in the fork.

I’ve cherry-picked both mentioned commits, but I dropped the change that commented out Issue.record, as it’s fixed by pointfreeco/swift-concurrency-extras#74 per #511. I guess the NB comments were dropped on purpose, so I left them out.

Should be good to go.