diff --git a/docs/long-term-plans/sync-core-extraction-plan.md b/docs/long-term-plans/sync-core-extraction-plan.md index d601df2c70..5719d5218a 100644 --- a/docs/long-term-plans/sync-core-extraction-plan.md +++ b/docs/long-term-plans/sync-core-extraction-plan.md @@ -1,9 +1,9 @@ # `@sp/sync-core` Extraction Plan -> **Status: In progress - PR 1, PR 2 guardrails/logger adapter work, and PR 3a -> vector-clock ownership are present on this branch. Remaining PR 2 cleanup is -> PR text alignment plus targeted future `SyncLogger` routing for files as they -> move; PR 3b can start with pure algorithmic moves.** +> **Status: In progress - PR 1, PR 2 guardrails/logger adapter work, PR 3a +> vector-clock ownership, and the PR 3b pure helper slices are present on this +> branch. Remaining PR 2 cleanup is PR text alignment plus targeted future +> `SyncLogger` routing for files as they move.** **Goal:** Carve the sync engine out of `src/app/op-log/` into a reusable, framework-agnostic, **domain-agnostic** `@sp/sync-core` package, plus a sibling @@ -109,21 +109,43 @@ groundwork: injection token. Existing helper functions still read the app-side `ENTITY_CONFIGS` singleton for compatibility. -Remaining immediate debt before PR 3b / further algorithm extraction: +Current extraction state and remaining immediate debt: - `FULL_STATE_OP_TYPES` still lives in `@sp/sync-core` as compatibility debt. - Vector-clock compare/merge/prune now lives in `@sp/sync-core`, with `@sp/shared-schema` re-exporting it for existing client/server imports. - `SyncLogger` exists, but movable app code still mostly calls `OpLog` directly. - `@sp/sync-core` has a Vitest package test runner and vector-clock tests. +- PR 3b has generic conflict helpers in + `packages/sync-core/src/conflict-resolution.ts`: deep equality, identical + conflict detection, conflict-resolution suggestion, entity frontier + construction, clock-corruption comparison adjustment, pure LWW conflict + resolution planning, and local-DELETE-loses-to-remote-UPDATE payload + extraction/merge helpers. It also owns pure LWW resolution partitioning: + local/remote winner counts, remote-winner ops after host processing, + local-winner remote ops, rejected-op id buckets, local-win op collection, and + remote-winner affected entity-key calculation. The Angular + `ConflictResolutionService` delegates to these helpers while keeping app + orchestration, IndexedDB/apply flow, entity lookup, NgRx, dev-error wiring, + app action-type ownership, fallback logging, and operation creation app-side. +- PR 3b also has the pure full-state import vector-clock decision helper in + `packages/sync-core/src/sync-import-filter.ts`. The Angular + `SyncImportFilterService` still owns full-state operation classification, + latest import lookup from batch/store, IndexedDB access, conflict-dialog + signaling, and logging. Suggested next order: 1. Finish PR 2 documentation and verification. 2. Add the app-side `SyncLogger` adapter before moving `OpLog`-using files. -3. Make full-state operation classification configurable or explicitly mark +3. Treat the PR 3b pure conflict-resolution and sync-import slices as complete + for this round. +4. Make full-state operation classification configurable or explicitly mark those op types as host-defined compatibility strings. -4. Start PR 3b with pure algorithmic moves only after the logger adapter exists. +5. Clean up logger/config dependencies before moving compression, prefix, or + error helpers. +6. Defer port/orchestration work until the remaining pure/config boundaries are + settled. ## PR 1 - Thin First Slice (#7546) @@ -515,6 +537,31 @@ contracts until those are separately decoupled. Move framework-agnostic, stateless sync algorithms. These should only need typed inputs and the logger port. +### Current State + +- `deepEqual`, `isIdenticalConflict`, `suggestConflictResolution`, + `buildEntityFrontier`, `adjustForClockCorruption`, and + `planLwwConflictResolutions` live in `@sp/sync-core` with package-level + Vitest coverage. +- `classifyOpAgainstSyncImport` lives in `@sp/sync-core` and owns only the + vector-clock keep/invalidate decision for an op against the latest full-state + import. It returns the raw comparison plus a reason so app logging stays + unchanged. +- Local DELETE losing to remote UPDATE conversion now delegates to + `extractEntityFromPayload`, `extractUpdateChanges`, and + `convertLocalDeleteRemoteUpdatesToLww` in `@sp/sync-core`. The app supplies + payload-key resolution, LWW action-type conversion, singleton-id handling, + and fallback warning logging. +- `ConflictResolutionService` keeps compatibility wrappers/call sites and + passes the app `SyncLogger` adapter into package helpers. It also supplies + the app-owned archive action predicate to LWW planning and creates + archive/local-win operations app-side. +- Pure remote/local operation partitioning now lives in `@sp/sync-core`; + NgRx state lookup and operation creation stay in the app. +- `SyncImportFilterService` still owns full-state op detection, latest import + selection from current batch/local store, IndexedDB access, local unsynced + import detection, and all `OpLog` messages. + ### What Moves - Conflict detection and LWW resolution algorithms from diff --git a/packages/sync-core/src/index.ts b/packages/sync-core/src/index.ts index 2a052bb2ca..d121961f6f 100644 --- a/packages/sync-core/src/index.ts +++ b/packages/sync-core/src/index.ts @@ -25,6 +25,16 @@ export { } from './vector-clock'; export type { VectorClockComparison } from './vector-clock'; +// Full-state import clean-slate vector-clock decisions. +export { classifyOpAgainstSyncImport } from './sync-import-filter'; +export type { + SyncImportFilterClockSource, + SyncImportFilterDecision, + SyncImportFilterDecisionReason, + SyncImportFilterInvalidateReason, + SyncImportFilterKeepReason, +} from './sync-import-filter'; + // LWW (Last-Writer-Wins) update action-type helpers — factory parameterized by // the host application's entity-type list, so the lib stays domain-agnostic. export { createLwwUpdateActionTypeHelpers } from './lww-update-action-types'; diff --git a/packages/sync-core/src/sync-import-filter.ts b/packages/sync-core/src/sync-import-filter.ts new file mode 100644 index 0000000000..35661ef6c3 --- /dev/null +++ b/packages/sync-core/src/sync-import-filter.ts @@ -0,0 +1,67 @@ +import { compareVectorClocks } from './vector-clock'; +import type { VectorClock, VectorClockComparison } from './vector-clock'; + +export type SyncImportFilterKeepReason = + | 'greater-than' + | 'equal' + | 'same-client-post-import' + | 'knows-import-counter'; + +export type SyncImportFilterInvalidateReason = 'concurrent' | 'less-than'; + +export type SyncImportFilterDecisionReason = + | SyncImportFilterKeepReason + | SyncImportFilterInvalidateReason; + +export interface SyncImportFilterClockSource { + clientId: string; + vectorClock: VectorClock; +} + +export interface SyncImportFilterDecision { + shouldKeep: boolean; + comparison: VectorClockComparison; + reason: SyncImportFilterDecisionReason; +} + +/** + * Classifies whether an operation survives a full-state import's clean-slate + * boundary using only vector-clock causality. + */ +export const classifyOpAgainstSyncImport = ( + op: SyncImportFilterClockSource, + latestImport: SyncImportFilterClockSource, +): SyncImportFilterDecision => { + const comparison = compareVectorClocks(op.vectorClock, latestImport.vectorClock); + + if (comparison === 'GREATER_THAN') { + return { shouldKeep: true, comparison, reason: 'greater-than' }; + } + + if (comparison === 'EQUAL') { + return { shouldKeep: true, comparison, reason: 'equal' }; + } + + if (comparison === 'CONCURRENT') { + const importClientCounter = latestImport.vectorClock[latestImport.clientId] ?? 0; + + if ( + op.clientId === latestImport.clientId && + (op.vectorClock[op.clientId] ?? 0) > importClientCounter + ) { + return { shouldKeep: true, comparison, reason: 'same-client-post-import' }; + } + + if ( + op.clientId !== latestImport.clientId && + (op.vectorClock[latestImport.clientId] ?? 0) >= importClientCounter && + importClientCounter > 0 + ) { + return { shouldKeep: true, comparison, reason: 'knows-import-counter' }; + } + + return { shouldKeep: false, comparison, reason: 'concurrent' }; + } + + return { shouldKeep: false, comparison, reason: 'less-than' }; +}; diff --git a/packages/sync-core/tests/sync-import-filter.spec.ts b/packages/sync-core/tests/sync-import-filter.spec.ts new file mode 100644 index 0000000000..056dfc71c4 --- /dev/null +++ b/packages/sync-core/tests/sync-import-filter.spec.ts @@ -0,0 +1,117 @@ +import { describe, expect, it } from 'vitest'; +import { classifyOpAgainstSyncImport } from '../src/sync-import-filter'; +import type { SyncImportFilterClockSource } from '../src/sync-import-filter'; + +const clockSource = ( + clientId: string, + vectorClock: Record, +): SyncImportFilterClockSource => ({ + clientId, + vectorClock, +}); + +describe('classifyOpAgainstSyncImport', () => { + it('keeps GREATER_THAN ops', () => { + expect( + classifyOpAgainstSyncImport( + clockSource('clientA', { clientA: 2, clientB: 1 }), + clockSource('clientB', { clientA: 1, clientB: 1 }), + ), + ).toEqual({ + shouldKeep: true, + comparison: 'GREATER_THAN', + reason: 'greater-than', + }); + }); + + it('keeps EQUAL ops', () => { + expect( + classifyOpAgainstSyncImport( + clockSource('clientA', { clientA: 1, clientB: 1 }), + clockSource('clientB', { clientA: 1, clientB: 1 }), + ), + ).toEqual({ + shouldKeep: true, + comparison: 'EQUAL', + reason: 'equal', + }); + }); + + it('keeps CONCURRENT same-client ops with a greater own counter', () => { + expect( + classifyOpAgainstSyncImport( + clockSource('clientB', { clientB: 6, clientC: 1 }), + clockSource('clientB', { clientA: 10, clientB: 5 }), + ), + ).toEqual({ + shouldKeep: true, + comparison: 'CONCURRENT', + reason: 'same-client-post-import', + }); + }); + + it('invalidates CONCURRENT same-client ops with an equal own counter', () => { + expect( + classifyOpAgainstSyncImport( + clockSource('clientB', { clientB: 5, clientC: 1 }), + clockSource('clientB', { clientA: 10, clientB: 5 }), + ), + ).toEqual({ + shouldKeep: false, + comparison: 'CONCURRENT', + reason: 'concurrent', + }); + }); + + it('keeps CONCURRENT different-client ops that know the import client counter', () => { + expect( + classifyOpAgainstSyncImport( + clockSource('clientA', { clientA: 1, clientB: 5 }), + clockSource('clientB', { clientB: 5, clientC: 10 }), + ), + ).toEqual({ + shouldKeep: true, + comparison: 'CONCURRENT', + reason: 'knows-import-counter', + }); + }); + + it('invalidates CONCURRENT different-client ops when the import client counter is 0', () => { + expect( + classifyOpAgainstSyncImport( + clockSource('clientA', { clientA: 1, clientB: 0 }), + clockSource('clientB', { clientB: 0, clientC: 10 }), + ), + ).toEqual({ + shouldKeep: false, + comparison: 'CONCURRENT', + reason: 'concurrent', + }); + }); + + it('invalidates other CONCURRENT ops', () => { + expect( + classifyOpAgainstSyncImport( + clockSource('clientA', { clientA: 1 }), + clockSource('clientB', { clientB: 5 }), + ), + ).toEqual({ + shouldKeep: false, + comparison: 'CONCURRENT', + reason: 'concurrent', + }); + }); + + it('invalidates LESS_THAN ops', () => { + expect( + classifyOpAgainstSyncImport( + clockSource('clientA', { clientA: 1 }), + clockSource('clientB', { clientA: 2, clientB: 1 }), + ), + ).toEqual({ + shouldKeep: false, + comparison: 'LESS_THAN', + reason: 'less-than', + }); + }); +}); diff --git a/src/app/op-log/sync/sync-import-filter.service.ts b/src/app/op-log/sync/sync-import-filter.service.ts index 84fac34a40..7fba805831 100644 --- a/src/app/op-log/sync/sync-import-filter.service.ts +++ b/src/app/op-log/sync/sync-import-filter.service.ts @@ -1,12 +1,9 @@ import { inject, Injectable } from '@angular/core'; import { OperationLogStoreService } from '../persistence/operation-log-store.service'; import { Operation, OpType } from '../core/operation.types'; -import { - compareVectorClocks, - VectorClockComparison, - vectorClockToString, -} from '../../core/util/vector-clock'; +import { vectorClockToString } from '../../core/util/vector-clock'; import { OpLog } from '../../core/log'; +import { classifyOpAgainstSyncImport } from '@sp/sync-core'; /** * Service responsible for filtering operations invalidated by SYNC_IMPORT, BACKUP_IMPORT, or REPAIR operations. @@ -173,7 +170,7 @@ export class SyncImportFilterService { // - CONCURRENT + proven post-import counter knowledge → KEEP // - CONCURRENT (all other cases) → FILTER // - LESS_THAN: Op is dominated by import → FILTER - const comparison = compareVectorClocks(op.vectorClock, importClockForComparison); + const decision = classifyOpAgainstSyncImport(op, latestImport); // DIAGNOSTIC LOGGING: Log vector clock comparison details // This helps debug issues where ops are incorrectly filtered as CONCURRENT @@ -181,20 +178,13 @@ export class SyncImportFilterService { `SyncImportFilterService: Comparing op ${op.id} (${op.actionType}) from client ${op.clientId}\n` + ` Op vectorClock: ${vectorClockToString(op.vectorClock)}\n` + ` Import vectorClock: ${vectorClockToString(importClockForComparison)}` + - `\n Comparison result: ${comparison}`, + `\n Comparison result: ${decision.comparison}`, ); - if ( - comparison === VectorClockComparison.GREATER_THAN || - comparison === VectorClockComparison.EQUAL - ) { + if (decision.reason === 'greater-than' || decision.reason === 'equal') { // Op was created by a client that had knowledge of the import validOps.push(op); - } else if ( - comparison === VectorClockComparison.CONCURRENT && - op.clientId === latestImport.clientId && - (op.vectorClock[op.clientId] ?? 0) > (importClockForComparison[op.clientId] ?? 0) - ) { + } else if (decision.reason === 'same-client-post-import') { // Op is from the SAME client that created the import, with a higher counter. // A client can't create ops concurrent with its own import — all post-import // ops from the import client necessarily have causal knowledge of the import. @@ -207,13 +197,7 @@ export class SyncImportFilterService { ` Client ${op.clientId} counter: op=${op.vectorClock[op.clientId]} > import=${importClockForComparison[op.clientId]} (post-import op).`, ); validOps.push(op); - } else if ( - comparison === VectorClockComparison.CONCURRENT && - op.clientId !== latestImport.clientId && - (op.vectorClock[latestImport.clientId] ?? 0) >= - (importClockForComparison[latestImport.clientId] ?? 0) && - (importClockForComparison[latestImport.clientId] ?? 0) > 0 - ) { + } else if (decision.reason === 'knows-import-counter') { // Op was created by a DIFFERENT client with knowledge of the import. // The SYNC_IMPORT incremented the importing client's counter, so any op whose // clock includes that counter value (or higher) must have received the import @@ -240,7 +224,7 @@ export class SyncImportFilterService { // CONCURRENT or LESS_THAN: Op was created without knowledge of import // Filter it to ensure clean slate semantics OpLog.warn( - `SyncImportFilterService: FILTERING op ${op.id} (${op.actionType}) as ${comparison}\n` + + `SyncImportFilterService: FILTERING op ${op.id} (${op.actionType}) as ${decision.comparison}\n` + ` Op vectorClock: ${vectorClockToString(op.vectorClock)}\n` + ` Import vectorClock: ${vectorClockToString(importClockForComparison)}` + `\n Import client: ${latestImport.clientId}\n` +