Fix user modification timestamp.

[mbrandonw]

If a write is made to the database while a batch of records is being prepared and sent to iCloud, it is possible for us to accidentally cache an old timestamp on the the records. That could prevent other devices from updating their local data with fresh server data.

[mbrandonw]

I need this bit of infrastructure in the mocks to wiggle in between the moment a batch is sent out to iCloud and the moment the sentRecordZoneChanges delegate method is called.

[mbrandonw]

This is the actual fix.

[mbrandonw]

I don't think we can write a failing test that shows data not properly syncing (we need to better support multiple sync engines in a test), but this at least shows the edit was made at the right time. Without the changes in this PR this timestamp was 1.

[mbrandonw]

I wanted a way to get visibility into record change tags.

[lukaskubanek]

Hi @mbrandonw, reading the description mentioning the step of preparing changes for upload and seeing the fix in setLastKnownServerRecord(_:) I wonder how exactly this is being called. Are you referring to this call stack?

• nextRecordZoneChangeBatch(reason:options:syncEngine:)
• refreshLastKnownServerRecord(_:) (after update)
• setLastKnownServerRecord(_:)

If so, I’d like to point out that this very spot exhibits different behavior between production and tests. I reported it in #356. The short version is that in tests, the record is always written, as the mock database doesn’t set CloudKit’s internal modificationDate, whereas in production this only happens for newly created records (if I’m reading this correctly).

However, even when aligning the behavior between test and production environments, I wonder why we would ever want to save an in-flight record as the last-known server record before it’s confirmed by the server. I point it out in step 4 of scenario 2.1 in my document on the current behavior. It breaks the model for a potential three-way merge conflict resolution, as it would prevent using the last-known server record as the ancestor.

[mbrandonw]

Are you referring to this call stack?

No, the problematic call stack actually originates from sentRecordZoneChanges, not nextRecordZoneChangeBatch.

If so, I’d like to point out that this very spot exhibits different behavior between production and tests. I reported it in #356. The short version is that in tests, the record is always written, as the mock database doesn’t set CloudKit’s internal modificationDate, whereas in production this only happens for newly created records (if I’m reading this correctly).

Yeah, it does seem like we should better emulate modificationDate as we do with recordChangeTag. It should be powered off of @Dependency(\.currentTime) too. Will think about this a bit more.

However, even when aligning the behavior between test and production environments, I wonder why we would ever want to save an in-flight record as the last-known server record before it’s confirmed by the server. I point it out in step 4 of scenario 2.1 in my document on the current behavior. It breaks the model for a potential three-way merge conflict resolution, as it would prevent using the last-known server record as the ancestor.

Yeah, to be honest I don't remember why we do that. And removing that call to setLastKnownServerRecord from nextRecordZoneChangeBatch makes only one single test fail (merge_clientRecordUpdatedBeforeServerRecord):

-        isCompleted🗓️: 30,
+        isCompleted🗓️: 60,

And I'm trying to think which is correct. The isCompleted field is edited at time 30, but the merge conflict is resolved at time 60. Will have to think about why we do that a bit more, but I do think this change is still good for us to make.