From 495f0fe0d3e308c4e5b4a146a61822ba8049072a Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Fri, 30 Jan 2026 19:35:51 +0100 Subject: [PATCH] fix(sync): improve vector clock pruning awareness and harden tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make hasVectorClockChanges pruning-aware by checking clock size before logging missing keys — downgrade to verbose when pruning is likely, warn when corruption is likely. Add return value assertion to LWW retry exhaustion test, asymmetric pruning test cases, server-side pruning tradeoff documentation, and fix stale "max 50" in docs. --- docs/sync-and-op-log/vector-clocks.md | 16 +++---- packages/shared-schema/src/vector-clock.ts | 4 ++ .../shared-schema/tests/vector-clock.spec.ts | 46 ++++++++++++++++--- .../src/sync/services/validation.service.ts | 7 +++ src/app/core/util/vector-clock.spec.ts | 30 ++++++++++-- src/app/core/util/vector-clock.ts | 29 ++++++++---- .../imex/sync/sync-wrapper.service.spec.ts | 4 +- 7 files changed, 109 insertions(+), 27 deletions(-) diff --git a/docs/sync-and-op-log/vector-clocks.md b/docs/sync-and-op-log/vector-clocks.md index c421f53ff0..0f51e22f01 100644 --- a/docs/sync-and-op-log/vector-clocks.md +++ b/docs/sync-and-op-log/vector-clocks.md @@ -278,14 +278,14 @@ opLog(2, 'Vector clock comparison', { ## Current Implementation Status -| Feature | Status | Notes | -| ------------------------------- | -------------- | -------------------------------------- | -| Vector clock conflict detection | ✅ Implemented | Used by both PFAPI and Operation Log | -| Entity-level conflict detection | ✅ Implemented | Operation Log tracks per-entity clocks | -| User conflict resolution UI | ✅ Implemented | `DialogConflictResolutionComponent` | -| Client pruning (max 50 entries) | ✅ Implemented | `limitVectorClockSize()` | -| Overflow protection | ✅ Implemented | Clocks reset at MAX_SAFE_INTEGER | -| Protected client IDs | ✅ Implemented | Preserves all keys from full-state ops | +| Feature | Status | Notes | +| ------------------------------------------- | -------------- | -------------------------------------- | +| Vector clock conflict detection | ✅ Implemented | Used by both PFAPI and Operation Log | +| Entity-level conflict detection | ✅ Implemented | Operation Log tracks per-entity clocks | +| User conflict resolution UI | ✅ Implemented | `DialogConflictResolutionComponent` | +| Client pruning (MAX_VECTOR_CLOCK_SIZE = 10) | ✅ Implemented | `limitVectorClockSize()` | +| Overflow protection | ✅ Implemented | Clocks reset at MAX_SAFE_INTEGER | +| Protected client IDs | ✅ Implemented | Preserves all keys from full-state ops | ## Protected Client IDs diff --git a/packages/shared-schema/src/vector-clock.ts b/packages/shared-schema/src/vector-clock.ts index 907bbf6598..822a8db37f 100644 --- a/packages/shared-schema/src/vector-clock.ts +++ b/packages/shared-schema/src/vector-clock.ts @@ -44,6 +44,8 @@ export const MAX_VECTOR_CLOCK_SIZE = 10; * have been pruned by different clients (each preserving its own clientId). * Missing keys could mean "pruned away" rather than "genuinely zero". Comparing * only shared keys avoids false CONCURRENT from cross-client pruning asymmetry. + * To avoid false dominance, if the side that appears behind on shared keys has + * any non-shared keys, we conservatively return CONCURRENT. * * Known limitation: A clock that naturally grew to exactly MAX_VECTOR_CLOCK_SIZE * entries (without pruning) is indistinguishable from a pruned clock. In that case, @@ -97,6 +99,8 @@ export const compareVectorClocks = ( } if (aGreater && bGreater) return 'CONCURRENT'; + if (bothPossiblyPruned && aGreater && bOnlyCount > 0) return 'CONCURRENT'; + if (bothPossiblyPruned && bGreater && aOnlyCount > 0) return 'CONCURRENT'; if (aGreater) return 'GREATER_THAN'; if (bGreater) return 'LESS_THAN'; diff --git a/packages/shared-schema/tests/vector-clock.spec.ts b/packages/shared-schema/tests/vector-clock.spec.ts index d701a79405..c41aa3626f 100644 --- a/packages/shared-schema/tests/vector-clock.spec.ts +++ b/packages/shared-schema/tests/vector-clock.spec.ts @@ -57,7 +57,7 @@ describe('compareVectorClocks', () => { return clock; }; - it('should compare only shared keys when both clocks >= MAX_VECTOR_CLOCK_SIZE', () => { + it('should return CONCURRENT when both clocks at MAX and both have non-shared keys', () => { // Build two max-size clocks with some shared and some unique keys const a: Record = {}; const b: Record = {}; @@ -73,11 +73,11 @@ describe('compareVectorClocks', () => { b[`b_only_${i}`] = 100; } - // Without pruning-aware mode, unique keys would cause CONCURRENT - // With pruning-aware mode (both at MAX), only shared keys are compared + // Even though shared keys show a dominance, non-shared keys on both sides + // mean we can't safely declare dominance. 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('GREATER_THAN'); + expect(compareVectorClocks(a, b)).toBe('CONCURRENT'); }); it('should return CONCURRENT when shared keys are equal but both sides have non-shared keys', () => { @@ -175,7 +175,7 @@ describe('compareVectorClocks', () => { expect(compareVectorClocks(a, b)).toBe('CONCURRENT'); }); - it('should use pruning-aware mode when both clocks exceed MAX (more than MAX entries)', () => { + it('should return CONCURRENT when both clocks exceed MAX and both have non-shared keys', () => { // This tests the >= condition — clocks larger than MAX (e.g., from merge before pruning) const a: Record = {}; const b: Record = {}; @@ -188,10 +188,10 @@ describe('compareVectorClocks', () => { b[`b_only_${i}`] = 100; } - // Both exceed MAX, pruning-aware mode → only shared keys compared → a dominates + // Both exceed MAX with unique keys on both sides → cannot safely declare dominance expect(Object.keys(a).length).toBeGreaterThan(MAX_VECTOR_CLOCK_SIZE); expect(Object.keys(b).length).toBeGreaterThan(MAX_VECTOR_CLOCK_SIZE); - expect(compareVectorClocks(a, b)).toBe('GREATER_THAN'); + expect(compareVectorClocks(a, b)).toBe('CONCURRENT'); }); it('should return EQUAL when fully-shared keys at MAX size are identical', () => { @@ -247,6 +247,38 @@ describe('compareVectorClocks', () => { // Only one side has non-shared keys → not escalated to CONCURRENT. expect(compareVectorClocks(a, b)).toBe('EQUAL'); }); + + it('asymmetric pruning: one clock pruned, other naturally at MAX size (known limitation)', () => { + // Known limitation: A clock that naturally grew to MAX entries (without pruning) + // is indistinguishable from a pruned clock. When one side was genuinely pruned + // and the other naturally reached MAX, missing keys on the pruned side are treated + // as "possibly pruned" rather than "genuinely zero". This may produce + // GREATER_THAN instead of CONCURRENT for the non-pruned side's unique keys. + + // Clock A: naturally has exactly MAX active clients (no pruning occurred) + const a: Record = {}; + for (let i = 0; i < MAX_VECTOR_CLOCK_SIZE; i++) { + a[`client_${i}`] = 10; + } + + // Clock B: was pruned to MAX, shares most keys but has one unique key + // (replacing client_9 with b_unique simulates pruning that dropped client_9 + // and kept a different client) + const b: Record = {}; + for (let i = 0; i < MAX_VECTOR_CLOCK_SIZE - 1; i++) { + b[`client_${i}`] = 5; + } + b['b_unique'] = 50; + + expect(Object.keys(a).length).toBe(MAX_VECTOR_CLOCK_SIZE); + expect(Object.keys(b).length).toBe(MAX_VECTOR_CLOCK_SIZE); + + // Both at MAX → pruning-aware mode. + // Shared keys: a dominates (10 > 5 for client_0..client_8). + // b has non-shared key b_unique → escalated to CONCURRENT. + // This is the safe direction: LWW conflict resolution will handle it. + expect(compareVectorClocks(a, b)).toBe('CONCURRENT'); + }); }); }); 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 2b0ad0dcd0..865d847348 100644 --- a/packages/super-sync-server/src/sync/services/validation.service.ts +++ b/packages/super-sync-server/src/sync/services/validation.service.ts @@ -148,6 +148,13 @@ export class ValidationService { // Enforce MAX_VECTOR_CLOCK_SIZE on server side. // A buggy or adversarial client may send oversized clocks. // Preserve the uploading client's ID during pruning. + // + // Tradeoff: If the stored entity has fewer than MAX entries (not triggering + // pruning-aware comparison), a key pruned here is treated as "genuinely zero" + // rather than "pruned away." This may produce false CONCURRENT instead of + // 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. op.vectorClock = limitVectorClockSize(op.vectorClock, [op.clientId]); // Validate payload complexity to prevent DoS attacks via deeply nested objects. diff --git a/src/app/core/util/vector-clock.spec.ts b/src/app/core/util/vector-clock.spec.ts index 4bc82a40a7..c67b0dfed6 100644 --- a/src/app/core/util/vector-clock.spec.ts +++ b/src/app/core/util/vector-clock.spec.ts @@ -228,7 +228,7 @@ describe('vector-clock', () => { }); describe('compareVectorClocks - pruning-aware via shared implementation', () => { - it('should return GREATER_THAN (not false CONCURRENT) when both clocks at MAX size with different unique keys', () => { + it('should return CONCURRENT when both clocks at MAX size with non-shared keys on both sides', () => { // Build two max-size clocks: shared keys where a dominates, plus unique keys const a: Record = {}; const b: Record = {}; @@ -242,8 +242,8 @@ describe('vector-clock', () => { b[`b_only_${i}`] = 100; } - // Pruning-aware: only shared keys compared -> a dominates -> GREATER_THAN - expect(compareVectorClocks(a, b)).toBe(VectorClockComparison.GREATER_THAN); + // Non-shared keys on both sides make dominance ambiguous + expect(compareVectorClocks(a, b)).toBe(VectorClockComparison.CONCURRENT); }); it('should use all keys when only one clock is at MAX size', () => { @@ -341,5 +341,29 @@ describe('vector-clock', () => { // Only one side has non-shared keys → stays EQUAL (not escalated). expect(compareVectorClocks(a, b)).toBe(VectorClockComparison.EQUAL); }); + + it('asymmetric pruning: one clock pruned, other naturally at MAX size (known limitation)', () => { + // Known limitation: A clock that naturally grew to MAX entries (without pruning) + // is indistinguishable from a pruned clock. When one side was genuinely pruned + // and the other naturally reached MAX, missing keys on the pruned side are treated + // as "possibly pruned" rather than "genuinely zero". This may produce + // GREATER_THAN instead of CONCURRENT for the non-pruned side's unique keys. + const a: Record = {}; + for (let i = 0; i < MAX_VECTOR_CLOCK_SIZE; i++) { + a[`client_${i}`] = 10; + } + const b: Record = {}; + for (let i = 0; i < MAX_VECTOR_CLOCK_SIZE - 1; i++) { + b[`client_${i}`] = 5; + } + b['b_unique'] = 50; + + expect(Object.keys(a).length).toBe(MAX_VECTOR_CLOCK_SIZE); + expect(Object.keys(b).length).toBe(MAX_VECTOR_CLOCK_SIZE); + + // Both at MAX → pruning-aware mode. + // Shared keys: a dominates. b has non-shared key → CONCURRENT (safe direction). + expect(compareVectorClocks(a, b)).toBe(VectorClockComparison.CONCURRENT); + }); }); }); diff --git a/src/app/core/util/vector-clock.ts b/src/app/core/util/vector-clock.ts index 4b0a4b7614..cd9538ce7a 100644 --- a/src/app/core/util/vector-clock.ts +++ b/src/app/core/util/vector-clock.ts @@ -277,16 +277,29 @@ export const hasVectorClockChanges = ( } } - // CRITICAL FIX: Check if reference has any clients missing from current - // This detects when a client's entry has been removed/corrupted + // Check if reference has any clients missing from current. + // This detects when a client's entry has been removed/corrupted. + // However, after legitimate pruning (limitVectorClockSize), low-counter entries + // are removed. When the current clock is at MAX size, missing keys are expected + // and don't indicate corruption — they were pruned away. + const currentSize = Object.keys(current!).length; for (const [clientId, refVal] of Object.entries(reference!)) { if (refVal > 0 && !(clientId in current!)) { - PFLog.error('Vector clock change detected: client missing from current', { - clientId, - refValue: refVal, - currentClock: vectorClockToString(current), - referenceClock: vectorClockToString(reference), - }); + if (currentSize >= MAX_VECTOR_CLOCK_SIZE) { + // Current clock was likely pruned — missing keys are expected. + PFLog.verbose( + 'Vector clock: reference client missing from current (likely pruned)', + { clientId, refValue: refVal }, + ); + } else { + // Current clock is small enough that pruning couldn't have removed this key. + PFLog.warn('Vector clock change detected: client missing from current', { + clientId, + refValue: refVal, + currentClock: vectorClockToString(current), + referenceClock: vectorClockToString(reference), + }); + } return true; } } diff --git a/src/app/imex/sync/sync-wrapper.service.spec.ts b/src/app/imex/sync/sync-wrapper.service.spec.ts index 7f39eba0e6..ecf93df19c 100644 --- a/src/app/imex/sync/sync-wrapper.service.spec.ts +++ b/src/app/imex/sync/sync-wrapper.service.spec.ts @@ -1260,7 +1260,7 @@ describe('SyncWrapperService', () => { }), ); - await service.sync(); + const result = await service.sync(); // 1 initial upload + 3 retries = 4 total calls expect(mockSyncService.uploadPendingOps).toHaveBeenCalledTimes(4); @@ -1268,6 +1268,8 @@ describe('SyncWrapperService', () => { expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith( 'UNKNOWN_OR_CHANGED', ); + // Should return UpdateRemote to signal that unuploaded ops remain + expect(result).toBe(SyncStatus.UpdateRemote); }); it('should exit early when retry returns localWinOpsCreated: 0', async () => {