mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
fix(sync): break iOS WebDAV conflict-dialog loop (#7339)
Two compounding bugs trapped iOS WebDAV users in a per-minute conflict dialog that no button could resolve: 1. FileBasedSyncAdapter's snapshotReplacement heuristic re-fires gapDetected on every sync from a non-writing client (clientId \!= excludeClient is true forever), so the download keeps coming back with snapshotState and OperationLogSyncService keeps throwing LocalDataConflictError despite the local clock already dominating the remote snapshot. Skip hydration and conflict when compareVectorClocks(local, remote) is EQUAL or GREATER_THAN, gated on both clocks being non-empty so a fresh client still hydrates a legacy/clockless snapshot. 2. SyncWrapperService._openConflictDialog$ filtered undefined out of the afterClosed() stream, so a programmatic close (iOS WebView lifecycle, re-entry) collapsed the observable and firstValueFrom threw EmptyError — the user's Use Local/Use Remote/Cancel click never reached the resolution branches. Drop the filter so undefined flows through to the existing cancellation path. The dominate-skip deliberately does NOT append result.newOps to the op log: VectorClockService.getEntityFrontier is last-write-wins by seq, and writing historical remote ops at the current tail would regress per-entity frontiers and let future LWW resolution overwrite local data. Trade-off documented inline. Adds an adapter-level integration reproducer that asserts gapDetected re-fires forever for a non-writing client (the upstream loop trigger), a 3-client WebDAV e2e that reproduces the loop end-to-end against a real provider, plus service-level tests for the dominate-skip, the empty-clock guard, the concurrent-clock conservative path, and consecutive-sync loop prevention.
This commit is contained in:
Johannes Millan
parent
4473559fbd
commit
654f4d80ee
6 changed files with 632 additions and 5 deletions
|
|
@ -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<boolean>((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);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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 () => {
|
||||
|
|
|
|||
|
|
@ -1292,7 +1292,7 @@ export class SyncWrapperService {
|
|||
|
||||
private _openConflictDialog$(
|
||||
conflictData: ConflictData,
|
||||
): Observable<DialogConflictResolutionResult> {
|
||||
): Observable<DialogConflictResolutionResult | undefined> {
|
||||
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();
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -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<SyncHydrationService>;
|
||||
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<SyncHydrationService>;
|
||||
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<SyncHydrationService>;
|
||||
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();
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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...',
|
||||
);
|
||||
|
|
|
|||
|
|
@ -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', () => {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue