From c5409bbd254f109ab74d43cbbe72fe33e4e8172e Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Fri, 30 Jan 2026 19:50:30 +0100 Subject: [PATCH] fix(sync): add server pruning log, harden test coverage for vector clocks - Add Logger.warn() in ValidationService when oversized vector clocks are pruned server-side, improving observability for buggy clients - Add tests for hasVectorClockChanges (previously zero coverage) - Add symmetric LESS_THAN test for pruning-aware comparison mode - Export MAX_LWW_REUPLOAD_RETRIES constant to eliminate magic number coupling between sync-wrapper service and its test - Add test verifying uploading client ID is preserved during server-side clock pruning - Add test for limitVectorClockSize when preserveClientIds exceeds MAX --- .../shared-schema/tests/vector-clock.spec.ts | 49 ++++++++++++++ .../src/sync/services/validation.service.ts | 7 ++ .../tests/conflict-detection.spec.ts | 35 +++++++++- src/app/core/util/vector-clock.spec.ts | 67 +++++++++++++++++++ .../imex/sync/sync-wrapper.service.spec.ts | 7 +- src/app/imex/sync/sync-wrapper.service.ts | 2 +- src/app/op-log/core/operation-log.const.ts | 7 ++ 7 files changed, 170 insertions(+), 4 deletions(-) diff --git a/packages/shared-schema/tests/vector-clock.spec.ts b/packages/shared-schema/tests/vector-clock.spec.ts index c41aa3626f..35e21bada1 100644 --- a/packages/shared-schema/tests/vector-clock.spec.ts +++ b/packages/shared-schema/tests/vector-clock.spec.ts @@ -231,6 +231,27 @@ describe('compareVectorClocks', () => { expect(compareVectorClocks(a, b)).toBe('GREATER_THAN'); }); + it('should return LESS_THAN when only b side has non-shared keys and shared keys show b dominates', () => { + // Symmetric case of the GREATER_THAN test above: b dominates on shared keys, + // only b has non-shared keys (aOnlyCount=0), so no escalation to CONCURRENT. + const a: Record = {}; + const b: Record = {}; + for (let i = 0; i < MAX_VECTOR_CLOCK_SIZE; i++) { + a[`shared_${i}`] = 5; + b[`shared_${i}`] = 10; + } + // Only b has extra keys + for (let i = 0; i < 3; i++) { + b[`b_only_${i}`] = 100; + } + + expect(Object.keys(a).length).toBe(MAX_VECTOR_CLOCK_SIZE); + expect(Object.keys(b).length).toBeGreaterThan(MAX_VECTOR_CLOCK_SIZE); + // Both >= MAX → pruning-aware mode. Shared keys: b dominates. + // Only b has non-shared keys (aOnlyCount=0), so no escalation to CONCURRENT. + expect(compareVectorClocks(a, b)).toBe('LESS_THAN'); + }); + it('should return EQUAL when only one side has non-shared keys and shared keys are equal', () => { const a: Record = {}; const b: Record = {}; @@ -349,6 +370,34 @@ describe('limitVectorClockSize', () => { expect(Object.keys(result).length).toBe(MAX_VECTOR_CLOCK_SIZE); expect(result['nonexistent']).toBeUndefined(); }); + + it('should cap at MAX_VECTOR_CLOCK_SIZE even when preserveClientIds exceeds MAX', () => { + // Build a clock with many entries including 15 "preserved" clients + const clock: Record = {}; + const preserveIds: string[] = []; + for (let i = 0; i < 15; i++) { + const id = `preserved_${i}`; + clock[id] = i + 1; + preserveIds.push(id); + } + // Add more clients with higher counters + for (let i = 0; i < 20; i++) { + clock[`high_${i}`] = 200 + i; + } + + const result = limitVectorClockSize(clock, preserveIds); + expect(Object.keys(result).length).toBe(MAX_VECTOR_CLOCK_SIZE); + + // Only the first MAX_VECTOR_CLOCK_SIZE preserved IDs should be kept + // (Set insertion order determines which ones) + let preservedCount = 0; + for (const id of preserveIds) { + if (result[id] !== undefined) { + preservedCount++; + } + } + expect(preservedCount).toBeLessThanOrEqual(MAX_VECTOR_CLOCK_SIZE); + }); }); describe('mergeVectorClocks', () => { diff --git a/packages/super-sync-server/src/sync/services/validation.service.ts b/packages/super-sync-server/src/sync/services/validation.service.ts index 865d847348..3883de911d 100644 --- a/packages/super-sync-server/src/sync/services/validation.service.ts +++ b/packages/super-sync-server/src/sync/services/validation.service.ts @@ -15,6 +15,7 @@ import { limitVectorClockSize, } from '../sync.types'; import { ENTITY_TYPES } from '@sp/shared-schema'; +import { Logger } from '../../logger'; /** * Valid entity types for operations. @@ -155,7 +156,13 @@ export class ValidationService { // GREATER_THAN for early-stage entities. This is the safe direction — it // triggers LWW conflict resolution rather than silent data loss. Only affects // buggy or adversarial clients sending oversized clocks. + const originalClockSize = Object.keys(op.vectorClock).length; op.vectorClock = limitVectorClockSize(op.vectorClock, [op.clientId]); + if (Object.keys(op.vectorClock).length < originalClockSize) { + Logger.warn( + `[client:${op.clientId}] Oversized vector clock pruned from ${originalClockSize} to ${Object.keys(op.vectorClock).length} entries`, + ); + } // Validate payload complexity to prevent DoS attacks via deeply nested objects. // Full-state ops (SYNC_IMPORT, BACKUP_IMPORT, REPAIR) get higher thresholds diff --git a/packages/super-sync-server/tests/conflict-detection.spec.ts b/packages/super-sync-server/tests/conflict-detection.spec.ts index 80d91a8eb4..319b4dcb22 100644 --- a/packages/super-sync-server/tests/conflict-detection.spec.ts +++ b/packages/super-sync-server/tests/conflict-detection.spec.ts @@ -170,7 +170,12 @@ vi.mock('../src/db', async () => { }); import { initSyncService, getSyncService } from '../src/sync/sync.service'; -import { Operation, SYNC_ERROR_CODES, VectorClock } from '../src/sync/sync.types'; +import { + Operation, + SYNC_ERROR_CODES, + VectorClock, + MAX_VECTOR_CLOCK_SIZE, +} from '../src/sync/sync.types'; describe('Conflict Detection', () => { const userId = 1; @@ -729,6 +734,34 @@ describe('Conflict Detection', () => { } }); + it('should preserve uploading client ID during server-side clock pruning', async () => { + const service = getSyncService(); + const entityId = 'task-1'; + + // Create clock with many entries where the uploading client (clientA) IS in the clock + // but has a low counter value that would normally be pruned + const largeClock: VectorClock = { [clientA]: 2 }; // Low counter + for (let i = 0; i < 50; i++) { + largeClock[`client-${i}`] = 100 + i; // All have higher counters + } + + const op = createOp({ + entityId, + clientId: clientA, + vectorClock: largeClock, + opType: 'CRT', + }); + const result = await service.uploadOps(userId, clientA, [op]); + expect(result[0].accepted).toBe(true); + + // Verify the clock was pruned to MAX_VECTOR_CLOCK_SIZE + const ops = await service.getOpsSince(userId, 0); + const storedClock = ops[0].op.vectorClock; + expect(Object.keys(storedClock).length).toBe(MAX_VECTOR_CLOCK_SIZE); + // clientA should be preserved despite having the lowest counter + expect(storedClock[clientA]).toBe(2); + }); + it('should handle first operation on entity (no existing op to conflict with)', async () => { const service = getSyncService(); diff --git a/src/app/core/util/vector-clock.spec.ts b/src/app/core/util/vector-clock.spec.ts index c67b0dfed6..c3a8c32805 100644 --- a/src/app/core/util/vector-clock.spec.ts +++ b/src/app/core/util/vector-clock.spec.ts @@ -2,6 +2,7 @@ import { limitVectorClockSize, VectorClockComparison, compareVectorClocks, + hasVectorClockChanges, } from './vector-clock'; import { MAX_VECTOR_CLOCK_SIZE } from '../../op-log/core/operation-log.const'; @@ -366,4 +367,70 @@ describe('vector-clock', () => { expect(compareVectorClocks(a, b)).toBe(VectorClockComparison.CONCURRENT); }); }); + + describe('hasVectorClockChanges', () => { + it('should return false when current equals reference', () => { + const clock = { clientA: 5, clientB: 3 }; + expect(hasVectorClockChanges(clock, clock)).toBe(false); + }); + + it('should return false when both are null', () => { + expect(hasVectorClockChanges(null, null)).toBe(false); + }); + + it('should return false when both are empty', () => { + expect(hasVectorClockChanges({}, {})).toBe(false); + }); + + it('should return true when current has higher counter than reference', () => { + const current = { clientA: 6, clientB: 3 }; + const reference = { clientA: 5, clientB: 3 }; + expect(hasVectorClockChanges(current, reference)).toBe(true); + }); + + it('should return true when current has a new client not in reference', () => { + const current = { clientA: 5, clientB: 3, clientC: 1 }; + const reference = { clientA: 5, clientB: 3 }; + expect(hasVectorClockChanges(current, reference)).toBe(true); + }); + + it('should return true when current is non-empty and reference is null', () => { + expect(hasVectorClockChanges({ clientA: 1 }, null)).toBe(true); + }); + + it('should return true when current is null and reference is non-empty', () => { + expect(hasVectorClockChanges(null, { clientA: 1 })).toBe(true); + }); + + it('should return true when reference has a client missing from current (below MAX size)', () => { + const current = { clientA: 5 }; + const reference = { clientA: 5, clientB: 3 }; + expect(hasVectorClockChanges(current, reference)).toBe(true); + }); + + it('should return true when reference has a client missing from current at MAX size (pruned)', () => { + // Build current clock at MAX_VECTOR_CLOCK_SIZE + const current: Record = {}; + for (let i = 0; i < MAX_VECTOR_CLOCK_SIZE; i++) { + current[`client_${i}`] = 10; + } + // Reference has a key not in current + const reference: Record = { ...current, extra_client: 5 }; + delete reference['client_0']; + + expect(Object.keys(current).length).toBe(MAX_VECTOR_CLOCK_SIZE); + expect(hasVectorClockChanges(current, reference)).toBe(true); + }); + + it('should return false when current has equal or lower counters than reference', () => { + const current = { clientA: 5, clientB: 3 }; + const reference = { clientA: 5, clientB: 4 }; + // current clientB (3) < reference clientB (4), but no current counter is higher + // and reference has no missing clients from current + // However, clientB is lower in current → that's not a "change" in current + // The missing direction: reference has clientB:4, current has clientB:3 + // hasVectorClockChanges checks if current > reference for any key, not the reverse + expect(hasVectorClockChanges(current, reference)).toBe(false); + }); + }); }); diff --git a/src/app/imex/sync/sync-wrapper.service.spec.ts b/src/app/imex/sync/sync-wrapper.service.spec.ts index ecf93df19c..ba1fc90501 100644 --- a/src/app/imex/sync/sync-wrapper.service.spec.ts +++ b/src/app/imex/sync/sync-wrapper.service.spec.ts @@ -25,6 +25,7 @@ import { SyncAlreadyInProgressError, LocalDataConflictError, } from '../../op-log/core/errors/sync-errors'; +import { MAX_LWW_REUPLOAD_RETRIES } from '../../op-log/core/operation-log.const'; import { LegacySyncProvider } from './legacy-sync-provider.model'; describe('SyncWrapperService', () => { @@ -1262,8 +1263,10 @@ describe('SyncWrapperService', () => { const result = await service.sync(); - // 1 initial upload + 3 retries = 4 total calls - expect(mockSyncService.uploadPendingOps).toHaveBeenCalledTimes(4); + // 1 initial upload + MAX_LWW_REUPLOAD_RETRIES retries + expect(mockSyncService.uploadPendingOps).toHaveBeenCalledTimes( + 1 + MAX_LWW_REUPLOAD_RETRIES, + ); // Should set UNKNOWN_OR_CHANGED since ops remain pending expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith( 'UNKNOWN_OR_CHANGED', diff --git a/src/app/imex/sync/sync-wrapper.service.ts b/src/app/imex/sync/sync-wrapper.service.ts index d2d1517ebc..668c6b9303 100644 --- a/src/app/imex/sync/sync-wrapper.service.ts +++ b/src/app/imex/sync/sync-wrapper.service.ts @@ -18,6 +18,7 @@ import { WebCryptoNotAvailableError, MissingRefreshTokenAPIError, } 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'; import { TranslateService } from '@ngx-translate/core'; import { MatDialog, MatDialogRef } from '@angular/material/dialog'; @@ -300,7 +301,6 @@ export class SyncWrapperService { } // 3. If LWW created local-win ops, upload them (with retry limit to prevent infinite loops) - const MAX_LWW_REUPLOAD_RETRIES = 3; let lwwRetries = 0; let pendingLwwOps = (downloadResult.localWinOpsCreated ?? 0) + diff --git a/src/app/op-log/core/operation-log.const.ts b/src/app/op-log/core/operation-log.const.ts index b2c7d17866..3d4762b221 100644 --- a/src/app/op-log/core/operation-log.const.ts +++ b/src/app/op-log/core/operation-log.const.ts @@ -216,6 +216,13 @@ export { MAX_VECTOR_CLOCK_SIZE } from '@sp/shared-schema'; */ export const MIN_CLIENT_ID_LENGTH = 5; +/** + * Maximum retry attempts for re-uploading LWW (Last-Writer-Wins) local-win operations. + * After this many retries, the sync reports UNKNOWN_OR_CHANGED and retries on next sync. + * Default: 3 + */ +export const MAX_LWW_REUPLOAD_RETRIES = 3; + /** * Duration in milliseconds to suppress selector-based effects after sync completes. * This prevents "repair" effects from creating redundant operations based on