From f37280e0821d7e486e64ac7041b930c8e83ffa33 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Thu, 12 Feb 2026 13:42:32 +0100 Subject: [PATCH] refactor(sync): reduce MAX_VECTOR_CLOCK_SIZE from 30 to 20 Lower the cap to leave headroom for future increases and surface size-related edge cases earlier. All pruning logic is MAX-agnostic so this is a safe constant change with documentation updates. --- CLAUDE.md | 2 +- .../long-term-plans/server-side-entity-versioning.md | 2 +- .../background-info/vector-clock-pruning-research.md | 2 +- .../fix-plan-vector-clock-merge-vs-replace.md | 2 +- docs/sync-and-op-log/vector-clocks.md | 12 ++++++------ .../sync/supersync-import-other-client-ops.spec.ts | 4 ++-- .../sync/supersync-import-same-client-ops.spec.ts | 2 +- .../sync/supersync-vector-clock-pruning.spec.ts | 2 +- packages/shared-schema/src/vector-clock.ts | 8 ++++---- .../src/sync/services/validation.service.ts | 2 +- packages/super-sync-server/src/sync/sync.types.ts | 2 +- src/app/op-log/core/operation-log.const.ts | 2 +- 12 files changed, 21 insertions(+), 21 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4d2e83ca1b..eadda40524 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -126,7 +126,7 @@ The app uses NgRx (Redux pattern) for state management. Key state slices: 10. **TODAY_TAG is a Virtual Tag**: TODAY_TAG (ID: `'TODAY'`) must **NEVER** be added to `task.tagIds`. It's a "virtual tag" where membership is determined by `task.dueDay`, and `TODAY_TAG.taskIds` only stores ordering. This keeps move operations uniform across all tags. See `docs/ai/today-tag-architecture.md`. 11. **Event Loop Yield After Bulk Dispatches**: When applying many operations to NgRx in rapid succession (e.g., during sync replay), add `await new Promise(resolve => setTimeout(resolve, 0))` after the dispatch loop. `store.dispatch()` is non-blocking and returns immediately. Without yielding, 50+ rapid dispatches can overwhelm the store and cause state updates to be lost. See `OperationApplierService.applyOperations()` for the reference implementation. 12. **SYNC_IMPORT Semantics**: `SYNC_IMPORT` (and `BACKUP_IMPORT`) operations represent a **complete fresh start** - they replace the entire application state. All operations without knowledge of the import (CONCURRENT or LESS_THAN by vector clock) are dropped for all clients. See `SyncImportFilterService.filterOpsInvalidatedBySyncImport()`. This is correct behavior: the import is an explicit user action to restore to a specific state, and concurrent work is intentionally discarded. -13. **Vector Clock Pruning**: `MAX_VECTOR_CLOCK_SIZE` is 30 (a 30-entry clock is ~500 bytes). Pruning is extremely rare (needs 31+ unique client IDs). The server MUST prune (`limitVectorClockSize`) **after** conflict detection but **before** storage — never before comparison. The server's `sanitizeVectorClock()` applies a DoS cap at `5x MAX_VECTOR_CLOCK_SIZE` (150 entries) but does NOT prune to MAX. See `docs/sync-and-op-log/vector-clocks.md`. +13. **Vector Clock Pruning**: `MAX_VECTOR_CLOCK_SIZE` is 20 (a 20-entry clock is ~333 bytes). Pruning is extremely rare (needs 21+ unique client IDs). The server MUST prune (`limitVectorClockSize`) **after** conflict detection but **before** storage — never before comparison. The server's `sanitizeVectorClock()` applies a DoS cap at `5x MAX_VECTOR_CLOCK_SIZE` (100 entries) but does NOT prune to MAX. See `docs/sync-and-op-log/vector-clocks.md`. ## Git Commit Messages diff --git a/docs/long-term-plans/server-side-entity-versioning.md b/docs/long-term-plans/server-side-entity-versioning.md index 237c63f348..c7d3b0d97f 100644 --- a/docs/long-term-plans/server-side-entity-versioning.md +++ b/docs/long-term-plans/server-side-entity-versioning.md @@ -8,7 +8,7 @@ ## Problem -Vector clocks grow linearly with the number of participating clients. Pruning to `MAX_VECTOR_CLOCK_SIZE=30` loses causal information, though at MAX=30 this requires 31+ unique client IDs — extremely rare for a personal productivity app. Two lightweight backward-compat checks (`isLikelyPruningArtifact` and same-client check) handle legacy edge cases, but the fundamental issue remains: +Vector clocks grow linearly with the number of participating clients. Pruning to `MAX_VECTOR_CLOCK_SIZE=20` loses causal information, though at MAX=20 this requires 21+ unique client IDs — extremely rare for a personal productivity app. Two lightweight backward-compat checks (`isLikelyPruningArtifact` and same-client check) handle legacy edge cases, but the fundamental issue remains: The fundamental issue: vector clocks were designed for peer-to-peer systems where no node is authoritative. Super Productivity has a central server -- the server can define ordering authoritatively, making vector clocks unnecessary for online conflict detection. diff --git a/docs/sync-and-op-log/background-info/vector-clock-pruning-research.md b/docs/sync-and-op-log/background-info/vector-clock-pruning-research.md index cc208d2a7e..faada1dfb4 100644 --- a/docs/sync-and-op-log/background-info/vector-clock-pruning-research.md +++ b/docs/sync-and-op-log/background-info/vector-clock-pruning-research.md @@ -149,7 +149,7 @@ The project's current architecture uses a simple 2+1 layer approach: ### Why MAX=30 Simplified Everything -The original 4-layer defense (protected client IDs, pruning-aware comparison, `isLikelyPruningArtifact`, same-client check) was designed to work around a root cause: MAX=10 was too small, making pruning a frequent occurrence that interacted badly with SYNC_IMPORT operations. Commit `d70f18a94d` increased MAX from 10 to 30 (a 30-entry clock is ~500 bytes — negligible overhead) and removed the defense layers that were treating symptoms rather than the cause. With MAX=30, pruning requires 31+ unique client IDs — an extremely rare scenario for a personal productivity app. The `isLikelyPruningArtifact` heuristic and same-client check remain as safety nets, but the protected-client-IDs mechanism and pruning-aware comparison were removed as unnecessary complexity. +The original 4-layer defense (protected client IDs, pruning-aware comparison, `isLikelyPruningArtifact`, same-client check) was designed to work around a root cause: MAX=10 was too small, making pruning a frequent occurrence that interacted badly with SYNC_IMPORT operations. Commit `d70f18a94d` increased MAX from 10 to 30, which was later reduced to 20 (a 20-entry clock is ~333 bytes — negligible overhead), and removed the defense layers that were treating symptoms rather than the cause. With MAX=20, pruning requires 21+ unique client IDs — an extremely rare scenario for a personal productivity app. The `isLikelyPruningArtifact` heuristic and same-client check remain as safety nets, but the protected-client-IDs mechanism and pruning-aware comparison were removed as unnecessary complexity. --- diff --git a/docs/sync-and-op-log/fix-plan-vector-clock-merge-vs-replace.md b/docs/sync-and-op-log/fix-plan-vector-clock-merge-vs-replace.md index e3b0b4919d..559e1970ba 100644 --- a/docs/sync-and-op-log/fix-plan-vector-clock-merge-vs-replace.md +++ b/docs/sync-and-op-log/fix-plan-vector-clock-merge-vs-replace.md @@ -15,7 +15,7 @@ these ops as CONCURRENT with the import and discard them. from `const mergedClock = { ...currentClock }` (the old clock) and merges on top 2. Result: old 10 entries + `B_sUq7:1` = 11 entries 3. Client creates new ops with 11-entry clock (no client-side pruning) -4. Server prunes to MAX_VECTOR_CLOCK_SIZE (was 10, now 30), dropping `B_sUq7:1` (lowest counter) +4. Server prunes to MAX_VECTOR_CLOCK_SIZE (was 10, now 20), dropping `B_sUq7:1` (lowest counter) 5. `SyncImportFilterService` on the importing client compares: op missing `B_sUq7` vs import `{B_sUq7:1}` → CONCURRENT → discarded 6. `isLikelyPruningArtifact` returns false because import clock has only 1 entry (< MAX) diff --git a/docs/sync-and-op-log/vector-clocks.md b/docs/sync-and-op-log/vector-clocks.md index 9cabcfcc4f..16849a50af 100644 --- a/docs/sync-and-op-log/vector-clocks.md +++ b/docs/sync-and-op-log/vector-clocks.md @@ -18,9 +18,9 @@ Each entry maps a client ID to a monotonically increasing counter. A clock with | Constant | Value | Purpose | | ----------------------- | ----- | --------------------------------- | -| `MAX_VECTOR_CLOCK_SIZE` | 30 | Maximum entries in a pruned clock | +| `MAX_VECTOR_CLOCK_SIZE` | 20 | Maximum entries in a pruned clock | -At 6-char client IDs, a 30-entry clock is ~500 bytes — negligible bandwidth. A user needs 31+ unique client IDs (reinstalls/new browsers) before pruning triggers, which is extremely unlikely for a personal productivity app. +At 6-char client IDs, a 20-entry clock is ~333 bytes — negligible bandwidth. A user needs 21+ unique client IDs (reinstalls/new browsers) before pruning triggers, which is extremely unlikely for a personal productivity app. --- @@ -106,7 +106,7 @@ In `operation-log.effects.ts`: In `sync.service.ts` (`processOperation`): -1. `ValidationService.validateOp()` sanitizes the clock (DoS cap at 5×MAX = 150 entries) but does **NOT** prune +1. `ValidationService.validateOp()` sanitizes the clock (DoS cap at 5×MAX = 100 entries) but does **NOT** prune 2. `detectConflict()` compares the **full unpruned** incoming clock against the existing entity clock 3. If accepted: `limitVectorClockSize(clock, [clientId])` prunes to MAX before storage, preserving only the uploading client's ID 4. The pruned clock is stored in the database @@ -129,7 +129,7 @@ Normal operations are **NEVER** pruned client-side. The server prunes **after** ### Why Pruning Exists -Clocks grow with each new client. Without bounds, a user who has used many devices would have ever-growing clocks. Pruning limits clocks to `MAX_VECTOR_CLOCK_SIZE` (30) entries. +Clocks grow with each new client. Without bounds, a user who has used many devices would have ever-growing clocks. Pruning limits clocks to `MAX_VECTOR_CLOCK_SIZE` (20) entries. ### The `limitVectorClockSize` Algorithm @@ -158,7 +158,7 @@ Implemented in `packages/shared-schema/src/vector-clock.ts`. The client wrapper ### Pruning is Rare -With MAX=30, a user needs 31+ unique client IDs before pruning triggers. In the unlikely event it does trigger, the worst case is one extra server round-trip (false CONCURRENT → client resolves → re-uploads with >MAX clock → GREATER_THAN → accepted). +With MAX=20, a user needs 21+ unique client IDs before pruning triggers. In the unlikely event it does trigger, the worst case is one extra server round-trip (false CONCURRENT → client resolves → re-uploads with >MAX clock → GREATER_THAN → accepted). --- @@ -348,7 +348,7 @@ Rules that must hold for the system to be correct. Use these to verify implement 7. **Global clock is REPLACED (not merged) on remote SYNC_IMPORT.** `mergeRemoteOpClocks()` starts from the import's clock as the base, then merges remaining ops on top. This prevents clock bloat. -8. **DoS cap is NOT pruning.** `sanitizeVectorClock()` rejects clocks with > 5×MAX (150) entries entirely — it doesn't prune them down. This is a validation gate, not a size reduction. +8. **DoS cap is NOT pruning.** `sanitizeVectorClock()` rejects clocks with > 5×MAX (100) entries entirely — it doesn't prune them down. This is a validation gate, not a size reduction. --- diff --git a/e2e/tests/sync/supersync-import-other-client-ops.spec.ts b/e2e/tests/sync/supersync-import-other-client-ops.spec.ts index 3d730720db..ad05df5340 100644 --- a/e2e/tests/sync/supersync-import-other-client-ops.spec.ts +++ b/e2e/tests/sync/supersync-import-other-client-ops.spec.ts @@ -32,7 +32,7 @@ import { waitForAppReady } from '../../utils/waits'; * * IMPORTANT LIMITATION: * With only 2 fresh E2E clients, vector clocks have ~3 entries — well below the - * MAX=30 pruning threshold. Without pruning, compareVectorClocks returns GREATER_THAN + * MAX=20 pruning threshold. Without pruning, compareVectorClocks returns GREATER_THAN * (correct!) even with the merge bug. The bug ONLY manifests when accumulated entries * exceed MAX and pruning removes the import entry. * @@ -150,7 +150,7 @@ test.describe('@supersync @pruning Other client post-import ops sync correctly', // ============ PHASE 6: Inject bloated vector clock into Client B ============ // This simulates real-world accumulated history from many old devices. - // Without this injection, the vector clock only has ~3 entries (well below MAX=30) + // Without this injection, the vector clock only has ~3 entries (well below MAX=20) // and pruning never happens, so the bug doesn't manifest. console.log( '[Other-Client Import] Phase 6: Injecting old device entries into Client B vector clock', diff --git a/e2e/tests/sync/supersync-import-same-client-ops.spec.ts b/e2e/tests/sync/supersync-import-same-client-ops.spec.ts index d9cb7cd4e5..13cc77a01c 100644 --- a/e2e/tests/sync/supersync-import-same-client-ops.spec.ts +++ b/e2e/tests/sync/supersync-import-same-client-ops.spec.ts @@ -18,7 +18,7 @@ import { waitForAppReady } from '../../utils/waits'; * a SYNC_IMPORT are correctly synced to other clients. * * BUG SCENARIO (vector clock pruning asymmetry): - * 1. Client A creates SYNC_IMPORT with a 30-entry (MAX) vector clock + * 1. Client A creates SYNC_IMPORT with a 20-entry (MAX) vector clock * 2. Client A continues creating ops; over time, new clients join, pushing * A's clock past MAX → server prunes entries * 3. A's ops have different pruned entries than A's frozen import clock diff --git a/e2e/tests/sync/supersync-vector-clock-pruning.spec.ts b/e2e/tests/sync/supersync-vector-clock-pruning.spec.ts index 7a617b6a9b..878baa2073 100644 --- a/e2e/tests/sync/supersync-vector-clock-pruning.spec.ts +++ b/e2e/tests/sync/supersync-vector-clock-pruning.spec.ts @@ -20,7 +20,7 @@ import { waitForAppReady } from '../../utils/waits'; * BUG SCENARIO: * 1. Client A creates SYNC_IMPORT with clock {A: 1} * 2. Client B receives it, merges clocks → {A: 1, B: x, ...other clients...} - * 3. When B has 91+ clients, pruning triggers (MAX_VECTOR_CLOCK_SIZE = 10) + * 3. When B has 21+ clients, pruning triggers (MAX_VECTOR_CLOCK_SIZE = 20) * 4. A's entry (counter=1, lowest) gets PRUNED * 5. New tasks from B have clock {B: y} - MISSING A's entry! * 6. Comparison: {A: 0} vs {A: 1} → CONCURRENT diff --git a/packages/shared-schema/src/vector-clock.ts b/packages/shared-schema/src/vector-clock.ts index 86969f60f4..5b5a8e8dd5 100644 --- a/packages/shared-schema/src/vector-clock.ts +++ b/packages/shared-schema/src/vector-clock.ts @@ -32,11 +32,11 @@ export type VectorClockComparison = 'EQUAL' | 'LESS_THAN' | 'GREATER_THAN' | 'CO * Maximum number of entries in a vector clock. * Shared between client and server to ensure consistent pruning. * - * At 6-char client IDs, a 30-entry clock is ~500 bytes — negligible bandwidth. - * A user needs 31+ unique client IDs (reinstalls/new browsers) before pruning + * At 6-char client IDs, a 20-entry clock is ~333 bytes — negligible bandwidth. + * A user needs 21+ unique client IDs (reinstalls/new browsers) before pruning * triggers, which is extremely unlikely for a personal productivity app. */ -export const MAX_VECTOR_CLOCK_SIZE = 30; +export const MAX_VECTOR_CLOCK_SIZE = 20; /** * Compare two vector clocks to determine their relationship. @@ -45,7 +45,7 @@ export const MAX_VECTOR_CLOCK_SIZE = 30; * Both implementations import from this shared module to ensure consistency. * * Standard vector clock comparison: missing keys mean "genuinely zero". - * With MAX_VECTOR_CLOCK_SIZE=30, pruning is extremely rare (needs 31+ unique + * With MAX_VECTOR_CLOCK_SIZE=20, pruning is extremely rare (needs 21+ unique * client IDs), so pruning-aware comparison is unnecessary. * * @param a First vector clock 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 a2e7c05496..c8c9383653 100644 --- a/packages/super-sync-server/src/sync/services/validation.service.ts +++ b/packages/super-sync-server/src/sync/services/validation.service.ts @@ -151,7 +151,7 @@ export class ValidationService { // MAX_VECTOR_CLOCK_SIZE (e.g., during conflict resolution where the client includes // all entity clock IDs + its own ID). The comparison then sees non-shared keys and // returns CONCURRENT instead of GREATER_THAN → infinite rejection loop. - // DoS protection: sanitizeVectorClock() above caps at 5x MAX_VECTOR_CLOCK_SIZE (150). + // DoS protection: sanitizeVectorClock() above caps at 5x MAX_VECTOR_CLOCK_SIZE (100). // 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/src/sync/sync.types.ts b/packages/super-sync-server/src/sync/sync.types.ts index 241906710f..aa35901316 100644 --- a/packages/super-sync-server/src/sync/sync.types.ts +++ b/packages/super-sync-server/src/sync/sync.types.ts @@ -76,7 +76,7 @@ export type OpType = (typeof OP_TYPES)[number]; * Returns a sanitized clock with validated entries, or an error. * * Validation rules: - * - Maximum MAX_VECTOR_CLOCK_SIZE * 5 (150) entries (prevents DoS via huge clocks) + * - Maximum MAX_VECTOR_CLOCK_SIZE * 5 (100) entries (prevents DoS via huge clocks) * - Keys must be non-empty strings, max 255 characters * - Values must be non-negative integers, max 10 million * - Invalid entries are removed (not rejected) diff --git a/src/app/op-log/core/operation-log.const.ts b/src/app/op-log/core/operation-log.const.ts index 9a8bf674c3..0bcc14b135 100644 --- a/src/app/op-log/core/operation-log.const.ts +++ b/src/app/op-log/core/operation-log.const.ts @@ -8,7 +8,7 @@ import { InjectionToken } from '@angular/core'; * * | Limit | Value | Constant | Notes | * |-------|-------|----------|-------| - * | Vector clock clients | 30 | MAX_VECTOR_CLOCK_SIZE | Pruning keeps most active clients | + * | Vector clock clients | 20 | MAX_VECTOR_CLOCK_SIZE | Pruning keeps most active clients | * | Client ID length | ≥5 chars | (vector-clock.ts) | Throws error if shorter | * | Vector clock counter | MAX_SAFE_INTEGER-1000 | (vector-clock.ts) | Requires SYNC_IMPORT on overflow | * | Ops per upload batch | 25 | MAX_OPS_PER_UPLOAD_REQUEST | Reduced from 100 to avoid 413 errors |