From 7f953aae0f8f951f6aedec8ac84699fdf73c9cc7 Mon Sep 17 00:00:00 2001 From: aakhter Date: Tue, 7 Jul 2026 11:07:54 -0400 Subject: [PATCH] fix(sync): causality-aware conflict gating to cut false conflict dialogs (#8787) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use vector clocks to resolve file-based sync conflicts by causality instead of always surfacing the binary USE_LOCAL/USE_REMOTE dialog: - keep-local when local strictly dominates the remote snapshot - apply-snapshot (no dialog) when the snapshot strictly dominates local - dialog when clocks are concurrent and can't be auto-resolved Cosmetic syncVersion-reset suppression is gated on the last-seen vector clock and treated as cosmetic ONLY when the clocks are EQUAL. A GREATER_THAN remote clock proves the writer did more work, not that this client received the intervening ops (a snapshot can compact ops we never saw), so it now conservatively triggers a seq-0 resync rather than being silently skipped. CONCURRENT-snapshot auto-merge defaults OFF and, when enabled, only merges if the retained recent ops provably bridge the whole gap to the snapshot clock (local ⊔ recentOps ⊒ snapshot); otherwise it falls back to the user-recoverable dialog instead of replaying only recentOps and silently dropping compacted-only entities. Re-enabling default-on is gated on multi-client E2E validation (SPAP-34). SPAP-9 Co-authored-by: Claude Opus 4.8 --- .../src/file-based-sync-data.ts | 23 +- .../tests/provider-types.spec.ts | 16 ++ .../file-based-sync-adapter.service.spec.ts | 209 ++++++++++++++ .../file-based-sync-adapter.service.ts | 57 +++- .../sync/operation-log-sync.service.spec.ts | 263 ++++++++++++++++++ .../op-log/sync/operation-log-sync.service.ts | 215 +++++++++++++- 6 files changed, 769 insertions(+), 14 deletions(-) diff --git a/packages/sync-providers/src/file-based-sync-data.ts b/packages/sync-providers/src/file-based-sync-data.ts index 296876ba9e..ea6d2c2c2b 100644 --- a/packages/sync-providers/src/file-based-sync-data.ts +++ b/packages/sync-providers/src/file-based-sync-data.ts @@ -82,7 +82,28 @@ export const FILE_BASED_SYNC_CONSTANTS = { BACKUP_FILE: 'sync-data.json.bak', MIGRATION_LOCK_FILE: 'migration.lock', FILE_VERSION: 2 as const, - MAX_RECENT_OPS: 500, + // SPAP-9: raised 500 -> 2000 to shrink the gap window. A client that missed up + // to this many ops while offline can still catch up incrementally instead of + // tripping snapshotReplacement/partialTrimGap detection and forcing a full + // seq-0 resync (and the conflict dialog that path can surface). Compact ops are + // small (~150-250 bytes serialized each), so the extra 1500 retained ops add + // roughly ~0.3 MB to sync-data.json only when the buffer is actually full. + MAX_RECENT_OPS: 2000, SYNC_VERSION_STORAGE_KEY_PREFIX: 'FILE_SYNC_VERSION_', LEGACY_META_FILE: '__meta_', + // SPAP-9: when a seq-0 snapshot download has CONCURRENT vector clocks with the + // local client, attempt an entity-level last-write-wins merge of the remote + // recent ops instead of forcing the binary USE_LOCAL/USE_REMOTE conflict + // dialog. + // + // Default OFF (review follow-up): the merge only replays the capped `recentOps` + // buffer and never re-hydrates the compacted `snapshotState`, so a snapshot whose + // compacted base holds an entity this client never downloaded would silently drop + // that entity — turning a user-recoverable conflict dialog into permanent, global, + // undetectable data loss. `_tryConcurrentSnapshotMerge` now additionally refuses to + // merge unless the retained recentOps provably bridge the entire gap, but until that + // guard is validated by a real multi-client E2E harness (SPAP-34) we keep the + // feature opt-in and fall back to the conflict dialog. Flip to true only alongside + // that validation. + AUTO_MERGE_CONCURRENT_SNAPSHOT: false, } as const; diff --git a/packages/sync-providers/tests/provider-types.spec.ts b/packages/sync-providers/tests/provider-types.spec.ts index f5b87efd6b..24c316b66b 100644 --- a/packages/sync-providers/tests/provider-types.spec.ts +++ b/packages/sync-providers/tests/provider-types.spec.ts @@ -129,4 +129,20 @@ describe('sync provider contracts', () => { expect(data.recentOps[0].sv).toBe(7); expect(FILE_BASED_SYNC_CONSTANTS.SYNC_FILE).toBe('sync-data.json'); }); + + // SPAP-9: shrink the gap window so slow-syncing clients that miss up to + // MAX_RECENT_OPS ops can still catch up via incremental download instead of + // falling into the snapshot-replacement full-resync path. + it('retains 2000 recent ops to shrink the gap window (SPAP-9)', () => { + expect(FILE_BASED_SYNC_CONSTANTS.MAX_RECENT_OPS).toBe(2000); + }); + + // SPAP-9 (review follow-up): concurrent-snapshot auto-merge defaults OFF. The + // merge replays only the retained recentOps and never re-hydrates the compacted + // snapshotState, so it can silently drop compacted-only entities. It stays opt-in + // (behind a gap-covers guard) until validated by a multi-client E2E harness + // (SPAP-34); the safe default is the user-recoverable conflict dialog. + it('defaults concurrent-snapshot auto-merge OFF (SPAP-9 review follow-up)', () => { + expect(FILE_BASED_SYNC_CONSTANTS.AUTO_MERGE_CONCURRENT_SNAPSHOT).toBe(false); + }); }); diff --git a/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts b/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts index cdc32c87e5..93ad80ad0f 100644 --- a/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts +++ b/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts @@ -874,6 +874,59 @@ describe('FileBasedSyncAdapterService', () => { // Should NOT detect gap because excludeClient === syncData.clientId expect(result.gapDetected).toBe(false); }); + + it('flags a gap when a dominating-clock reset compacted ops the client had not yet seen', async () => { + // Review follow-up (SPAP-9): a syncVersion regression whose remote clock is + // only GREATER_THAN last-seen must NOT be treated as cosmetic. GREATER_THAN + // proves the writer did more work, not that this client received it. + + // First download: client syncs up to syncVersion 5, last-seen {client1:5}. + const compactOp = ( + id: string, + v: number, + ): FileBasedSyncData['recentOps'][number] => ({ + id, + c: 'client1', + a: 'HA', + o: 'ADD', + e: 'TASK', + d: id, + v: { client1: v }, + t: Date.now(), + s: 1, + p: {}, + }); + const first = createMockSyncData({ + syncVersion: 5, + vectorClock: { client1: 5 }, + recentOps: [compactOp('op-5', 5)], + }); + mockProvider.downloadFile.and.returnValue( + Promise.resolve({ dataStr: addPrefix(first), rev: 'rev-1' }), + ); + await adapter.downloadOps(1); + + // Writer made ops 6..10 (client never saw them), took a snapshot compacting + // 1..10 into `state` and reset recentOps, then made op 11. The file's clock + // {client1:11} is GREATER_THAN last-seen {client1:5}, but ops 6..10 survive + // only inside `state` — they are NOT in recentOps. + const second = createMockSyncData({ + syncVersion: 2, // reset to 1 by snapshot, +1 for op 11 + vectorClock: { client1: 11 }, // GREATER_THAN {client1:5} + recentOps: [compactOp('op-11', 11)], + oldestOpSyncVersion: 2, + state: { tasks: [] }, + }); + mockProvider.downloadFile.and.returnValue( + Promise.resolve({ dataStr: addPrefix(second), rev: 'rev-2' }), + ); + + const result = await adapter.downloadOps(5); // client's real cursor is 5 + // The compacted ops 6..10 would be lost without a full seq-0 resync, so the + // regression must be reported as a gap (was false under the EQUAL||GREATER_THAN + // suppression). + expect(result.gapDetected).toBe(true); + }); }); it('should set seq counter to syncVersion after snapshot upload', async () => { @@ -1679,6 +1732,68 @@ describe('FileBasedSyncAdapterService', () => { expect(result.gapDetected).toBeFalsy(); }); + it('should NOT detect gap at the boundary sinceSeq === oldestOpSyncVersion - 1 (SPAP-9 off-by-one)', async () => { + // Boundary: the oldest surviving op has sv = sinceSeq + 1. The client + // already has everything up to sinceSeq, and the very next op it needs + // (sinceSeq + 1) is present — nothing was trimmed out from under it, so + // this must NOT be treated as a gap. The old `> sinceSeq` test flagged it. + const maxOps = FILE_BASED_SYNC_CONSTANTS.MAX_RECENT_OPS; + const fullOps = Array.from({ length: maxOps }, (_, i) => ({ + id: `op-${i}`, + c: 'other-client', + a: 'HA', + o: 'ADD', + e: 'TASK', + d: `task-${i}`, + v: { otherClient: i + 1 }, + t: Date.now() + i, + s: 1, + p: {}, + sv: 6 + Math.floor(i / 10), // oldest sv=6 + })); + + const data = createMockSyncData({ + syncVersion: 60, + recentOps: fullOps, + oldestOpSyncVersion: 6, + }); + mockProvider.downloadFile.and.returnValue( + Promise.resolve({ dataStr: addPrefix(data), rev: 'rev-1' }), + ); + + const result = await adapter.downloadOps(5); // sinceSeq=5, oldest=6 → contiguous + expect(result.gapDetected).toBeFalsy(); + }); + + it('should still detect gap when oldestOpSyncVersion > sinceSeq + 1 (ops actually trimmed)', async () => { + const maxOps = FILE_BASED_SYNC_CONSTANTS.MAX_RECENT_OPS; + const fullOps = Array.from({ length: maxOps }, (_, i) => ({ + id: `op-${i}`, + c: 'other-client', + a: 'HA', + o: 'ADD', + e: 'TASK', + d: `task-${i}`, + v: { otherClient: i + 1 }, + t: Date.now() + i, + s: 1, + p: {}, + sv: 7 + Math.floor(i / 10), // oldest sv=7 + })); + + const data = createMockSyncData({ + syncVersion: 60, + recentOps: fullOps, + oldestOpSyncVersion: 7, + }); + mockProvider.downloadFile.and.returnValue( + Promise.resolve({ dataStr: addPrefix(data), rev: 'rev-1' }), + ); + + const result = await adapter.downloadOps(5); // sinceSeq=5, oldest=7 → op 6 missing + expect(result.gapDetected).toBe(true); + }); + it('should NOT detect gap when recentOps < MAX_RECENT_OPS (not trimmed)', async () => { // Even if oldestOpSyncVersion > sinceSeq, buffer not full → no trimming → no gap const data = createMockSyncData({ @@ -1827,6 +1942,100 @@ describe('FileBasedSyncAdapterService', () => { }); }); + describe('benign syncVersion reset gating (SPAP-9)', () => { + const opAt = (sv: number): FileBasedSyncData['recentOps'][number] => ({ + id: `op-${sv}`, + c: 'client1', + a: 'HA', + o: 'ADD', + e: 'TASK', + d: `task-${sv}`, + v: { client1: sv }, + t: Date.now(), + s: 1, + p: {}, + sv, + }); + + it('does NOT flag a gap when syncVersion regresses but the vector clock is unchanged (cosmetic reset)', async () => { + // First download establishes the expected syncVersion (5) and the last-seen + // vector clock ({client1:5}). + const first = createMockSyncData({ + syncVersion: 5, + vectorClock: { client1: 5 }, + recentOps: [opAt(5)], + }); + mockProvider.downloadFile.and.returnValue( + Promise.resolve({ dataStr: addPrefix(first), rev: 'rev-1' }), + ); + await adapter.downloadOps(1); + + // Second download: syncVersion regressed 5 -> 2 (would normally look like a + // reset), but the causal vector clock is IDENTICAL, so nothing was actually + // lost. Must NOT trigger the full-gap resync path. + const second = createMockSyncData({ + syncVersion: 2, + vectorClock: { client1: 5 }, + recentOps: [opAt(5)], + }); + mockProvider.downloadFile.and.returnValue( + Promise.resolve({ dataStr: addPrefix(second), rev: 'rev-2' }), + ); + const result = await adapter.downloadOps(1); + expect(result.gapDetected).toBeFalsy(); + }); + + it('flags a gap when the reset clock strictly dominates the last-seen clock (GREATER_THAN is not proof the ops were received)', async () => { + // Review follow-up: a strictly-dominating (GREATER_THAN) remote clock is NOT + // treated as cosmetic. It only proves the writer did more work, not that this + // client received the intervening ops — a snapshot can compact ops we never + // saw — so the regression must conservatively trigger a seq-0 resync. + const first = createMockSyncData({ + syncVersion: 5, + vectorClock: { client1: 5 }, + recentOps: [opAt(5)], + }); + mockProvider.downloadFile.and.returnValue( + Promise.resolve({ dataStr: addPrefix(first), rev: 'rev-1' }), + ); + await adapter.downloadOps(1); + + const second = createMockSyncData({ + syncVersion: 2, + vectorClock: { client1: 6 }, // GREATER_THAN last-seen — remote strictly ahead + recentOps: [opAt(6)], + }); + mockProvider.downloadFile.and.returnValue( + Promise.resolve({ dataStr: addPrefix(second), rev: 'rev-2' }), + ); + const result = await adapter.downloadOps(1); + expect(result.gapDetected).toBeTruthy(); + }); + + it('STILL flags a gap when the reset clock is causally behind / concurrent (genuine reset)', async () => { + const first = createMockSyncData({ + syncVersion: 5, + vectorClock: { client1: 5 }, + recentOps: [opAt(5)], + }); + mockProvider.downloadFile.and.returnValue( + Promise.resolve({ dataStr: addPrefix(first), rev: 'rev-1' }), + ); + await adapter.downloadOps(1); + + const second = createMockSyncData({ + syncVersion: 2, + vectorClock: { client1: 3 }, // LESS_THAN last-seen — remote regressed + recentOps: [opAt(3)], + }); + mockProvider.downloadFile.and.returnValue( + Promise.resolve({ dataStr: addPrefix(second), rev: 'rev-2' }), + ); + const result = await adapter.downloadOps(1); + expect(result.gapDetected).toBe(true); + }); + }); + describe('legacy pfapi format detection (v16.x cross-version guard)', () => { it('throws LegacySyncFormatDetectedError when __meta_ exists but sync-data.json does not', async () => { // Simulate a v16.x provider: __meta_ present, sync-data.json absent diff --git a/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts b/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts index 572541ca62..cadcb1abb7 100644 --- a/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts +++ b/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts @@ -38,7 +38,7 @@ import { } from '../../core/errors/sync-errors'; import { SnackService } from '../../../core/snack/snack.service'; import { T } from '../../../t.const'; -import { mergeVectorClocks } from '../../../core/util/vector-clock'; +import { mergeVectorClocks, compareVectorClocks } from '../../../core/util/vector-clock'; import { ArchiveDbAdapter } from '../../../core/persistence/archive-db-adapter.service'; import { StateSnapshotService } from '../../backup/state-snapshot.service'; import { stripLocalOnlySyncSettingsFromAppData } from '../../../features/config/local-only-sync-settings.util'; @@ -96,6 +96,14 @@ export class FileBasedSyncAdapterService { /** Expected sync version for optimistic locking, keyed by provider+user */ private _expectedSyncVersions = new Map(); + /** + * SPAP-9: last-seen remote vector clock per provider+user. Used to tell a + * benign (cosmetic) syncVersion reset apart from a genuine one: if the file's + * causal clock did not regress, a lower syncVersion counter lost no data and + * must not trigger the full-gap resync path. + */ + private _lastSeenVectorClocks = new Map(); + /** Local sequence counters (simulates server seq for file-based) */ private _localSeqCounters = new Map(); @@ -720,7 +728,7 @@ export class FileBasedSyncAdapterService { // When syncVersion resets to a lower value, we need to signal this to trigger // a re-download from seq 0 so the caller can get the snapshotState. const previousExpectedVersion = this._expectedSyncVersions.get(providerKey) ?? 0; - const versionWasReset = + const syncVersionRegressed = previousExpectedVersion > 0 && syncData.syncVersion < previousExpectedVersion; // Guard against stale in-memory state: if the persisted expected syncVersion is @@ -736,6 +744,40 @@ export class FileBasedSyncAdapterService { ); } + // SPAP-9: a syncVersion regression only implies data loss if the causal state + // also regressed. Compare the file's vector clock against the one we last saw + // for this provider. Only an EQUAL clock proves this client already holds the + // exact same causal state, so the reset is purely cosmetic (a counter reset + // that composed with a snapshot rewrite of identical content) and we can keep + // syncing incrementally at the expected version instead of forcing a full + // seq-0 resync. + // + // GREATER_THAN is deliberately NOT treated as cosmetic (review follow-up): it + // only proves the writer did strictly more work, not that this client received + // the intervening ops. A snapshot can compact ops this client never downloaded + // and the writer then make one more op — dominating our last-seen clock — so + // suppressing the reset there would silently drop the compacted ops. Anything + // that is not EQUAL (GREATER_THAN, behind, or concurrent) is treated as a + // genuine reset and triggers a seq-0 resync so the caller re-hydrates the + // snapshot. Implemented generally via the last-seen clock — no dependency on + // any provider-specific recovery mechanism. + const lastSeenClock = this._lastSeenVectorClocks.get(providerKey); + let versionWasReset = syncVersionRegressed; + if (syncVersionRegressed && lastSeenClock) { + const resetClockComparison = compareVectorClocks( + syncData.vectorClock, + lastSeenClock, + ); + if (resetClockComparison === 'EQUAL') { + versionWasReset = false; + OpLog.normal( + `FileBasedSyncAdapter: syncVersion regressed ` + + `(${previousExpectedVersion} → ${syncData.syncVersion}) but remote vector clock is ` + + `EQUAL to last-seen — treating reset as cosmetic (no gap).`, + ); + } + } + // Also detect snapshot replacement: if client expected ops (sinceSeq > 0) but file has // no recent ops AND has a snapshot state, another client uploaded a fresh snapshot. // This happens when "Use Local" is chosen in conflict resolution - the snapshot replaces @@ -760,10 +802,16 @@ export class FileBasedSyncAdapterService { // If the oldest surviving op was uploaded AFTER the client's last download, // AND the buffer is full (trimming occurred), ops between sinceSeq and // oldestOpSyncVersion were trimmed and the client never saw them. + // SPAP-9 off-by-one fix: the client already holds every op up to and + // including sinceSeq. The oldest surviving op has syncVersion + // oldestOpSyncVersion, so the first op the client still needs is sinceSeq+1. + // A gap exists only if that op was trimmed away, i.e. the oldest survivor is + // at least sinceSeq+2 (oldestOpSyncVersion > sinceSeq + 1). The boundary + // oldestOpSyncVersion === sinceSeq + 1 is contiguous and must NOT be a gap. const partialTrimGap = sinceSeq > 0 && syncData.oldestOpSyncVersion !== undefined && - syncData.oldestOpSyncVersion > sinceSeq && + syncData.oldestOpSyncVersion > sinceSeq + 1 && syncData.recentOps.length >= FILE_BASED_SYNC_CONSTANTS.MAX_RECENT_OPS; const needsGapDetection = versionWasReset || snapshotReplacement || partialTrimGap; @@ -783,6 +831,9 @@ export class FileBasedSyncAdapterService { // Update expected version for next upload this._expectedSyncVersions.set(providerKey, syncData.syncVersion); + // SPAP-9: remember this file's causal clock so the next download can decide + // whether a syncVersion regression is cosmetic or a genuine reset. + this._lastSeenVectorClocks.set(providerKey, syncData.vectorClock); // Filter ops using operation IDs instead of synthetic seq numbers. // Synthetic seq numbers based on array indices shift when the array is trimmed, diff --git a/src/app/op-log/sync/operation-log-sync.service.spec.ts b/src/app/op-log/sync/operation-log-sync.service.spec.ts index 8b6c9fb0bf..799a727360 100644 --- a/src/app/op-log/sync/operation-log-sync.service.spec.ts +++ b/src/app/op-log/sync/operation-log-sync.service.spec.ts @@ -1,5 +1,6 @@ import { TestBed } from '@angular/core/testing'; import { OperationLogSyncService } from './operation-log-sync.service'; +import { FILE_BASED_SYNC_CONSTANTS } from '../sync-providers/file-based/file-based-sync.types'; import { SchemaMigrationService } from '../persistence/schema-migration.service'; import { SnackService } from '../../core/snack/snack.service'; import { OperationLogStoreService } from '../persistence/operation-log-store.service'; @@ -1823,6 +1824,268 @@ describe('OperationLogSyncService', () => { ); }); + describe('SPAP-9 causality-aware conflict gating', () => { + const meaningfulLocalOp = ( + clock: Record, + ): OperationLogEntry => ({ + seq: 1, + op: { + id: 'local-op-1', + clientId: 'clientA', + actionType: 'test' as ActionType, + opType: OpType.Update, + entityType: 'TASK', + entityId: 'task-1', + payload: { title: 'Local edit' }, + vectorClock: clock, + timestamp: Date.now(), + schemaVersion: 1, + }, + appliedAt: Date.now(), + source: 'local', + }); + + it('(a) applies a strictly-ahead remote snapshot with NO conflict dialog', async () => { + // Remote snapshot {clientA:5} strictly dominates local {clientA:2}: + // local is clean relative to remote, so adopt the snapshot silently. + opLogStoreSpy.getUnsynced.and.returnValue( + Promise.resolve([meaningfulLocalOp({ clientA: 2 })]), + ); + opLogStoreSpy.getVectorClock.and.resolveTo({ clientA: 2 }); + + const syncHydrationServiceSpy = TestBed.inject( + SyncHydrationService, + ) as jasmine.SpyObj; + syncHydrationServiceSpy.hydrateFromRemoteSync.and.resolveTo(); + + downloadServiceSpy.downloadRemoteOps.and.returnValue( + Promise.resolve({ + newOps: [], + hasMore: false, + latestSeq: 1, + needsFullStateUpload: false, + success: true, + providerMode: 'fileSnapshotOps', + failedFileCount: 0, + snapshotState: { tasks: [{ id: 'remote-task' }] }, + snapshotVectorClock: { clientA: 5 }, + latestServerSeq: 1, + }), + ); + + const mockProvider = { + isReady: () => Promise.resolve(true), + supportsOperationSync: true, + setLastServerSeq: jasmine.createSpy('setLastServerSeq').and.resolveTo(), + } as any; + + await expectAsync(service.downloadRemoteOps(mockProvider)).toBeResolved(); + expect(syncHydrationServiceSpy.hydrateFromRemoteSync).toHaveBeenCalled(); + }); + + it('(b) keeps strictly-ahead local with NO dialog and no hydration (upload left to the normal cycle)', async () => { + // Local {clientA:5} strictly dominates snapshot {clientA:2}: keep local, + // do not hydrate, do not reject the pending op (it uploads next cycle). + opLogStoreSpy.getUnsynced.and.returnValue( + Promise.resolve([meaningfulLocalOp({ clientA: 5 })]), + ); + opLogStoreSpy.getVectorClock.and.resolveTo({ clientA: 5 }); + + const syncHydrationServiceSpy = TestBed.inject( + SyncHydrationService, + ) as jasmine.SpyObj; + syncHydrationServiceSpy.hydrateFromRemoteSync.and.resolveTo(); + + const setLastServerSeqSpy = jasmine + .createSpy('setLastServerSeq') + .and.resolveTo(); + downloadServiceSpy.downloadRemoteOps.and.returnValue( + Promise.resolve({ + newOps: [], + hasMore: false, + latestSeq: 1, + needsFullStateUpload: false, + success: true, + providerMode: 'fileSnapshotOps', + failedFileCount: 0, + snapshotState: { tasks: [{ id: 'old-task' }] }, + snapshotVectorClock: { clientA: 2 }, + latestServerSeq: 1, + }), + ); + + const mockProvider = { + isReady: () => Promise.resolve(true), + supportsOperationSync: true, + setLastServerSeq: setLastServerSeqSpy, + } as any; + + const result = await service.downloadRemoteOps(mockProvider); + expect(syncHydrationServiceSpy.hydrateFromRemoteSync).not.toHaveBeenCalled(); + expect(result.kind).toBe('no_new_ops'); + expect(setLastServerSeqSpy).toHaveBeenCalledWith(1); + }); + + const remoteOpWithClock = (clock: Record): Operation => ({ + id: 'remote-op-1', + clientId: 'clientB', + actionType: 'test' as ActionType, + opType: OpType.Update, + entityType: 'TASK', + entityId: 'task-1', + payload: { title: 'Remote edit' }, + vectorClock: clock, + timestamp: Date.now(), + schemaVersion: 1, + }); + + const withAutoMergeEnabled = async (fn: () => Promise): Promise => { + const prev = FILE_BASED_SYNC_CONSTANTS.AUTO_MERGE_CONCURRENT_SNAPSHOT; + ( + FILE_BASED_SYNC_CONSTANTS as { AUTO_MERGE_CONCURRENT_SNAPSHOT: boolean } + ).AUTO_MERGE_CONCURRENT_SNAPSHOT = true; + try { + await fn(); + } finally { + ( + FILE_BASED_SYNC_CONSTANTS as { AUTO_MERGE_CONCURRENT_SNAPSHOT: boolean } + ).AUTO_MERGE_CONCURRENT_SNAPSHOT = prev; + } + }; + + it('(c) CONCURRENT snapshot with meaningful local data falls back to the dialog when auto-merge is disabled (the default)', async () => { + // Review follow-up: auto-merge defaults OFF, so a CONCURRENT seq-0 + // snapshot with meaningful local data must surface the user-recoverable + // conflict dialog rather than silently merging. + expect(FILE_BASED_SYNC_CONSTANTS.AUTO_MERGE_CONCURRENT_SNAPSHOT).toBe(false); + opLogStoreSpy.getUnsynced.and.returnValue( + Promise.resolve([meaningfulLocalOp({ clientA: 5 })]), + ); + opLogStoreSpy.getVectorClock.and.resolveTo({ clientA: 5 }); + + downloadServiceSpy.downloadRemoteOps.and.returnValue( + Promise.resolve({ + newOps: [remoteOpWithClock({ clientB: 3 })], + allOpClocks: [{ clientB: 3 }], + hasMore: false, + latestSeq: 1, + needsFullStateUpload: false, + success: true, + providerMode: 'fileSnapshotOps', + failedFileCount: 0, + snapshotState: { tasks: [{ id: 'remote-task' }] }, + snapshotVectorClock: { clientB: 3 }, + latestServerSeq: 1, + }), + ); + + const mockProvider = { + isReady: () => Promise.resolve(true), + supportsOperationSync: true, + setLastServerSeq: jasmine.createSpy('setLastServerSeq').and.resolveTo(), + } as any; + + await expectAsync(service.downloadRemoteOps(mockProvider)).toBeRejectedWith( + jasmine.any(LocalDataConflictError), + ); + expect(remoteOpsProcessingServiceSpy.processRemoteOps).not.toHaveBeenCalled(); + }); + + it('(d) auto-merges via LWW when enabled AND retained ops bridge the full gap', async () => { + // Local {clientA:5}, snapshot {clientB:3} — CONCURRENT. The retained op + // clocks reconstruct the snapshot on top of local + // (local ⊔ {clientB:3} = {clientA:5,clientB:3} ⊒ {clientB:3}), so the + // merge is provably lossless and runs instead of the dialog. + await withAutoMergeEnabled(async () => { + opLogStoreSpy.getUnsynced.and.returnValue( + Promise.resolve([meaningfulLocalOp({ clientA: 5 })]), + ); + opLogStoreSpy.getVectorClock.and.resolveTo({ clientA: 5 }); + + const syncHydrationServiceSpy = TestBed.inject( + SyncHydrationService, + ) as jasmine.SpyObj; + syncHydrationServiceSpy.hydrateFromRemoteSync.and.resolveTo(); + + const remoteOp = remoteOpWithClock({ clientB: 3 }); + downloadServiceSpy.downloadRemoteOps.and.returnValue( + Promise.resolve({ + newOps: [remoteOp], + allOpClocks: [{ clientB: 3 }], + hasMore: false, + latestSeq: 1, + needsFullStateUpload: false, + success: true, + providerMode: 'fileSnapshotOps', + failedFileCount: 0, + snapshotState: { tasks: [{ id: 'remote-task' }] }, + snapshotVectorClock: { clientB: 3 }, + latestServerSeq: 1, + }), + ); + + const mockProvider = { + isReady: () => Promise.resolve(true), + supportsOperationSync: true, + setLastServerSeq: jasmine.createSpy('setLastServerSeq').and.resolveTo(), + } as any; + + const result = await service.downloadRemoteOps(mockProvider); + expect(remoteOpsProcessingServiceSpy.processRemoteOps).toHaveBeenCalledWith( + [remoteOp], + ); + expect( + syncHydrationServiceSpy.hydrateFromRemoteSync, + ).not.toHaveBeenCalled(); + expect(result.kind).toBe('ops_processed'); + }); + }); + + it('(e) refuses to auto-merge and falls back to the dialog when the snapshot base holds compacted ops the client never saw', async () => { + // The data-loss case: snapshot clock {clientB:3, clientC:2} but only + // clientB:3 survives as a retained op — clientC:2 was compacted into the + // snapshot base. Replaying recentOps on top of local {clientA:5} yields + // {clientA:5, clientB:3}, which is CONCURRENT with the snapshot (missing + // clientC:2). Merging only recentOps would silently drop clientC's + // entities, so the guard must refuse and surface the dialog. + await withAutoMergeEnabled(async () => { + opLogStoreSpy.getUnsynced.and.returnValue( + Promise.resolve([meaningfulLocalOp({ clientA: 5 })]), + ); + opLogStoreSpy.getVectorClock.and.resolveTo({ clientA: 5 }); + + downloadServiceSpy.downloadRemoteOps.and.returnValue( + Promise.resolve({ + newOps: [remoteOpWithClock({ clientB: 3 })], + allOpClocks: [{ clientB: 3 }], + hasMore: false, + latestSeq: 1, + needsFullStateUpload: false, + success: true, + providerMode: 'fileSnapshotOps', + failedFileCount: 0, + snapshotState: { tasks: [{ id: 'remote-task' }] }, + snapshotVectorClock: { clientB: 3, clientC: 2 }, + latestServerSeq: 1, + }), + ); + + const mockProvider = { + isReady: () => Promise.resolve(true), + supportsOperationSync: true, + setLastServerSeq: jasmine.createSpy('setLastServerSeq').and.resolveTo(), + } as any; + + await expectAsync(service.downloadRemoteOps(mockProvider)).toBeRejectedWith( + jasmine.any(LocalDataConflictError), + ); + expect( + remoteOpsProcessingServiceSpy.processRemoteOps, + ).not.toHaveBeenCalled(); + }); + }); + }); + it('should hydrate (NOT skip) when both clocks are empty — fresh client receiving a legacy snapshot', async () => { // Edge case from codex review of issue #7339 fix: an empty remote // snapshot clock compares EQUAL to a fresh local client. Without the diff --git a/src/app/op-log/sync/operation-log-sync.service.ts b/src/app/op-log/sync/operation-log-sync.service.ts index 05ae830afa..61801c3758 100644 --- a/src/app/op-log/sync/operation-log-sync.service.ts +++ b/src/app/op-log/sync/operation-log-sync.service.ts @@ -1,6 +1,13 @@ import { inject, Injectable } from '@angular/core'; import { Store } from '@ngrx/store'; import { planSnapshotHydration } from '@sp/sync-core'; +import { + VectorClock, + compareVectorClocks, + isVectorClockEmpty, + mergeVectorClocks, +} from '../../core/util/vector-clock'; +import { FILE_BASED_SYNC_CONSTANTS } from '../sync-providers/file-based/file-based-sync.types'; import { OperationLogStoreService } from '../persistence/operation-log-store.service'; import { BackupService } from '../backup/backup.service'; import { FULL_STATE_OP_TYPES } from '../core/operation.types'; @@ -520,11 +527,17 @@ export class OperationLogSyncService { this.syncLocalStateService.hasMeaningfulStoreData(exampleTaskIds); if (hasMeaningfulUserData) { - // Client has meaningful user data - show conflict dialog - OpLog.warn( - `OperationLogSyncService: Client has ${unsyncedOps.length} unsynced local ops ` + - 'with meaningful user data (pending ops or store data). ' + - 'Throwing LocalDataConflictError for conflict resolution dialog.', + // SPAP-9: before surfacing the binary USE_LOCAL/USE_REMOTE dialog, use + // the vector clocks to decide the safe outcome by causality. Only a + // client with genuine sync history (a populated local clock) can be + // auto-resolved — a missing/empty local clock (provider switch, legacy + // store-only data) still falls through to the dialog, preserving the + // existing "genuinely can't auto-decide" behaviour. + const localClock = await this.opLogStore.getVectorClock(); + const gate = this._classifySnapshotConflict( + localClock, + result.snapshotVectorClock, + FILE_BASED_SYNC_CONSTANTS.AUTO_MERGE_CONCURRENT_SNAPSHOT, ); // The local snapshot's vector clock is this client's last-synced @@ -534,11 +547,71 @@ export class OperationLogSyncService { const lastSyncedVectorClock = (await this.vectorClockService.getSnapshotVectorClock()) ?? null; - throw new LocalDataConflictError( - unsyncedOps.length, - result.snapshotState as Record, - result.snapshotVectorClock, - lastSyncedVectorClock, + if (gate === 'keep-local') { + // Local strictly dominates the snapshot: keep local, no dialog. The + // pending ops are left untouched so the normal upload phase ships them. + OpLog.normal( + 'OperationLogSyncService: Local vector clock strictly ahead of remote snapshot — ' + + 'keeping local and deferring to the upload phase (no conflict dialog).', + ); + if (result.latestServerSeq !== undefined) { + await syncProvider.setLastServerSeq(result.latestServerSeq); + } + return { + kind: 'no_new_ops', + allOpClocks: result.allOpClocks, + snapshotVectorClock: result.snapshotVectorClock, + }; + } + + if (gate === 'merge') { + const mergeOutcome = await this._tryConcurrentSnapshotMerge( + result, + syncProvider, + localClock, + ); + if (mergeOutcome) { + return mergeOutcome; + } + // Merge could not run (divergence lives only in the compacted + // snapshot, no incremental ops to LWW-merge). Surface the dialog + // rather than silently picking a side. + OpLog.warn( + 'OperationLogSyncService: CONCURRENT snapshot with no incremental remote ops to merge — ' + + 'falling back to the conflict resolution dialog.', + ); + throw new LocalDataConflictError( + unsyncedOps.length, + result.snapshotState as Record, + result.snapshotVectorClock, + lastSyncedVectorClock, + ); + } + + if (gate === 'dialog') { + // Client has meaningful user data and clocks can't be auto-resolved - + // show conflict dialog. + OpLog.warn( + `OperationLogSyncService: Client has ${unsyncedOps.length} unsynced local ops ` + + 'with meaningful user data (pending ops or store data). ' + + 'Throwing LocalDataConflictError for conflict resolution dialog.', + ); + + throw new LocalDataConflictError( + unsyncedOps.length, + result.snapshotState as Record, + result.snapshotVectorClock, + lastSyncedVectorClock, + ); + } + + // gate === 'apply-snapshot': the remote snapshot strictly dominates the + // local clock, so local holds nothing the snapshot lacks. Adopt the + // snapshot without a dialog by falling through to hydration below. + exampleTaskOpIdsToDiscard = exampleTaskOpIds; + OpLog.normal( + 'OperationLogSyncService: Remote snapshot strictly ahead of local clock — ' + + 'applying snapshot without conflict dialog.', ); } else { // Defer the markRejected call until hydration has succeeded — see @@ -881,6 +954,128 @@ export class OperationLogSyncService { } } + /** + * SPAP-9: classify a seq-0 file-based snapshot conflict by causality so we can + * avoid the binary USE_LOCAL/USE_REMOTE dialog when the vector clocks make the + * safe outcome unambiguous. + * + * Comparison direction is snapshot-vs-local: + * - GREATER_THAN / EQUAL → remote strictly ahead (or identical): apply snapshot. + * - LESS_THAN → local strictly ahead: keep local, upload later. + * - CONCURRENT → true divergence: merge if enabled, else dialog. + * + * A missing or empty local clock means a client with no genuine sync history + * (provider switch, legacy store-only data). Such a client cannot be + * auto-resolved, so it keeps the existing dialog behaviour — mirroring the + * fresh-client throws elsewhere in this method. + */ + private _classifySnapshotConflict( + localClock: VectorClock | null | undefined, + snapshotClock: Record | undefined, + mergeEnabled: boolean, + ): 'apply-snapshot' | 'keep-local' | 'merge' | 'dialog' { + if ( + !localClock || + isVectorClockEmpty(localClock) || + !snapshotClock || + isVectorClockEmpty(snapshotClock) + ) { + return 'dialog'; + } + + switch (compareVectorClocks(snapshotClock, localClock)) { + case 'GREATER_THAN': + case 'EQUAL': + return 'apply-snapshot'; + case 'LESS_THAN': + return 'keep-local'; + case 'CONCURRENT': + return mergeEnabled ? 'merge' : 'dialog'; + } + } + + /** + * SPAP-9: attempt an entity-level merge of a CONCURRENT seq-0 snapshot instead + * of the conflict dialog. The client already holds the shared base (it has a + * populated vector clock), so the only divergent remote work is the file's + * incremental recent ops. Routing those through the existing remote-ops + * pipeline runs the standard LWW conflict resolution (remote-wins-ties, which + * emits LWW_CONFLICTS_AUTO_RESOLVED) and leaves the local pending ops queued + * for upload — a genuine merge rather than picking a side. + * + * Returns null when the merge cannot be proven lossless — either there are no + * incremental ops to merge, or the retained ops do not bridge the full gap to + * the snapshot (see guard below). The caller then falls back to the dialog so + * no data is silently discarded. + */ + private async _tryConcurrentSnapshotMerge( + result: Awaited>, + syncProvider: OperationSyncCapable, + localClock: VectorClock | null | undefined, + ): Promise { + if (result.newOps.length === 0) { + return null; + } + + // GUARD (review follow-up): this merge replays only the file's retained + // `recentOps` (result.newOps) and never re-hydrates the compacted + // `snapshotState`. That is lossless ONLY if replaying those ops on top of the + // local state reconstructs the snapshot's full causal state — i.e. the local + // client already holds the snapshot's compacted base. A populated local clock + // proves the client has *its own* history, NOT that it received another + // client's ops that were later compacted into the snapshot base. + // + // Reconstruct the causal state we would reach by replaying the retained ops on + // top of local (local ⊔ ⨆ recentOp clocks). If that does not dominate the + // snapshot's clock, the snapshot's compacted base contains ops this client + // never downloaded; merging only recentOps would silently and permanently drop + // those entities. Refuse and fall back to the dialog, which can hydrate the + // full snapshot via USE_REMOTE — a user-recoverable choice, not silent loss. + const snapshotClock = result.snapshotVectorClock; + const bridgedClock = (result.allOpClocks ?? []).reduce( + (acc, opClock) => mergeVectorClocks(acc, opClock), + { ...(localClock ?? {}) } as VectorClock, + ); + const bridgeComparison = snapshotClock + ? compareVectorClocks(bridgedClock, snapshotClock) + : 'GREATER_THAN'; + if (bridgeComparison !== 'EQUAL' && bridgeComparison !== 'GREATER_THAN') { + OpLog.warn( + 'OperationLogSyncService: CONCURRENT snapshot auto-merge refused — retained recent ops ' + + 'do not bridge the full gap to the snapshot (local+recentOps is ' + + `${bridgeComparison} vs the snapshot clock, so its compacted base holds ops this client ` + + 'never saw). Falling back to the conflict dialog to avoid silent data loss.', + ); + return null; + } + + OpLog.normal( + `OperationLogSyncService: CONCURRENT snapshot with ${result.newOps.length} incremental remote op(s) — ` + + 'auto-merging via LWW conflict resolution instead of the conflict dialog.', + ); + + const processResult = await this.remoteOpsProcessingService.processRemoteOps( + result.newOps, + ); + + // Persist the cursor only AFTER the ops are applied, matching the normal + // incremental path's crash-safety ordering. + if (result.latestServerSeq !== undefined) { + await syncProvider.setLastServerSeq(result.latestServerSeq); + } + + const pendingOps = await this.opLogStore.getUnsynced(); + this.superSyncStatusService.updatePendingOpsStatus(pendingOps.length > 0); + + return { + kind: 'ops_processed', + newOpsCount: result.newOps.length, + localWinOpsCreated: processResult.localWinOpsCreated, + allOpClocks: result.allOpClocks, + snapshotVectorClock: result.snapshotVectorClock, + }; + } + /** * Shows the SYNC_IMPORT conflict dialog and executes the user's chosen action. *