diff --git a/e2e/tests/sync/webdav-first-sync-conflict.spec.ts b/e2e/tests/sync/webdav-first-sync-conflict.spec.ts index 08cd3156e6..8d801cf504 100644 --- a/e2e/tests/sync/webdav-first-sync-conflict.spec.ts +++ b/e2e/tests/sync/webdav-first-sync-conflict.spec.ts @@ -534,4 +534,195 @@ test.describe('@webdav WebDAV First Sync Conflict', () => { await closeContextsSafely(contextA, contextB, contextC); }); + + /** + * Regression test for issue #7339: + * "WebDAV/Dropbox conflict popup appears every minute on iOS" + * + * Reproduces the steady-state where one client uploaded a *snapshot* + * (not regular ops) and a second client, after downloading it, makes a + * local change and syncs. The file-based adapter's snapshot-replacement + * detection then fires on every download because (clientId mismatch + + * empty recentOps), and pre-fix `OperationLogSyncService` throws + * `LocalDataConflictError` even though the second client's clock strictly + * dominates the remote snapshot's clock — so there is no real conflict. + * + * To set up the snapshot file (recentOps=[]) we need someone to do a + * snapshot upload. The simplest way in e2e is the USE_LOCAL conflict + * resolution path, which calls `forceUploadLocalState` → SYNC_IMPORT. + * Hence the 3-client setup. + * + * Setup: + * - Client SEED: regular ops upload (file: clientId=SEED, recentOps=[seedOp]). + * - Client A: connects with own data → conflict dialog → USE_LOCAL → + * uploads snapshot (file: clientId=A, recentOps=[], snapshotClock={clientA}). + * - Client B: fresh, downloads A's snapshot cleanly (B's clock = {clientA}). + * + * Trigger: Client B adds a task → B's clock = {clientA, clientB:1}, which + * strictly dominates A's snapshot clock {clientA}. + * + * Pre-fix: B's next sync detects snapshot replacement (clientId=A != B, + * recentOps=[]), returns snapshotState, throws LocalDataConflictError + * because B has 1 meaningful pending op — dialog appears every sync. + * Post-fix: vector-clock guard skips the dialog because local dominates + * remote. + */ + test('should NOT loop conflict dialog when downstream client edits after foreign snapshot (#7339)', async ({ + browser, + baseURL, + request, + }) => { + test.slow(); + const SYNC_FOLDER_NAME = generateSyncFolderName('e2e-issue-7339'); + await createSyncFolder(request, SYNC_FOLDER_NAME); + const WEBDAV_CONFIG = { + ...WEBDAV_CONFIG_TEMPLATE, + syncFolderPath: `/${SYNC_FOLDER_NAME}`, + }; + const url = baseURL || 'http://localhost:4242'; + + // --- Client SEED: regular ops upload, populates server --- + const { context: contextSeed, page: pageSeed } = await setupSyncClient(browser, url); + const syncPageSeed = new SyncPage(pageSeed); + const workViewPageSeed = new WorkViewPage(pageSeed); + await workViewPageSeed.waitForTaskList(); + + await syncPageSeed.setupWebdavSync(WEBDAV_CONFIG); + const taskSeed = 'Seed task - ' + Date.now(); + await workViewPageSeed.addTask(taskSeed); + await waitForStatePersistence(pageSeed); + await syncPageSeed.triggerSync(); + await waitForSyncComplete(pageSeed, syncPageSeed); + console.log('[Test] SEED uploaded initial data'); + + // --- Client A: own data → conflict → USE_LOCAL → uploads SNAPSHOT --- + // We use a raw context (not setupSyncClient) because we want the MAT + // conflict dialog to actually appear (setupSyncClient only auto-accepts + // window.confirm, which is fine — but we'll need to interact with the + // MAT dialog manually here). + const contextA = await browser.newContext({ baseURL: url }); + const pageA = await contextA.newPage(); + await pageA.goto('/'); + await waitForAppReady(pageA); + + const syncPageA = new SyncPage(pageA); + const workViewPageA = new WorkViewPage(pageA); + await workViewPageA.waitForTaskList(); + + const taskA = 'Snapshot uploader task - ' + Date.now(); + await workViewPageA.addTask(taskA); + await waitForStatePersistence(pageA); + + await syncPageA.setupWebdavSync(WEBDAV_CONFIG); + const conflictDialogA = pageA.locator('mat-dialog-container', { + hasText: 'Conflicting Data', + }); + await expect(conflictDialogA).toBeVisible({ timeout: 30000 }); + await conflictDialogA.locator('button', { hasText: /Keep local/i }).click(); + // Possible "are you sure?" overwrite confirmation + const confirmDialogA = pageA.locator('dialog-confirm'); + try { + await confirmDialogA.waitFor({ state: 'visible', timeout: 3000 }); + await confirmDialogA + .locator('button[color="warn"], button:has-text("OK")') + .first() + .click(); + } catch { + // No confirmation needed + } + await waitForSyncComplete(pageA, syncPageA, 30000); + console.log('[Test] A uploaded SNAPSHOT via USE_LOCAL'); + + // --- Client B: fresh, downloads A's snapshot --- + const { context: contextB, page: pageB } = await setupSyncClient(browser, url); + + // Capture sync-related console output for debugging. + pageB.on('console', (msg) => { + const text = msg.text(); + if ( + text.includes('OperationLogSyncService') || + text.includes('FileBasedSyncAdapter') || + text.includes('LocalDataConflict') || + text.includes('Gap detected') || + text.includes('snapshot') || + text.includes('Local clock is at-or-ahead') + ) { + console.log(`[B] ${text}`); + } + }); + + const syncPageB = new SyncPage(pageB); + const workViewPageB = new WorkViewPage(pageB); + await workViewPageB.waitForTaskList(); + + await syncPageB.setupWebdavSync(WEBDAV_CONFIG); + await waitForSyncComplete(pageB, syncPageB, 30000); + console.log('[Test] B downloaded snapshot'); + + // Confirm B has A's task (snapshot was hydrated, not B's seed task — + // SEED's data was overwritten when A picked USE_LOCAL). + await expect(pageB.locator('task', { hasText: taskA })).toBeVisible({ + timeout: 10000, + }); + await expect(pageB.locator('task', { hasText: taskSeed })).not.toBeVisible(); + + // --- Trigger: B adds a task --- + const taskB = 'Task from B - ' + Date.now(); + await workViewPageB.addTask(taskB); + await waitForStatePersistence(pageB); + + // Poll until the TASK op is in IDB so triggerSync's download phase + // sees an unsynced op (avoids racing the addTask IDB write). + await pageB.waitForFunction( + () => + new Promise((resolve) => { + const dbReq = indexedDB.open('SUP_OPS'); + dbReq.onsuccess = () => { + const db = dbReq.result; + try { + const tx = db.transaction('ops', 'readonly'); + const all = tx.objectStore('ops').getAll(); + all.onsuccess = () => { + const unsynced = ( + all.result as { syncedAt?: number; rejectedAt?: number }[] + ).filter((op) => !op.syncedAt && !op.rejectedAt); + resolve(unsynced.length > 0); + }; + all.onerror = () => resolve(false); + } catch { + resolve(false); + } + }; + dbReq.onerror = () => resolve(false); + }), + { timeout: 10000 }, + ); + console.log('[Test] B added local task (TASK op queued in IDB)'); + + await syncPageB.triggerSync(); + + const conflictDialogB = pageB.locator('mat-dialog-container', { + hasText: 'Conflicting Data', + }); + await pageB.waitForTimeout(3000); + expect(await conflictDialogB.isVisible()).toBe(false); + console.log('[Test] Verified NO conflict dialog after change-and-sync'); + + await waitForSyncComplete(pageB, syncPageB, 30000); + + // Second sync — pre-fix would re-trigger the dialog because the snapshot + // file's clientId is still A, recentOps still empty. The fix's guard + // should keep firing. + await syncPageB.triggerSync(); + await pageB.waitForTimeout(3000); + expect(await conflictDialogB.isVisible()).toBe(false); + console.log('[Test] Verified NO conflict dialog on second sync (loop broken)'); + + await waitForSyncComplete(pageB, syncPageB, 30000); + + await expect(pageB.locator('task', { hasText: taskA })).toBeVisible(); + await expect(pageB.locator('task', { hasText: taskB })).toBeVisible(); + + await closeContextsSafely(contextSeed, contextA, contextB); + }); }); diff --git a/src/app/imex/sync/sync-wrapper.service.spec.ts b/src/app/imex/sync/sync-wrapper.service.spec.ts index a739d29dab..7bd40fa559 100644 --- a/src/app/imex/sync/sync-wrapper.service.spec.ts +++ b/src/app/imex/sync/sync-wrapper.service.spec.ts @@ -1117,6 +1117,13 @@ describe('SyncWrapperService', () => { expect(result).toBe('HANDLED_ERROR'); expect(mockSnackService.open).toHaveBeenCalled(); + // Issue #7339: previously, filter(undefined) on the dialog stream caused + // firstValueFrom() to throw EmptyError, which surfaced as the generic + // ERROR snack. After the fix, an undefined close (e.g., iOS app + // lifecycle killing the dialog) flows through as a clean cancellation. + expect(mockSnackService.open).not.toHaveBeenCalledWith( + jasmine.objectContaining({ type: 'ERROR' }), + ); }); it('should return HANDLED_ERROR when forceUploadLocalState fails', async () => { diff --git a/src/app/imex/sync/sync-wrapper.service.ts b/src/app/imex/sync/sync-wrapper.service.ts index b5db8d023c..cd57bf75a5 100644 --- a/src/app/imex/sync/sync-wrapper.service.ts +++ b/src/app/imex/sync/sync-wrapper.service.ts @@ -1292,7 +1292,7 @@ export class SyncWrapperService { private _openConflictDialog$( conflictData: ConflictData, - ): Observable { + ): Observable { if (this.lastConflictDialog) { this.lastConflictDialog.close(); } @@ -1301,10 +1301,11 @@ export class SyncWrapperService { disableClose: true, data: conflictData, }); - // disableClose: true ensures the dialog always closes with a result - return this.lastConflictDialog - .afterClosed() - .pipe(filter((r): r is DialogConflictResolutionResult => r !== undefined)); + // disableClose blocks ESC/backdrop, but a programmatic close (iOS app + // lifecycle, navigation, or re-entry calling close()) emits `undefined`. + // Forward it as-is so _handleLocalDataConflict treats it as cancellation; + // filtering it would leave firstValueFrom() to throw EmptyError (issue #7339). + return this.lastConflictDialog.afterClosed(); } /** 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 c0020879b3..33a5be7664 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 @@ -56,9 +56,17 @@ describe('OperationLogSyncService', () => { 'markRejected', 'setVectorClock', 'clearFullStateOps', + 'getVectorClock', + 'appendBatchSkipDuplicates', ]); opLogStoreSpy.setVectorClock.and.resolveTo(); opLogStoreSpy.clearFullStateOps.and.resolveTo(); + opLogStoreSpy.getVectorClock.and.resolveTo(null); + opLogStoreSpy.appendBatchSkipDuplicates.and.resolveTo({ + seqs: [], + writtenOps: [], + skippedCount: 0, + }); serverMigrationServiceSpy = jasmine.createSpyObj('ServerMigrationService', [ 'checkAndHandleMigration', 'handleServerMigration', @@ -1075,6 +1083,339 @@ describe('OperationLogSyncService', () => { await expectAsync(service.downloadRemoteOps(mockProvider)).toBeResolved(); expect(syncHydrationServiceSpy.hydrateFromRemoteSync).toHaveBeenCalled(); }); + + it('should skip hydration AND conflict when local clock dominates remote snapshot (issue #7339)', async () => { + // Reproduces the iOS WebDAV loop: a foreign-written snapshot with the + // same syncVersion fires gap detection on every sync from a client that + // never uploaded its own snapshot. If our local clock already dominates + // that snapshot's clock, hydration would discard local-only ops and a + // conflict dialog has nothing to resolve. + const unsyncedEntry: OperationLogEntry = { + seq: 1, + op: { + id: 'local-op-1', + clientId: 'iosClient', + actionType: '[Global Config] Update Global Config Section' as ActionType, + opType: OpType.Update, + entityType: 'GLOBAL_CONFIG', + entityId: 'config-1', + payload: { sectionKey: 'sync' }, + vectorClock: { windowsClient: 1, iosClient: 5 }, + timestamp: Date.now(), + schemaVersion: 1, + }, + appliedAt: Date.now(), + source: 'local', + }; + opLogStoreSpy.getUnsynced.and.returnValue(Promise.resolve([unsyncedEntry])); + + // Store has real user data — without the dominate-check this would + // trigger the conflict dialog every sync. + stateSnapshotServiceSpy.getStateSnapshot.and.returnValue({ + task: { ids: ['task-1', 'task-2'] }, + project: { ids: [INBOX_PROJECT.id] }, + tag: { ids: [TODAY_TAG.id] }, + note: { ids: [] }, + } as any); + + // Local strictly dominates remote: local has both clients, remote only windowsClient. + opLogStoreSpy.getVectorClock.and.resolveTo({ windowsClient: 1, iosClient: 5 }); + + 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, + failedFileCount: 0, + snapshotState: { tasks: [{ id: 'old-windows-task' }] }, + snapshotVectorClock: { windowsClient: 1 }, + latestServerSeq: 1, + }), + ); + + const setLastServerSeqSpy = jasmine + .createSpy('setLastServerSeq') + .and.resolveTo(); + const mockProvider = { + isReady: () => Promise.resolve(true), + supportsOperationSync: true, + setLastServerSeq: setLastServerSeqSpy, + } as any; + + const result = await service.downloadRemoteOps(mockProvider); + + // No conflict dialog, no hydration — local already has everything. + expect(syncHydrationServiceSpy.hydrateFromRemoteSync).not.toHaveBeenCalled(); + expect(result.kind).toBe('no_new_ops'); + // lastServerSeq still advanced so future syncs use the right cursor. + expect(setLastServerSeqSpy).toHaveBeenCalledWith(1); + }); + + it('should NOT persist accompanying newOps on the dominate path — would corrupt per-entity frontiers (codex re-review)', async () => { + // VectorClockService.getEntityFrontier() builds per-entity frontiers + // by iterating the op log in seq order with last-write-wins semantics. + // Appending historical remote ops at the current tail would regress + // the frontier for any entity where local already has newer ops, + // letting future remote ops be classified as non-conflicting and + // silently overwrite local changes. The dominate path must therefore + // skip the append; the trade-off is bounded re-download bandwidth + // (those ops keep coming back in result.newOps each sync until the + // file's snapshot advances), with no risk of state-level duplication + // because the dominate path never replays ops to NgRx. + const unsyncedEntry: OperationLogEntry = { + seq: 1, + op: { + id: 'local-op-1', + clientId: 'iosClient', + actionType: '[Global Config] Update Global Config Section' as ActionType, + opType: OpType.Update, + entityType: 'GLOBAL_CONFIG', + entityId: 'config-1', + payload: { sectionKey: 'sync' }, + vectorClock: { windowsClient: 5, iosClient: 5 }, + timestamp: Date.now(), + schemaVersion: 1, + }, + appliedAt: Date.now(), + source: 'local', + }; + opLogStoreSpy.getUnsynced.and.returnValue(Promise.resolve([unsyncedEntry])); + stateSnapshotServiceSpy.getStateSnapshot.and.returnValue({ + task: { ids: ['task-1'] }, + project: { ids: [INBOX_PROJECT.id] }, + tag: { ids: [TODAY_TAG.id] }, + note: { ids: [] }, + } as any); + + opLogStoreSpy.getVectorClock.and.resolveTo({ windowsClient: 5, iosClient: 5 }); + + const remoteOps: Operation[] = [ + { + id: 'remote-op-2', + clientId: 'windowsClient', + actionType: 'test' as ActionType, + opType: OpType.Update, + entityType: 'TASK', + entityId: 'task-w-2', + payload: {}, + vectorClock: { windowsClient: 2 }, + timestamp: Date.now(), + schemaVersion: 1, + }, + { + id: 'remote-op-3', + clientId: 'windowsClient', + actionType: 'test' as ActionType, + opType: OpType.Update, + entityType: 'TASK', + entityId: 'task-w-3', + payload: {}, + vectorClock: { windowsClient: 3 }, + timestamp: Date.now(), + schemaVersion: 1, + }, + ]; + + downloadServiceSpy.downloadRemoteOps.and.returnValue( + Promise.resolve({ + newOps: remoteOps, + hasMore: false, + latestSeq: 5, + needsFullStateUpload: false, + success: true, + failedFileCount: 0, + snapshotState: { tasks: [{ id: 'task-w-1' }] }, + snapshotVectorClock: { windowsClient: 5 }, + latestServerSeq: 5, + }), + ); + + const mockProvider = { + isReady: () => Promise.resolve(true), + supportsOperationSync: true, + setLastServerSeq: jasmine.createSpy('setLastServerSeq').and.resolveTo(), + } as any; + + const result = await service.downloadRemoteOps(mockProvider); + + expect(result.kind).toBe('no_new_ops'); + // CRITICAL: the dominate path must NOT append historical remote ops + // at the current op-log tail; doing so regresses per-entity frontiers + // and enables future LWW resolution to overwrite local data. + expect(opLogStoreSpy.appendBatchSkipDuplicates).not.toHaveBeenCalled(); + }); + + it('should still throw LocalDataConflictError when remote snapshot has work local does not (concurrent clocks)', async () => { + // Sanity check that the dominate-check is conservative: only skips when + // local truly has every entry of the remote snapshot. + const unsyncedEntry: OperationLogEntry = { + seq: 1, + op: { + id: 'local-op-1', + clientId: 'client-A', + actionType: 'test' as ActionType, + opType: OpType.Update, + entityType: 'TASK', + entityId: 'task-1', + payload: {}, + vectorClock: { clientA: 5 }, + timestamp: Date.now(), + schemaVersion: 1, + }, + appliedAt: Date.now(), + source: 'local', + }; + opLogStoreSpy.getUnsynced.and.returnValue(Promise.resolve([unsyncedEntry])); + + // CONCURRENT: local has clientA only, remote has clientB only. + opLogStoreSpy.getVectorClock.and.resolveTo({ clientA: 5 }); + + downloadServiceSpy.downloadRemoteOps.and.returnValue( + Promise.resolve({ + newOps: [], + hasMore: false, + latestSeq: 1, + needsFullStateUpload: false, + success: true, + 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), + ); + }); + + 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 + // non-empty guard, the dominate-shortcut would silently skip hydrating + // a snapshot that carries real legacy state. + opLogStoreSpy.getUnsynced.and.returnValue(Promise.resolve([])); + // Fresh local: no vector clock at all. + opLogStoreSpy.getVectorClock.and.resolveTo(null); + // No meaningful local data → fresh client hydration path applies. + stateSnapshotServiceSpy.getStateSnapshot.and.returnValue({ + task: { ids: [] }, + project: { ids: [INBOX_PROJECT.id] }, + tag: { ids: [TODAY_TAG.id] }, + note: { ids: [] }, + } as any); + + 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, + failedFileCount: 0, + snapshotState: { tasks: [{ id: 'legacy-task' }] }, + snapshotVectorClock: {}, // empty — legacy file or never populated + latestServerSeq: 1, + }), + ); + + const mockProvider = { + isReady: () => Promise.resolve(true), + supportsOperationSync: true, + setLastServerSeq: jasmine.createSpy('setLastServerSeq').and.resolveTo(), + } as any; + + await expectAsync(service.downloadRemoteOps(mockProvider)).toBeResolved(); + // Hydration must run — the empty-clock guard prevents the dominate + // shortcut from silently dropping the snapshot's state. + expect(syncHydrationServiceSpy.hydrateFromRemoteSync).toHaveBeenCalled(); + }); + + it('should NOT loop on consecutive syncs when remote keeps returning the same dominated snapshot (issue #7339)', async () => { + // The iOS bug: file-based gap detection signals snapshot replacement on + // every sync from a non-writing client. Without the dominate-check, the + // conflict dialog re-fires every sync. Verify the dominate-check breaks + // the loop across multiple consecutive sync attempts. + const unsyncedEntry: OperationLogEntry = { + seq: 1, + op: { + id: 'local-op-1', + clientId: 'iosClient', + actionType: '[Global Config] Update Global Config Section' as ActionType, + opType: OpType.Update, + entityType: 'GLOBAL_CONFIG', + entityId: 'config-1', + payload: { sectionKey: 'sync' }, + vectorClock: { windowsClient: 1, iosClient: 5 }, + timestamp: Date.now(), + schemaVersion: 1, + }, + appliedAt: Date.now(), + source: 'local', + }; + opLogStoreSpy.getUnsynced.and.returnValue(Promise.resolve([unsyncedEntry])); + stateSnapshotServiceSpy.getStateSnapshot.and.returnValue({ + task: { ids: ['task-1'] }, + project: { ids: [INBOX_PROJECT.id] }, + tag: { ids: [TODAY_TAG.id] }, + note: { ids: [] }, + } as any); + + opLogStoreSpy.getVectorClock.and.resolveTo({ windowsClient: 1, iosClient: 5 }); + + downloadServiceSpy.downloadRemoteOps.and.returnValue( + Promise.resolve({ + newOps: [], + hasMore: false, + latestSeq: 1, + needsFullStateUpload: false, + success: true, + failedFileCount: 0, + snapshotState: { tasks: [{ id: 'old-windows-task' }] }, + snapshotVectorClock: { windowsClient: 1 }, + latestServerSeq: 1, + }), + ); + + const syncHydrationServiceSpy = TestBed.inject( + SyncHydrationService, + ) as jasmine.SpyObj; + syncHydrationServiceSpy.hydrateFromRemoteSync.and.resolveTo(); + + const mockProvider = { + isReady: () => Promise.resolve(true), + supportsOperationSync: true, + setLastServerSeq: jasmine.createSpy('setLastServerSeq').and.resolveTo(), + } as any; + + // First sync — local already has all of remote. + const first = await service.downloadRemoteOps(mockProvider); + // Second sync immediately after — server still returns same snapshot + // (because iOS hasn't uploaded yet); must not throw or hydrate. + const second = await service.downloadRemoteOps(mockProvider); + + expect(first.kind).toBe('no_new_ops'); + expect(second.kind).toBe('no_new_ops'); + expect(syncHydrationServiceSpy.hydrateFromRemoteSync).not.toHaveBeenCalled(); + }); }); }); }); 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 20f4ed521d..e83f3c7d96 100644 --- a/src/app/op-log/sync/operation-log-sync.service.ts +++ b/src/app/op-log/sync/operation-log-sync.service.ts @@ -37,6 +37,7 @@ import { StateSnapshotService } from '../backup/state-snapshot.service'; import { INBOX_PROJECT } from '../../features/project/project.const'; import { SYSTEM_TAG_IDS } from '../../features/tag/tag.const'; import { confirmDialog } from '../../util/native-dialogs'; +import { compareVectorClocks, isVectorClockEmpty } from '../../core/util/vector-clock'; /** * Type guard for NgRx entity state (has an `ids` array). @@ -460,6 +461,52 @@ export class OperationLogSyncService { // we receive the complete application state in snapshotState. This must be hydrated // directly instead of processing incremental ops (which are already reflected in the state). if (result.snapshotState) { + // Issue #7339: a file-based snapshot whose vector clock is dominated by the + // local clock contains nothing the local client doesn't already have. Hydrating + // would discard local-only ops and a conflict dialog has nothing to resolve. + // Without this short-circuit, FileBasedSyncAdapter's snapshot-replacement gap + // detection re-fires every sync for clients that haven't uploaded their own + // snapshot, trapping them in a perpetual conflict-dialog loop. + // + // Both clocks must be non-empty for the comparison to be meaningful: an empty + // remote clock would compare EQUAL to a fresh local client and incorrectly skip + // hydrating a snapshot that may carry real state from a legacy file. + if (result.snapshotVectorClock && !isVectorClockEmpty(result.snapshotVectorClock)) { + const localClock = await this.opLogStore.getVectorClock(); + if (!isVectorClockEmpty(localClock)) { + const cmp = compareVectorClocks(localClock, result.snapshotVectorClock); + if (cmp === 'EQUAL' || cmp === 'GREATER_THAN') { + OpLog.normal( + `OperationLogSyncService: Local vector clock ${cmp} remote snapshot — ` + + 'skipping snapshot hydration (local already has all remote data).', + ); + // Deliberately do NOT call appendBatchSkipDuplicates(result.newOps). + // VectorClockService.getEntityFrontier() builds per-entity frontiers + // by iterating the op log in seq order with last-write-wins semantics. + // Appending historical remote ops at the current tail would regress + // the frontier for any entity where local already has newer ops, + // which then lets future remote ops be classified as non-conflicting + // and silently overwrite local changes. + // + // The trade-off: those ops keep coming back in result.newOps on each + // sync until the file's snapshot advances or the user uploads their + // own snapshot. They are never re-applied to state, because (a) the + // dominate-check skips state mutation, and (b) the regular hydration + // path replaces state wholesale from snapshotState, not by replaying + // individual ops. So the cost is bounded re-download bandwidth, not + // data corruption. + if (result.latestServerSeq !== undefined) { + await syncProvider.setLastServerSeq(result.latestServerSeq); + } + return { + kind: 'no_new_ops', + allOpClocks: result.allOpClocks, + snapshotVectorClock: result.snapshotVectorClock, + }; + } + } + } + OpLog.normal( 'OperationLogSyncService: Received snapshotState from file-based sync. Hydrating...', ); diff --git a/src/app/op-log/testing/integration/file-based-sync/conflict-resolution.integration.spec.ts b/src/app/op-log/testing/integration/file-based-sync/conflict-resolution.integration.spec.ts index 34c59d4aca..23b0731f8d 100644 --- a/src/app/op-log/testing/integration/file-based-sync/conflict-resolution.integration.spec.ts +++ b/src/app/op-log/testing/integration/file-based-sync/conflict-resolution.integration.spec.ts @@ -169,6 +169,46 @@ describe('File-Based Sync Integration - Conflict Resolution', () => { const freshDownload = await clientB.downloadOps(0); expect(freshDownload.snapshotState).toBeDefined(); }); + + it('should keep firing snapshot-replacement gap detection on every sync from a non-writing client (issue #7339 reproducer)', async () => { + // Reproduces the iOS loop. The adapter's snapshotReplacement heuristic + // uses `syncData.clientId !== excludeClient` when excludeClient is set, + // which is true forever for any client that hasn't uploaded its own + // snapshot. Without an "I already applied this snapshot" memory upstream, + // the conflict path re-fires every sync. + const clientA = harness.createClient('windows-client'); + const clientB = harness.createClient('ios-client'); + + // Windows uploads the only snapshot. recentOps stays empty. + await clientA.adapter.uploadSnapshot( + { task: { ids: [], entities: {} } }, + 'windows-client', + 'initial', + // eslint-disable-next-line @typescript-eslint/naming-convention + { 'windows-client': 1 }, + 1, + undefined, + 'snap-op-issue-7339', + ); + + // iOS does its first fresh-bootstrap download. + const first = await clientB.downloadOps(0); + expect(first.snapshotState).toBeDefined(); + expect(first.gapDetected).toBeFalsy(); + + // iOS now believes it's caught up at sinceSeq = latestSeq. Subsequent + // downloads MUST keep returning gapDetected=true because the snapshot + // file is still authored by `windows-client` and recentOps is still + // empty — that's the behavior that traps the conflict dialog upstream. + const second = await clientB.downloadOps(first.latestSeq); + expect(second.gapDetected).toBe(true); + + const third = await clientB.downloadOps(second.latestSeq); + expect(third.gapDetected).toBe(true); + // No actual ops are ever returned — the loop has nothing to apply, + // it just keeps signalling "there's a snapshot you need to resolve". + expect(third.ops).toEqual([]); + }); }); describe('Partial Trimming Gap Detection', () => {