From cb6780933bf05362e3a3801c15b6fb5edb700b00 Mon Sep 17 00:00:00 2001 From: aakhter Date: Tue, 7 Jul 2026 05:57:25 -0400 Subject: [PATCH] =?UTF-8?q?fix(sync):=20atomic=20file-based=20sync=20write?= =?UTF-8?q?s=20=E2=80=94=20backup-before-overwrite=20+=20remove=20force-ov?= =?UTF-8?q?erwrite=20race=20(#8786)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sync): atomic file-based sync writes - backup-before-overwrite + remove force-overwrite race Adds a two-phase write for file-based sync (Dropbox/WebDAV/LocalFile): the current remote sync-data.json is copied to sync-data.json.bak before being overwritten (non-fatal if unsupported), and downloads recover from .bak when the main file is corrupt/empty/unparseable, surfacing a recovery notice. Removes the unconditional isForceOverwrite=true on the rev-match retry path in _uploadWithMismatchFallback: retries stay conditional and a genuine concurrent change throws a retryable error so the next cycle merges the concurrent ops. Force-overwrite of the primary file now originates only from explicit user actions. Also reconciles a stale in-memory syncVersion against the remote on download. SPAP-8 * fix(sync): recover encrypted/compressed corrupt files and heal the primary Addresses review feedback on the atomic-writes .bak recovery path: 1. Recovery now triggers for encrypted/compressed primary files. A truncated or garbage ENCRYPTED/COMPRESSED sync-data.json fails during decrypt/decompress (DecryptError/DecompressError), not JSON parse, so those cases were never recoverable — .bak recovery silently no-opped for exactly the users who enable encryption or compression, the interrupted-write scenario the feature targets. Add both errors to _isRecoverableCorruption. Safe: an undecodable .bak makes _recoverFromBackup return null and the original error still surfaces. 2. Recovery heals the primary instead of getting stuck. _downloadSyncFile now annotates the corrupt primary's rev onto the decode error, and the recovery path seeds the sync-cycle cache with THAT rev (not the .bak rev). The next conditional upload then matches sync-data.json and overwrites (repairs) it, instead of mismatching forever and re-recovering/re-failing every cycle. Also de-dup the recovery snack per corrupt rev so a download-only client (which cannot heal the file) does not re-toast on every sync. Tests: cover both via a REAL decode failure (a compressed body that is not valid gzip, producing a genuine DecompressError rather than an injected error type) — one asserting .bak recovery, one asserting the follow-up upload conditions on the corrupt primary rev and heals the file. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- .../file-based-sync-adapter.service.spec.ts | 383 +++++++++++++++++- .../file-based-sync-adapter.service.ts | 327 ++++++++++++--- src/app/t.const.ts | 1 + src/assets/i18n/en.json | 1 + 4 files changed, 638 insertions(+), 74 deletions(-) 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 540a25d805..cdc32c87e5 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 @@ -20,12 +20,14 @@ import { ArchiveDbAdapter } from '../../../core/persistence/archive-db-adapter.s import { ArchiveModel } from '../../../features/time-tracking/time-tracking.model'; import { StateSnapshotService } from '../../backup/state-snapshot.service'; import { DEFAULT_GLOBAL_CONFIG } from '../../../features/config/default-global-config.const'; +import { SnackService } from '../../../core/snack/snack.service'; describe('FileBasedSyncAdapterService', () => { let service: FileBasedSyncAdapterService; let mockProvider: jasmine.SpyObj>; let mockArchiveDbAdapter: jasmine.SpyObj; let mockStateSnapshotService: jasmine.SpyObj; + let mockSnackService: jasmine.SpyObj; let adapter: OperationSyncCapable; const mockCfg: EncryptAndCompressCfg = { @@ -151,11 +153,14 @@ describe('FileBasedSyncAdapterService', () => { }, } as any); + mockSnackService = jasmine.createSpyObj('SnackService', ['open']); + TestBed.configureTestingModule({ providers: [ FileBasedSyncAdapterService, { provide: ArchiveDbAdapter, useValue: mockArchiveDbAdapter }, { provide: StateSnapshotService, useValue: mockStateSnapshotService }, + { provide: SnackService, useValue: mockSnackService }, ], }); @@ -397,24 +402,38 @@ describe('FileBasedSyncAdapterService', () => { UploadRevToMatchMismatchAPIError, ); - // Only one upload attempt (no retry loop) - expect(mockProvider.uploadFile).toHaveBeenCalledTimes(1); + // Only one CONDITIONAL attempt on the MAIN sync file (no retry loop), and it + // must never force-overwrite. A separate .bak backup write is allowed. + const mainUploads = mockProvider.uploadFile.calls + .allArgs() + .filter((args) => args[0] === FILE_BASED_SYNC_CONSTANTS.SYNC_FILE); + expect(mainUploads.length).toBe(1); + expect(mainUploads.every((args) => args[3] === false)).toBe(true); // Download called twice: initial cache + re-download to check rev expect(mockProvider.downloadFile).toHaveBeenCalledTimes(2); }); - it('should force upload on retry when freshRev equals original revToMatch', async () => { + it('retries CONDITIONALLY (never force) when freshRev equals original revToMatch (SPAP-8)', async () => { const syncData = createMockSyncData({ syncVersion: 1 }); - // Initial download returns rev-1 + // Every download returns the same rev-1 → server rev inconsistency, NOT a real + // concurrent upload. The old code force-overwrote here; the new code must retry + // the conditional upload and NEVER pass isForceOverwrite=true. mockProvider.downloadFile.and.returnValue( Promise.resolve({ dataStr: addPrefix(syncData), rev: 'rev-1' }), ); - let uploadCalls = 0; + const mainUploadForceFlags: (boolean | undefined)[] = []; + let mainUploadAttempts = 0; mockProvider.uploadFile.and.callFake( - (_path: string, _dataStr: string, _rev: string | null, _force: boolean) => { - uploadCalls++; - if (uploadCalls === 1) { + (path: string, _dataStr: string, _rev: string | null, force?: boolean) => { + if (path === FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE) { + // Non-fatal backup write — irrelevant to the force assertion. + return Promise.resolve({ rev: 'bak-1' }); + } + mainUploadAttempts++; + mainUploadForceFlags.push(force); + if (mainUploadAttempts === 1) { + // First conditional attempt hits a spurious rev mismatch. return Promise.reject(new UploadRevToMatchMismatchAPIError('Rev mismatch')); } return Promise.resolve({ rev: 'rev-2' }); @@ -424,18 +443,14 @@ describe('FileBasedSyncAdapterService', () => { // Download to populate cache with rev-1 await adapter.downloadOps(0); - // Re-download on retry also returns rev-1 (same rev = server timestamp inconsistency) - // mockProvider.downloadFile already returns rev-1 - 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: true - const retryCall = mockProvider.uploadFile.calls.argsFor(1); - expect(retryCall[3]).toBe(true); // isForceOverwrite + // Two attempts on the main file: initial + one conditional retry. + expect(mainUploadAttempts).toBe(2); + // CRUCIAL (SPAP-8): no attempt on the main sync file used force-overwrite. + expect(mainUploadForceFlags.every((f) => f === false)).toBe(true); }); it('should clear cache after successful upload', async () => { @@ -1861,4 +1876,340 @@ describe('FileBasedSyncAdapterService', () => { expect(result.ops).toEqual([]); }); }); + + describe('atomic remote writes (SPAP-8)', () => { + const compactOp = (id: string, client = 'client1'): Record => ({ + id, + c: client, + a: 'HA', + o: 'ADD', + e: 'TASK', + d: `task-${id}`, + v: { [client]: 1 }, + t: Date.now(), + s: 1, + p: { title: `Task ${id}` }, + }); + + it('(a) writes .bak with the PREVIOUS remote content before overwriting sync-data.json', async () => { + const prevData = createMockSyncData({ + syncVersion: 3, + clientId: 'prev-client', + recentOps: [compactOp('prev-op') as never], + }); + mockProvider.downloadFile.and.returnValue( + Promise.resolve({ dataStr: addPrefix(prevData), rev: 'rev-1' }), + ); + + const uploadOrder: string[] = []; + let backupDataStr = ''; + mockProvider.uploadFile.and.callFake((path: string, dataStr: string) => { + uploadOrder.push(path); + if (path === FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE) { + backupDataStr = dataStr; + } + return Promise.resolve({ rev: 'rev-2' }); + }); + + // Populate the sync-cycle cache (mirrors a real download→upload cycle). + await adapter.downloadOps(0); + + const op = createMockSyncOp(); + await adapter.uploadOps([op], 'client1'); + + const bakIdx = uploadOrder.indexOf(FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE); + const mainIdx = uploadOrder.indexOf(FILE_BASED_SYNC_CONSTANTS.SYNC_FILE); + // Backup must exist AND be written strictly before the main overwrite. + expect(bakIdx).toBeGreaterThanOrEqual(0); + expect(mainIdx).toBeGreaterThanOrEqual(0); + expect(bakIdx).toBeLessThan(mainIdx); + + // The backup must contain the PREVIOUS remote content, not the new upload. + const bak = parseWithPrefix(backupDataStr); + expect(bak.syncVersion).toBe(3); + expect(bak.clientId).toBe('prev-client'); + expect(bak.recentOps[0].id).toBe('prev-op'); + }); + + it('(a) does NOT write a backup on first sync when no remote file exists yet', async () => { + mockProvider.downloadFile.and.throwError( + new RemoteFileNotFoundAPIError('sync-data.json'), + ); + mockProvider.uploadFile.and.returnValue(Promise.resolve({ rev: 'rev-1' })); + + await adapter.uploadOps([createMockSyncOp()], 'client1'); + + expect(mockProvider.uploadFile).not.toHaveBeenCalledWith( + FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE, + jasmine.any(String), + jasmine.anything(), + jasmine.anything(), + ); + }); + + it('(a) backup-write failure is non-fatal — the main upload still succeeds', async () => { + const prevData = createMockSyncData({ syncVersion: 2 }); + mockProvider.downloadFile.and.returnValue( + Promise.resolve({ dataStr: addPrefix(prevData), rev: 'rev-1' }), + ); + mockProvider.uploadFile.and.callFake((path: string) => { + if (path === FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE) { + // Provider without copy support / transient failure on the backup. + return Promise.reject(new Error('backup not supported')); + } + return Promise.resolve({ rev: 'rev-2' }); + }); + + await adapter.downloadOps(0); + + const result = await adapter.uploadOps([createMockSyncOp()], 'client1'); + expect(result.results[0].accepted).toBe(true); + }); + + it('(b) recovers from .bak when the main file is corrupt and surfaces a recovery snack', async () => { + const backupData = createMockSyncData({ + syncVersion: 2, + recentOps: [compactOp('recovered-op') as never], + }); + mockProvider.downloadFile.and.callFake((path: string) => { + if (path === FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE) { + return Promise.resolve({ dataStr: addPrefix(backupData), rev: 'bak-rev-1' }); + } + return Promise.reject( + new SyncDataCorruptedError('corrupt', FILE_BASED_SYNC_CONSTANTS.SYNC_FILE), + ); + }); + + const result = await adapter.downloadOps(0); + + expect(result.ops.length).toBe(1); + expect(result.ops[0].op.id).toBe('recovered-op'); + expect(result.latestSeq).toBe(2); + // User-visible recovery notice. + expect(mockSnackService.open).toHaveBeenCalled(); + }); + + it('(b) recovers from .bak when the main file is empty (InvalidDataSPError)', async () => { + const backupData = createMockSyncData({ + syncVersion: 4, + recentOps: [compactOp('from-empty-recovery') as never], + }); + mockProvider.downloadFile.and.callFake((path: string) => { + if (path === FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE) { + return Promise.resolve({ dataStr: addPrefix(backupData), rev: 'bak-rev-2' }); + } + return Promise.reject( + new InvalidDataSPError('empty body', FILE_BASED_SYNC_CONSTANTS.SYNC_FILE), + ); + }); + + const result = await adapter.downloadOps(0); + + expect(result.ops.length).toBe(1); + expect(result.latestSeq).toBe(4); + expect(mockSnackService.open).toHaveBeenCalled(); + }); + + it('(b) rethrows the original corruption error when no usable backup exists', async () => { + mockProvider.downloadFile.and.callFake((path: string) => { + if (path === FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE) { + return Promise.reject(new RemoteFileNotFoundAPIError('no backup')); + } + return Promise.reject( + new SyncDataCorruptedError('corrupt', FILE_BASED_SYNC_CONSTANTS.SYNC_FILE), + ); + }); + + await expectAsync(adapter.downloadOps(0)).toBeRejectedWith( + jasmine.any(SyncDataCorruptedError), + ); + expect(mockSnackService.open).not.toHaveBeenCalled(); + }); + + it('(b) recovers from .bak when the primary file fails to DECODE (real DecompressError, not an injected error)', async () => { + // The tests above inject SyncDataCorruptedError/InvalidDataSPError directly. + // This one drives a REAL decode failure end-to-end: the primary file's prefix + // claims gzip compression but the body is not valid gzip, so the actual + // decompressAndDecryptData path throws DecompressError. The encrypted-file + // case (DecryptError) is symmetric. Without both in _isRecoverableCorruption, + // recovery silently no-ops for users who enable compression or encryption. + const backupData = createMockSyncData({ + syncVersion: 5, + recentOps: [compactOp('recovered-from-decode-failure') as never], + }); + const undecodableMain = + getSyncFilePrefix({ isCompress: true, isEncrypt: false, modelVersion: 2 }) + + 'this-is-not-valid-gzip-base64'; + mockProvider.downloadFile.and.callFake((path: string) => { + if (path === FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE) { + return Promise.resolve({ + dataStr: addPrefix(backupData), + rev: 'bak-rev-decode', + }); + } + return Promise.resolve({ dataStr: undecodableMain, rev: 'corrupt-main-rev' }); + }); + + const result = await adapter.downloadOps(0); + + expect(result.ops.length).toBe(1); + expect(result.ops[0].op.id).toBe('recovered-from-decode-failure'); + expect(result.latestSeq).toBe(5); + expect(mockSnackService.open).toHaveBeenCalled(); + }); + + it('(b) after recovery, the next upload heals the corrupt primary via ITS rev (no revToMatch pollution)', async () => { + // Regression for the self-perpetuating degraded state: recovery must seed the + // cache with the CORRUPT PRIMARY rev, not the .bak rev, so the follow-up + // conditional upload matches sync-data.json and overwrites (heals) it. + const backupData = createMockSyncData({ syncVersion: 2 }); + const CORRUPT_MAIN_REV = 'corrupt-main-rev-42'; + const undecodableMain = + getSyncFilePrefix({ isCompress: true, isEncrypt: false, modelVersion: 2 }) + + 'not-valid-gzip'; + mockProvider.downloadFile.and.callFake((path: string) => { + if (path === FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE) { + return Promise.resolve({ dataStr: addPrefix(backupData), rev: 'bak-rev-heal' }); + } + return Promise.resolve({ dataStr: undecodableMain, rev: CORRUPT_MAIN_REV }); + }); + + const mainRevToMatch: (string | null)[] = []; + mockProvider.uploadFile.and.callFake( + (path: string, _dataStr: string, revToMatch: string | null) => { + if (path === FILE_BASED_SYNC_CONSTANTS.SYNC_FILE) { + mainRevToMatch.push(revToMatch); + } + return Promise.resolve({ rev: 'healed-rev' }); + }, + ); + + // Download recovers from .bak; a subsequent upload should heal the primary. + await adapter.downloadOps(0); + await adapter.uploadOps([createMockSyncOp()], 'client1'); + + expect(mainRevToMatch).toContain(CORRUPT_MAIN_REV); + expect(mainRevToMatch).not.toContain('bak-rev-heal'); + }); + + it('(c) never issues isForceOverwrite=true on the rev-mismatch retry path', async () => { + const syncData = createMockSyncData({ syncVersion: 1 }); + mockProvider.downloadFile.and.returnValue( + Promise.resolve({ dataStr: addPrefix(syncData), rev: 'rev-1' }), + ); + + const mainForceFlags: (boolean | undefined)[] = []; + let mainAttempts = 0; + mockProvider.uploadFile.and.callFake( + (path: string, _d: string, _r: string | null, force?: boolean) => { + if (path === FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE) { + return Promise.resolve({ rev: 'bak-1' }); + } + mainAttempts++; + mainForceFlags.push(force); + // Deterministic conditional rejection → exhausts retries → retryable throw. + return Promise.reject(new UploadRevToMatchMismatchAPIError('Rev mismatch')); + }, + ); + + await adapter.downloadOps(0); + + await expectAsync( + adapter.uploadOps([createMockSyncOp()], 'client1'), + ).toBeRejectedWithError(UploadRevToMatchMismatchAPIError); + + // Multiple attempts occurred, but NOT ONE forced. + expect(mainAttempts).toBeGreaterThan(1); + expect(mainForceFlags.every((f) => f === false)).toBe(true); + }); + + it("(d) interleaved clients: client A's ops survive when B races; B fails retryably without forcing", async () => { + // Honest rev model: the remote rev changes whenever its content changes. + let remote = { + dataStr: addPrefix(createMockSyncData({ syncVersion: 1, recentOps: [] })), + rev: 'rev-1', + }; + const aOp = compactOp('op-A', 'client-A'); + + mockProvider.downloadFile.and.callFake((path: string) => { + if (path === FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE) { + return Promise.reject(new RemoteFileNotFoundAPIError('no bak')); + } + return Promise.resolve({ dataStr: remote.dataStr, rev: remote.rev }); + }); + + let sawForceOnMain = false; + mockProvider.uploadFile.and.callFake( + (path: string, dataStr: string, revToMatch: string | null, force?: boolean) => { + if (path === FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE) { + return Promise.resolve({ rev: 'bak-1' }); + } + if (force) { + sawForceOnMain = true; + } + // Conditional upload: reject if the expected rev no longer matches remote. + if (!force && revToMatch !== null && revToMatch !== remote.rev) { + return Promise.reject(new UploadRevToMatchMismatchAPIError('rev changed')); + } + remote = { dataStr, rev: `rev-${Math.random()}` }; + return Promise.resolve({ rev: remote.rev }); + }, + ); + + // Client B downloads (caches rev-1). + await adapter.downloadOps(0); + + // Client A concurrently commits op-A → remote advances to rev-2. + remote = { + dataStr: addPrefix( + createMockSyncData({ syncVersion: 2, recentOps: [aOp as never] }), + ), + rev: 'rev-2', + }; + + // Client B now uploads its own op; its cached rev-1 is stale. + const bOp = createMockSyncOp({ id: 'op-B', clientId: 'client-B' }); + await expectAsync(adapter.uploadOps([bOp], 'client-B')).toBeRejectedWithError( + UploadRevToMatchMismatchAPIError, + ); + + // A's op must NOT have vanished from the remote, and B must not have forced. + const finalRemote = parseWithPrefix(remote.dataStr); + expect(finalRemote.recentOps.some((o) => o.id === 'op-A')).toBe(true); + expect(sawForceOnMain).toBe(false); + }); + + it('(guard) reconciles a stale in-memory expected syncVersion that is ahead of remote', async () => { + // Establish expected syncVersion = 5. + mockProvider.downloadFile.and.returnValue( + Promise.resolve({ + dataStr: addPrefix(createMockSyncData({ syncVersion: 5, recentOps: [] })), + rev: 'rev-5', + }), + ); + await adapter.downloadOps(0); + + // Remote regresses to syncVersion 2 (e.g. another client's recovery snapshot). + mockProvider.downloadFile.and.returnValue( + Promise.resolve({ + dataStr: addPrefix(createMockSyncData({ syncVersion: 2, recentOps: [] })), + rev: 'rev-2', + }), + ); + const dl = await adapter.downloadOps(1); + expect(dl.gapDetected).toBe(true); + + // The next upload must build on the REMOTE version (→ 3), proving the stale + // in-memory counter (5) was reconciled rather than trusted. + let uploadedMain = ''; + mockProvider.uploadFile.and.callFake((path: string, dataStr: string) => { + if (path === FILE_BASED_SYNC_CONSTANTS.SYNC_FILE) { + uploadedMain = dataStr; + } + return Promise.resolve({ rev: 'rev-new' }); + }); + await adapter.uploadOps([createMockSyncOp()], 'client1'); + expect(parseWithPrefix(uploadedMain).syncVersion).toBe(3); + }); + }); }); 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 d3875983ba..572541ca62 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 @@ -1,4 +1,4 @@ -import { inject, Injectable } from '@angular/core'; +import { inject, Injectable, Injector } from '@angular/core'; import { SyncProviderId } from '../provider.const'; import { FileSyncProvider, @@ -27,12 +27,17 @@ import { } from './file-based-sync.types'; import { OpLog } from '../../../core/log'; import { + DecompressError, + DecryptError, InvalidDataSPError, + JsonParseError, LegacySyncFormatDetectedError, RemoteFileNotFoundAPIError, SyncDataCorruptedError, UploadRevToMatchMismatchAPIError, } 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 { ArchiveDbAdapter } from '../../../core/persistence/archive-db-adapter.service'; import { StateSnapshotService } from '../../backup/state-snapshot.service'; @@ -77,6 +82,16 @@ export class FileBasedSyncAdapterService { private _encryptAndCompressHandler = new EncryptAndCompressHandlerService(); private _archiveDbAdapter = inject(ArchiveDbAdapter); private _stateSnapshotService = inject(StateSnapshotService); + // Resolved lazily (not eagerly injected) to avoid a construction-time DI cycle: + // SnackService's dependency graph transitively reaches the sync providers. We only + // need it for the non-fatal backup-recovery notice, well after construction. + private _injector = inject(Injector); + + /** + * Max CONDITIONAL upload retries after a rev mismatch before surfacing a + * retryable error. The retry path never force-overwrites the primary sync file. + */ + private readonly _MAX_UPLOAD_RETRIES = 2; /** Expected sync version for optimistic locking, keyed by provider+user */ private _expectedSyncVersions = new Map(); @@ -84,6 +99,13 @@ export class FileBasedSyncAdapterService { /** Local sequence counters (simulates server seq for file-based) */ private _localSeqCounters = new Map(); + /** + * Last corrupt primary rev we surfaced a backup-recovery notice for, keyed by + * provider+user. Prevents re-toasting the same corruption every sync cycle for a + * download-only client that cannot heal the primary file itself. + */ + private _lastRecoveredCorruptRev = new Map(); + /** Cache for downloaded sync data within a sync cycle (avoids redundant downloads) */ private _syncCycleCache = new Map< string, @@ -444,10 +466,22 @@ export class FileBasedSyncAdapterService { } /** - * 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). + * Uploads sync data with a CONDITIONAL write (optimistic lock on `revToMatch`). + * + * On a rev mismatch it re-downloads to classify the failure, but it NEVER + * force-overwrites the primary sync file — that would silently clobber a + * concurrent client's ops if the rev check is fooled (caching / rev reuse / + * eventual consistency). Force-overwrite of `sync-data.json` is reachable ONLY + * from explicit user actions (forceUploadLocalState / conflict "Use Local"). + * + * - Same rev after re-download → transient server rev/timestamp inconsistency + * (no real concurrent upload). Retry the CONDITIONAL upload (bounded by + * `_MAX_UPLOAD_RETRIES`); if still failing, surface a retryable error. + * - Different rev after re-download → a genuine concurrent upload. Throw a + * retryable error so the next sync cycle downloads the concurrent ops + * (applying them to the store) and rebuilds a consistent snapshot via + * `_buildMergedSyncData()`. Re-uploading here would embed a stale in-memory + * snapshot whose state never saw those ops. */ private async _uploadWithMismatchFallback( provider: FileSyncProvider, @@ -467,53 +501,61 @@ export class FileBasedSyncAdapterService { 'FileBasedSyncAdapter._uploadWithMismatchFallback', ); - try { - await provider.uploadFile( - FILE_BASED_SYNC_CONSTANTS.SYNC_FILE, - uploadData, - revToMatch, - false, - ); - return { finalSyncVersion: newData.syncVersion }; - } catch (e) { - if (!(e instanceof UploadRevToMatchMismatchAPIError)) { - throw e; + const maxAttempts = 1 + this._MAX_UPLOAD_RETRIES; + let currentRevToMatch = revToMatch; + + for (let attempt = 1; attempt <= maxAttempts; attempt++) { + try { + // CONDITIONAL upload only (isForceOverwrite=false). Never forces. + await provider.uploadFile( + FILE_BASED_SYNC_CONSTANTS.SYNC_FILE, + uploadData, + currentRevToMatch, + false, + ); + return { finalSyncVersion: newData.syncVersion }; + } catch (e) { + if (!(e instanceof UploadRevToMatchMismatchAPIError)) { + throw e; + } } - } - // 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); + if (attempt === maxAttempts) { + break; + } - 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. + // Rev mismatch — re-download 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); + + if (freshRev !== currentRevToMatch) { + // Genuine concurrent upload: another client wrote between our download and + // upload. Do NOT overwrite. Throw retryable so the next sync cycle downloads + // the concurrent ops (applying them to the store) and rebuilds a consistent + // snapshot. This guarantees the concurrent client's ops are never clobbered. + throw new UploadRevToMatchMismatchAPIError( + 'FileBasedSyncAdapter: Concurrent upload detected. Next sync cycle will ' + + 'download the concurrent ops and retry with a consistent snapshot.', + ); + } + + // Same rev after re-download → transient server rev/timestamp inconsistency + // (the remote is confirmed identical to what we were trying to overwrite). + // Retry the CONDITIONAL upload — never force. OpLog.warn( - 'FileBasedSyncAdapter: Rev unchanged after re-download, server has inconsistent ' + - 'timestamp handling. Force-uploading.', + 'FileBasedSyncAdapter: Rev unchanged after re-download (server rev/timestamp ' + + 'inconsistency). Retrying conditional upload without force-overwrite.', ); - await provider.uploadFile( - FILE_BASED_SYNC_CONSTANTS.SYNC_FILE, - uploadData, - freshRev, - true, - ); - return { finalSyncVersion: newData.syncVersion }; + currentRevToMatch = freshRev; } - // 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. + // Retries exhausted without a successful conditional upload. Surface a retryable + // error rather than silently force-overwriting the primary sync file. throw new UploadRevToMatchMismatchAPIError( - 'FileBasedSyncAdapter: Concurrent upload detected. Next sync cycle will ' + - 'download the concurrent ops and retry with a consistent snapshot.', + 'FileBasedSyncAdapter: Upload retries exhausted after repeated rev mismatch. ' + + 'Sync will retry on the next cycle.', ); } @@ -560,7 +602,16 @@ export class FileBasedSyncAdapterService { currentSyncVersion, ); - // Step 3: Upload; on rev mismatch re-download once and either force-upload or throw + // Step 2.5: Backup-before-overwrite (two-phase write). Copy the CURRENT remote + // content to sync-data.json.bak before overwriting the primary file, so an + // interrupted/corrupt write can be recovered on the next download. Non-fatal: + // a provider without copy support must still be able to sync. + if (fileExists && currentData) { + await this._backupCurrentRemote(provider, cfg, encryptKey, currentData); + } + + // Step 3: Upload conditionally; on rev mismatch re-download and retry + // conditionally or throw retryably (never force-overwrite). const { finalSyncVersion } = await this._uploadWithMismatchFallback( provider, cfg, @@ -628,7 +679,41 @@ export class FileBasedSyncAdapterService { latestSeq: 0, }; } - throw e; + if (this._isRecoverableCorruption(e)) { + // Primary sync-data.json is corrupt/empty/unparseable (e.g. interrupted + // write). Try to recover from the .bak artifact before failing. + const recovered = await this._recoverFromBackup(provider, cfg, encryptKey); + if (recovered) { + OpLog.warn( + 'FileBasedSyncAdapter: Primary sync file unreadable; recovered from backup', + ); + syncData = recovered.data; + // Seed the cache with the CORRUPT PRIMARY file's rev (annotated onto the + // error in _downloadSyncFile), not the .bak rev. The next conditional + // upload then matches sync-data.json and OVERWRITES (heals) it. Using the + // .bak rev would mismatch the primary on every upload, leaving sync stuck + // in a re-recover/re-fail loop. Fall back to the .bak rev if the primary + // rev is unavailable (a later mismatch just re-recovers — no data loss). + const primaryRev = + (e as { primaryRev?: string } | null)?.primaryRev ?? recovered.rev; + rev = primaryRev; + this._setCachedSyncData(providerKey, syncData, rev); + // De-dup the user-facing notice: only surface it the first time we see a + // given corrupt primary rev, so a download-only client (which cannot heal + // the file itself) does not re-toast on every sync cycle. + if (this._lastRecoveredCorruptRev.get(providerKey) !== primaryRev) { + this._lastRecoveredCorruptRev.set(providerKey, primaryRev); + // Lazy resolve — absent in some unit harnesses (returns null → no notice). + this._injector + .get(SnackService, null) + ?.open({ msg: T.F.SYNC.S.SYNC_DATA_RECOVERED_FROM_BACKUP }); + } + } else { + throw e; + } + } else { + throw e; + } } // Detect syncVersion reset (e.g., another client uploaded a snapshot). @@ -638,6 +723,19 @@ export class FileBasedSyncAdapterService { const versionWasReset = previousExpectedVersion > 0 && syncData.syncVersion < previousExpectedVersion; + // Guard against stale in-memory state: if the persisted expected syncVersion is + // AHEAD of what the remote file reports, our counter is stale (e.g. after another + // client uploaded a lower-version recovery snapshot). Log and reconcile to the + // remote's authoritative value rather than trusting the stale counter — the + // `_expectedSyncVersions.set(...)` below always adopts the remote version, and + // `versionWasReset` drives gap detection so the caller re-downloads from seq 0. + if (previousExpectedVersion > syncData.syncVersion) { + OpLog.warn( + `FileBasedSyncAdapter: Persisted expected syncVersion (${previousExpectedVersion}) is ` + + `ahead of remote (${syncData.syncVersion}); reconciling to remote (stale in-memory counter).`, + ); + } + // 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 @@ -912,6 +1010,106 @@ export class FileBasedSyncAdapterService { } } + /** + * Writes the CURRENT remote content to sync-data.json.bak before the primary + * file is overwritten (two-phase write). We re-encode the already-in-hand + * `currentData` from `_getCurrentSyncState()` rather than issuing another GET. + * + * MUST be non-fatal: a provider that cannot write the backup (e.g. no copy + * support, transient failure) must still be able to sync. The backup is a + * recovery artifact — losing it only degrades recoverability, never sync. + * + * Force-overwrite is used here because .bak is a disposable recovery artifact, + * NOT the source-of-truth sync file — the lost-update guarantee on + * sync-data.json is unaffected. + */ + private async _backupCurrentRemote( + provider: FileSyncProvider, + cfg: EncryptAndCompressCfg, + encryptKey: string | undefined, + currentData: FileBasedSyncData, + ): Promise { + try { + const backupData = await this._encryptAndCompressHandler.compressAndEncryptData( + cfg, + encryptKey, + currentData, + FILE_BASED_SYNC_CONSTANTS.FILE_VERSION, + ); + if (!backupData || backupData.trim().length === 0) { + OpLog.warn( + 'FileBasedSyncAdapter: Skipping backup — encoded backup data was empty', + ); + return; + } + await provider.uploadFile( + FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE, + backupData, + null, + true, + ); + OpLog.normal('FileBasedSyncAdapter: Wrote backup before overwrite'); + } catch (e) { + // Non-fatal by design — proceed with the primary upload regardless. + OpLog.warn( + 'FileBasedSyncAdapter: Backup-before-overwrite failed (non-fatal), proceeding', + e, + ); + } + } + + /** + * Whether a download error indicates a corrupt/empty/unparseable primary file + * that we should attempt to recover from the .bak artifact. Missing files are + * NOT included — that is a legitimate fresh-start signal handled separately. + */ + private _isRecoverableCorruption(e: unknown): boolean { + return ( + e instanceof SyncDataCorruptedError || + // Covers EmptyRemoteBodySPError (empty file) via its InvalidDataSPError base. + e instanceof InvalidDataSPError || + e instanceof JsonParseError || + // A truncated/garbage ENCRYPTED or COMPRESSED primary file fails during the + // decrypt/decompress stage rather than JSON parse. Without these, .bak + // recovery silently no-ops for exactly the users who enable encryption or + // compression — the interrupted-write case this feature targets. Safe to + // include: if the .bak is also undecodable, _recoverFromBackup returns null + // and the original error still surfaces. + e instanceof DecryptError || + e instanceof DecompressError + ); + } + + /** + * Attempts to recover sync data from sync-data.json.bak. Returns null if the + * backup is missing, unreadable, or has an unsupported version. + */ + private async _recoverFromBackup( + provider: FileSyncProvider, + cfg: EncryptAndCompressCfg, + encryptKey: string | undefined, + ): Promise<{ data: FileBasedSyncData; rev: string } | null> { + try { + const response = await provider.downloadFile(FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE); + const data = + await this._encryptAndCompressHandler.decompressAndDecryptData( + cfg, + encryptKey, + response.dataStr, + ); + if (data.version !== FILE_BASED_SYNC_CONSTANTS.FILE_VERSION) { + OpLog.warn( + `FileBasedSyncAdapter: Backup has unsupported version ${data.version}; cannot recover`, + ); + return null; + } + return { data, rev: response.rev }; + } catch (e) { + OpLog.warn('FileBasedSyncAdapter: Backup recovery failed', e); + return null; + } + } + /** * Downloads and decrypts the sync file. * @@ -956,19 +1154,32 @@ export class FileBasedSyncAdapterService { } throw e; } - const data = - await this._encryptAndCompressHandler.decompressAndDecryptData( - cfg, - encryptKey, - response.dataStr, - ); + let data: FileBasedSyncData; + try { + data = + await this._encryptAndCompressHandler.decompressAndDecryptData( + cfg, + encryptKey, + response.dataStr, + ); - // Validate file version - if (data.version !== FILE_BASED_SYNC_CONSTANTS.FILE_VERSION) { - throw new SyncDataCorruptedError( - `Unsupported file version: ${data.version} (expected ${FILE_BASED_SYNC_CONSTANTS.FILE_VERSION})`, - FILE_BASED_SYNC_CONSTANTS.SYNC_FILE, - ); + // Validate file version + if (data.version !== FILE_BASED_SYNC_CONSTANTS.FILE_VERSION) { + throw new SyncDataCorruptedError( + `Unsupported file version: ${data.version} (expected ${FILE_BASED_SYNC_CONSTANTS.FILE_VERSION})`, + FILE_BASED_SYNC_CONSTANTS.SYNC_FILE, + ); + } + } catch (decodeErr) { + // Annotate the corrupt primary file's current rev onto the error so the + // download-time recovery path can seed the cache with IT (not the .bak rev) + // and heal sync-data.json via a matching conditional overwrite, instead of + // mismatching forever and re-recovering every cycle. We already hold the rev + // here (from the download above), so no extra request is needed. + if (decodeErr && typeof decodeErr === 'object' && !('primaryRev' in decodeErr)) { + (decodeErr as { primaryRev?: string }).primaryRev = response.rev; + } + throw decodeErr; } return { data, rev: response.rev }; diff --git a/src/app/t.const.ts b/src/app/t.const.ts index a8028916e0..6aacd000dd 100644 --- a/src/app/t.const.ts +++ b/src/app/t.const.ts @@ -1644,6 +1644,7 @@ const T = { STORAGE_RECOVERED_AFTER_COMPACTION: 'F.SYNC.S.STORAGE_RECOVERED_AFTER_COMPACTION', SUCCESS_DOWNLOAD: 'F.SYNC.S.SUCCESS_DOWNLOAD', SUCCESS_VIA_BUTTON: 'F.SYNC.S.SUCCESS_VIA_BUTTON', + SYNC_DATA_RECOVERED_FROM_BACKUP: 'F.SYNC.S.SYNC_DATA_RECOVERED_FROM_BACKUP', SYNC_VALIDATION_FAILED: 'F.SYNC.S.SYNC_VALIDATION_FAILED', TIMEOUT_ERROR: 'F.SYNC.S.TIMEOUT_ERROR', TOO_MANY_OPS_TO_DOWNLOAD: 'F.SYNC.S.TOO_MANY_OPS_TO_DOWNLOAD', diff --git a/src/assets/i18n/en.json b/src/assets/i18n/en.json index 0c5042c359..2905b1e5b7 100644 --- a/src/assets/i18n/en.json +++ b/src/assets/i18n/en.json @@ -1596,6 +1596,7 @@ "STORAGE_RECOVERED_AFTER_COMPACTION": "Storage was running low. Old data cleaned up successfully.", "SUCCESS_DOWNLOAD": "Synced data from remote", "SUCCESS_VIA_BUTTON": "Data successfully synced", + "SYNC_DATA_RECOVERED_FROM_BACKUP": "Recovered sync data from backup after the main file could not be read.", "SYNC_VALIDATION_FAILED": "State validation failed after sync. Some data may be inconsistent.", "TIMEOUT_ERROR": "Sync operation timed out. {{suggestion}}", "TOO_MANY_OPS_TO_DOWNLOAD": "Too many sync operations to download. Some changes may be missing.",