Fix permanent sync divergence when userModificationTime stamps tie

[notcome]

Summary

Two devices writing the same field with identical microsecond userModificationTime stamps can diverge permanently and silently: one device keeps its own value forever while the server and all other devices converge on a different value, and the stuck device reports fully synced. We hit this reproducibly (8/9 scripted trials) with near-simultaneous same-primary-key inserts from two processes — concurrent writers land nanosecond-identical stamps often, since the clock has microsecond resolution and timers coalesce.

Mechanism

Two behaviors compound:

1. Self-preferring tie-break. The merge guards in CloudKit+StructuredQueries.swift (setValue/setAsset/removeValue) use <=, so on a per-field stamp tie the local/base value overwrites the incoming committed server value. Self-preference is not a total order: both replicas of a tie each conclude "mine wins" and resolve the same tie oppositely.
2. Override without enqueue on the fetch path. When handleFetchedRecordZoneChanges → upsertFromServerRecord preserves local values over an incoming committed record, no upload is enqueued — the .serverRecordChanged conflict path re-enqueues (SyncEngine.swift), but the fetch path does not.

End to end: A and B insert the same key with stamp t. A commits first. B's save conflicts; the tie keeps B's value; the conflict path re-enqueues; B's retry commits — B and the server agree. A then fetches B's commit: the tie keeps A's value, the merge-mutated record (server change tag around A's values) is stored as A's base, and nothing is enqueued. A's row now differs from its base on every future delivery, so the isRowValueModified branch discards every subsequent server value while advancing the base — a silent absorbing state.

The fix

1. Strict < for inbound merges. The three setters gain a requireStrictlyNewer parameter (default false, so outgoing record construction — where an equal-stamp local edit must still apply — is unchanged). The inbound merge passes true: a base field overrules an incoming committed value only when strictly newer. On a tie the committed value wins. CloudKit's CAS chain already totally orders each record's history and replays it identically to every replica, so commit order serves as a deterministic tie-breaker shared by all replicas — no schema change, and safe in a mixed fleet of patched and unpatched clients (a patched device defers on ties; an unpatched peer's conflict path pushes its value; everyone converges on it).
2. Enqueue on override. When an inbound merge on the fetch path actively overrules incoming committed field values (base field strictly newer — e.g. clock skew between writers), a .saveRecord is enqueued for that record, mirroring the conflict path, so the surviving value is re-asserted to the server and replicas that never saw it converge. Two deliberate scope limits, both shaped by the existing test suite: stale replays (incoming older than the stored base) are suppressed, since a replay legitimately loses and must not cause churn; and row-level preservation of pending local edits (isRowValueModified) does not enqueue, since those uploads are already owned by the change triggers.

Tests

Two regression tests in MergeConflictTests using the existing mocks:

• equalTimestampConflictConvergesToServerValue: a fetched record carrying a different value at an equal field stamp must be adopted, leaving row, server, and base converged.
• olderTimestampFetchReassertsLocalWinToServer: a later commit carrying an older field stamp loses the merge, and the surviving local value is enqueued and pushed so the server converges on the LWW winner.

Full suite passes (241 tests). With Sources/ reverted to main and the tests kept, both fail with the expected signatures (device keeps "Mine" over committed "Theirs"; the .saveRecord never enqueued).

One observation for maintainers, made while validating the red case: MockCloudDatabase.modifyRecords stores and returns the same CKRecord instance (MockCloudDatabase.swift:204-205), so a merge that mutates a fetched record in place also mutates mock server storage — under the buggy code the mock "server" appears to adopt the stale device's value when real CloudKit would not. Happy to file that separately.

🤖 Generated with Claude Code

[notcome]

Human message: so Fable was doing some automatic integration testing by hand and hit this issue. At its core it is not hard to understand: if two devices modify at the same time, we will have a tie, and our code is essentially local always wins. We should end up with a ping pong situation, but the reconciliation code on the original winner masks it.

The fix is essentially, if values differ and field timestamp coincide, we always treat server record as winner (Change 1). We additionally add an re-enqueue write on conflict from syncing (Change 2). Change 1 is very understandable to me, but the diff is still bigger than I would hope. And Change 2 is arguably a bit over defensive, but it could rescue existing deployment from this bug – if such bug ever materializes, which would be so unlucky.

I tried my best to understand your codebase but this is a very intricate matter, so please take everything other than the idea a huge grain of salt. I did do the PR under a fresher context, if that helps boost your confidence.

[mbrandonw]

Hi @notcome, thanks for disclosing your AI use to open this PR and providing a shorter analysis of the situation. But just to confirm, you didn't actually encounter this on real devices, right? It is of course trivial to demonstrate the problem with the test suite because time is controlled, and so easy to perform two saves at the same time, but this happening on two real devices is about as likely as winning the lottery. That doesn't mean this shouldn't be investigated, but I do want to make it clear that any time we put into this is almost certainly chasing a problem that will never happen in reality.

Can you explain more how you ran into the problem in an integration test?

And as for this PR, I haven't looked at the implementation, but I was interested to see what the tests were trying to show. Unfortunately the tests aren't really testing a real life situation. The first one is emulating a record being updated locally and sync'd to the server, and then a remote client downloads that record and edits it at the same time and sends it back to the server. The chances of that are even less than two devices editing at the same microsecond. And the second test is also doing something illegitimate in that it is directly editing the timestamp in the record rather than going through the setValue(_:forKey:at:) helper.

I do think a few (legitimate) tests in the suite with some withKnownIssues to document the behavior would be a good thing. And while investigating this I did find a few problems with the MockCloudDatabase that needed to be fixed.

[mbrandonw]

Hi @notcome, I'm curious to get your thoughts on the above. And also inclined to close this PR as this approach is probably not the one we would take. And as I mentioned above, we would prefer to start with a rock solid test case that shows the actual problem, which the current tests do not (they take shortcuts to force a problem that would not happen in real life).

[lukaskubanek]

Hi @mbrandonw and @notcome, chiming in, as I’ve spent some time in this part of the codebase. Here are my observations…

The upsert logic has quite a few issues beyond the one reported here. I started reporting them in #274 and #356, but then decided to holistically document the current behavior of the CloudKit sync in this document. I’ve highlighted several issues in the warning sections, including the equal-timestamp tie in the <= comparison that this PR targets (see the warnings in section 2.3).

The most notable problem is that the merge logic runs on every upsert (of an already synced record), gated only on whether a last-known server record and a local row exist, never on whether a logical conflict actually exists.

That is what makes this scenario possible in the first place. The fetching device has no pending changes, so there is nothing to resolve, and the only correct outcome is to apply the incoming server record as is. The tie-break inside the merge is just where the missing distinction becomes observable. A fix at that level treats the symptom rather than the cause.

On the implementation here: change tags are the right tool for detecting server-side changes, but they are opaque values with no defined ordering, so comparing them with < is not a legitimate operation. The comparison in this PR also operates on _recordChangeTag, a test-only field that only the mock server populates (with an ordered counter, which is the only reason the comparison behaves in tests). In production that field is never set, so this branch is effectively dead code on real devices. The reliable check is a mere inequality of the actual recordChangeTag between the last-known server record and the incoming one, which tells whether the server record has changed. The mock infrastructure can’t exercise that yet, since it doesn’t expose its tags through the real recordChangeTag property, but I have that change lined up as a follow-up to PR #411, using the same swizzling approach as for modificationDate.

I’ve put a lot of effort into revising this whole area while implementing customizable conflict resolution. Besides other improvements, the logic explicitly classifies each upsert as skip (server unchanged, detected via change-tag equality), apply (no local changes, the server record is applied wholesale), or resolve (a genuine conflict handed to a resolution policy). Under that model the scenario in this PR never reaches a merge, because a device without pending changes simply applies the server record. A working prototype is ready in my fork (branch sp-main).

I’m ready to extract pieces into focused PRs and submit them. 😉

[lukaskubanek]

I do think a few (legitimate) tests in the suite with some withKnownIssues to document the behavior would be a good thing.

See #414.

[notcome]

Sorry for this late action! I got a bit carried over in preparing our own app! To answer @mbrandonw your questions:

But just to confirm, you didn't actually encounter this on real devices, right? It is of course trivial to demonstrate the problem with the test suite because time is controlled, and so easy to perform two saves at the same time, but this happening on two real devices is about as likely as winning the lottery. Can you explain more how you ran into the problem in an integration test?

Yes, we didn't hit it during real devices. We only hit them in a Claude authored end-to-end test. Basically, the setup was to run two processes that modify the same record simultaenously. We had a bash script that launched two Swift processes within microseconds, and then each Swift process chose to sleep for three seconds before doing CloudKit operations. We managed to hit this issue using this setup 8 out of 9 times. Claude confidently told me that this is due to Darwin choose to coalesce timers, which I don't know if it s true. But in any case, we can get identical timestamps from different processes on the same Mac fairly easily.

I totally agree with you that this is just so unlikely to happen. It just itches a little when you know a tie is theoretically possible, :).

@lukaskubanek Your note is amazing and answered some of my own confusion about the situation. Since you have a way more systematic approach to this problem, I would close this PR.