-
Notifications
You must be signed in to change notification settings - Fork 74
Description
Description
I’m on a quest of contributing support for conflict resolution customization to SQLiteData per #272. It seems to be a monstrous task, which is why I’m trying to divide it into smaller subtasks and tackle them one at a time. This issue focuses on the behavior of the built-in “last edit wins” conflict resolution strategy, currently hardcoded into the library, with the aim of abstracting it so it can later be replaced by a custom strategy.
To do that, the exact behavior first has to be defined. However, while investigating the current implementation of the strategy, I uncovered a flaw as described in this comment. This flaw leads to nondeterministic results and contributes to the stale-data problem described in #2741.
The issue occurs in a conflict scenario where there is an updated record on the server with a newer timestamp than a pending local modification.
I’ve shown examples in both #274 and #272 (scenario C1), but to keep things simple and concrete, let’s look at MergeConflictTests from the test suite, in particular with the changes from PR #353 I just submitted, which adds one additional test along with some minor improvements.
Let’s zoom in on these two test cases:
serverRecordEditedAfterClientAndProcessedAfterClient()
- Client writes “Get milk” @ t=30 (pending)
- Server sets “Buy milk” @ t=60
- Client tries to sync and gets a
.serverRecordChangederror - Server change is processed
- Client syncs again
Result: “Buy milk” wins (server value @ t=60)
serverRecordEditedAfterClientAndProcessedBeforeClient()
- Client writes “Get milk” @ t=30 (pending)
- Server sets “Buy milk” @ t=60
- Server change is processed first
- Client syncs pending changes
Result: “Get milk” wins (client value kept, but gets the server’s t=60 timestamp)
Even though we are performing the exact same edits at the exact same times, the mere scheduling order of CKSyncEngine events leads to different results.
I argue that this behavior violates the generic “last edit wins” strategy, and that both tests should produce the same result: “Buy milk” @ t=60.
The current behavior further prevents us from expressing a correct “last edit wins” field merge policy as a three-way merge function with access to only the value and its modification timestamp for each version (ancestor, client, server). To encode the current behavior, we would also need access to the current local database value in order to detect whether a pending change conflicts with what’s stored locally, since the local row may already have been updated by a newer server change. However, with the current semantics, any pending change always wins over the local database value, even if that value reflects a more recent server timestamp.
Changing the behavior to prefer the truly latest value should not have any impact on existing data, as the logic is only evaluated at runtime when a conflict occurs.
@mbrandonw @stephencelis I’d appreciate your perspective on whether you see this the same way. If so, I’d be happy to follow up with a PR that adjusts the logic as a first, minimal step toward #272, while still keeping the implementation hardcoded for now.
Checklist
- I have determined whether this bug is also reproducible in a vanilla SwiftUI project.
- I have determined whether this bug is also reproducible in a vanilla GRDB project.
- If possible, I've reproduced the issue using the
mainbranch of this package. - This issue hasn't been addressed in an existing GitHub issue or discussion.
SQLiteData version information
1.4.1
Sharing version information
2.7.4
GRDB version information
7.8.0
Destination operating system
No response
Xcode version information
Version 26.2 (17C52)
Swift Compiler version information
swift-driver version: 1.127.14.1 Apple Swift version 6.2.3 (swiftlang-6.2.3.3.21 clang-1700.6.3.2)
Target: arm64-apple-macosx15.0Footnotes
-
This is only one half of what’s causing #274, as there is also the issue of accepting server values with older timestamps (encoded as scenario NC4 in the table). ↩