From b113a2d7dd997afb4bbbf2ce0dcbde62af8fc567 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Fri, 30 Jan 2026 19:21:45 +0100 Subject: [PATCH] fix(sync): return CONCURRENT instead of EQUAL when pruned clocks have non-shared keys When both vector clocks are at MAX size (pruning-aware mode) and shared keys compare as equal, non-shared keys on both sides indicate genuinely different causal histories. Returning EQUAL caused silent data loss by skipping operations as duplicates. Returning CONCURRENT safely triggers LWW conflict resolution instead. Also fixes server snapshot pruning to preserve the requesting client's ID and the snapshot author's client ID when limiting vector clock size. --- packages/shared-schema/src/vector-clock.ts | 10 ++++ .../shared-schema/tests/vector-clock.spec.ts | 59 ++++++++++++++++++- .../services/operation-download.service.ts | 11 ++-- src/app/core/util/vector-clock.spec.ts | 44 ++++++++++++++ 4 files changed, 118 insertions(+), 6 deletions(-) diff --git a/packages/shared-schema/src/vector-clock.ts b/packages/shared-schema/src/vector-clock.ts index 6d823cc17b..907bbf6598 100644 --- a/packages/shared-schema/src/vector-clock.ts +++ b/packages/shared-schema/src/vector-clock.ts @@ -72,11 +72,15 @@ export const compareVectorClocks = ( aKeys.length >= MAX_VECTOR_CLOCK_SIZE && bKeys.length >= MAX_VECTOR_CLOCK_SIZE; let keysToCompare: Set; + let aOnlyCount = 0; + let bOnlyCount = 0; if (bothPossiblyPruned) { const bKeySet = new Set(bKeys); keysToCompare = new Set(aKeys.filter((k) => bKeySet.has(k))); // If no shared keys at all, clocks are from independent client populations → CONCURRENT if (keysToCompare.size === 0) return 'CONCURRENT'; + aOnlyCount = aKeys.length - keysToCompare.size; + bOnlyCount = bKeys.length - keysToCompare.size; } else { keysToCompare = new Set([...aKeys, ...bKeys]); } @@ -95,6 +99,12 @@ export const compareVectorClocks = ( if (aGreater && bGreater) return 'CONCURRENT'; if (aGreater) return 'GREATER_THAN'; if (bGreater) return 'LESS_THAN'; + + // In pruning-aware mode, if shared keys are equal but both sides have + // non-shared keys, the clocks have genuinely different causal histories. + // Returning EQUAL here would cause silent data loss (skip as duplicate). + // Returning CONCURRENT triggers LWW conflict resolution (safe). + if (bothPossiblyPruned && aOnlyCount > 0 && bOnlyCount > 0) return 'CONCURRENT'; return 'EQUAL'; }; diff --git a/packages/shared-schema/tests/vector-clock.spec.ts b/packages/shared-schema/tests/vector-clock.spec.ts index a78d1e6d6c..d701a79405 100644 --- a/packages/shared-schema/tests/vector-clock.spec.ts +++ b/packages/shared-schema/tests/vector-clock.spec.ts @@ -80,7 +80,7 @@ describe('compareVectorClocks', () => { expect(compareVectorClocks(a, b)).toBe('GREATER_THAN'); }); - it('should return EQUAL when shared keys are equal and both at max size', () => { + it('should return CONCURRENT when shared keys are equal but both sides have non-shared keys', () => { const a: Record = {}; const b: Record = {}; @@ -93,7 +93,8 @@ describe('compareVectorClocks', () => { b[`b_only_${i}`] = 50; } - expect(compareVectorClocks(a, b)).toBe('EQUAL'); + // Non-shared keys on both sides prove genuinely different causal histories + expect(compareVectorClocks(a, b)).toBe('CONCURRENT'); }); it('should return CONCURRENT when shared keys are genuinely concurrent', () => { @@ -192,6 +193,60 @@ describe('compareVectorClocks', () => { expect(Object.keys(b).length).toBeGreaterThan(MAX_VECTOR_CLOCK_SIZE); expect(compareVectorClocks(a, b)).toBe('GREATER_THAN'); }); + + it('should return EQUAL when fully-shared keys at MAX size are identical', () => { + // Both clocks have the exact same keys — no non-shared keys + const a: Record = {}; + const b: Record = {}; + for (let i = 0; i < MAX_VECTOR_CLOCK_SIZE; i++) { + a[`client_${i}`] = 10; + b[`client_${i}`] = 10; + } + + expect(Object.keys(a).length).toBe(MAX_VECTOR_CLOCK_SIZE); + expect(Object.keys(b).length).toBe(MAX_VECTOR_CLOCK_SIZE); + expect(compareVectorClocks(a, b)).toBe('EQUAL'); + }); + + it('should preserve shared-key result when only one side has non-shared keys', () => { + // a has all of b's keys plus extras, shared keys show a dominates + const a: Record = {}; + const b: Record = {}; + for (let i = 0; i < MAX_VECTOR_CLOCK_SIZE; i++) { + a[`shared_${i}`] = 10; + b[`shared_${i}`] = 5; + } + // Only a has extra keys (b has none unique) + for (let i = 0; i < 3; i++) { + a[`a_only_${i}`] = 100; + } + // b needs to reach MAX size too for pruning-aware mode + // Since b already has MAX_VECTOR_CLOCK_SIZE shared keys, it's at MAX + // a has MAX + 3 keys + + expect(Object.keys(a).length).toBeGreaterThan(MAX_VECTOR_CLOCK_SIZE); + expect(Object.keys(b).length).toBe(MAX_VECTOR_CLOCK_SIZE); + // Both >= MAX → pruning-aware mode. Shared keys: a dominates. + // Only a has non-shared keys (bOnlyCount=0), so no escalation to CONCURRENT. + expect(compareVectorClocks(a, b)).toBe('GREATER_THAN'); + }); + + it('should return EQUAL when only one side has non-shared keys and shared keys are equal', () => { + const a: Record = {}; + const b: Record = {}; + for (let i = 0; i < MAX_VECTOR_CLOCK_SIZE; i++) { + a[`shared_${i}`] = 10; + b[`shared_${i}`] = 10; + } + // Only a has extra keys + for (let i = 0; i < 3; i++) { + a[`a_only_${i}`] = 100; + } + + // Both >= MAX → pruning-aware. Shared keys equal. + // Only one side has non-shared keys → not escalated to CONCURRENT. + expect(compareVectorClocks(a, b)).toBe('EQUAL'); + }); }); }); diff --git a/packages/super-sync-server/src/sync/services/operation-download.service.ts b/packages/super-sync-server/src/sync/services/operation-download.service.ts index 7f36f2fd28..43866a40b6 100644 --- a/packages/super-sync-server/src/sync/services/operation-download.service.ts +++ b/packages/super-sync-server/src/sync/services/operation-download.service.ts @@ -85,7 +85,7 @@ export class OperationDownloadService { opType: { in: ['SYNC_IMPORT', 'BACKUP_IMPORT', 'REPAIR'] }, }, orderBy: { serverSeq: 'desc' }, - select: { serverSeq: true }, + select: { serverSeq: true, clientId: true }, }); const latestSnapshotSeq = latestFullStateOp?.serverSeq ?? undefined; @@ -130,9 +130,12 @@ export class OperationDownloadService { } // Limit snapshot clock to MAX_VECTOR_CLOCK_SIZE to prevent oversized - // clocks from being sent to clients. No specific client to preserve, - // so we keep the most active entries. - snapshotVectorClock = limitVectorClockSize(snapshotVectorClock); + // clocks from being sent to clients. Preserve the requesting client's ID + // and the snapshot author's ID to avoid false EQUAL in pruning-aware comparison. + const preserveIds: string[] = []; + if (excludeClient) preserveIds.push(excludeClient); + if (latestFullStateOp?.clientId) preserveIds.push(latestFullStateOp.clientId); + snapshotVectorClock = limitVectorClockSize(snapshotVectorClock, preserveIds); Logger.info( `[user:${userId}] Computed snapshotVectorClock from ${skippedOps.length} ops: ${JSON.stringify(snapshotVectorClock)}`, diff --git a/src/app/core/util/vector-clock.spec.ts b/src/app/core/util/vector-clock.spec.ts index 10b0b37da2..4bc82a40a7 100644 --- a/src/app/core/util/vector-clock.spec.ts +++ b/src/app/core/util/vector-clock.spec.ts @@ -297,5 +297,49 @@ describe('vector-clock', () => { expect(Object.keys(b).length).toBe(MAX_VECTOR_CLOCK_SIZE - 1); expect(compareVectorClocks(a, b)).toBe(VectorClockComparison.CONCURRENT); }); + + it('should return CONCURRENT when shared keys equal but both sides have non-shared keys', () => { + const a: Record = {}; + const b: Record = {}; + for (let i = 0; i < 5; i++) { + a[`shared_${i}`] = 10; + b[`shared_${i}`] = 10; + } + for (let i = 0; i < MAX_VECTOR_CLOCK_SIZE - 5; i++) { + a[`a_only_${i}`] = 50; + b[`b_only_${i}`] = 50; + } + + // Non-shared keys on both sides → genuinely different causal histories → CONCURRENT + expect(compareVectorClocks(a, b)).toBe(VectorClockComparison.CONCURRENT); + }); + + it('should return EQUAL when fully-shared keys at MAX size are identical', () => { + const a: Record = {}; + const b: Record = {}; + for (let i = 0; i < MAX_VECTOR_CLOCK_SIZE; i++) { + a[`client_${i}`] = 10; + b[`client_${i}`] = 10; + } + + expect(compareVectorClocks(a, b)).toBe(VectorClockComparison.EQUAL); + }); + + it('should preserve shared-key result when only one side has non-shared keys', () => { + const a: Record = {}; + const b: Record = {}; + for (let i = 0; i < MAX_VECTOR_CLOCK_SIZE; i++) { + a[`shared_${i}`] = 10; + b[`shared_${i}`] = 10; + } + // Only a has extra keys + for (let i = 0; i < 3; i++) { + a[`a_only_${i}`] = 100; + } + + // Both >= MAX → pruning-aware. Shared keys equal. + // Only one side has non-shared keys → stays EQUAL (not escalated). + expect(compareVectorClocks(a, b)).toBe(VectorClockComparison.EQUAL); + }); }); });