Mirrors/super-productivity
1
0
Fork 0
mirror of https://github.com/johannesjo/super-productivity.git synced 2026-07-18 00:46:45 +00:00
Code Issues Projects Releases Wiki Activity

fix(sync): prevent data loss for fresh clients from upload retry race condition

Browse source 
Replace the unsafe upload retry loop with a single-attempt strategy that
correctly handles concurrent uploads to file-based sync providers (WebDAV,
Dropbox, local file).

The old _uploadWithRetry called getStateSnapshot() inside a retry loop while
concurrent client ops could be in recentOps but not yet applied to NgRx,
creating a stale snapshot that caused data loss for fresh-bootstrap clients.

The new _uploadWithMismatchFallback:
- Attempts upload once with the current snapshot
- On UploadRevToMatchMismatchAPIError: re-downloads to distinguish
  - Same rev (WebDAV ETag glitch): force-uploads the already-built uploadData
  - Different rev (genuine concurrent upload): throws so the next sync cycle
    downloads the concurrent ops first, then uploads with a consistent snapshot
- SyncWrapperService handles the thrown error as transient (UNKNOWN_OR_CHANGED,
  no error snackbar)

Removes snapshotSyncVersion, extraOpIds, retry constants, and the retry-loop
infrastructure that was part of the broken approach.
This commit is contained in:
Johannes Millan 2026-04-14 16:38:12 +02:00
parent 5324512ae5
commit 09f5ced2c9
8 changed files with 108 additions and 282 deletions
Download patch file Download diff file Expand all files Collapse all files

18
src/app/imex/sync/sync-wrapper.service.spec.ts
View file

@ -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()', () => {

11
src/app/imex/sync/sync-wrapper.service.ts
View file

@ -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);

204
src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts
View file

@ -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(

121
src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts
View file

@ -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<SyncProviderId>,
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;
}
// ═══════════════════════════════════════════════════════════════════════════

6
src/app/op-log/sync-providers/file-based/file-based-sync.types.ts
View file

@ -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;

2
src/app/op-log/sync-providers/file-based/webdav/webdav-base-provider.ts
View file

@ -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);
}

16
src/app/op-log/testing/integration/file-based-sync/conflict-resolution.integration.spec.ts
View file

@ -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);

12
src/app/op-log/testing/integration/file-based-sync/edge-cases.integration.spec.ts
View file

@ -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);