diff --git a/CLAUDE.md b/CLAUDE.md index 84998a1509..6ae1200544 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -106,6 +106,7 @@ The app uses NgRx (Redux pattern) for state management. Key state slices: 9. **Atomic Multi-Entity Changes**: When one action affects multiple entities (e.g., deleting a tag removes it from tasks), use **meta-reducers** instead of effects to ensure all changes happen in a single reducer pass. This creates one operation in the sync log, preventing partial sync and state inconsistency. See `src/app/root-store/meta/task-shared-meta-reducers/` and Part F in the architecture docs. 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. ## 🚫 Known Anti-Patterns to Avoid diff --git a/src/app/core/persistence/operation-log/sync/operation-log-sync.service.spec.ts b/src/app/core/persistence/operation-log/sync/operation-log-sync.service.spec.ts index 86a9cb31e6..c0df3ce43f 100644 --- a/src/app/core/persistence/operation-log/sync/operation-log-sync.service.spec.ts +++ b/src/app/core/persistence/operation-log/sync/operation-log-sync.service.spec.ts @@ -1213,1003 +1213,10 @@ describe('OperationLogSyncService', () => { }); }); - describe('_replayLocalSyncedOpsAfterImport (late joiner scenario)', () => { - // Helper to create operations - const createOp = (partial: Partial): Operation => ({ - id: '019afd68-0000-7000-0000-000000000000', - actionType: '[Test] Action', - opType: OpType.Update, - entityType: 'TASK', - entityId: 'entity-1', - payload: {}, - clientId: 'client-A', - vectorClock: { clientA: 1 }, - timestamp: Date.now(), - schemaVersion: 1, - ...partial, - }); - - // Helper to create op log entries - const createEntry = ( - op: Operation, - options: { syncedAt?: number; source?: 'local' | 'remote' } = {}, - ): any => ({ - seq: 1, - op, - appliedAt: Date.now(), - source: options.source ?? 'local', - syncedAt: options.syncedAt, - }); - - it('should replay local synced ops created AFTER SYNC_IMPORT', async () => { - // Scenario: "Late joiner" - Client B has local synced ops that were created - // AFTER the SYNC_IMPORT and need to be replayed - - const testClientId = 'test-client-id'; - const syncImportOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', - opType: OpType.SyncImport, - clientId: 'client-A', // From another client - entityType: 'ALL', - payload: { task: {}, project: {} }, - }); - - // Local synced ops created by THIS client AFTER the SYNC_IMPORT (higher UUIDs) - // UUIDv7 is time-ordered, so higher UUID = created after - const localSyncedOp1 = createOp({ - id: '019afd68-0051-7000-0000-000000000000', // AFTER 0050 - opType: OpType.Create, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-B1', - actionType: '[Task] Add Task', - }); - const localSyncedOp2 = createOp({ - id: '019afd68-0052-7000-0000-000000000000', // AFTER 0050 - opType: OpType.Create, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-B2', - actionType: '[Task] Add Task', - }); - - // Mock getOpsAfterSeq to return local synced entries - (opLogStoreSpy as any).getOpsAfterSeq = jasmine - .createSpy('getOpsAfterSeq') - .and.returnValue( - Promise.resolve([ - createEntry(localSyncedOp1, { syncedAt: Date.now() - 1000 }), - createEntry(localSyncedOp2, { syncedAt: Date.now() - 500 }), - ]), - ); - - // Set up other mocks - opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); - opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ appliedOps: [] }), - ); - - // Process the SYNC_IMPORT - await (service as any)._processRemoteOps([syncImportOp]); - - // Verify that applyOperations was called twice: - // 1. First call: Apply the SYNC_IMPORT - // 2. Second call: Replay local synced ops - expect(operationApplierServiceSpy.applyOperations).toHaveBeenCalledTimes(2); - - // Second call should have the local synced ops (created AFTER the SYNC_IMPORT) - const secondCallArgs = operationApplierServiceSpy.applyOperations.calls.argsFor(1); - expect(secondCallArgs[0].length).toBe(2); - expect(secondCallArgs[0].map((op: Operation) => op.id)).toContain( - '019afd68-0051-7000-0000-000000000000', - ); - expect(secondCallArgs[0].map((op: Operation) => op.id)).toContain( - '019afd68-0052-7000-0000-000000000000', - ); - }); - - it('should NOT replay ops from other clients', async () => { - const testClientId = 'test-client-id'; - const syncImportOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', - opType: OpType.SyncImport, - clientId: 'client-A', - entityType: 'ALL', - }); - - // Op from THIS client created AFTER SYNC_IMPORT (should be replayed) - const localOp = createOp({ - id: '019afd68-0051-7000-0000-000000000000', // AFTER 0050 - clientId: testClientId, - }); - - // Op from OTHER client created AFTER SYNC_IMPORT (should NOT be replayed - wrong client) - const otherClientOp = createOp({ - id: '019afd68-0052-7000-0000-000000000000', // AFTER 0050 - clientId: 'other-client', - }); - - (opLogStoreSpy as any).getOpsAfterSeq = jasmine - .createSpy('getOpsAfterSeq') - .and.returnValue( - Promise.resolve([ - createEntry(localOp, { syncedAt: Date.now() }), - createEntry(otherClientOp, { syncedAt: Date.now() }), - ]), - ); - - opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); - opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ appliedOps: [] }), - ); - - await (service as any)._processRemoteOps([syncImportOp]); - - // Second call (replay) should only have local client's op - const secondCallArgs = operationApplierServiceSpy.applyOperations.calls.argsFor(1); - expect(secondCallArgs[0].length).toBe(1); - expect(secondCallArgs[0][0].clientId).toBe(testClientId); - }); - - it('should NOT replay unsynced ops (pending upload)', async () => { - const testClientId = 'test-client-id'; - const syncImportOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', - opType: OpType.SyncImport, - clientId: 'client-A', - entityType: 'ALL', - }); - - // Synced op created AFTER SYNC_IMPORT (should be replayed) - const syncedOp = createOp({ - id: '019afd68-0051-7000-0000-000000000000', // AFTER 0050 - clientId: testClientId, - }); - - // Unsynced op created AFTER SYNC_IMPORT (should NOT be replayed - will be uploaded later) - const unsyncedOp = createOp({ - id: '019afd68-0052-7000-0000-000000000000', // AFTER 0050 - clientId: testClientId, - }); - - (opLogStoreSpy as any).getOpsAfterSeq = jasmine - .createSpy('getOpsAfterSeq') - .and.returnValue( - Promise.resolve([ - createEntry(syncedOp, { syncedAt: Date.now() }), - createEntry(unsyncedOp, { syncedAt: undefined }), // Not synced - ]), - ); - - opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); - opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ appliedOps: [] }), - ); - - await (service as any)._processRemoteOps([syncImportOp]); - - // Second call (replay) should only have the synced op (created AFTER SYNC_IMPORT) - const secondCallArgs = operationApplierServiceSpy.applyOperations.calls.argsFor(1); - expect(secondCallArgs[0].length).toBe(1); - expect(secondCallArgs[0][0].id).toBe('019afd68-0051-7000-0000-000000000000'); - }); - - it('should NOT replay SYNC_IMPORT or BACKUP_IMPORT ops', async () => { - const testClientId = 'test-client-id'; - const syncImportOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', - opType: OpType.SyncImport, - clientId: 'client-A', - entityType: 'ALL', - }); - - // Regular op created AFTER SYNC_IMPORT (should be replayed) - const regularOp = createOp({ - id: '019afd68-0051-7000-0000-000000000000', // AFTER 0050 - opType: OpType.Create, - clientId: testClientId, - }); - - // Old SYNC_IMPORT from this client created AFTER (should NOT be replayed - import ops excluded) - const oldImportOp = createOp({ - id: '019afd68-0052-7000-0000-000000000000', // AFTER 0050 - opType: OpType.SyncImport, - clientId: testClientId, - entityType: 'ALL', - }); - - (opLogStoreSpy as any).getOpsAfterSeq = jasmine - .createSpy('getOpsAfterSeq') - .and.returnValue( - Promise.resolve([ - createEntry(regularOp, { syncedAt: Date.now() }), - createEntry(oldImportOp, { syncedAt: Date.now() }), - ]), - ); - - opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); - opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ appliedOps: [] }), - ); - - await (service as any)._processRemoteOps([syncImportOp]); - - // Second call (replay) should only have the regular op, not the old import - const secondCallArgs = operationApplierServiceSpy.applyOperations.calls.argsFor(1); - expect(secondCallArgs[0].length).toBe(1); - expect(secondCallArgs[0][0].opType).toBe(OpType.Create); - }); - - it('should not call applyOperations for replay if no local synced ops exist', async () => { - const syncImportOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', - opType: OpType.SyncImport, - clientId: 'client-A', - entityType: 'ALL', - }); - - // No local ops at all - (opLogStoreSpy as any).getOpsAfterSeq = jasmine - .createSpy('getOpsAfterSeq') - .and.returnValue(Promise.resolve([])); - - opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); - opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ appliedOps: [] }), - ); - - await (service as any)._processRemoteOps([syncImportOp]); - - // applyOperations should only be called once (for the SYNC_IMPORT itself) - expect(operationApplierServiceSpy.applyOperations).toHaveBeenCalledTimes(1); - }); - - // ───────────────────────────────────────────────────────────────────────── - // UUIDv7 Timestamp Filtering Tests - // These tests verify that only ops created AFTER the SYNC_IMPORT - // (higher UUIDv7) are replayed. Ops created BEFORE (lower UUIDv7) are - // filtered out since they reference the old state. - // ───────────────────────────────────────────────────────────────────────── - - it('should NOT replay ops created BEFORE SYNC_IMPORT (lower UUIDv7)', async () => { - const testClientId = 'test-client-id'; - - // SYNC_IMPORT - const syncImportOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', - opType: OpType.SyncImport, - clientId: 'client-A', - entityType: 'ALL', - }); - - // Op created BEFORE the SYNC_IMPORT (lower UUIDv7) - should NOT be replayed - const preImportOp = createOp({ - id: '019afd68-0040-7000-0000-000000000000', // BEFORE 0050 - opType: OpType.Create, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-old', - actionType: '[Task] Add Task', - }); - - // Op created AFTER the SYNC_IMPORT (higher UUIDv7) - should be replayed - const postImportOp = createOp({ - id: '019afd68-0051-7000-0000-000000000000', // AFTER 0050 - opType: OpType.Create, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-new', - actionType: '[Task] Add Task', - }); - - (opLogStoreSpy as any).getOpsAfterSeq = jasmine - .createSpy('getOpsAfterSeq') - .and.returnValue( - Promise.resolve([ - createEntry(preImportOp, { syncedAt: Date.now() - 1000 }), - createEntry(postImportOp, { syncedAt: Date.now() - 500 }), - ]), - ); - - opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); - opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ appliedOps: [] }), - ); - - await (service as any)._processRemoteOps([syncImportOp]); - - // Second call (replay) should only have the post-import op - expect(operationApplierServiceSpy.applyOperations).toHaveBeenCalledTimes(2); - const secondCallArgs = operationApplierServiceSpy.applyOperations.calls.argsFor(1); - expect(secondCallArgs[0].length).toBe(1); - expect(secondCallArgs[0][0].entityId).toBe('task-new'); - }); - - it('should replay ops created AFTER SYNC_IMPORT (UUIDv7 comparison)', async () => { - const testClientId = 'test-client-id'; - - // SYNC_IMPORT - const syncImportOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', - opType: OpType.SyncImport, - clientId: 'client-A', - entityType: 'ALL', - }); - - // Op created AFTER SYNC_IMPORT (higher UUIDv7 = later timestamp) - // UUIDv7 is time-ordered: higher UUID means created later - const afterOp = createOp({ - id: '019afd68-0051-7000-0000-000000000000', // AFTER 0050 - opType: OpType.Create, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-after', - actionType: '[Task] Add Task', - }); - - (opLogStoreSpy as any).getOpsAfterSeq = jasmine - .createSpy('getOpsAfterSeq') - .and.returnValue( - Promise.resolve([createEntry(afterOp, { syncedAt: Date.now() })]), - ); - - opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); - opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ appliedOps: [] }), - ); - - await (service as any)._processRemoteOps([syncImportOp]); - - // Second call should have the op created after SYNC_IMPORT - expect(operationApplierServiceSpy.applyOperations).toHaveBeenCalledTimes(2); - const secondCallArgs = operationApplierServiceSpy.applyOperations.calls.argsFor(1); - expect(secondCallArgs[0].length).toBe(1); - expect(secondCallArgs[0][0].entityId).toBe('task-after'); - }); - - it('should NOT replay ops created BEFORE SYNC_IMPORT (UUIDv7 comparison)', async () => { - const testClientId = 'test-client-id'; - - // SYNC_IMPORT - const syncImportOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', - opType: OpType.SyncImport, - clientId: 'client-A', - entityType: 'ALL', - }); - - // Op created BEFORE SYNC_IMPORT (lower UUIDv7 = earlier timestamp) - // These ops reference the old state and should be discarded - const beforeOp = createOp({ - id: '019afd68-0040-7000-0000-000000000000', // BEFORE 0050 - opType: OpType.Create, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-before', - actionType: '[Task] Add Task', - }); - - (opLogStoreSpy as any).getOpsAfterSeq = jasmine - .createSpy('getOpsAfterSeq') - .and.returnValue( - Promise.resolve([createEntry(beforeOp, { syncedAt: Date.now() })]), - ); - - opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); - opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ appliedOps: [] }), - ); - - await (service as any)._processRemoteOps([syncImportOp]); - - // applyOperations should only be called once (SYNC_IMPORT), no replay - expect(operationApplierServiceSpy.applyOperations).toHaveBeenCalledTimes(1); - }); - - it('should replay ops with EQUAL UUIDv7 to SYNC_IMPORT (edge case)', async () => { - const testClientId = 'test-client-id'; - - // SYNC_IMPORT - const syncImportOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', - opType: OpType.SyncImport, - clientId: 'client-A', - entityType: 'ALL', - }); - - // Op with EQUAL UUID - created at same instant as SYNC_IMPORT - // Since op.id is NOT < syncImportOp.id, this op IS replayed - // (In practice, UUIDv7 collisions are extremely rare due to random bits) - const equalOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', // EQUAL to SYNC_IMPORT - opType: OpType.Create, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-equal', - actionType: '[Task] Add Task', - }); - - (opLogStoreSpy as any).getOpsAfterSeq = jasmine - .createSpy('getOpsAfterSeq') - .and.returnValue( - Promise.resolve([createEntry(equalOp, { syncedAt: Date.now() })]), - ); - - opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); - opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ appliedOps: [] }), - ); - - await (service as any)._processRemoteOps([syncImportOp]); - - // Equal ID ops ARE replayed (not strictly less than import) - // applyOperations called twice: once for SYNC_IMPORT, once for replay - expect(operationApplierServiceSpy.applyOperations).toHaveBeenCalledTimes(2); - const secondCallArgs = operationApplierServiceSpy.applyOperations.calls.argsFor(1); - expect(secondCallArgs[0].length).toBe(1); - expect(secondCallArgs[0][0].entityId).toBe('task-equal'); - }); - - it('should filter ops by UUIDv7 correctly in mixed scenario', async () => { - const testClientId = 'test-client-id'; - - // SYNC_IMPORT - const syncImportOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', - opType: OpType.SyncImport, - clientId: 'client-A', - entityType: 'ALL', - }); - - // Mix of ops: some BEFORE SYNC_IMPORT (should be filtered), some AFTER (should be replayed) - const beforeOp1 = createOp({ - id: '019afd68-0001-7000-0000-000000000000', // BEFORE 0050 - clientId: testClientId, - entityId: 'task-before-1', - }); - const beforeOp2 = createOp({ - id: '019afd68-0002-7000-0000-000000000000', // BEFORE 0050 - clientId: testClientId, - entityId: 'task-before-2', - }); - const afterOp1 = createOp({ - id: '019afd68-0051-7000-0000-000000000000', // AFTER 0050 - clientId: testClientId, - entityId: 'task-after-1', - }); - const afterOp2 = createOp({ - id: '019afd68-0052-7000-0000-000000000000', // AFTER 0050 - clientId: testClientId, - entityId: 'task-after-2', - }); - - (opLogStoreSpy as any).getOpsAfterSeq = jasmine - .createSpy('getOpsAfterSeq') - .and.returnValue( - Promise.resolve([ - createEntry(beforeOp1, { syncedAt: Date.now() - 400 }), - createEntry(beforeOp2, { syncedAt: Date.now() - 300 }), - createEntry(afterOp1, { syncedAt: Date.now() - 200 }), - createEntry(afterOp2, { syncedAt: Date.now() - 100 }), - ]), - ); - - opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); - opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ appliedOps: [] }), - ); - - await (service as any)._processRemoteOps([syncImportOp]); - - // Second call should have only the ops created AFTER SYNC_IMPORT - expect(operationApplierServiceSpy.applyOperations).toHaveBeenCalledTimes(2); - const secondCallArgs = operationApplierServiceSpy.applyOperations.calls.argsFor(1); - expect(secondCallArgs[0].length).toBe(2); - const replayedIds = secondCallArgs[0].map((op: Operation) => op.entityId); - expect(replayedIds).toContain('task-after-1'); - expect(replayedIds).toContain('task-after-2'); - expect(replayedIds).not.toContain('task-before-1'); - expect(replayedIds).not.toContain('task-before-2'); - }); - - // ───────────────────────────────────────────────────────────────────────── - // Bug Fix Verification Test - // This test verifies the specific bug fix where vector clock comparison - // failed after restore because fresh vector clocks have no overlapping - // client IDs with existing ops. - // ───────────────────────────────────────────────────────────────────────── - - it('should NOT replay old ops even when SYNC_IMPORT has fresh vector clock with no overlapping client IDs', async () => { - // This is the exact bug scenario: - // 1. Client A restores a backup with isForceConflict=true - // 2. This creates a SYNC_IMPORT with a FRESH vector clock (new clientId: freshClient) - // 3. Client B has old ops with vector clock { clientB: 10 } - // 4. When comparing { clientB: 10 } with { freshClient: 1 }: - // - With vector clock comparison: CONCURRENT (bug - both clocks have components > 0) - // - With UUIDv7 comparison: old ops correctly filtered (fix) - - const testClientId = 'test-client-id'; - - // SYNC_IMPORT with a FRESH vector clock (new client ID after restore) - // This simulates what happens when isForceConflict=true creates a new vector clock - const freshClientId = 'freshRestoreClient'; - const syncImportOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', - opType: OpType.SyncImport, - clientId: freshClientId, // NEW client ID after restore - entityType: 'ALL', - vectorClock: { [freshClientId]: 1 }, // FRESH clock with no history - }); - - // Old op from this client with a vector clock that has NO overlap with SYNC_IMPORT's clock - // In vector clock comparison, { testClientId: 10 } vs { freshClient: 1 } = CONCURRENT - // because each has components the other doesn't have - // But with UUIDv7 comparison, this op is BEFORE the import and should be filtered - const oldOpWithNoOverlap = createOp({ - id: '019afd68-0040-7000-0000-000000000000', // BEFORE 0050 - opType: OpType.Update, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-old-no-overlap', - vectorClock: { [testClientId]: 10 }, // No overlap with { freshClient: 1 } - actionType: '[Task] Update', - }); - - (opLogStoreSpy as any).getOpsAfterSeq = jasmine - .createSpy('getOpsAfterSeq') - .and.returnValue( - Promise.resolve([createEntry(oldOpWithNoOverlap, { syncedAt: Date.now() })]), - ); - - opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); - opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ appliedOps: [] }), - ); - - await (service as any)._processRemoteOps([syncImportOp]); - - // The old op should NOT be replayed because: - // 1. Its UUIDv7 (0040) < SYNC_IMPORT's UUIDv7 (0050) - // 2. Therefore it was created BEFORE the restore and references old state - // applyOperations should only be called once (for the SYNC_IMPORT itself) - expect(operationApplierServiceSpy.applyOperations).toHaveBeenCalledTimes(1); - }); - - it('should catch errors in replay, notify user, and run validation instead of crashing', async () => { - const errorSpy = spyOn(console, 'error'); - const testClientId = 'test-client-id'; - - const syncImportOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', - opType: OpType.SyncImport, - clientId: 'client-A', - entityType: 'ALL', - }); - - const localOp = createOp({ - id: '019afd68-0051-7000-0000-000000000000', // AFTER SYNC_IMPORT - opType: OpType.Create, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-1', - actionType: '[Task] Add Task', - }); - - (opLogStoreSpy as any).getOpsAfterSeq = jasmine - .createSpy('getOpsAfterSeq') - .and.returnValue( - Promise.resolve([createEntry(localOp, { syncedAt: Date.now() })]), - ); - - opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); - opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); - - // First call (SYNC_IMPORT) succeeds - let applyCallCount = 0; - operationApplierServiceSpy.applyOperations.and.callFake(() => { - applyCallCount++; - if (applyCallCount === 1) { - // First call succeeds (SYNC_IMPORT) - return Promise.resolve({ appliedOps: [] }); - } else { - // Second call (replay) throws an error - return Promise.reject(new Error('Replay failed: entity not found')); - } - }); - - // Should NOT throw - error should be caught - await expectAsync( - (service as any)._processRemoteOps([syncImportOp]), - ).toBeResolved(); - - // Should have logged the error - expect(errorSpy).toHaveBeenCalled(); - const errorCalls = errorSpy.calls.allArgs(); - // OpLog.err calls console.error with prefix '[ol]' as first arg, message as second - const hasReplayError = errorCalls.some((args) => - args.some( - (arg) => - typeof arg === 'string' && - arg.includes('Failed to replay') && - arg.includes('local ops'), - ), - ); - expect(hasReplayError).toBe(true); - - // Should have triggered state validation - expect(validateStateServiceSpy.validateAndRepairCurrentState).toHaveBeenCalledWith( - 'replay-local-ops-failed', - ); - - // Should have shown error snack to user - expect(snackServiceSpy.open).toHaveBeenCalledWith( - jasmine.objectContaining({ - type: 'ERROR', - }), - ); - }); - - describe('entity existence filtering (ADD-then-DELETE sequence)', () => { - // These tests verify the fix for the bug where DELETE ops were incorrectly - // skipped when their target entity was created by an earlier ADD op in the - // same replay sequence. The filtering logic now tracks entities that will be - // created by CREATE ops and doesn't skip UPDATE/DELETE ops targeting them. - // - // Uses createOp and createEntry helpers from parent describe block. - - const testClientId = 'test-client-id'; - - it('should NOT skip DELETE ops when ADD op for same entity is in the sequence', async () => { - // Bug scenario: ADD creates entity, DELETE for same entity was incorrectly skipped - // because entity didn't exist in SYNC_IMPORT state yet - - const syncImportOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', - opType: OpType.SyncImport, - clientId: 'client-A', - entityType: 'ALL', - }); - - // Sequence: ADD task -> UPDATE task -> DELETE task - const addTaskOp = createOp({ - id: '019afd68-0051-7000-0000-000000000000', // AFTER SYNC_IMPORT - opType: OpType.Create, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-to-delete', - actionType: '[Task Shared] addTask', - }); - - const updateTaskOp = createOp({ - id: '019afd68-0052-7000-0000-000000000000', - opType: OpType.Update, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-to-delete', - actionType: '[Task Shared] updateTask', - }); - - const deleteTaskOp = createOp({ - id: '019afd68-0053-7000-0000-000000000000', - opType: OpType.Delete, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-to-delete', - actionType: '[Task Shared] deleteTask', - }); - - // Mock: return ADD, UPDATE, DELETE sequence as local synced ops - (opLogStoreSpy as any).getOpsAfterSeq = jasmine - .createSpy('getOpsAfterSeq') - .and.returnValue( - Promise.resolve([ - createEntry(addTaskOp, { syncedAt: Date.now() - 1000 }), - createEntry(updateTaskOp, { syncedAt: Date.now() - 900 }), - createEntry(deleteTaskOp, { syncedAt: Date.now() - 800 }), - ]), - ); - - // Mock: entity doesn't exist in store (SYNC_IMPORT doesn't have it) - // But since ADD is in sequence, DELETE should still be included - spyOn(service as any, '_checkOperationEntitiesExist').and.callFake( - async (op: Operation) => { - // Simulate: entity doesn't exist in current state after SYNC_IMPORT - // This would cause DELETE to be skipped without the fix - if (op.entityId === 'task-to-delete' && op.opType !== OpType.Create) { - return ['task-to-delete']; // Report as missing - } - return []; - }, - ); - - opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); - opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ appliedOps: [] }), - ); - - await (service as any)._processRemoteOps([syncImportOp]); - - // Verify applyOperations was called twice (SYNC_IMPORT + replay) - expect(operationApplierServiceSpy.applyOperations).toHaveBeenCalledTimes(2); - - // Second call (replay) should include ALL THREE ops: ADD, UPDATE, DELETE - const replayCallArgs = - operationApplierServiceSpy.applyOperations.calls.argsFor(1); - const replayedOps = replayCallArgs[0] as Operation[]; - - expect(replayedOps.length).toBe(3); - expect(replayedOps.map((op) => op.opType)).toEqual([ - OpType.Create, - OpType.Update, - OpType.Delete, - ]); - expect(replayedOps.every((op) => op.entityId === 'task-to-delete')).toBe(true); - }); - - it('should still skip ops for entities NOT created in the sequence', async () => { - // Verify that ops targeting truly missing entities are still skipped - - const syncImportOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', - opType: OpType.SyncImport, - clientId: 'client-A', - entityType: 'ALL', - }); - - // Op targeting entity that was never created in this sequence - const updateMissingTaskOp = createOp({ - id: '019afd68-0051-7000-0000-000000000000', - opType: OpType.Update, - clientId: testClientId, - entityType: 'TASK', - entityId: 'truly-missing-task', - actionType: '[Task Shared] updateTask', - }); - - (opLogStoreSpy as any).getOpsAfterSeq = jasmine - .createSpy('getOpsAfterSeq') - .and.returnValue( - Promise.resolve([ - createEntry(updateMissingTaskOp, { syncedAt: Date.now() - 1000 }), - ]), - ); - - // Mock: entity doesn't exist and NO create op for it - spyOn(service as any, '_checkOperationEntitiesExist').and.returnValue( - Promise.resolve(['truly-missing-task']), - ); - - opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); - opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ appliedOps: [] }), - ); - - await (service as any)._processRemoteOps([syncImportOp]); - - // applyOperations should only be called once (for SYNC_IMPORT) - // No replay because the only op was skipped - expect(operationApplierServiceSpy.applyOperations).toHaveBeenCalledTimes(1); - }); - - it('should handle mixed scenario: some entities created, some truly missing', async () => { - const syncImportOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', - opType: OpType.SyncImport, - clientId: 'client-A', - entityType: 'ALL', - }); - - // Entity 1: Created then deleted (should be included) - const addTask1 = createOp({ - id: '019afd68-0051-7000-0000-000000000000', - opType: OpType.Create, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-created', - actionType: '[Task Shared] addTask', - }); - const deleteTask1 = createOp({ - id: '019afd68-0052-7000-0000-000000000000', - opType: OpType.Delete, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-created', - actionType: '[Task Shared] deleteTask', - }); - - // Entity 2: Truly missing (should be skipped) - const updateMissing = createOp({ - id: '019afd68-0053-7000-0000-000000000000', - opType: OpType.Update, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-truly-missing', - actionType: '[Task Shared] updateTask', - }); - - // Entity 3: Already exists in store (should be included) - const updateExisting = createOp({ - id: '019afd68-0054-7000-0000-000000000000', - opType: OpType.Update, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-existing', - actionType: '[Task Shared] updateTask', - }); - - (opLogStoreSpy as any).getOpsAfterSeq = jasmine - .createSpy('getOpsAfterSeq') - .and.returnValue( - Promise.resolve([ - createEntry(addTask1, { syncedAt: Date.now() - 1000 }), - createEntry(deleteTask1, { syncedAt: Date.now() - 900 }), - createEntry(updateMissing, { syncedAt: Date.now() - 800 }), - createEntry(updateExisting, { syncedAt: Date.now() - 700 }), - ]), - ); - - spyOn(service as any, '_checkOperationEntitiesExist').and.callFake( - async (op: Operation) => { - // task-created: missing in store (but has CREATE op) - if (op.entityId === 'task-created' && op.opType !== OpType.Create) { - return ['task-created']; - } - // task-truly-missing: missing and no CREATE op - if (op.entityId === 'task-truly-missing') { - return ['task-truly-missing']; - } - // task-existing: exists in store - return []; - }, - ); - - opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); - opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ appliedOps: [] }), - ); - - await (service as any)._processRemoteOps([syncImportOp]); - - expect(operationApplierServiceSpy.applyOperations).toHaveBeenCalledTimes(2); - - const replayCallArgs = - operationApplierServiceSpy.applyOperations.calls.argsFor(1); - const replayedOps = replayCallArgs[0] as Operation[]; - - // Should include: addTask1, deleteTask1, updateExisting - // Should NOT include: updateMissing - expect(replayedOps.length).toBe(3); - expect(replayedOps.map((op) => op.entityId)).toEqual([ - 'task-created', - 'task-created', - 'task-existing', - ]); - expect(replayedOps.map((op) => op.opType)).toEqual([ - OpType.Create, - OpType.Delete, - OpType.Update, - ]); - }); - - it('should handle multiple entities created by same ADD sequence', async () => { - // Scenario: Multiple tasks created and some deleted - - const syncImportOp = createOp({ - id: '019afd68-0050-7000-0000-000000000000', - opType: OpType.SyncImport, - clientId: 'client-A', - entityType: 'ALL', - }); - - const addTask1 = createOp({ - id: '019afd68-0051-7000-0000-000000000000', - opType: OpType.Create, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-1', - actionType: '[Task Shared] addTask', - }); - const addTask2 = createOp({ - id: '019afd68-0052-7000-0000-000000000000', - opType: OpType.Create, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-2', - actionType: '[Task Shared] addTask', - }); - const deleteTask1 = createOp({ - id: '019afd68-0053-7000-0000-000000000000', - opType: OpType.Delete, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-1', - actionType: '[Task Shared] deleteTask', - }); - const updateTask2 = createOp({ - id: '019afd68-0054-7000-0000-000000000000', - opType: OpType.Update, - clientId: testClientId, - entityType: 'TASK', - entityId: 'task-2', - actionType: '[Task Shared] updateTask', - }); - - (opLogStoreSpy as any).getOpsAfterSeq = jasmine - .createSpy('getOpsAfterSeq') - .and.returnValue( - Promise.resolve([ - createEntry(addTask1, { syncedAt: Date.now() - 1000 }), - createEntry(addTask2, { syncedAt: Date.now() - 900 }), - createEntry(deleteTask1, { syncedAt: Date.now() - 800 }), - createEntry(updateTask2, { syncedAt: Date.now() - 700 }), - ]), - ); - - // Both entities missing from store but have CREATE ops - spyOn(service as any, '_checkOperationEntitiesExist').and.callFake( - async (op: Operation) => { - if (op.opType !== OpType.Create) { - return [op.entityId!]; // Report as missing - } - return []; - }, - ); - - opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); - opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ appliedOps: [] }), - ); - - await (service as any)._processRemoteOps([syncImportOp]); - - expect(operationApplierServiceSpy.applyOperations).toHaveBeenCalledTimes(2); - - const replayCallArgs = - operationApplierServiceSpy.applyOperations.calls.argsFor(1); - const replayedOps = replayCallArgs[0] as Operation[]; - - // All 4 ops should be included - expect(replayedOps.length).toBe(4); - expect(replayedOps.map((op) => op.id)).toEqual([ - '019afd68-0051-7000-0000-000000000000', - '019afd68-0052-7000-0000-000000000000', - '019afd68-0053-7000-0000-000000000000', - '019afd68-0054-7000-0000-000000000000', - ]); - }); - }); - }); + // NOTE: _replayLocalSyncedOpsAfterImport tests removed. + // Clean Slate Semantics: SYNC_IMPORT/BACKUP_IMPORT replaces entire state. + // Local synced ops are NOT replayed - the import is an explicit user action + // to restore all clients to a specific point in time. describe('localWinOpsCreated propagation', () => { let uploadServiceSpy: jasmine.SpyObj; diff --git a/src/app/core/persistence/operation-log/sync/operation-log-sync.service.ts b/src/app/core/persistence/operation-log/sync/operation-log-sync.service.ts index cee3a7468d..d4ff13dee5 100644 --- a/src/app/core/persistence/operation-log/sync/operation-log-sync.service.ts +++ b/src/app/core/persistence/operation-log/sync/operation-log-sync.service.ts @@ -33,9 +33,6 @@ import { } from '../store/schema-migration.service'; import { SnackService } from '../../../snack/snack.service'; import { T } from '../../../../t.const'; -import { getEntityConfig, isAdapterEntity } from '../entity-registry'; -import { firstValueFrom } from 'rxjs'; -import { Dictionary } from '@ngrx/entity'; import { PfapiService } from '../../../../pfapi/pfapi.service'; import { PfapiStoreDelegateService } from '../../../../pfapi/pfapi-store-delegate.service'; import { uuidv7 } from '../../../../util/uuid-v7'; @@ -748,232 +745,6 @@ export class OperationLogSyncService { return window.confirm(`${title}\n\n${message}`); } - /** - * Checks if the primary target entities of an operation exist in the current store. - * - * For UPDATE/DELETE operations, the target entities (entityId/entityIds) must exist - * in the store for the operation to be applied successfully. This method checks - * existence using the DependencyResolverService. - * - * ## Why This Is Needed - * - * Operations don't carry their own state - they're like Redux actions that describe - * "what happened" but rely on the current store state to apply correctly. - * - * After a SYNC_IMPORT replaces the entire state, some local operations may reference - * entities that no longer exist. For example: - * - Local client has task T1 (created long ago, CREATE op compacted) - * - Local client does `planTasksForToday({ taskIds: [T1] })` → operation O2 - * - SYNC_IMPORT from remote client arrives (doesn't include T1) - * - SYNC_IMPORT replaces state → T1 is gone - * - Replay tries to apply O2 → fails because T1 doesn't exist - * - * @param op - The operation to check - * @returns Array of missing entity IDs (empty if all exist or check not applicable) - */ - private async _checkOperationEntitiesExist(op: Operation): Promise { - // Get entity IDs directly from the operation metadata. - // Operations have: entityId (single entity) and entityIds (bulk operations) - const entityIds: string[] = op.entityIds?.length - ? op.entityIds - : op.entityId && op.entityId !== '*' - ? [op.entityId] - : []; - - if (entityIds.length === 0) { - return []; // No specific entities to check (e.g., global config) - } - - // Skip check for CREATE operations - entities won't exist yet by definition - if (op.opType === OpType.Create) { - return []; - } - - // Check if entities exist in the store using entity registry - const config = getEntityConfig(op.entityType); - if (!config || !isAdapterEntity(config) || !config.selectEntities) { - return []; // Non-adapter entities (singletons, etc.) - assume they exist - } - - try { - const entities = await firstValueFrom( - this.store.select( - config.selectEntities as (state: object) => Dictionary, - ), - ); - - // If entities is undefined/null, assume all entities exist (graceful degradation) - if (!entities) { - return []; - } - - return entityIds.filter((id) => !entities[id]); - } catch { - // If selector throws (e.g., missing state slice), assume entities exist - return []; - } - } - - /** - * Re-applies local synced operations after a SYNC_IMPORT is applied. - * - * This handles the "late joiner" scenario where: - * 1. Client B creates local tasks (B1, B2, B3) - * 2. Client B uploads them to server (server accepts) - * 3. Client B receives piggybacked SYNC_IMPORT from Client A - * 4. SYNC_IMPORT replaces entire state (B's tasks disappear!) - * 5. This method re-applies B's synced ops to restore them - * - * The key insight: piggybacked ops exclude the client's own ops, - * so the SYNC_IMPORT doesn't include Client B's changes. - * But those ops ARE on the server and SHOULD be in the final state. - * - * ## Entity Existence Check - * - * Before replaying, we verify that the operation's target entities still exist. - * SYNC_IMPORT may have deleted entities that local operations reference. - * Operations referencing deleted entities are skipped to prevent dangling references. - * - * @param appliedOps - The ops that were just applied (includes the SYNC_IMPORT) - */ - private async _replayLocalSyncedOpsAfterImport(appliedOps: Operation[]): Promise { - // Get the SYNC_IMPORT's vector clock - we need to replay ops that happened AFTER it - const syncImportOp = appliedOps.find( - (op) => op.opType === OpType.SyncImport || op.opType === OpType.BackupImport, - ); - if (!syncImportOp) { - return; // Shouldn't happen, but be safe - } - - // Get the current client ID - const clientId = await this.clientIdProvider.loadClientId(); - if (!clientId) { - return; - } - - // Get all local ops that: - // 1. Were created by THIS client (so they're not in the piggybacked ops) - // 2. Are already synced (accepted by server) - // 3. Were created AFTER the SYNC_IMPORT (by UUIDv7 timestamp comparison) - const allEntries = await this.opLogStore.getOpsAfterSeq(0); - const localSyncedOps = allEntries - .filter((entry) => { - // Must be created by this client - if (entry.op.clientId !== clientId) return false; - // Must be synced (accepted by server) - if (!entry.syncedAt) return false; - // Must NOT be a full-state op itself - if ( - entry.op.opType === OpType.SyncImport || - entry.op.opType === OpType.BackupImport - ) { - return false; - } - // Must be created AFTER the SYNC_IMPORT/BACKUP_IMPORT. - // UUIDv7 is time-ordered: first 48 bits = millisecond timestamp. - // Ops created BEFORE the import should NOT be replayed - they reference - // the old state that was replaced by the import. - // NOTE: We use UUIDv7 comparison instead of vector clock comparison - // because imports with isForceConflict=true create a fresh vector clock - // with a new client ID, breaking vector clock causality detection. - if (entry.op.id < syncImportOp.id) { - OpLog.verbose( - `OperationLogSyncService: Skipping op ${entry.op.id} - created before SYNC_IMPORT`, - ); - return false; - } - return true; - }) - .map((entry) => entry.op); - - if (localSyncedOps.length === 0) { - OpLog.normal( - 'OperationLogSyncService: No local synced ops to replay after SYNC_IMPORT.', - ); - return; - } - - // ───────────────────────────────────────────────────────────────────────── - // Filter out operations whose target entities were deleted by SYNC_IMPORT - // ───────────────────────────────────────────────────────────────────────── - // SYNC_IMPORT replaces the entire state. Local operations may reference - // entities that existed before but are now gone. We must skip these to - // prevent creating dangling references (e.g., Today tag → non-existent task). - // - // IMPORTANT: We must track entities that will be CREATED by ops in this sequence. - // If an ADD op creates entity X, then UPDATE/DELETE ops for X should NOT be skipped, - // even if X doesn't exist in the SYNC_IMPORT state yet. - const validOps: Operation[] = []; - const skippedOps: { op: Operation; missingIds: string[] }[] = []; - - // First pass: collect entity IDs that will be created by CREATE ops in this sequence - const entitiesCreatedBySequence = new Set(); - for (const op of localSyncedOps) { - if (op.opType === OpType.Create && op.entityId) { - entitiesCreatedBySequence.add(op.entityId); - } - } - - for (const op of localSyncedOps) { - const missingEntityIds = await this._checkOperationEntitiesExist(op); - - // Filter out missing entities that will be created by earlier ops in this sequence - const trulyMissingIds = missingEntityIds.filter( - (id) => !entitiesCreatedBySequence.has(id), - ); - - if (trulyMissingIds.length > 0) { - OpLog.warn( - `OperationLogSyncService: Skipping op ${op.id} (${op.actionType}) - ` + - `target entities deleted by SYNC_IMPORT: ${trulyMissingIds.join(', ')}`, - ); - skippedOps.push({ op, missingIds: trulyMissingIds }); - } else { - validOps.push(op); - } - } - - if (validOps.length === 0) { - OpLog.normal( - `OperationLogSyncService: All ${localSyncedOps.length} local ops skipped - ` + - `target entities deleted by SYNC_IMPORT.`, - ); - return; - } - - OpLog.normal( - `OperationLogSyncService: Replaying ${validOps.length} local synced ops ` + - `(${skippedOps.length} skipped) after SYNC_IMPORT.`, - ); - - // Re-apply these ops to restore the local changes on top of the SYNC_IMPORT state - // Operations are already in server sequence order, which is causally correct - // Use applyOperations which handles action dispatching - try { - await this.operationApplier.applyOperations(validOps); - } catch (replayError) { - // If replay fails after SYNC_IMPORT succeeded, user loses their local synced operations. - // Log the error, notify user, and trigger validation to detect/repair any inconsistencies. - OpLog.err( - `OperationLogSyncService: Failed to replay ${validOps.length} local ops after SYNC_IMPORT`, - replayError, - ); - - // Run validation to detect and repair any state inconsistencies - await this.validateStateService.validateAndRepairCurrentState( - 'replay-local-ops-failed', - ); - - // Notify user that some changes may be lost - this.snackService.open({ - type: 'ERROR', - msg: T.F.SYNC.S.REPLAY_LOCAL_OPS_FAILED, - }); - - // Don't re-throw - the SYNC_IMPORT itself succeeded, so sync should continue - } - } - // ═══════════════════════════════════════════════════════════════════════════ // REMOTE OPS PROCESSING (Core Pipeline) // ═══════════════════════════════════════════════════════════════════════════ @@ -1099,14 +870,9 @@ export class OperationLogSyncService { ); await this._applyNonConflictingOps(validOps); - // IMPORTANT: After applying a SYNC_IMPORT, re-apply any local ops that were - // already synced to the server. This handles the "late joiner" scenario where: - // 1. Client B creates local tasks (B1, B2, B3) - // 2. Client B uploads them (server accepts) - // 3. Client B receives piggybacked SYNC_IMPORT from Client A - // 4. SYNC_IMPORT replaces state (B's tasks lost!) - // 5. We need to re-apply B's synced ops to restore them - await this._replayLocalSyncedOpsAfterImport(validOps); + // Clean Slate Semantics: SYNC_IMPORT/BACKUP_IMPORT replaces entire state. + // Local synced ops are NOT replayed - the import is an explicit user action + // to restore all clients to a specific point in time. await this._validateAfterSync(); return { localWinOpsCreated: 0 }; diff --git a/src/app/core/persistence/operation-log/sync/sync-import-filter.service.spec.ts b/src/app/core/persistence/operation-log/sync/sync-import-filter.service.spec.ts index a4d0e2e135..64fa153b1c 100644 --- a/src/app/core/persistence/operation-log/sync/sync-import-filter.service.spec.ts +++ b/src/app/core/persistence/operation-log/sync/sync-import-filter.service.spec.ts @@ -244,11 +244,12 @@ describe('SyncImportFilterService', () => { const result = await service.filterOpsInvalidatedBySyncImport(ops); - // Updated behavior: Client A was UNKNOWN to the latest import (no clientA entry) - // So ALL of Client A's ops are kept (parallel branch of work) - // Valid: all 5 ops (2 imports + 3 updates from unknown client) - expect(result.validOps.length).toBe(5); - expect(result.invalidatedOps.length).toBe(0); + // Clean Slate Semantics: Only ops with knowledge of the latest import are kept. + // - First two Client A ops are CONCURRENT (no knowledge of latest import) → filtered + // - Third Client A op is GREATER_THAN (has latest import's clock) → kept + // - Both SYNC_IMPORTs are kept + expect(result.validOps.length).toBe(3); // 2 imports + 1 post-import op + expect(result.invalidatedOps.length).toBe(2); // 2 concurrent ops from Client A }); it('should filter pre-import ops when SYNC_IMPORT was downloaded in a PREVIOUS sync cycle', async () => { @@ -271,6 +272,7 @@ describe('SyncImportFilterService', () => { ); // These are OLD ops from Client A, created BEFORE the import + // They are CONCURRENT with the import (no knowledge of it) const oldOpsFromClientA: Operation[] = [ { id: '019afd60-0001-7000-0000-000000000000', @@ -280,7 +282,7 @@ describe('SyncImportFilterService', () => { entityId: 'task-1', payload: { title: 'Old title' }, clientId: 'client-A', - vectorClock: { clientA: 5 }, + vectorClock: { clientA: 5 }, // CONCURRENT - no knowledge of import timestamp: Date.now(), schemaVersion: 1, }, @@ -292,7 +294,7 @@ describe('SyncImportFilterService', () => { entityId: 'task-2', payload: { title: 'Another old title' }, clientId: 'client-A', - vectorClock: { clientA: 6 }, + vectorClock: { clientA: 6 }, // CONCURRENT - no knowledge of import timestamp: Date.now(), schemaVersion: 1, }, @@ -300,10 +302,10 @@ describe('SyncImportFilterService', () => { const result = await service.filterOpsInvalidatedBySyncImport(oldOpsFromClientA); - // Updated behavior: Client A was UNKNOWN to the import (no clientA entry) - // So Client A's ops are kept (parallel branch of work) - expect(result.validOps.length).toBe(2); - expect(result.invalidatedOps.length).toBe(0); + // Clean Slate Semantics: CONCURRENT ops are filtered, even from unknown clients. + // These ops have no knowledge of the import, so they're invalidated. + expect(result.validOps.length).toBe(0); + expect(result.invalidatedOps.length).toBe(2); expect(opLogStoreSpy.getLatestFullStateOp).toHaveBeenCalled(); }); @@ -379,10 +381,10 @@ describe('SyncImportFilterService', () => { const result = await service.filterOpsInvalidatedBySyncImport(ops); - // Updated behavior: Client B was UNKNOWN to the import (no clientB entry) - // So Client B's ops are kept (parallel branch of work) - expect(result.validOps.length).toBe(2); - expect(result.invalidatedOps.length).toBe(0); + // Clean Slate Semantics: CONCURRENT ops are filtered, even from unknown clients. + // Client B's op has no knowledge of the import, so it's invalidated. + expect(result.validOps.length).toBe(1); // Only SYNC_IMPORT + expect(result.invalidatedOps.length).toBe(1); // Client B's concurrent op }); it('should filter LESS_THAN ops (dominated by import)', async () => { @@ -519,10 +521,11 @@ describe('SyncImportFilterService', () => { const result = await service.filterOpsInvalidatedBySyncImport(ops); - // Updated behavior: Client B was UNKNOWN to the import (no clientB entry) - // So Client B's ops are kept (parallel branch of work) - expect(result.validOps.length).toBe(2); - expect(result.invalidatedOps.length).toBe(0); + // Clean Slate Semantics: CONCURRENT ops are filtered based on vector clock, + // NOT UUIDv7 timestamp. Even though UUIDv7 is later, vector clock shows + // no knowledge of import, so it's filtered. + expect(result.validOps.length).toBe(1); // Only SYNC_IMPORT + expect(result.invalidatedOps.length).toBe(1); // Client B's concurrent op }); it('should handle REPAIR operations the same as SYNC_IMPORT', async () => { @@ -555,10 +558,10 @@ describe('SyncImportFilterService', () => { const result = await service.filterOpsInvalidatedBySyncImport(ops); - // Updated behavior: Client B was UNKNOWN to the repair (no clientB entry) - // So Client B's ops are kept (parallel branch of work) - expect(result.validOps.length).toBe(2); - expect(result.invalidatedOps.length).toBe(0); + // Clean Slate Semantics: REPAIR ops are handled the same as SYNC_IMPORT. + // CONCURRENT ops are filtered, even from unknown clients. + expect(result.validOps.length).toBe(1); // Only REPAIR + expect(result.invalidatedOps.length).toBe(1); // Client B's concurrent op }); }); @@ -624,15 +627,15 @@ describe('SyncImportFilterService', () => { expect(result.invalidatedOps.length).toBe(0); }); - it('should correctly keep ops from unknown clients', async () => { - // Updated behavior: ops from clients unknown to the SYNC_IMPORT are KEPT - // Rationale: if the import didn't know about this client, the op is NOT - // "pre-import state" - it's from a parallel branch of work. + it('should filter ops from unknown clients (clean slate semantics)', async () => { + // Clean Slate Semantics: ops from clients unknown to the SYNC_IMPORT are FILTERED. + // Rationale: the import is an explicit user action to restore ALL clients to + // a specific state. Concurrent work is intentionally discarded. // // Scenario: // 1. Client A creates SYNC_IMPORT with clock {clientA: 1} // 2. Client B (unknown to A) creates op with clock {clientB: 6} - // 3. Client B's op should be KEPT (client B was unknown to import) + // 3. Client B's op should be FILTERED (no knowledge of import) const existingSyncImport: Operation = { id: '019afd68-0050-7000-0000-000000000000', @@ -669,9 +672,9 @@ describe('SyncImportFilterService', () => { const result = await service.filterOpsInvalidatedBySyncImport(unknownClientOp); - // Should be KEPT since client-B was unknown to the import - expect(result.validOps.length).toBe(1); - expect(result.invalidatedOps.length).toBe(0); + // Should be FILTERED - no knowledge of import means it's pre-import state + expect(result.validOps.length).toBe(0); + expect(result.invalidatedOps.length).toBe(1); }); it('should handle multiple clients scenario correctly', async () => { diff --git a/src/app/core/persistence/operation-log/sync/sync-import-filter.service.ts b/src/app/core/persistence/operation-log/sync/sync-import-filter.service.ts index 77485c354f..65d40ed434 100644 --- a/src/app/core/persistence/operation-log/sync/sync-import-filter.service.ts +++ b/src/app/core/persistence/operation-log/sync/sync-import-filter.service.ts @@ -21,11 +21,21 @@ import { OpLog } from '../../../log'; * - Applying them causes "Task not found" errors * ``` * - * ## The Solution - * Use VECTOR CLOCK comparison to determine if ops were created with knowledge - * of the import. This is more reliable than UUIDv7 timestamps because vector - * clocks track CAUSALITY (did the client know about the import?) rather than - * wall-clock time (which can be affected by clock drift). + * ## The Solution: Clean Slate Semantics + * SYNC_IMPORT and BACKUP_IMPORT are explicit user actions to restore ALL clients + * to a specific state. ALL operations without knowledge of the import are dropped: + * + * - **GREATER_THAN / EQUAL**: Op was created with knowledge of import → KEEP + * - **CONCURRENT**: Op was created without knowledge of import → DROP + * - **LESS_THAN**: Op is dominated by import → DROP + * + * This ensures a true "restore to point in time" semantic. Concurrent work from + * other clients is intentionally discarded because the user explicitly chose to + * reset all state to the imported snapshot. + * + * We use vector clock comparison (not UUIDv7 timestamps) because vector clocks + * track CAUSALITY (did the client know about the import?) rather than wall-clock + * time (which can be affected by clock drift). */ @Injectable({ providedIn: 'root', @@ -36,6 +46,10 @@ export class SyncImportFilterService { /** * Filters out operations invalidated by a SYNC_IMPORT, BACKUP_IMPORT, or REPAIR. * + * ## Clean Slate Semantics + * Imports are explicit user actions to restore all clients to a specific state. + * ALL operations without knowledge of the import are dropped - no exceptions. + * * ## Vector Clock Comparison Results * | Comparison | Meaning | Action | * |----------------|--------------------------------------|---------| @@ -44,8 +58,8 @@ export class SyncImportFilterService { * | LESS_THAN | Op dominated by import | ❌ Filter| * | CONCURRENT | Op created without knowledge of import| ❌ Filter| * - * Note: CONCURRENT ops are filtered because they reference pre-import state, - * even if they were created "after" the import in wall-clock time. + * CONCURRENT ops are filtered even if they come from a client the import + * didn't know about. This ensures a true "restore to point in time" semantic. * * The import can be in the current batch OR in the local store from a * previous sync cycle. We check both to handle the case where old ops from @@ -115,17 +129,15 @@ export class SyncImportFilterService { // Vector clocks track CAUSALITY ("did this client know about the import?") // rather than wall-clock time, making them immune to client clock drift. // - // Comparison results: + // Clean Slate Semantics: // - GREATER_THAN: Op was created by a client that SAW the import → KEEP // - EQUAL: Same causal history as import → KEEP - // - LESS_THAN: Op is dominated by import (created before with less history) → FILTER - // - CONCURRENT: Op created WITHOUT knowledge of import → handled below + // - CONCURRENT: Op created WITHOUT knowledge of import → FILTER + // - LESS_THAN: Op is dominated by import → FILTER // - // SPECIAL CASE for CONCURRENT ops: - // If the op's client is NOT in the SYNC_IMPORT's clock, this means the - // SYNC_IMPORT was created without knowledge of that client. In this case, - // the op is NOT "pre-import state" - it's from a client that the import - // didn't know about. These ops should be KEPT and applied. + // CONCURRENT ops are filtered even from "unknown" clients. The import is + // an explicit user action to restore to a specific state - any concurrent + // work is intentionally discarded to ensure a clean slate. const comparison = compareVectorClocks(op.vectorClock, latestImport.vectorClock); if ( @@ -134,36 +146,9 @@ export class SyncImportFilterService { ) { // Op was created by a client that had knowledge of the import validOps.push(op); - } else if (comparison === VectorClockComparison.CONCURRENT) { - // Check if the op's client was unknown when the SYNC_IMPORT was created - const opClientValue = op.vectorClock[op.clientId] ?? 0; - const importClientValue = latestImport.vectorClock[op.clientId] ?? 0; - - if (importClientValue === 0) { - // The SYNC_IMPORT didn't know about this client at all - // This op is NOT "pre-import state" - it's from a parallel branch - // that the import didn't include. Keep it and apply. - OpLog.verbose( - `SyncImportFilterService: Keeping CONCURRENT op ${op.id} - ` + - `client ${op.clientId} was unknown to SYNC_IMPORT`, - ); - validOps.push(op); - } else if (opClientValue > importClientValue) { - // The op's client counter is higher than what the import knew about - // This means the op was created AFTER the import's knowledge of this client - OpLog.verbose( - `SyncImportFilterService: Keeping CONCURRENT op ${op.id} - ` + - `client counter ${opClientValue} > import's ${importClientValue}`, - ); - validOps.push(op); - } else { - // The op was created before or at the import's knowledge level for this client - // AND the import has entries the op doesn't know about - // This is truly pre-import state - filter it - invalidatedOps.push(op); - } } else { - // LESS_THAN: Op is dominated by import - definitely pre-import state + // CONCURRENT or LESS_THAN: Op was created without knowledge of import + // Filter it to ensure clean slate semantics invalidatedOps.push(op); } }