From cbeecfd39a0a908ecd28fcb94cdf99cf31e697d4 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Sat, 3 Jan 2026 18:05:11 +0100 Subject: [PATCH] fix(sync): multi-tab vector clock staleness + shared code extraction Bug fix: - Fix vector clock cache staleness in multi-tab scenarios by clearing cache when acquiring operation write lock. Each browser tab has its own in-memory cache, so Tab B's cache could be stale if Tab A wrote while Tab B was waiting for the lock. Shared code extraction (client/server consistency): - Extract vector clock comparison to @sp/shared-schema - Client wraps shared impl with null handling - Server imports directly from shared - Extract entity types to @sp/shared-schema - Single source of truth for ENTITY_TYPES array - Removes duplicated "must match" comments Files: - packages/shared-schema/src/vector-clock.ts (new) - packages/shared-schema/src/entity-types.ts (new) - src/app/op-log/store/operation-log-store.service.ts (cache clear) - src/app/op-log/capture/operation-log.effects.ts (call cache clear) --- packages/shared-schema/src/entity-types.ts | 42 +++++++++ packages/shared-schema/src/index.ts | 8 ++ packages/shared-schema/src/vector-clock.ts | 80 +++++++++++++++++ .../src/sync/services/validation.service.ts | 27 +----- .../super-sync-server/src/sync/sync.types.ts | 52 +++-------- src/app/core/util/vector-clock.ts | 86 ++++++------------- .../op-log/capture/operation-log.effects.ts | 5 ++ src/app/op-log/core/operation.types.ts | 29 ++----- .../store/operation-log-store.service.spec.ts | 34 ++++++++ .../store/operation-log-store.service.ts | 21 +++++ 10 files changed, 239 insertions(+), 145 deletions(-) create mode 100644 packages/shared-schema/src/entity-types.ts create mode 100644 packages/shared-schema/src/vector-clock.ts diff --git a/packages/shared-schema/src/entity-types.ts b/packages/shared-schema/src/entity-types.ts new file mode 100644 index 000000000..2b959d95a --- /dev/null +++ b/packages/shared-schema/src/entity-types.ts @@ -0,0 +1,42 @@ +/** + * Entity Types for distributed synchronization. + * + * These are the valid entity types for operations in the sync system. + * Shared between client and server to ensure consistency. + * + * IMPORTANT: This module is shared between client and server. + * Any changes must be compatible with both environments. + */ + +/** + * Valid entity types for operations. + * Operations with unknown entity types will be rejected by the server. + */ +export const ENTITY_TYPES = [ + 'TASK', + 'PROJECT', + 'TAG', + 'NOTE', + 'GLOBAL_CONFIG', + 'SIMPLE_COUNTER', + 'WORK_CONTEXT', + 'TIME_TRACKING', + 'TASK_REPEAT_CFG', + 'ISSUE_PROVIDER', + 'PLANNER', + 'MENU_TREE', + 'METRIC', + 'BOARD', + 'REMINDER', + 'PLUGIN_USER_DATA', + 'PLUGIN_METADATA', + 'MIGRATION', + 'RECOVERY', // For disaster recovery imports + 'ALL', // For full state imports (sync, backup) +] as const; + +/** + * Entity type - identifies the kind of data entity being operated on. + * Derived from ENTITY_TYPES array for single source of truth. + */ +export type EntityType = (typeof ENTITY_TYPES)[number]; diff --git a/packages/shared-schema/src/index.ts b/packages/shared-schema/src/index.ts index 7c0373265..1c6799063 100644 --- a/packages/shared-schema/src/index.ts +++ b/packages/shared-schema/src/index.ts @@ -26,3 +26,11 @@ export { // Migration registry (for inspection/debugging) export { MIGRATIONS } from './migrations/index.js'; + +// Vector clock types and comparison (shared between client and server) +export type { VectorClock, VectorClockComparison } from './vector-clock.js'; +export { compareVectorClocks, mergeVectorClocks } from './vector-clock.js'; + +// Entity types (shared between client and server) +export type { EntityType } from './entity-types.js'; +export { ENTITY_TYPES } from './entity-types.js'; diff --git a/packages/shared-schema/src/vector-clock.ts b/packages/shared-schema/src/vector-clock.ts new file mode 100644 index 000000000..54b72ea7d --- /dev/null +++ b/packages/shared-schema/src/vector-clock.ts @@ -0,0 +1,80 @@ +/** + * Vector Clock types and comparison functions for distributed synchronization. + * + * A vector clock is a data structure used to determine the partial ordering of events + * in a distributed system and detect causality violations. + * + * Each process/device maintains its own component in the vector, incrementing it + * on local updates. This allows us to determine if two states are: + * - EQUAL: Same vector values + * - LESS_THAN: A happened before B + * - GREATER_THAN: B happened before A + * - CONCURRENT: Neither happened before the other (true conflict) + * + * IMPORTANT: This module is shared between client and server. + * Any changes must be compatible with both environments. + */ + +/** + * Vector clock data structure. + * Maps client IDs to their respective clock values. + */ +export interface VectorClock { + [clientId: string]: number; +} + +/** + * Result of comparing two vector clocks. + */ +export type VectorClockComparison = 'EQUAL' | 'LESS_THAN' | 'GREATER_THAN' | 'CONCURRENT'; + +/** + * Compare two vector clocks to determine their relationship. + * + * CRITICAL: This algorithm must produce identical results on client and server. + * Both implementations import from this shared module to ensure consistency. + * + * @param a First vector clock + * @param b Second vector clock + * @returns The comparison result + */ +export const compareVectorClocks = ( + a: VectorClock, + b: VectorClock, +): VectorClockComparison => { + const allKeys = new Set([...Object.keys(a), ...Object.keys(b)]); + + let aGreater = false; + let bGreater = false; + + for (const key of allKeys) { + const aVal = a[key] ?? 0; + const bVal = b[key] ?? 0; + + if (aVal > bVal) aGreater = true; + if (bVal > aVal) bGreater = true; + } + + if (aGreater && bGreater) return 'CONCURRENT'; + if (aGreater) return 'GREATER_THAN'; + if (bGreater) return 'LESS_THAN'; + return 'EQUAL'; +}; + +/** + * Merge two vector clocks, taking the maximum value for each client. + * Creates a new clock that dominates both inputs. + * + * @param a First vector clock + * @param b Second vector clock + * @returns A new merged vector clock + */ +export const mergeVectorClocks = (a: VectorClock, b: VectorClock): VectorClock => { + const merged: VectorClock = { ...a }; + + for (const [key, value] of Object.entries(b)) { + merged[key] = Math.max(merged[key] ?? 0, value); + } + + return merged; +}; 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 78e4ac543..51199d4b0 100644 --- a/packages/super-sync-server/src/sync/services/validation.service.ts +++ b/packages/super-sync-server/src/sync/services/validation.service.ts @@ -13,34 +13,15 @@ import { sanitizeVectorClock, validatePayload, } from '../sync.types'; +import { ENTITY_TYPES } from '@sp/shared-schema'; /** * Valid entity types for operations. - * Must match the EntityType union in the client's operation.types.ts. + * Uses shared ENTITY_TYPES from @sp/shared-schema to ensure client/server consistency. * Operations with unknown entity types will be rejected. + * Typed as Set since we're validating unknown input strings. */ -export const ALLOWED_ENTITY_TYPES = new Set([ - 'TASK', - 'PROJECT', - 'TAG', - 'NOTE', - 'GLOBAL_CONFIG', - 'TIME_TRACKING', - 'SIMPLE_COUNTER', - 'WORK_CONTEXT', - 'TASK_REPEAT_CFG', - 'ISSUE_PROVIDER', - 'PLANNER', - 'MENU_TREE', - 'METRIC', - 'BOARD', - 'REMINDER', - 'MIGRATION', - 'RECOVERY', - 'ALL', - 'PLUGIN_USER_DATA', - 'PLUGIN_METADATA', -]); +export const ALLOWED_ENTITY_TYPES: Set = new Set(ENTITY_TYPES); export interface ValidationResult { valid: boolean; diff --git a/packages/super-sync-server/src/sync/sync.types.ts b/packages/super-sync-server/src/sync/sync.types.ts index e784982a5..c5a5d0c5d 100644 --- a/packages/super-sync-server/src/sync/sync.types.ts +++ b/packages/super-sync-server/src/sync/sync.types.ts @@ -1,4 +1,12 @@ import { Logger } from '../logger'; +import { + VectorClock, + VectorClockComparison, + compareVectorClocks, +} from '@sp/shared-schema'; + +// Re-export for consumers of this module +export { VectorClock, VectorClockComparison, compareVectorClocks }; // Structured error codes for client handling export const SYNC_ERROR_CODES = { @@ -50,21 +58,8 @@ export const OP_TYPES = [ export type OpType = (typeof OP_TYPES)[number]; -export type VectorClock = Record; - -/** - * Compare two vector clocks. - * - * SYNC NOTE: This algorithm must match the client implementation at: - * src/app/core/util/vector-clock.ts - compareVectorClocks() - * - * Returns: - * - 'LESS_THAN': a happened before b - * - 'GREATER_THAN': b happened before a - * - 'EQUAL': clocks are identical - * - 'CONCURRENT': neither happened before the other (conflict!) - */ -export type VectorClockComparison = 'LESS_THAN' | 'GREATER_THAN' | 'EQUAL' | 'CONCURRENT'; +// VectorClock, VectorClockComparison, and compareVectorClocks are imported from @sp/shared-schema +// and re-exported above. This ensures client and server use identical implementations. /** * Validates and sanitizes a vector clock. @@ -122,32 +117,7 @@ export const sanitizeVectorClock = ( return { valid: true, clock: sanitized }; }; -/** - * SYNC NOTE: This algorithm must match the client implementation at: - * src/app/core/util/vector-clock.ts - compareVectorClocks() - */ -export const compareVectorClocks = ( - a: VectorClock, - b: VectorClock, -): VectorClockComparison => { - const allKeys = new Set([...Object.keys(a), ...Object.keys(b)]); - - let aGreater = false; - let bGreater = false; - - for (const key of allKeys) { - const aVal = a[key] ?? 0; - const bVal = b[key] ?? 0; - - if (aVal > bVal) aGreater = true; - if (bVal > aVal) bGreater = true; - } - - if (aGreater && bGreater) return 'CONCURRENT'; - if (aGreater) return 'GREATER_THAN'; - if (bGreater) return 'LESS_THAN'; - return 'EQUAL'; -}; +// compareVectorClocks is imported from @sp/shared-schema (see imports at top of file) export interface Operation { id: string; diff --git a/src/app/core/util/vector-clock.ts b/src/app/core/util/vector-clock.ts index 65044cb43..8eef94cfe 100644 --- a/src/app/core/util/vector-clock.ts +++ b/src/app/core/util/vector-clock.ts @@ -1,4 +1,9 @@ import { PFLog } from '../log'; +import { + VectorClock as SharedVectorClock, + compareVectorClocks as sharedCompareVectorClocks, + mergeVectorClocks as sharedMergeVectorClocks, +} from '@sp/shared-schema'; /** * Vector Clock implementation for distributed synchronization @@ -12,18 +17,20 @@ import { PFLog } from '../log'; * - LESS_THAN: A happened before B * - GREATER_THAN: B happened before A * - CONCURRENT: Neither happened before the other (true conflict) + * + * IMPORTANT: Core comparison logic is shared with the server via @sp/shared-schema. + * This file wraps the shared logic with null handling for client-side use. */ /** * Vector clock data structure * Maps client IDs to their respective clock values */ -export interface VectorClock { - [clientId: string]: number; -} +export type VectorClock = SharedVectorClock; /** - * Result of comparing two vector clocks + * Result of comparing two vector clocks. + * Uses enum for client-side ergonomics, values match shared string literals. */ export enum VectorClockComparison { EQUAL = 'EQUAL', @@ -112,10 +119,11 @@ export const sanitizeVectorClock = (clock: any): VectorClock => { }; /** - * Compare two vector clocks to determine their relationship + * Compare two vector clocks to determine their relationship. * - * SYNC NOTE: This algorithm must match the server implementation at: - * packages/super-sync-server/src/sync/sync.types.ts - compareVectorClocks() + * Uses the shared implementation from @sp/shared-schema to ensure + * client and server produce identical results. This wrapper adds + * null/undefined handling for client-side convenience. * * @param a First vector clock * @param b Second vector clock @@ -125,56 +133,20 @@ export const compareVectorClocks = ( a: VectorClock | null | undefined, b: VectorClock | null | undefined, ): VectorClockComparison => { - // Handle null/undefined cases + // Handle null/undefined cases (shared implementation requires non-null) if (isVectorClockEmpty(a) && isVectorClockEmpty(b)) { - // Both empty = equal (both have no history) return VectorClockComparison.EQUAL; } if (isVectorClockEmpty(a)) { - // a is empty, b has history = a happened before b (a < b) return VectorClockComparison.LESS_THAN; } if (isVectorClockEmpty(b)) { - // b is empty, a has history = a happened after b (a > b) return VectorClockComparison.GREATER_THAN; } - // Safe type assertion after null checks - const clockA = a!; - const clockB = b!; - - // Get all client IDs from both clocks - const allClientIds = new Set([...Object.keys(clockA), ...Object.keys(clockB)]); - - let aHasGreater = false; - let bHasGreater = false; - - // Compare each component - for (const clientId of allClientIds) { - const aVal = clockA[clientId] || 0; - const bVal = clockB[clientId] || 0; - - if (aVal > bVal) { - aHasGreater = true; - } - if (bVal > aVal) { - bHasGreater = true; - } - } - - // Determine relationship - if (!aHasGreater && !bHasGreater) { - return VectorClockComparison.EQUAL; - } else if (!aHasGreater) { - // B has some greater components, A has none -> A < B - return VectorClockComparison.LESS_THAN; - } else if (!bHasGreater) { - // A has some greater components, B has none -> A > B - return VectorClockComparison.GREATER_THAN; - } else { - // Both have some greater components -> concurrent - return VectorClockComparison.CONCURRENT; - } + // Delegate to shared implementation and convert string result to enum + const result = sharedCompareVectorClocks(a!, b!); + return VectorClockComparison[result]; }; /** @@ -229,8 +201,11 @@ export const incrementVectorClock = ( }; /** - * Merge two vector clocks by taking the maximum value for each component - * This is used when receiving updates to ensure we have the most recent view + * Merge two vector clocks by taking the maximum value for each component. + * + * Uses the shared implementation from @sp/shared-schema to ensure + * client and server produce identical results. This wrapper adds + * null/undefined handling for client-side convenience. * * @param a First vector clock * @param b Second vector clock @@ -240,19 +215,12 @@ export const mergeVectorClocks = ( a: VectorClock | null | undefined, b: VectorClock | null | undefined, ): VectorClock => { + // Handle null/undefined cases (shared implementation requires non-null) if (isVectorClockEmpty(a)) return { ...(b || {}) }; if (isVectorClockEmpty(b)) return { ...(a || {}) }; - const merged: VectorClock = {}; - const allClientIds = new Set([...Object.keys(a!), ...Object.keys(b!)]); - - for (const clientId of allClientIds) { - const aVal = a![clientId] || 0; - const bVal = b![clientId] || 0; - merged[clientId] = Math.max(aVal, bVal); - } - - return merged; + // Delegate to shared implementation + return sharedMergeVectorClocks(a!, b!); }; /** diff --git a/src/app/op-log/capture/operation-log.effects.ts b/src/app/op-log/capture/operation-log.effects.ts index 4477c5f4e..145e11231 100644 --- a/src/app/op-log/capture/operation-log.effects.ts +++ b/src/app/op-log/capture/operation-log.effects.ts @@ -152,6 +152,11 @@ export class OperationLogEffects { // // By acquiring the lock first, flushPendingWrites() must wait for us to finish writing. await this.lockService.request(LOCK_NAMES.OPERATION_LOG, async () => { + // MULTI-TAB SAFETY: Clear vector clock cache to ensure fresh read after other tabs + // may have written while we were waiting for the lock. Each tab has its own in-memory + // cache, so Tab B's cache could be stale if Tab A wrote while Tab B was waiting. + this.opLogStore.clearVectorClockCache(); + // Get entity changes from the FIFO queue (for TIME_TRACKING and TASK time sync) // For most actions, this returns empty array since action payloads are sufficient. // IMPORTANT: This MUST happen inside the lock to prevent race with flushPendingWrites. diff --git a/src/app/op-log/core/operation.types.ts b/src/app/op-log/core/operation.types.ts index 7163ba39f..fa2150647 100644 --- a/src/app/op-log/core/operation.types.ts +++ b/src/app/op-log/core/operation.types.ts @@ -1,6 +1,7 @@ import { VectorClock } from '../../core/util/vector-clock'; import { ActionType } from './action-types.enum'; -export { VectorClock, ActionType }; +import { EntityType as SharedEntityType, ENTITY_TYPES } from '@sp/shared-schema'; +export { VectorClock, ActionType, ENTITY_TYPES }; export enum OpType { Create = 'CRT', @@ -13,27 +14,11 @@ export enum OpType { Repair = 'REPAIR', // Auto-repair operation with full repaired state } -export type EntityType = - | 'TASK' - | 'PROJECT' - | 'TAG' - | 'NOTE' - | 'GLOBAL_CONFIG' - | 'SIMPLE_COUNTER' - | 'WORK_CONTEXT' - | 'TIME_TRACKING' - | 'TASK_REPEAT_CFG' - | 'ISSUE_PROVIDER' - | 'PLANNER' - | 'MENU_TREE' - | 'METRIC' - | 'BOARD' - | 'REMINDER' - | 'PLUGIN_USER_DATA' - | 'PLUGIN_METADATA' - | 'MIGRATION' - | 'RECOVERY' // For disaster recovery imports - | 'ALL'; // For full state imports (sync, backup) +/** + * Entity type - identifies the kind of data entity being operated on. + * Re-exported from @sp/shared-schema to ensure client/server consistency. + */ +export type EntityType = SharedEntityType; export interface Operation { /** diff --git a/src/app/op-log/store/operation-log-store.service.spec.ts b/src/app/op-log/store/operation-log-store.service.spec.ts index 852e400c5..5e64d0080 100644 --- a/src/app/op-log/store/operation-log-store.service.spec.ts +++ b/src/app/op-log/store/operation-log-store.service.spec.ts @@ -1757,6 +1757,40 @@ describe('OperationLogStoreService', () => { const clock3 = await service.getVectorClock(); expect(clock3).toEqual({ client: 5 }); }); + + it('should clear cache when clearVectorClockCache is called', async () => { + // Set vector clock and read it (populates cache) + await service.setVectorClock({ tabA: 10, tabB: 5 }); + const cachedClock = await service.getVectorClock(); + expect(cachedClock).toEqual({ tabA: 10, tabB: 5 }); + + // Clear the cache + service.clearVectorClockCache(); + + // Next read should fetch from IndexedDB (which still has the value) + const freshClock = await service.getVectorClock(); + expect(freshClock).toEqual({ tabA: 10, tabB: 5 }); + }); + + it('should force fresh read from IndexedDB after clearVectorClockCache', async () => { + // This test simulates the multi-tab scenario where another tab updates IndexedDB + // while this tab has a stale cache. + + // Set initial clock and populate cache + await service.setVectorClock({ originalClient: 1 }); + await service.getVectorClock(); + + // Simulate another tab writing directly to IndexedDB (bypassing our cache) + // We do this by using setVectorClock which updates both DB and cache + await service.setVectorClock({ originalClient: 1, anotherTabClient: 99 }); + + // Now clear cache to simulate what happens after acquiring a lock + service.clearVectorClockCache(); + + // Should get the updated value from IndexedDB + const clock = await service.getVectorClock(); + expect(clock).toEqual({ originalClient: 1, anotherTabClient: 99 }); + }); }); describe('mergeRemoteOpClocks', () => { diff --git a/src/app/op-log/store/operation-log-store.service.ts b/src/app/op-log/store/operation-log-store.service.ts index 2d783f277..a2039d2fb 100644 --- a/src/app/op-log/store/operation-log-store.service.ts +++ b/src/app/op-log/store/operation-log-store.service.ts @@ -898,6 +898,27 @@ export class OperationLogStoreService { this._vectorClockCache = clock; } + /** + * Clears the in-memory vector clock cache, forcing next read to fetch from IndexedDB. + * + * MULTI-TAB SAFETY: Each browser tab maintains its own in-memory cache. When Tab A + * writes a new operation and updates its cache, Tab B's cache remains stale. + * Call this before reading the vector clock inside a Web Lock to ensure freshness + * after other tabs may have written. + * + * The typical pattern is: + * ``` + * await lockService.request(OPERATION_LOG, async () => { + * opLogStore.clearVectorClockCache(); // Force fresh read + * const clock = await vectorClockService.getCurrentVectorClock(); + * // ... create operation with correct clock + * }); + * ``` + */ + clearVectorClockCache(): void { + this._vectorClockCache = null; + } + /** * Merges remote operations' vector clocks into the local vector clock. *