diff --git a/src/app/imex/sync/sync-wrapper.service.spec.ts b/src/app/imex/sync/sync-wrapper.service.spec.ts index c6fddcf786..a739d29dab 100644 --- a/src/app/imex/sync/sync-wrapper.service.spec.ts +++ b/src/app/imex/sync/sync-wrapper.service.spec.ts @@ -31,6 +31,7 @@ import { MissingRefreshTokenAPIError, JsonParseError, SyncDataCorruptedError, + UploadRevToMatchMismatchAPIError, } from '../../op-log/core/errors/sync-errors'; import { MAX_LWW_REUPLOAD_RETRIES } from '../../op-log/core/operation-log.const'; @@ -1279,6 +1280,23 @@ describe('SyncWrapperService', () => { }), ); }); + + it('should treat UploadRevToMatchMismatchAPIError as transient: set UNKNOWN_OR_CHANGED, no error snackbar', async () => { + mockSyncService.uploadPendingOps.and.returnValue( + Promise.reject( + new UploadRevToMatchMismatchAPIError('Concurrent upload detected'), + ), + ); + + const result = await service.sync(); + + expect(result).toBe('HANDLED_ERROR'); + expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith( + 'UNKNOWN_OR_CHANGED', + ); + expect(mockSnackService.open).not.toHaveBeenCalled(); + expect(mockProviderManager.setSyncStatus).not.toHaveBeenCalledWith('ERROR'); + }); }); describe('isSyncInProgressSync()', () => { diff --git a/src/app/imex/sync/sync-wrapper.service.ts b/src/app/imex/sync/sync-wrapper.service.ts index 953343db2b..25463b8fc0 100644 --- a/src/app/imex/sync/sync-wrapper.service.ts +++ b/src/app/imex/sync/sync-wrapper.service.ts @@ -21,6 +21,7 @@ import { EmptyRemoteBodySPError, JsonParseError, SyncDataCorruptedError, + UploadRevToMatchMismatchAPIError, } from '../../op-log/core/errors/sync-errors'; import { MAX_LWW_REUPLOAD_RETRIES } from '../../op-log/core/operation-log.const'; import { SyncConfig } from '../../features/config/global-config.model'; @@ -705,6 +706,16 @@ export class SyncWrapperService { config: { duration: 12000 }, }); return 'HANDLED_ERROR'; + } else if (error instanceof UploadRevToMatchMismatchAPIError) { + // Another client uploaded between our download and upload — self-healing. + // The next sync cycle will download their ops first, then upload successfully. + // Do not show an error snackbar; just mark as UNKNOWN_OR_CHANGED so the + // next sync cycle triggers and resolves the state. + SyncLog.log( + 'SyncWrapperService: Concurrent upload detected, will retry on next sync cycle', + ); + this._providerManager.setSyncStatus('UNKNOWN_OR_CHANGED'); + return 'HANDLED_ERROR'; } else { this._providerManager.setSyncStatus('ERROR'); const errStr = getSyncErrorStr(error); 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 fd8a3d690d..d1521430e1 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 @@ -117,12 +117,9 @@ describe('FileBasedSyncAdapterService', () => { // Reset TestBed to ensure fresh service instance for each test // This is critical because FileBasedSyncAdapterService caches state in memory TestBed.resetTestingModule(); - (FILE_BASED_SYNC_CONSTANTS as any).RETRY_BASE_DELAY_MS = 500; }); beforeEach(() => { - // Eliminate real retry delays to prevent slow tests and timer accumulation - (FILE_BASED_SYNC_CONSTANTS as any).RETRY_BASE_DELAY_MS = 0; mockArchiveDbAdapter = jasmine.createSpyObj('ArchiveDbAdapter', [ 'loadArchiveYoung', 'loadArchiveOld', @@ -365,33 +362,30 @@ describe('FileBasedSyncAdapterService', () => { ); }); - it('should retry upload once when UploadRevToMatchMismatchAPIError occurs', async () => { + it('should throw UploadRevToMatchMismatchAPIError on genuine concurrent upload (rev changed)', async () => { const syncData = createMockSyncData({ syncVersion: 1 }); - mockProvider.downloadFile.and.returnValue( + // Initial download: rev-1 + mockProvider.downloadFile.and.returnValues( Promise.resolve({ dataStr: addPrefix(syncData), rev: 'rev-1' }), + // Re-download after mismatch returns rev-2 (another client uploaded for real) + Promise.resolve({ dataStr: addPrefix(syncData), rev: 'rev-2' }), + ); + // All upload attempts fail with rev mismatch + mockProvider.uploadFile.and.returnValue( + Promise.reject(new UploadRevToMatchMismatchAPIError('Rev mismatch')), ); - // First upload fails with rev mismatch, second succeeds - let uploadCalls = 0; - mockProvider.uploadFile.and.callFake(() => { - uploadCalls++; - if (uploadCalls === 1) { - return Promise.reject(new UploadRevToMatchMismatchAPIError('Rev mismatch')); - } - return Promise.resolve({ rev: 'rev-3' }); - }); - - // Download to populate cache await adapter.downloadOps(0); const op = createMockSyncOp(); - const result = await adapter.uploadOps([op], 'client1'); + // Genuine mismatch: adapter throws so next sync cycle can fix the snapshot + await expectAsync(adapter.uploadOps([op], 'client1')).toBeRejectedWithError( + UploadRevToMatchMismatchAPIError, + ); - // Should succeed after retry - expect(result.results[0].accepted).toBe(true); - // Upload was called twice (initial + retry) - expect(uploadCalls).toBe(2); - // Download was called twice (initial cache + re-download on retry) + // Only one upload attempt (no retry loop) + expect(mockProvider.uploadFile).toHaveBeenCalledTimes(1); + // Download called twice: initial cache + re-download to check rev expect(mockProvider.downloadFile).toHaveBeenCalledTimes(2); }); @@ -430,172 +424,6 @@ describe('FileBasedSyncAdapterService', () => { expect(retryCall[3]).toBe(true); // isForceOverwrite }); - it('should use conditional upload on retry when freshRev differs', async () => { - const syncData = createMockSyncData({ syncVersion: 1 }); - // Initial download returns rev-1 - mockProvider.downloadFile.and.returnValues( - Promise.resolve({ dataStr: addPrefix(syncData), rev: 'rev-1' }), - // Re-download on retry returns rev-2 (different rev = real concurrent modification) - Promise.resolve({ dataStr: addPrefix(syncData), rev: 'rev-2' }), - ); - - let uploadCalls = 0; - mockProvider.uploadFile.and.callFake( - (_path: string, _dataStr: string, _rev: string | null, _force: boolean) => { - uploadCalls++; - if (uploadCalls === 1) { - return Promise.reject(new UploadRevToMatchMismatchAPIError('Rev mismatch')); - } - return Promise.resolve({ rev: 'rev-3' }); - }, - ); - - // Download to populate cache with rev-1 - await adapter.downloadOps(0); - - const op = createMockSyncOp(); - const result = await adapter.uploadOps([op], 'client1'); - - expect(result.results[0].accepted).toBe(true); - expect(uploadCalls).toBe(2); - - // Retry upload should be called with isForceOverwrite: false - const retryCall = mockProvider.uploadFile.calls.argsFor(1); - expect(retryCall[3]).toBe(false); // isForceOverwrite - }); - - describe('oldestOpSyncVersion and sv tagging in retry path', () => { - it('should tag retry compact ops with the correct sv (freshData.syncVersion + 1)', async () => { - // Existing file at syncVersion=3 with an op tagged sv=3 - const existingData = createMockSyncData({ - syncVersion: 3, - recentOps: [ - { - id: 'existing-op', - c: 'other-client', - a: 'HA', - o: 'ADD', - e: 'TASK', - d: 'task-existing', - v: { otherClient: 1 }, - t: Date.now() - 10_000, - s: 1, - p: {}, - sv: 3, - }, - ], - }); - - // First download populates cache - mockProvider.downloadFile.and.returnValue( - Promise.resolve({ dataStr: addPrefix(existingData), rev: 'rev-1' }), - ); - await adapter.downloadOps(0); - - // Upload: first attempt fails, retry re-downloads and succeeds - let uploadCallCount = 0; - let retryUploadedDataStr = ''; - mockProvider.uploadFile.and.callFake( - (_path: string, dataStr: string, _rev: string | null, _force: boolean) => { - uploadCallCount++; - if (uploadCallCount === 1) { - return Promise.reject(new UploadRevToMatchMismatchAPIError('Rev mismatch')); - } - retryUploadedDataStr = dataStr; - return Promise.resolve({ rev: 'rev-3' }); - }, - ); - - // Re-download on retry returns file at syncVersion=5 (another client uploaded twice) - const freshData = createMockSyncData({ - syncVersion: 5, - recentOps: [ - { - id: 'existing-op', - c: 'other-client', - a: 'HA', - o: 'ADD', - e: 'TASK', - d: 'task-existing', - v: { otherClient: 1 }, - t: Date.now() - 10_000, - s: 1, - p: {}, - sv: 3, - }, - ], - }); - mockProvider.downloadFile.and.returnValue( - Promise.resolve({ dataStr: addPrefix(freshData), rev: 'rev-2' }), - ); - - const newOp = createMockSyncOp({ id: 'new-op' }); - await adapter.uploadOps([newOp], 'client1'); - - const uploadedData = parseWithPrefix(retryUploadedDataStr); - // New op should be tagged with freshData.syncVersion + 1 = 6 - const newCompactOp = uploadedData.recentOps.find((op: any) => op.id === 'new-op'); - expect(newCompactOp).toBeDefined(); - expect((newCompactOp as any).sv).toBe(6); - // Existing op keeps its original sv=3 - const existingOp = uploadedData.recentOps.find( - (op: any) => op.id === 'existing-op', - ); - expect((existingOp as any).sv).toBe(3); - }); - - it('should compute oldestOpSyncVersion from first op in merged array', async () => { - // Existing file has an op tagged sv=2 - const existingData = createMockSyncData({ - syncVersion: 3, - recentOps: [ - { - id: 'old-op', - c: 'other-client', - a: 'HA', - o: 'ADD', - e: 'TASK', - d: 'task-old', - v: { otherClient: 1 }, - t: Date.now() - 60_000, - s: 1, - p: {}, - sv: 2, - }, - ], - }); - - mockProvider.downloadFile.and.returnValue( - Promise.resolve({ dataStr: addPrefix(existingData), rev: 'rev-1' }), - ); - await adapter.downloadOps(0); - - let uploadCallCount = 0; - let retryUploadedDataStr = ''; - mockProvider.uploadFile.and.callFake( - (_path: string, dataStr: string, _rev: string | null, _force: boolean) => { - uploadCallCount++; - if (uploadCallCount === 1) { - return Promise.reject(new UploadRevToMatchMismatchAPIError('Rev mismatch')); - } - retryUploadedDataStr = dataStr; - return Promise.resolve({ rev: 'rev-3' }); - }, - ); - - mockProvider.downloadFile.and.returnValue( - Promise.resolve({ dataStr: addPrefix(existingData), rev: 'rev-2' }), - ); - - const newOp = createMockSyncOp({ id: 'newer-op' }); - await adapter.uploadOps([newOp], 'client1'); - - const uploadedData = parseWithPrefix(retryUploadedDataStr); - // oldestOpSyncVersion should be sv of first (oldest) op = 2 - expect(uploadedData.oldestOpSyncVersion).toBe(2); - }); - }); - it('should clear cache after successful upload', async () => { const syncData = createMockSyncData({ syncVersion: 1 }); mockProvider.downloadFile.and.returnValue( 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 4583a2be65..80dbc6d991 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 @@ -429,26 +429,28 @@ export class FileBasedSyncAdapterService { } /** - * Uploads sync data with retry on revision mismatch. - * Returns the final sync version after a successful upload. + * Attempts to upload sync data. On revision mismatch, re-downloads once to + * distinguish a WebDAV server timestamp inconsistency (same rev → force-upload) + * from a genuine concurrent upload (different rev → throw so the next sync cycle + * can download the concurrent ops and retry with a consistent snapshot). */ - private async _uploadWithRetry( + private async _uploadWithMismatchFallback( provider: SyncProviderServiceInterface, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, newData: FileBasedSyncData, revToMatch: string | null, - ops: SyncOperation[], - clientId: string, ): Promise<{ finalSyncVersion: number }> { - // Initial attempt const uploadData = await this._encryptAndCompressHandler.compressAndEncryptData( cfg, encryptKey, newData, FILE_BASED_SYNC_CONSTANTS.FILE_VERSION, ); - this._assertUploadDataNotEmpty(uploadData, 'FileBasedSyncAdapter._uploadWithRetry'); + this._assertUploadDataNotEmpty( + uploadData, + 'FileBasedSyncAdapter._uploadWithMismatchFallback', + ); try { await provider.uploadFile( @@ -464,78 +466,39 @@ export class FileBasedSyncAdapterService { } } - // Retry loop on revision mismatch - const maxRetries = FILE_BASED_SYNC_CONSTANTS.MAX_UPLOAD_RETRIES; - let previousRev = revToMatch; + // Rev mismatch — re-download once to check whether a real concurrent upload occurred. + OpLog.normal( + 'FileBasedSyncAdapter: Rev mismatch detected, re-downloading to check...', + ); + const { rev: freshRev } = await this._downloadSyncFile(provider, cfg, encryptKey); - for (let attempt = 1; attempt <= maxRetries; attempt++) { - // Exponential backoff with jitter: base * 2^(attempt-1) + random(0..50%) - const baseDelay = FILE_BASED_SYNC_CONSTANTS.RETRY_BASE_DELAY_MS; - const delay = baseDelay * Math.pow(2, attempt - 1); - const jitter = Math.random() * delay * 0.5; - const totalDelay = Math.round(delay + jitter); - - OpLog.normal( - `FileBasedSyncAdapter: Rev mismatch detected, retry ${attempt}/${maxRetries} after ${totalDelay}ms...`, + if (freshRev === revToMatch) { + // Rev unchanged after re-download → WebDAV server timestamp/ETag inconsistency + // (no real concurrent upload). Reuse the already-built uploadData — the remote + // file is confirmed identical to what we were trying to overwrite, so no need + // to re-snapshot state, re-merge ops, or re-encrypt. + OpLog.warn( + 'FileBasedSyncAdapter: Rev unchanged after re-download, server has inconsistent ' + + 'timestamp handling. Force-uploading.', ); - await new Promise((resolve) => setTimeout(resolve, totalDelay)); - - const { data: freshData, rev: freshRev } = await this._downloadSyncFile( - provider, - cfg, - encryptKey, + await provider.uploadFile( + FILE_BASED_SYNC_CONSTANTS.SYNC_FILE, + uploadData, + freshRev, + true, ); - - // Reuse _buildMergedSyncData to rebuild from fresh data - const freshNewData = await this._buildMergedSyncData( - freshData, - ops, - clientId, - freshData.syncVersion, - ); - - const freshUploadData = - await this._encryptAndCompressHandler.compressAndEncryptData( - cfg, - encryptKey, - freshNewData, - FILE_BASED_SYNC_CONSTANTS.FILE_VERSION, - ); - this._assertUploadDataNotEmpty( - freshUploadData, - 'FileBasedSyncAdapter._uploadWithRetry(retry)', - ); - - // Same content hash means the file has not changed since our last download. - // Safe to overwrite unconditionally. - const isServerUnchanged = freshRev === previousRev; - if (isServerUnchanged) { - OpLog.warn( - 'FileBasedSyncAdapter: Rev unchanged after re-download, force-uploading.', - ); - } - - try { - await provider.uploadFile( - FILE_BASED_SYNC_CONSTANTS.SYNC_FILE, - freshUploadData, - freshRev, - isServerUnchanged, - ); - OpLog.normal(`FileBasedSyncAdapter: Retry ${attempt} upload successful`); - return { finalSyncVersion: freshNewData.syncVersion }; - } catch (retryErr) { - if (!(retryErr instanceof UploadRevToMatchMismatchAPIError)) { - throw retryErr; - } - // If force-upload was used (isServerUnchanged), this shouldn't happen - // but if it does, let the loop continue - previousRev = freshRev; - } + return { finalSyncVersion: newData.syncVersion }; } - throw new Error( - `FileBasedSyncAdapter: Upload failed after ${maxRetries} retries due to repeated revision mismatches`, + // Real concurrent upload: another client uploaded between our download and upload. + // Re-uploading here would embed a stale NgRx snapshot (their ops were never applied + // to our store), creating a recentOps/state inconsistency for fresh-bootstrap clients. + // Throw so SyncWrapperService can handle it as a known-transient condition. + // The next sync cycle downloads the concurrent ops (applying them to NgRx), then + // uploads with a consistent snapshot. + throw new UploadRevToMatchMismatchAPIError( + 'FileBasedSyncAdapter: Concurrent upload detected. Next sync cycle will ' + + 'download the concurrent ops and retry with a consistent snapshot.', ); } @@ -582,15 +545,13 @@ export class FileBasedSyncAdapterService { currentSyncVersion, ); - // Step 3: Upload with retry on revision mismatch - const { finalSyncVersion } = await this._uploadWithRetry( + // Step 3: Upload; on rev mismatch re-download once and either force-upload or throw + const { finalSyncVersion } = await this._uploadWithMismatchFallback( provider, cfg, encryptKey, newData, revToMatch, - ops, - clientId, ); // Step 4: Post-upload processing @@ -778,7 +739,7 @@ export class FileBasedSyncAdapterService { } : undefined; - const result = { + return { ops: limitedOps, hasMore, latestSeq, @@ -791,8 +752,6 @@ export class FileBasedSyncAdapterService { // This allows new clients to bootstrap with complete state, not just recent ops ...(snapshotStateWithArchives ? { snapshotState: snapshotStateWithArchives } : {}), }; - - return result; } // ═══════════════════════════════════════════════════════════════════════════ diff --git a/src/app/op-log/sync-providers/file-based/file-based-sync.types.ts b/src/app/op-log/sync-providers/file-based/file-based-sync.types.ts index 5cf1d2bf91..ad8dd7b90f 100644 --- a/src/app/op-log/sync-providers/file-based/file-based-sync.types.ts +++ b/src/app/op-log/sync-providers/file-based/file-based-sync.types.ts @@ -124,10 +124,4 @@ export const FILE_BASED_SYNC_CONSTANTS = { /** Storage key prefix for last known sync version */ SYNC_VERSION_STORAGE_KEY_PREFIX: 'FILE_SYNC_VERSION_', - - /** Maximum number of upload retry attempts on revision mismatch */ - MAX_UPLOAD_RETRIES: 2, - - /** Base delay in ms for exponential backoff between retries */ - RETRY_BASE_DELAY_MS: 500, } as const; diff --git a/src/app/op-log/sync-providers/file-based/webdav/webdav-base-provider.ts b/src/app/op-log/sync-providers/file-based/webdav/webdav-base-provider.ts index 2a73f47ca1..45bb9723fb 100644 --- a/src/app/op-log/sync-providers/file-based/webdav/webdav-base-provider.ts +++ b/src/app/op-log/sync-providers/file-based/webdav/webdav-base-provider.ts @@ -97,7 +97,7 @@ export abstract class WebdavBaseProvider< return { rev: result.rev }; } catch (e) { // Translate RemoteFileChangedUnexpectedly to UploadRevToMatchMismatchAPIError - // so the retry mechanism in FileBasedSyncAdapterService._uploadWithRetry() can handle it + // so FileBasedSyncAdapterService._uploadWithMismatchFallback() can handle it if (e instanceof RemoteFileChangedUnexpectedly) { throw new UploadRevToMatchMismatchAPIError(e.message); } 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 15f3cc4ba4..34c59d4aca 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 @@ -7,14 +7,11 @@ describe('File-Based Sync Integration - Conflict Resolution', () => { let harness: FileBasedSyncTestHarness; beforeEach(() => { - // Eliminate real retry delays to prevent slow tests and timer accumulation - (FILE_BASED_SYNC_CONSTANTS as any).RETRY_BASE_DELAY_MS = 0; harness = FileBasedSyncTestHarness.create({}); }); afterEach(() => { harness.reset(); - (FILE_BASED_SYNC_CONSTANTS as any).RETRY_BASE_DELAY_MS = 500; }); describe('syncVersion Mismatch', () => { @@ -279,10 +276,19 @@ describe('File-Based Sync Integration - Conflict Resolution', () => { title: 'Task B', }); - // Client A uploads first + // Client A uploads first (changes the rev) await clientA.uploadOps([opA]); - // Client B uploads (should merge without conflict) + // Client B's upload throws: genuine concurrent upload detected + // (A changed the rev, so B's cached rev is now stale) + await expectAsync(clientB.uploadOps([opB])).toBeRejectedWithError( + UploadRevToMatchMismatchAPIError, + ); + + // Client B downloads (next sync cycle: picks up A's new op and fresh rev) + await clientB.downloadOps(); + + // Client B retries upload with the fresh state — succeeds const responseB = await clientB.uploadOps([opB]); expect(responseB.results[0].accepted).toBe(true); diff --git a/src/app/op-log/testing/integration/file-based-sync/edge-cases.integration.spec.ts b/src/app/op-log/testing/integration/file-based-sync/edge-cases.integration.spec.ts index e4dc01baaa..853356f8e6 100644 --- a/src/app/op-log/testing/integration/file-based-sync/edge-cases.integration.spec.ts +++ b/src/app/op-log/testing/integration/file-based-sync/edge-cases.integration.spec.ts @@ -1,6 +1,7 @@ import { FileBasedSyncTestHarness } from '../helpers/file-based-sync-test-harness'; import { FILE_BASED_SYNC_CONSTANTS } from '../../../sync-providers/file-based/file-based-sync.types'; import { SyncOperation } from '../../../sync-providers/provider.interface'; +import { UploadRevToMatchMismatchAPIError } from '../../../core/errors/sync-errors'; describe('File-Based Sync Integration - Edge Cases', () => { let harness: FileBasedSyncTestHarness; @@ -509,10 +510,19 @@ describe('File-Based Sync Integration - Encryption Round-Trip', () => { }); await clientA.uploadOps([opA]); - // Client B uploads (merges with A's data) + // Client B's upload throws: genuine concurrent upload detected + // (A changed the rev, so B's cached rev is now stale) const opB = clientB.createOp('Task', 'task-b', 'CRT', 'TaskActionTypes.ADD_TASK', { title: 'From B', }); + await expectAsync(clientB.uploadOps([opB])).toBeRejectedWithError( + UploadRevToMatchMismatchAPIError, + ); + + // Client B downloads (next sync cycle: picks up A's new op and fresh rev) + await clientB.downloadOps(); + + // Client B retries upload — succeeds with the fresh merged state const responseB = await clientB.uploadOps([opB]); expect(responseB.results[0].accepted).toBe(true);