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
This commit is contained in:
Johannes Millan 2026-01-30 19:50:30 +01:00
parent 495f0fe0d3
commit c5409bbd25
7 changed files with 170 additions and 4 deletions

View file

@ -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<string, number> = {};
const b: Record<string, number> = {};
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<string, number> = {};
const b: Record<string, number> = {};
@ -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<string, number> = {};
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', () => {

View file

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

View file

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

View file

@ -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<string, number> = {};
for (let i = 0; i < MAX_VECTOR_CLOCK_SIZE; i++) {
current[`client_${i}`] = 10;
}
// Reference has a key not in current
const reference: Record<string, number> = { ...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);
});
});
});

View file

@ -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',

View file

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

View file

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