Status: Done, fixed on branch feat/conflict-strategies
Priority: P1 (blocks integration with any API whose per-row timestamp field isn't literally named updatedAt)
Area: C++ (core) — cpp/sync/SyncOperationApplier.cpp, cpp/sync/SyncContract.cpp, cpp/database/MigrationEngine.cpp; TypeScript — src/types/sync/ISyncDefinition.ts
Description
sync.conflict declared a ConflictStrategy (lastWriteWins | serverWins | clientWins | manual) in ISyncDefinition, but the native engine never read it — the field was purely decorative. Actual pull-conflict behavior was hardcoded in SyncOperationApplier::apply():
// cpp/sync/SyncOperationApplier.cpp (before)
auto updatedAt = readOptionalNumber(row, "updatedAt");
auto existing = _conn->execute("SELECT updatedAt FROM \"" + expectedEntity + "\" WHERE ...", ...);Two problems fell out of this:
- Only
lastWriteWins existed in practice. serverWins/clientWins were in the type union but produced identical (lastWriteWins) behavior — picking either silently did nothing different.
- The comparison field name was fixed to
"updatedAt", on both the local SQLite column and the API's JSON payload. MigrationEngine::registerSchema unconditionally required a NOT NULL datetime column literally named updatedAt whenever sync.enabled was true — regardless of strategy, and with no way to point at a different column. Any API that named its per-row timestamp something else (modifiedAt, lastModified, ...) couldn't integrate without renaming its response shape to match this library's convention, which is backwards: the library should adapt to the API's contract, not the other way around.
Fix (done)
sync.conflict is now a discriminated union: { strategy: "lastWriteWins", field?: string } (default "updatedAt") | { strategy: "serverWins" } | { strategy: "clientWins" }. manual was dropped from the type until it's actually implemented (would need to expose pending conflicts back to JS).serverWins/clientWins are real, native-enforced strategies in SyncOperationApplier::apply() — serverWins always overwrites (including tombstones), clientWins never overwrites an existing local row (but still inserts new ones).lastWriteWins's comparison column is configurable per schema via sync.conflict.field; Database.register() validates at registration time that the named column exists as NOT NULL datetime, with an error message that clarifies updatedAt is only the default when field isn't set.- The incremental-pull cursor's timestamp field (needed for pagination regardless of conflict strategy) was split out into its own setting,
sync.endpoint.cursorField — it doesn't belong under conflict since it's unrelated to conflict resolution.
Native test coverage
Real JSI coverage (no mocks) in SyncContractTests, MigrationEngineTests, SyncOperationApplierTests, and end-to-end triggerSync sessions in SyncOrchestratorTests for serverWins/clientWins and a custom cursorField, independent of strategy.
Status: Done, fixed on branch
feat/conflict-strategiesPriority: P1 (blocks integration with any API whose per-row timestamp field isn't literally named
updatedAt)Area: C++ (core) —
cpp/sync/SyncOperationApplier.cpp,cpp/sync/SyncContract.cpp,cpp/database/MigrationEngine.cpp; TypeScript —src/types/sync/ISyncDefinition.tsDescription
sync.conflictdeclared aConflictStrategy(lastWriteWins | serverWins | clientWins | manual) inISyncDefinition, but the native engine never read it — the field was purely decorative. Actual pull-conflict behavior was hardcoded inSyncOperationApplier::apply():Two problems fell out of this:
lastWriteWinsexisted in practice.serverWins/clientWinswere in the type union but produced identical (lastWriteWins) behavior — picking either silently did nothing different."updatedAt", on both the local SQLite column and the API's JSON payload.MigrationEngine::registerSchemaunconditionally required aNOT NULL datetimecolumn literally namedupdatedAtwheneversync.enabledwas true — regardless of strategy, and with no way to point at a different column. Any API that named its per-row timestamp something else (modifiedAt,lastModified, ...) couldn't integrate without renaming its response shape to match this library's convention, which is backwards: the library should adapt to the API's contract, not the other way around.Fix (done)
sync.conflictis now a discriminated union:{ strategy: "lastWriteWins", field?: string }(default"updatedAt") |{ strategy: "serverWins" }|{ strategy: "clientWins" }.manualwas dropped from the type until it's actually implemented (would need to expose pending conflicts back to JS).serverWins/clientWinsare real, native-enforced strategies inSyncOperationApplier::apply()— serverWins always overwrites (including tombstones), clientWins never overwrites an existing local row (but still inserts new ones).lastWriteWins's comparison column is configurable per schema viasync.conflict.field;Database.register()validates at registration time that the named column exists asNOT NULL datetime, with an error message that clarifiesupdatedAtis only the default whenfieldisn't set.sync.endpoint.cursorField— it doesn't belong underconflictsince it's unrelated to conflict resolution.Native test coverage
Real JSI coverage (no mocks) in
SyncContractTests,MigrationEngineTests,SyncOperationApplierTests, and end-to-endtriggerSyncsessions inSyncOrchestratorTestsforserverWins/clientWinsand a customcursorField, independent of strategy.