From f38342eeb9d540bea183723051d9a8458c204e6a Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Fri, 10 Jul 2026 15:24:51 +0200 Subject: [PATCH] fix(sync): harden replay and conflict recovery Make remote replay and authoritative state replacement crash-resumable and atomic. Preserve pending changes until piggyback processing commits, and stop cleanly on incompatible operations. Validate sync identities and schemas while strengthening request deduplication and privacy-safe diagnostics. --- ...ersync-conflict-gate-non-task-work.spec.ts | 56 ++- ...sc-to-tasks-settings-migration-v1-to-v2.ts | 2 + .../src/supersync-http-contract.ts | 4 +- .../tests/supersync-http-contract.spec.ts | 27 ++ .../super-sync-server/src/sync/conflict.ts | 28 ++ .../services/operation-download.service.ts | 3 + .../sync/services/operation-upload.service.ts | 29 +- .../services/request-deduplication.service.ts | 59 ++- .../src/sync/services/validation.service.ts | 6 +- .../src/sync/sync.routes.ops-handler.ts | 12 +- .../src/sync/sync.routes.snapshot-handler.ts | 28 +- .../src/sync/sync.service.ts | 26 +- .../tests/operation-download.service.spec.ts | 28 ++ ...quest-dedup-failure-caching.routes.spec.ts | 3 + .../request-deduplication.service.spec.ts | 52 ++- .../tests/sync-compressed-body.routes.spec.ts | 2 + .../tests/sync.service.spec.ts | 34 +- .../tests/validation.service.spec.ts | 10 + packages/sync-core/src/operation.types.ts | 2 +- packages/sync-core/src/ports.ts | 5 +- packages/sync-core/src/remote-apply.ts | 35 +- packages/sync-core/src/replay-coordinator.ts | 3 + packages/sync-core/tests/remote-apply.spec.ts | 34 +- .../tests/replay-coordinator.spec.ts | 5 + .../imex/sync/sync-wrapper.service.spec.ts | 24 ++ src/app/imex/sync/sync-wrapper.service.ts | 21 + .../op-log/apply/operation-applier.service.ts | 1 + .../backup/state-snapshot.service.spec.ts | 27 ++ .../op-log/backup/state-snapshot.service.ts | 19 +- .../operation-capture.meta-reducer.spec.ts | 11 +- .../capture/operation-capture.meta-reducer.ts | 48 ++- .../capture/operation-log.effects.spec.ts | 28 +- .../op-log/capture/operation-log.effects.ts | 27 +- src/app/op-log/core/types/apply.types.ts | 7 + .../op-log/core/types/sync-results.types.ts | 24 +- .../compact/compact-operation.types.ts | 2 +- .../operation-log-hydrator.service.spec.ts | 7 +- .../operation-log-hydrator.service.ts | 17 +- .../operation-log-recovery.service.spec.ts | 20 +- .../operation-log-recovery.service.ts | 12 +- .../operation-log-store.service.spec.ts | 159 +++++++ .../operation-log-store.service.ts | 136 +++++- .../schema-migration.service.spec.ts | 34 ++ .../persistence/schema-migration.service.ts | 24 +- .../sync/conflict-resolution.service.spec.ts | 30 +- .../sync/conflict-resolution.service.ts | 28 +- .../sync/immediate-upload.service.spec.ts | 13 + .../op-log/sync/immediate-upload.service.ts | 8 + .../operation-log-download.service.spec.ts | 45 ++ .../sync/operation-log-download.service.ts | 20 +- .../sync/operation-log-sync.service.spec.ts | 385 ++++++++++++----- .../op-log/sync/operation-log-sync.service.ts | 403 +++++++++++------- .../sync/operation-log-upload.service.spec.ts | 24 ++ .../sync/operation-log-upload.service.ts | 30 +- .../remote-ops-processing.service.spec.ts | 114 ++++- .../sync/remote-ops-processing.service.ts | 50 ++- .../sync/server-migration.service.spec.ts | 70 ++- .../op-log/sync/server-migration.service.ts | 222 +++++----- .../sync-import-conflict-gate.service.spec.ts | 100 ++++- .../sync/sync-import-conflict-gate.service.ts | 57 +-- .../ws-triggered-download.service.spec.ts | 21 +- .../sync/ws-triggered-download.service.ts | 8 + ...emote-apply-store-port.integration.spec.ts | 22 +- 63 files changed, 2170 insertions(+), 621 deletions(-) diff --git a/e2e/tests/sync/supersync-conflict-gate-non-task-work.spec.ts b/e2e/tests/sync/supersync-conflict-gate-non-task-work.spec.ts index 314f7e08bc..5c38ccea49 100644 --- a/e2e/tests/sync/supersync-conflict-gate-non-task-work.spec.ts +++ b/e2e/tests/sync/supersync-conflict-gate-non-task-work.spec.ts @@ -40,7 +40,6 @@ const createClickCounter = async ( ): Promise => { await client.page.goto('/#/habits'); await client.page.waitForURL(/habits/); - await client.page.waitForTimeout(500); const addBtn = client.page.locator('.add-habit-btn'); await addBtn.waitFor({ state: 'visible', timeout: 10000 }); @@ -55,28 +54,51 @@ const createClickCounter = async ( const typeSelect = dialog.locator('mat-select').first(); await typeSelect.waitFor({ state: 'visible', timeout: 5000 }); - await client.page.waitForTimeout(500); await typeSelect.click(); - await client.page.waitForTimeout(300); - await client.page.locator('mat-option:has-text("Click Counter")').click(); - await client.page.waitForTimeout(300); + const clickCounterOption = client.page.locator('mat-option:has-text("Click Counter")'); + await clickCounterOption.waitFor({ state: 'visible', timeout: 5000 }); + await clickCounterOption.click(); await dialog.locator('button[type="submit"]').click(); await dialog.waitFor({ state: 'hidden', timeout: 10000 }); - await client.page.waitForTimeout(500); await client.page.goto('/#/tag/TODAY/tasks'); await client.page.waitForURL(/(active\/tasks|tag\/TODAY\/tasks)/); - await client.page.waitForTimeout(500); }; -const incrementClickCounter = async (client: SimulatedE2EClient): Promise => { +const getNamedClickCounter = async ( + client: SimulatedE2EClient, + title: string, +): Promise> => { const counter = client.page .locator('.counters-action-group simple-counter-button') .last(); await counter.waitFor({ state: 'visible', timeout: 15000 }); + + // Counter buttons render only their initial, so verify the exact counter via + // its accessible Material tooltip before interacting with it. + await counter.hover(); + await expect(client.page.getByRole('tooltip').filter({ hasText: title })).toBeVisible(); + return counter; +}; + +const expectClickCounterValue = async ( + client: SimulatedE2EClient, + title: string, + expectedValue: number, +): Promise => { + const counter = await getNamedClickCounter(client, title); + await expect(counter.locator('.label')).toHaveText(String(expectedValue)); +}; + +const incrementClickCounter = async ( + client: SimulatedE2EClient, + title: string, + expectedValue: number, +): Promise => { + const counter = await getNamedClickCounter(client, title); await counter.locator('.main-btn').click(); - await client.page.waitForTimeout(200); + await expect(counter.locator('.label')).toHaveText(String(expectedValue)); }; test.describe.configure({ mode: 'serial' }); @@ -101,7 +123,7 @@ test.describe('@supersync incoming SYNC_IMPORT preserves non-task local work', ( clientA = await createSimulatedClient(browser, baseURL!, 'A', testRunId); await clientA.sync.setupSuperSync(syncConfig); await createClickCounter(clientA, counterTitle); - await incrementClickCounter(clientA); + await incrementClickCounter(clientA, counterTitle, 1); await clientA.sync.syncAndWait(); console.log('[Gate] Client A created + synced a simple counter'); @@ -111,10 +133,11 @@ test.describe('@supersync incoming SYNC_IMPORT preserves non-task local work', ( await clientB.sync.syncAndWait(); await clientB.page.goto('/#/tag/TODAY/tasks'); await clientB.page.waitForLoadState('networkidle'); + await expectClickCounterValue(clientB, counterTitle, 1); console.log('[Gate] Client B synced and received the counter'); // ===== PHASE 3: B makes a pending NON-task change (does NOT sync) ===== - await incrementClickCounter(clientB); + await incrementClickCounter(clientB, counterTitle, 2); console.log('[Gate] Client B incremented the counter locally (pending, unsynced)'); // ===== PHASE 4: A imports a backup (SYNC_IMPORT) and syncs it ===== @@ -138,6 +161,17 @@ test.describe('@supersync incoming SYNC_IMPORT preserves non-task local work', ( state: 'hidden', timeout: 10000, }); + await clientB.sync.waitForSyncToComplete({ timeout: 30000 }); + + // The dialog itself is not the guarantee: prove the exact pending counter + // value won, then run another sync and prove it remains durable. + await clientB.page.goto('/#/tag/TODAY/tasks'); + await clientB.page.waitForURL(/(active\/tasks|tag\/TODAY\/tasks)/); + await expectClickCounterValue(clientB, counterTitle, 2); + + await clientB.sync.syncAndWait(); + await expectClickCounterValue(clientB, counterTitle, 2); + console.log('[Gate] ✓ Pending counter value survived resolution and re-sync'); } finally { if (clientA) await closeClient(clientA); if (clientB) await closeClient(clientB); diff --git a/packages/shared-schema/src/migrations/misc-to-tasks-settings-migration-v1-to-v2.ts b/packages/shared-schema/src/migrations/misc-to-tasks-settings-migration-v1-to-v2.ts index 9a3158ba86..9a0d35b9b9 100644 --- a/packages/shared-schema/src/migrations/misc-to-tasks-settings-migration-v1-to-v2.ts +++ b/packages/shared-schema/src/migrations/misc-to-tasks-settings-migration-v1-to-v2.ts @@ -97,6 +97,7 @@ export const MiscToTasksSettingsMigration_v1v2: SchemaMigration = { result.push({ ...op, id: `${op.id}_misc`, + entityIds: ['misc'], payload: buildPayload(miscCfg, 'misc'), }); } @@ -106,6 +107,7 @@ export const MiscToTasksSettingsMigration_v1v2: SchemaMigration = { ...op, id: `${op.id}_tasks`, entityId: 'tasks', + entityIds: ['tasks'], payload: buildPayload(tasksCfg, 'tasks'), }); } diff --git a/packages/shared-schema/src/supersync-http-contract.ts b/packages/shared-schema/src/supersync-http-contract.ts index f9ac187e8e..b128400cbf 100644 --- a/packages/shared-schema/src/supersync-http-contract.ts +++ b/packages/shared-schema/src/supersync-http-contract.ts @@ -69,7 +69,7 @@ export const SuperSyncOperationSchema = z.object({ payload: z.unknown(), vectorClock: SuperSyncVectorClockSchema, timestamp: z.number(), - schemaVersion: z.number(), + schemaVersion: z.number().int().min(1).max(100), isPayloadEncrypted: z.boolean().optional(), syncImportReason: z.enum(SUPER_SYNC_IMPORT_REASONS).optional(), }); @@ -92,7 +92,7 @@ export const SuperSyncUploadSnapshotRequestSchema = z.object({ clientId: SuperSyncClientIdSchema, reason: z.enum(SUPER_SYNC_SNAPSHOT_REASONS), vectorClock: SuperSyncVectorClockSchema, - schemaVersion: z.number().optional(), + schemaVersion: z.number().int().min(1).max(100).optional(), isPayloadEncrypted: z.boolean().optional(), syncImportReason: z.enum(SUPER_SYNC_IMPORT_REASONS).optional(), opId: z.string().uuid().optional(), diff --git a/packages/shared-schema/tests/supersync-http-contract.spec.ts b/packages/shared-schema/tests/supersync-http-contract.spec.ts index 65faa56474..c0f725540b 100644 --- a/packages/shared-schema/tests/supersync-http-contract.spec.ts +++ b/packages/shared-schema/tests/supersync-http-contract.spec.ts @@ -72,6 +72,33 @@ describe('SuperSync HTTP contract schemas', () => { ).toThrow(); }); + it.each([0, 1.5, -1, 101])( + 'rejects malformed operation schema version %s', + (schemaVersion) => { + expect(() => + SuperSyncUploadOpsRequestSchema.parse({ + ops: [{ ...createValidOperation(), schemaVersion }], + clientId: 'client_1', + }), + ).toThrow(); + }, + ); + + it.each([0, 1.5, -1, 101])( + 'rejects malformed snapshot schema version %s', + (schemaVersion) => { + expect(() => + SuperSyncUploadSnapshotRequestSchema.parse({ + state: {}, + clientId: 'client_1', + reason: 'recovery', + vectorClock: { client_1: 1 }, + schemaVersion, + }), + ).toThrow(); + }, + ); + it('coerces download query numbers like the route-level schema', () => { const parsed = SuperSyncDownloadOpsQuerySchema.parse({ sinceSeq: '5', diff --git a/packages/super-sync-server/src/sync/conflict.ts b/packages/super-sync-server/src/sync/conflict.ts index 1aad6b9d99..008733516a 100644 --- a/packages/super-sync-server/src/sync/conflict.ts +++ b/packages/super-sync-server/src/sync/conflict.ts @@ -265,6 +265,34 @@ export const isSameDuplicateOperation = ( ); }; +/** Complete identity comparison for two validated operations in one request. */ +export const isSameIncomingOperation = ( + first: Operation, + second: Operation, + firstOriginalTimestamp: number = first.timestamp, + secondOriginalTimestamp: number = second.timestamp, +): boolean => { + const bothEncrypted = + (first.isPayloadEncrypted ?? false) && (second.isPayloadEncrypted ?? false); + return ( + first.clientId === second.clientId && + first.actionType === second.actionType && + first.opType === second.opType && + first.entityType === second.entityType && + first.entityId === second.entityId && + areJsonValuesEqual(getStoredEntityIds(first), getStoredEntityIds(second)) && + (bothEncrypted || areJsonValuesEqual(first.payload, second.payload)) && + areJsonValuesEqual( + limitVectorClockSize(first.vectorClock, [first.clientId]), + limitVectorClockSize(second.vectorClock, [second.clientId]), + ) && + first.schemaVersion === second.schemaVersion && + firstOriginalTimestamp === secondOriginalTimestamp && + (first.isPayloadEncrypted ?? false) === (second.isPayloadEncrypted ?? false) && + (first.syncImportReason ?? null) === (second.syncImportReason ?? null) + ); +}; + export const isSameDuplicateTimestamp = ( existingTimestamp: bigint | number | string, existingReceivedAt: bigint | number | string, diff --git a/packages/super-sync-server/src/sync/services/operation-download.service.ts b/packages/super-sync-server/src/sync/services/operation-download.service.ts index 0a718ddd8c..724608e353 100644 --- a/packages/super-sync-server/src/sync/services/operation-download.service.ts +++ b/packages/super-sync-server/src/sync/services/operation-download.service.ts @@ -21,6 +21,7 @@ const OPERATION_DOWNLOAD_SELECT = { opType: true, entityType: true, entityId: true, + entityIds: true, payload: true, vectorClock: true, schemaVersion: true, @@ -54,6 +55,7 @@ type OperationDownloadRow = { opType: string; entityType: string; entityId: string | null; + entityIds: string[]; payload: unknown; vectorClock: unknown; schemaVersion: number; @@ -72,6 +74,7 @@ const mapOperationRow = (row: OperationDownloadRow): ServerOperation => ({ opType: row.opType as Operation['opType'], entityType: row.entityType, entityId: row.entityId ?? undefined, + entityIds: row.entityIds.length > 0 ? row.entityIds : undefined, payload: row.payload, vectorClock: row.vectorClock as VectorClock, schemaVersion: row.schemaVersion, diff --git a/packages/super-sync-server/src/sync/services/operation-upload.service.ts b/packages/super-sync-server/src/sync/services/operation-upload.service.ts index 00c69cc7b4..550b4e4276 100644 --- a/packages/super-sync-server/src/sync/services/operation-upload.service.ts +++ b/packages/super-sync-server/src/sync/services/operation-upload.service.ts @@ -24,6 +24,7 @@ import { getStoredEntityIds, getEntityConflictKey, isSameDuplicateOperation, + isSameIncomingOperation, prefetchLatestEntityOpsForBatch, pruneVectorClockForStorage, resolveConflictForExistingOp, @@ -317,10 +318,9 @@ export class OperationUploadService { } /** - * Stage 2: within a single batch, accept the first op for an id and reject - * every later op sharing that id as DUPLICATE_OPERATION (by id, not content - * — see plan §1a step 2 / the C4 divergence note). Must run before sequence - * reservation so a duplicate never consumes a server_seq. + * Stage 2: within a single batch, accept the first op for an id. Exact retries + * are DUPLICATE_OPERATION; same-id operations with different complete identity + * are INVALID_OP_ID. Must run before sequence reservation. */ private rejectIntraBatchDuplicates( userId: number, @@ -328,20 +328,31 @@ export class OperationUploadService { validatedCandidates: BatchUploadCandidate[], results: UploadResult[], ): BatchUploadCandidate[] { - const seenOpIds = new Set(); + const firstCandidateByOpId = new Map(); const uniqueCandidates: BatchUploadCandidate[] = []; for (const candidate of validatedCandidates) { - if (seenOpIds.has(candidate.op.id)) { + const firstCandidate = firstCandidateByOpId.get(candidate.op.id); + if (firstCandidate) { + const isExactRetry = isSameIncomingOperation( + firstCandidate.op, + candidate.op, + firstCandidate.originalTimestamp, + candidate.originalTimestamp, + ); results[candidate.resultIndex] = this.rejectedUploadResult( userId, clientId, candidate.op, - 'Duplicate operation ID', - SYNC_ERROR_CODES.DUPLICATE_OPERATION, + isExactRetry + ? 'Duplicate operation ID' + : 'Operation ID already belongs to a different operation', + isExactRetry + ? SYNC_ERROR_CODES.DUPLICATE_OPERATION + : SYNC_ERROR_CODES.INVALID_OP_ID, ); continue; } - seenOpIds.add(candidate.op.id); + firstCandidateByOpId.set(candidate.op.id, candidate); uniqueCandidates.push(candidate); } return uniqueCandidates; diff --git a/packages/super-sync-server/src/sync/services/request-deduplication.service.ts b/packages/super-sync-server/src/sync/services/request-deduplication.service.ts index 6c45ca235f..2ea3b1e496 100644 --- a/packages/super-sync-server/src/sync/services/request-deduplication.service.ts +++ b/packages/super-sync-server/src/sync/services/request-deduplication.service.ts @@ -8,6 +8,9 @@ * Entries expire after 5 minutes. */ import type { UploadResult } from '../sync.types'; +import type { Operation } from '../sync.types'; +import { createHash } from 'node:crypto'; +import { stableJsonStringify } from '../conflict'; /** * Maximum entries in deduplication cache to prevent unbounded memory growth. @@ -38,8 +41,18 @@ export interface SnapshotDedupResponse { * even if the same `requestId` string is reused across namespaces. */ type RequestDeduplicationEntry = - | { namespace: 'ops'; processedAt: number; results: UploadResult[] } - | { namespace: 'snapshot'; processedAt: number; results: SnapshotDedupResponse }; + | { + namespace: 'ops'; + processedAt: number; + results: UploadResult[]; + fingerprint?: string; + } + | { + namespace: 'snapshot'; + processedAt: number; + results: SnapshotDedupResponse; + fingerprint?: string; + }; type DedupPayload = N extends 'ops' ? UploadResult[] @@ -56,6 +69,7 @@ export class RequestDeduplicationService { userId: number, namespace: N, requestId: string, + fingerprint?: string, ): DedupPayload | null { const key = this._key(userId, namespace, requestId); const entry = this.cache.get(key); @@ -66,6 +80,7 @@ export class RequestDeduplicationService { } // Defensive: keying already isolates namespaces, but verify before casting. if (entry.namespace !== namespace) return null; + if (fingerprint !== undefined && entry.fingerprint !== fingerprint) return null; return entry.results as DedupPayload; } @@ -77,6 +92,7 @@ export class RequestDeduplicationService { namespace: N, requestId: string, results: DedupPayload, + fingerprint?: string, ): void { const key = this._key(userId, namespace, requestId); if (this.cache.size >= MAX_CACHE_SIZE) { @@ -88,6 +104,7 @@ export class RequestDeduplicationService { namespace, processedAt: Date.now(), results, + fingerprint, } as RequestDeduplicationEntry); } @@ -138,3 +155,41 @@ export class RequestDeduplicationService { return `${userId}:${namespace}:${requestId}`; } } + +export const createOpsRequestFingerprint = ( + clientId: string, + ops: Operation[], +): string => { + const logicalOps = ops.map((op) => ({ + ...op, + payload: op.isPayloadEncrypted ? '[encrypted-payload]' : op.payload, + })); + return createHash('sha256') + .update(stableJsonStringify({ clientId, ops: logicalOps })) + .digest('base64url'); +}; + +export interface SnapshotRequestFingerprintInput { + state: unknown; + clientId: string; + reason: string; + vectorClock: Record; + schemaVersion?: number; + isPayloadEncrypted?: boolean; + syncImportReason?: string; + opId?: string; + isCleanSlate?: boolean; + snapshotOpType?: string; +} + +export const createSnapshotRequestFingerprint = ( + request: SnapshotRequestFingerprintInput, +): string => { + const logicalRequest = { + ...request, + state: request.isPayloadEncrypted ? '[encrypted-state]' : request.state, + }; + return createHash('sha256') + .update(stableJsonStringify(logicalRequest)) + .digest('base64url'); +}; 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 382467ee12..971bece805 100644 --- a/packages/super-sync-server/src/sync/services/validation.service.ts +++ b/packages/super-sync-server/src/sync/services/validation.service.ts @@ -158,7 +158,11 @@ export class ValidationService { }; } if (op.schemaVersion !== undefined) { - if (op.schemaVersion < 1 || op.schemaVersion > 100) { + if ( + !Number.isInteger(op.schemaVersion) || + op.schemaVersion < 1 || + op.schemaVersion > 100 + ) { return { valid: false, error: `Invalid schema version: ${op.schemaVersion}`, diff --git a/packages/super-sync-server/src/sync/sync.routes.ops-handler.ts b/packages/super-sync-server/src/sync/sync.routes.ops-handler.ts index 0024759d59..24ed6eed35 100644 --- a/packages/super-sync-server/src/sync/sync.routes.ops-handler.ts +++ b/packages/super-sync-server/src/sync/sync.routes.ops-handler.ts @@ -30,6 +30,7 @@ import { getRawOpsCount, sendOpsBatchTooLargeReply, } from './sync.routes.quota'; +import { createOpsRequestFingerprint } from './services/request-deduplication.service'; export const uploadOpsHandler = async ( req: FastifyRequest<{ Body: UploadOpsRequest }>, @@ -86,6 +87,9 @@ export const uploadOpsHandler = async ( } const { ops, clientId, lastKnownServerSeq, requestId } = parseResult.data; + const requestFingerprint = requestId + ? createOpsRequestFingerprint(clientId, ops as unknown as Operation[]) + : undefined; const syncService = getSyncService(); Logger.info( @@ -112,7 +116,11 @@ export const uploadOpsHandler = async ( // This ensures retries after successful uploads don't fail with 413 // if the original upload pushed the user over quota if (requestId) { - const cachedResults = syncService.checkOpsRequestDedup(userId, requestId); + const cachedResults = syncService.checkOpsRequestDedup( + userId, + requestId, + requestFingerprint, + ); if (cachedResults) { Logger.info(`[user:${userId}] Returning cached results for request ${requestId}`); @@ -214,7 +222,7 @@ export const uploadOpsHandler = async ( requestId && !results.some((r) => r.errorCode === SYNC_ERROR_CODES.INTERNAL_ERROR) ) { - syncService.cacheOpsRequestResults(userId, requestId, results); + syncService.cacheOpsRequestResults(userId, requestId, results, requestFingerprint); } const accepted = results.filter((r) => r.accepted).length; diff --git a/packages/super-sync-server/src/sync/sync.routes.snapshot-handler.ts b/packages/super-sync-server/src/sync/sync.routes.snapshot-handler.ts index 2c01f0daa8..8f59879597 100644 --- a/packages/super-sync-server/src/sync/sync.routes.snapshot-handler.ts +++ b/packages/super-sync-server/src/sync/sync.routes.snapshot-handler.ts @@ -29,6 +29,7 @@ import { sendQuotaExceededReply, sendSyncImportExistsReply, } from './sync.routes.quota'; +import { createSnapshotRequestFingerprint } from './services/request-deduplication.service'; export const uploadSnapshotHandler = async ( req: FastifyRequest<{ Body: unknown }>, @@ -99,9 +100,27 @@ export const uploadSnapshotHandler = async ( requestId, } = snapshotRequest; const syncService = getSyncService(); + const requestFingerprint = requestId + ? createSnapshotRequestFingerprint({ + state, + clientId, + reason, + vectorClock, + schemaVersion, + isPayloadEncrypted, + syncImportReason, + opId, + isCleanSlate, + snapshotOpType, + }) + : undefined; if (requestId) { - const cachedResponse = syncService.checkSnapshotRequestDedup(userId, requestId); + const cachedResponse = syncService.checkSnapshotRequestDedup( + userId, + requestId, + requestFingerprint, + ); if (cachedResponse) { Logger.info( `[user:${userId}] Returning cached snapshot result for request ${requestId}`, @@ -340,7 +359,12 @@ export const uploadSnapshotHandler = async ( finalResult.errorCode !== SYNC_ERROR_CODES.DUPLICATE_OPERATION && finalResult.errorCode !== SYNC_ERROR_CODES.INTERNAL_ERROR ) { - syncService.cacheSnapshotRequestResult(userId, requestId, responseBody); + syncService.cacheSnapshotRequestResult( + userId, + requestId, + responseBody, + requestFingerprint, + ); } return reply.send(responseBody); diff --git a/packages/super-sync-server/src/sync/sync.service.ts b/packages/super-sync-server/src/sync/sync.service.ts index 9ac22cd7ca..d4ac70c58d 100644 --- a/packages/super-sync-server/src/sync/sync.service.ts +++ b/packages/super-sync-server/src/sync/sync.service.ts @@ -454,26 +454,44 @@ export class SyncService { return this.rateLimitService.cleanupExpiredCounters(); } - checkOpsRequestDedup(userId: number, requestId: string): UploadResult[] | null { - return this.requestDeduplicationService.checkDeduplication(userId, 'ops', requestId); + checkOpsRequestDedup( + userId: number, + requestId: string, + fingerprint?: string, + ): UploadResult[] | null { + return this.requestDeduplicationService.checkDeduplication( + userId, + 'ops', + requestId, + fingerprint, + ); } cacheOpsRequestResults( userId: number, requestId: string, results: UploadResult[], + fingerprint?: string, ): void { - this.requestDeduplicationService.cacheResults(userId, 'ops', requestId, results); + this.requestDeduplicationService.cacheResults( + userId, + 'ops', + requestId, + results, + fingerprint, + ); } checkSnapshotRequestDedup( userId: number, requestId: string, + fingerprint?: string, ): SnapshotDedupResponse | null { return this.requestDeduplicationService.checkDeduplication( userId, 'snapshot', requestId, + fingerprint, ); } @@ -481,12 +499,14 @@ export class SyncService { userId: number, requestId: string, response: SnapshotDedupResponse, + fingerprint?: string, ): void { this.requestDeduplicationService.cacheResults( userId, 'snapshot', requestId, response, + fingerprint, ); } diff --git a/packages/super-sync-server/tests/operation-download.service.spec.ts b/packages/super-sync-server/tests/operation-download.service.spec.ts index c53c2e667a..7c71cb113e 100644 --- a/packages/super-sync-server/tests/operation-download.service.spec.ts +++ b/packages/super-sync-server/tests/operation-download.service.spec.ts @@ -1,4 +1,5 @@ import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; +import type { Prisma } from '@prisma/client'; import { OperationDownloadService } from '../src/sync/services/operation-download.service'; // Mock prisma @@ -35,6 +36,7 @@ const EXPECTED_OPERATION_DOWNLOAD_SELECT = { opType: true, entityType: true, entityId: true, + entityIds: true, payload: true, vectorClock: true, schemaVersion: true, @@ -54,6 +56,7 @@ const createMockOpRow = ( actionType: string; entityType: string; entityId: string | null; + entityIds: string[]; payload: unknown; vectorClock: Record; schemaVersion: number; @@ -71,6 +74,7 @@ const createMockOpRow = ( entityType: overrides.entityType ?? 'Task', // Use 'in' check to allow null to be explicitly set entityId: 'entityId' in overrides ? overrides.entityId : `task-${serverSeq}`, + entityIds: overrides.entityIds ?? [], payload: overrides.payload ?? { title: `Task ${serverSeq}` }, vectorClock: overrides.vectorClock ?? { [clientId]: serverSeq }, schemaVersion: overrides.schemaVersion ?? 1, @@ -358,6 +362,30 @@ describe('OperationDownloadService', () => { ); }); + it('should round-trip batch entityIds in downloaded operations', async () => { + vi.mocked(prisma.$transaction).mockImplementation( + async (fn: (tx: Prisma.TransactionClient) => Promise) => + fn({ + operation: { + findFirst: vi.fn().mockResolvedValue(null), + findMany: vi.fn().mockResolvedValue([ + createMockOpRow(1, 'batch-client', { + entityId: 'task-1', + entityIds: ['task-1', 'task-2'], + }), + ]), + }, + userSyncState: { + findUnique: vi.fn().mockResolvedValue({ lastSeq: 1 }), + }, + } as unknown as Prisma.TransactionClient), + ); + + const result = await service.getOpsSinceWithSeq(1, 0); + + expect(result.ops[0].op.entityIds).toEqual(['task-1', 'task-2']); + }); + it('should detect gap when client is ahead of server', async () => { vi.mocked(prisma.$transaction).mockImplementation(async (fn: any) => { const mockTx = { diff --git a/packages/super-sync-server/tests/request-dedup-failure-caching.routes.spec.ts b/packages/super-sync-server/tests/request-dedup-failure-caching.routes.spec.ts index fae2d6bac2..862ad0ebe7 100644 --- a/packages/super-sync-server/tests/request-dedup-failure-caching.routes.spec.ts +++ b/packages/super-sync-server/tests/request-dedup-failure-caching.routes.spec.ts @@ -196,6 +196,7 @@ describe('Request dedup — transaction-failure results are not cached (#8332)', userId, 'ops-v1-success', results, + expect.any(String), ); }); @@ -218,6 +219,7 @@ describe('Request dedup — transaction-failure results are not cached (#8332)', userId, 'ops-v1-conflict', results, + expect.any(String), ); }); }); @@ -245,6 +247,7 @@ describe('Request dedup — transaction-failure results are not cached (#8332)', userId, 'snapshot-v1-success', { accepted: true, serverSeq: 5, error: undefined }, + expect.any(String), ); }); }); diff --git a/packages/super-sync-server/tests/request-deduplication.service.spec.ts b/packages/super-sync-server/tests/request-deduplication.service.spec.ts index a4cfa87dec..41acd782df 100644 --- a/packages/super-sync-server/tests/request-deduplication.service.spec.ts +++ b/packages/super-sync-server/tests/request-deduplication.service.spec.ts @@ -1,5 +1,8 @@ import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; -import { RequestDeduplicationService } from '../src/sync/services/request-deduplication.service'; +import { + RequestDeduplicationService, + createSnapshotRequestFingerprint, +} from '../src/sync/services/request-deduplication.service'; import { UploadResult } from '../src/sync/sync.types'; describe('RequestDeduplicationService', () => { @@ -36,6 +39,18 @@ describe('RequestDeduplicationService', () => { expect(result).toEqual(mockResults); }); + it('should treat the same requestId with a different body fingerprint as a cache miss', () => { + const mockResults = createMockResults(1); + service.cacheResults(1, 'ops', 'request-123', mockResults, 'fingerprint-a'); + + expect( + service.checkDeduplication(1, 'ops', 'request-123', 'fingerprint-a'), + ).toEqual(mockResults); + expect( + service.checkDeduplication(1, 'ops', 'request-123', 'fingerprint-b'), + ).toBeNull(); + }); + it('should return null for expired request', () => { const mockResults = createMockResults(1); service.cacheResults(1, 'ops', 'request-123', mockResults); @@ -114,6 +129,41 @@ describe('RequestDeduplicationService', () => { }); }); + describe('snapshot request fingerprints', () => { + const request = { + state: { task: { ids: ['task-1'] } }, + clientId: 'client-1', + reason: 'recovery', + vectorClock: { 'client-1': 1 }, + schemaVersion: 4, + }; + + it('changes when an unencrypted snapshot body changes', () => { + expect(createSnapshotRequestFingerprint(request)).not.toBe( + createSnapshotRequestFingerprint({ + ...request, + state: { task: { ids: ['task-2'] } }, + }), + ); + }); + + it('stays stable across encrypted snapshot IV changes', () => { + expect( + createSnapshotRequestFingerprint({ + ...request, + state: 'ciphertext-a', + isPayloadEncrypted: true, + }), + ).toBe( + createSnapshotRequestFingerprint({ + ...request, + state: 'ciphertext-b', + isPayloadEncrypted: true, + }), + ); + }); + }); + describe('cleanupExpiredEntries', () => { it('should remove all expired entries', () => { service.cacheResults(1, 'ops', 'request-1', createMockResults(1)); diff --git a/packages/super-sync-server/tests/sync-compressed-body.routes.spec.ts b/packages/super-sync-server/tests/sync-compressed-body.routes.spec.ts index fd60cfd4a2..60d282be76 100644 --- a/packages/super-sync-server/tests/sync-compressed-body.routes.spec.ts +++ b/packages/super-sync-server/tests/sync-compressed-body.routes.spec.ts @@ -810,6 +810,7 @@ describe('Sync compressed body routes', () => { expect(mocks.syncService.checkSnapshotRequestDedup).toHaveBeenCalledWith( 1, 'snapshot-v1-retry', + expect.any(String), ); expect(mocks.syncService.checkOpsRequestDedup).not.toHaveBeenCalled(); expect(mocks.prisma.operation.findFirst).not.toHaveBeenCalled(); @@ -879,6 +880,7 @@ describe('Sync compressed body routes', () => { 1, 'snapshot-v1-dup-test', { accepted: true, serverSeq: 77 }, + expect.any(String), ); }); diff --git a/packages/super-sync-server/tests/sync.service.spec.ts b/packages/super-sync-server/tests/sync.service.spec.ts index 7a46555be9..bcb192276a 100644 --- a/packages/super-sync-server/tests/sync.service.spec.ts +++ b/packages/super-sync-server/tests/sync.service.spec.ts @@ -708,15 +708,7 @@ describe('SyncService', () => { expect(testState.operations.size).toBe(25); }); - it('rejects an intra-batch same-id op as DUPLICATE_OPERATION even when its content differs (deliberate divergence from the legacy per-op path, which yields INVALID_OP_ID)', async () => { - // C4: the batch path dedups intra-batch purely by op.id (plan §1a step 2) - // and rejects the later op as DUPLICATE_OPERATION regardless of content. - // The legacy per-op path inserts the first then catches the second at the - // DB and returns INVALID_OP_ID for differing content. Both are terminal - // rejections with no row and no sequence gap, so sync invariants hold; - // the client treats DUPLICATE_OPERATION as a silent success and - // INVALID_OP_ID as a hard rejection. This test pins the chosen batch - // semantics so the divergence stays intentional, not accidental drift. + it('rejects an intra-batch same-id collision as INVALID_OP_ID', async () => { const service = new SyncService({ batchUpload: true }); const opId = uuidv7(); const first = makeOp({ @@ -747,7 +739,7 @@ describe('SyncService', () => { expect(results[1]).toEqual( expect.objectContaining({ accepted: false, - errorCode: SYNC_ERROR_CODES.DUPLICATE_OPERATION, + errorCode: SYNC_ERROR_CODES.INVALID_OP_ID, }), ); // No sequence gap: lastSeq advanced by exactly 1, exactly one row. @@ -755,6 +747,28 @@ describe('SyncService', () => { expect(testState.operations.size).toBe(1); }); + it('preserves an exact intra-batch retry as DUPLICATE_OPERATION', async () => { + const service = new SyncService({ batchUpload: true }); + const retry = makeOp({ + id: uuidv7(), + entityId: 'task-1', + entityIds: ['task-1', 'task-2'], + vectorClock: { [clientId]: 1 }, + }); + + const results = await service.uploadOps(userId, clientId, [retry, { ...retry }]); + + expect(results[0]).toEqual( + expect.objectContaining({ accepted: true, serverSeq: 1 }), + ); + expect(results[1]).toEqual( + expect.objectContaining({ + accepted: false, + errorCode: SYNC_ERROR_CODES.DUPLICATE_OPERATION, + }), + ); + }); + it('should reject intra-batch entity conflicts in order', async () => { const service = new SyncService({ batchUpload: true }); const ops: Operation[] = [ diff --git a/packages/super-sync-server/tests/validation.service.spec.ts b/packages/super-sync-server/tests/validation.service.spec.ts index b2e65c4c31..f98939be11 100644 --- a/packages/super-sync-server/tests/validation.service.spec.ts +++ b/packages/super-sync-server/tests/validation.service.spec.ts @@ -315,6 +315,16 @@ describe('ValidationService', () => { expect(result.errorCode).toBe(SYNC_ERROR_CODES.INVALID_SCHEMA_VERSION); }); + it('should reject non-integer schema versions', () => { + const result = validationService.validateOp( + createValidOp({ schemaVersion: 1.5 }), + clientId, + ); + + expect(result.valid).toBe(false); + expect(result.errorCode).toBe(SYNC_ERROR_CODES.INVALID_SCHEMA_VERSION); + }); + it('should accept valid schema versions 1-100', () => { for (const schemaVersion of [1, 50, 100]) { const op = createValidOp({ schemaVersion }); diff --git a/packages/sync-core/src/operation.types.ts b/packages/sync-core/src/operation.types.ts index ceb37f8edb..67e35f4ee1 100644 --- a/packages/sync-core/src/operation.types.ts +++ b/packages/sync-core/src/operation.types.ts @@ -151,7 +151,7 @@ export interface OperationLogEntry = Operat * For remote ops only: tracks whether the op was successfully applied to local state. * Used for crash recovery: on startup, any 'pending' or 'failed' remote ops are re-dispatched. */ - applicationStatus?: 'pending' | 'applied' | 'failed'; + applicationStatus?: 'pending' | 'archive_pending' | 'applied' | 'failed'; /** * For 'failed' ops: number of retry attempts. diff --git a/packages/sync-core/src/ports.ts b/packages/sync-core/src/ports.ts index 3aac4c98d3..76396607a4 100644 --- a/packages/sync-core/src/ports.ts +++ b/packages/sync-core/src/ports.ts @@ -20,7 +20,10 @@ export interface SyncActionLike { export interface OperationApplyPort = Operation> { applyOperations( ops: TOperation[], - options?: ApplyOperationsOptions, + options?: ApplyOperationsOptions & { + /** Persist reducer-commit bookkeeping before post-dispatch side effects start. */ + onReducersCommitted?: (ops: TOperation[]) => Promise; + }, ): Promise>; } diff --git a/packages/sync-core/src/remote-apply.ts b/packages/sync-core/src/remote-apply.ts index 2546770de6..3cc8f32f77 100644 --- a/packages/sync-core/src/remote-apply.ts +++ b/packages/sync-core/src/remote-apply.ts @@ -24,6 +24,7 @@ export interface RemoteOperationApplyStorePort< source: 'remote', options: { pendingApply: true }, ): Promise>; + markArchivePending(seqs: number[]): Promise; markApplied(seqs: number[]): Promise; markFailed(opIds: string[]): Promise; mergeRemoteOpClocks(ops: TOperation[]): Promise; @@ -111,7 +112,25 @@ export const applyRemoteOperations = async < } }); - const applyResult = await applier.applyOperations(appendResult.writtenOps); + const applyResult = await applier.applyOperations(appendResult.writtenOps, { + onReducersCommitted: async (reducerCommittedOps) => { + const reducerCommittedSeqs = reducerCommittedOps + .map((op) => opIdToSeq.get(op.id)) + .filter((seq): seq is number => seq !== undefined); + if (reducerCommittedSeqs.length !== reducerCommittedOps.length) { + throw new Error( + 'applyRemoteOperations: reducer commit contained an operation outside the appended batch.', + ); + } + await store.markArchivePending(reducerCommittedSeqs); + await store.mergeRemoteOpClocks(reducerCommittedOps); + }, + }); + if (applyResult.failedOp && !opIdToSeq.has(applyResult.failedOp.op.id)) { + throw new Error( + `applyRemoteOperations: applier reported unknown failed op ${applyResult.failedOp.op.id}.`, + ); + } const appliedSeqs = applyResult.appliedOps .map((op) => opIdToSeq.get(op.id)) .filter((seq): seq is number => seq !== undefined); @@ -120,7 +139,6 @@ export const applyRemoteOperations = async < if (appliedSeqs.length > 0) { await store.markApplied(appliedSeqs); - await store.mergeRemoteOpClocks(applyResult.appliedOps); const appliedFullStateOpIds = applyResult.appliedOps .filter(isFullStateOperation) @@ -132,9 +150,7 @@ export const applyRemoteOperations = async < } } - const failedOpIds = applyResult.failedOp - ? getFailedOpIds(appendResult.writtenOps, applyResult.failedOp.op) - : []; + const failedOpIds = applyResult.failedOp ? [applyResult.failedOp.op.id] : []; if (failedOpIds.length > 0) { await store.markFailed(failedOpIds); @@ -150,12 +166,3 @@ export const applyRemoteOperations = async < failedOpIds, }; }; - -const getFailedOpIds = >( - opsToApply: TOperation[], - failedOp: TOperation, -): string[] => { - const failedOpIndex = opsToApply.findIndex((op) => op.id === failedOp.id); - const failedOps = failedOpIndex === -1 ? [failedOp] : opsToApply.slice(failedOpIndex); - return failedOps.map((op) => op.id); -}; diff --git a/packages/sync-core/src/replay-coordinator.ts b/packages/sync-core/src/replay-coordinator.ts index 567c5374af..57792e3d47 100644 --- a/packages/sync-core/src/replay-coordinator.ts +++ b/packages/sync-core/src/replay-coordinator.ts @@ -35,6 +35,7 @@ export interface OperationReplayCoordinatorOptions< operationToAction?: (op: TOperation) => TReplayAction; isArchiveAffectingAction?: (action: TReplayAction) => boolean; onRemoteArchiveDataApplied?: () => void; + onReducersCommitted?: (ops: TOperation[]) => Promise; onArchiveSideEffectError?: ( context: OperationReplayArchiveFailureContext, ) => void; @@ -94,6 +95,7 @@ export const replayOperationBatch = async < operationToAction, isArchiveAffectingAction = () => false, onRemoteArchiveDataApplied, + onReducersCommitted, onArchiveSideEffectError, onPostSyncCooldownError, yieldToEventLoop: waitForEventLoop = yieldToEventLoop, @@ -118,6 +120,7 @@ export const replayOperationBatch = async < if (!skipReducerDispatch) { dispatcher.dispatch(createBulkApplyAction(ops)); await waitForEventLoop(); + await onReducersCommitted?.(ops); } if (!isLocalHydration && archiveSideEffects !== undefined && operationToAction) { diff --git a/packages/sync-core/tests/remote-apply.spec.ts b/packages/sync-core/tests/remote-apply.spec.ts index b2aca4f786..9aa8e29a06 100644 --- a/packages/sync-core/tests/remote-apply.spec.ts +++ b/packages/sync-core/tests/remote-apply.spec.ts @@ -22,6 +22,7 @@ const createStore = (appendResult: { skippedCount: number; }): RemoteOperationApplyStorePort> => ({ appendBatchSkipDuplicates: vi.fn().mockResolvedValue(appendResult), + markArchivePending: vi.fn().mockResolvedValue(undefined), markApplied: vi.fn().mockResolvedValue(undefined), markFailed: vi.fn().mockResolvedValue(undefined), mergeRemoteOpClocks: vi.fn().mockResolvedValue(undefined), @@ -31,7 +32,10 @@ const createStore = (appendResult: { const createApplier = ( result: Awaited>['applyOperations']>>, ): OperationApplyPort> => ({ - applyOperations: vi.fn().mockResolvedValue(result), + applyOperations: vi.fn(async (ops, options) => { + await options?.onReducersCommitted?.(ops); + return result; + }), }); describe('applyRemoteOperations', () => { @@ -50,7 +54,11 @@ describe('applyRemoteOperations', () => { expect(store.appendBatchSkipDuplicates).toHaveBeenCalledWith([op1, op2], 'remote', { pendingApply: true, }); - expect(applier.applyOperations).toHaveBeenCalledWith([op2]); + expect(applier.applyOperations).toHaveBeenCalledWith( + [op2], + jasmineLikeObjectContainingFunction(), + ); + expect(store.markArchivePending).toHaveBeenCalledWith([10]); expect(store.markApplied).toHaveBeenCalledWith([10]); expect(store.mergeRemoteOpClocks).toHaveBeenCalledWith([op2]); expect(result).toEqual({ @@ -104,7 +112,7 @@ describe('applyRemoteOperations', () => { expect(result.clearedFullStateOpCount).toBe(3); }); - it('marks the failed op and remaining unapplied ops as failed', async () => { + it('checkpoints the whole reducer batch but charges only the attempted archive failure', async () => { const op1 = createOperation('op-1'); const op2 = createOperation('op-2'); const op3 = createOperation('op-3'); @@ -126,13 +134,14 @@ describe('applyRemoteOperations', () => { }); expect(store.markApplied).toHaveBeenCalledWith([1]); - expect(store.mergeRemoteOpClocks).toHaveBeenCalledWith([op1]); - expect(store.markFailed).toHaveBeenCalledWith(['op-2', 'op-3']); + expect(store.markArchivePending).toHaveBeenCalledWith([1, 2, 3]); + expect(store.mergeRemoteOpClocks).toHaveBeenCalledWith([op1, op2, op3]); + expect(store.markFailed).toHaveBeenCalledWith(['op-2']); expect(result.failedOp).toEqual({ op: op2, error }); - expect(result.failedOpIds).toEqual(['op-2', 'op-3']); + expect(result.failedOpIds).toEqual(['op-2']); }); - it('marks only the reported failed op if it is not in the appended batch', async () => { + it('rejects an applier result whose failed op is not in the appended batch', async () => { const op1 = createOperation('op-1'); const unknownFailedOp = createOperation('unknown-failed-op'); const store = createStore({ seqs: [1], writtenOps: [op1], skippedCount: 0 }); @@ -141,9 +150,12 @@ describe('applyRemoteOperations', () => { failedOp: { op: unknownFailedOp, error: new Error('unexpected') }, }); - const result = await applyRemoteOperations({ ops: [op1], store, applier }); - - expect(store.markFailed).toHaveBeenCalledWith(['unknown-failed-op']); - expect(result.failedOpIds).toEqual(['unknown-failed-op']); + await expect(applyRemoteOperations({ ops: [op1], store, applier })).rejects.toThrow( + 'unknown-failed-op', + ); + expect(store.markFailed).not.toHaveBeenCalled(); }); }); + +const jasmineLikeObjectContainingFunction = (): object => + expect.objectContaining({ onReducersCommitted: expect.any(Function) }); diff --git a/packages/sync-core/tests/replay-coordinator.spec.ts b/packages/sync-core/tests/replay-coordinator.spec.ts index a94dc95621..0b866de933 100644 --- a/packages/sync-core/tests/replay-coordinator.spec.ts +++ b/packages/sync-core/tests/replay-coordinator.spec.ts @@ -69,6 +69,9 @@ describe('replayOperationBatch', () => { const onRemoteArchiveDataApplied = vi.fn(() => { callOrder.push('remoteArchiveDataApplied'); }); + const onReducersCommitted = vi.fn(async () => { + callOrder.push('reducersCommitted'); + }); const result = await replayOperationBatch({ ops, @@ -87,6 +90,7 @@ describe('replayOperationBatch', () => { }), isArchiveAffectingAction: (action) => action.archiveAffecting === true, onRemoteArchiveDataApplied, + onReducersCommitted, yieldToEventLoop, }); @@ -101,6 +105,7 @@ describe('replayOperationBatch', () => { 'startApplyingRemoteOps', 'dispatchBulk', 'yield', + 'reducersCommitted', 'archive:op-1', 'archive:op-2', 'yield', diff --git a/src/app/imex/sync/sync-wrapper.service.spec.ts b/src/app/imex/sync/sync-wrapper.service.spec.ts index 01562c659f..cc09584fdf 100644 --- a/src/app/imex/sync/sync-wrapper.service.spec.ts +++ b/src/app/imex/sync/sync-wrapper.service.spec.ts @@ -583,6 +583,19 @@ describe('SyncWrapperService', () => { expect(mockProviderManager.setLastSyncedProviderId).not.toHaveBeenCalled(); }); + + it('should stop with ERROR when download is blocked by an incompatible op', async () => { + mockSyncService.downloadRemoteOps.and.resolveTo({ + kind: 'blocked_incompatible', + }); + + const result = await service.sync(); + + expect(result).toBe('HANDLED_ERROR'); + expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith('ERROR'); + expect(mockProviderManager.setLastSyncedProviderId).not.toHaveBeenCalled(); + expect(mockSyncService.uploadPendingOps).not.toHaveBeenCalled(); + }); }); describe('_sync() - Sync flow', () => { @@ -1000,6 +1013,17 @@ describe('SyncWrapperService', () => { expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith('IN_SYNC'); }); + it('should stop with ERROR when upload piggyback is blocked by an incompatible op', async () => { + mockSyncService.uploadPendingOps.and.resolveTo({ + kind: 'blocked_incompatible', + }); + + const result = await service.sync(); + + expect(result).toBe('HANDLED_ERROR'); + expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith('ERROR'); + }); + it('should set IN_SYNC when permanentRejectionCount is 0 even with empty rejectedOps array', async () => { mockSyncService.uploadPendingOps.and.returnValue( Promise.resolve({ diff --git a/src/app/imex/sync/sync-wrapper.service.ts b/src/app/imex/sync/sync-wrapper.service.ts index d98d8e213f..0815ca6cbf 100644 --- a/src/app/imex/sync/sync-wrapper.service.ts +++ b/src/app/imex/sync/sync-wrapper.service.ts @@ -506,6 +506,13 @@ export class SyncWrapperService { this._providerManager.setSyncStatus('UNKNOWN_OR_CHANGED'); return 'HANDLED_ERROR'; } + if (downloadResult.kind === 'blocked_incompatible') { + SyncLog.warn( + 'SyncWrapperService: Download blocked by an incompatible operation.', + ); + this._providerManager.setSyncStatus('ERROR'); + return 'HANDLED_ERROR'; + } // Track the successfully synced provider for switch detection on next sync this._providerManager.setLastSyncedProviderId(providerId); @@ -515,6 +522,13 @@ export class SyncWrapperService { syncCapableProvider, { isNeverSynced: isNeverSyncedAtSyncStart }, ); + if (uploadResult.kind === 'blocked_incompatible') { + SyncLog.warn( + 'SyncWrapperService: Upload piggyback blocked by an incompatible operation.', + ); + this._providerManager.setSyncStatus('ERROR'); + return 'HANDLED_ERROR'; + } const completedUploadResults: CompletedUploadOutcome[] = uploadResult.kind === 'completed' ? [uploadResult] : []; if (uploadResult.kind === 'completed') { @@ -582,6 +596,13 @@ export class SyncWrapperService { this._providerManager.setSyncStatus('UNKNOWN_OR_CHANGED'); return 'HANDLED_ERROR'; } + if (reuploadResult.kind === 'blocked_incompatible') { + SyncLog.warn( + 'SyncWrapperService: LWW re-upload blocked by an incompatible operation.', + ); + this._providerManager.setSyncStatus('ERROR'); + return 'HANDLED_ERROR'; + } if (reuploadResult.kind === 'completed') { completedUploadResults.push(reuploadResult); } diff --git a/src/app/op-log/apply/operation-applier.service.ts b/src/app/op-log/apply/operation-applier.service.ts index 4f0dc8b4c1..186b235e4c 100644 --- a/src/app/op-log/apply/operation-applier.service.ts +++ b/src/app/op-log/apply/operation-applier.service.ts @@ -129,6 +129,7 @@ export class OperationApplierService implements OperationApplyPort { // this action is now disabled to prevent UI freezes during bulk archive sync. this.store.dispatch(remoteArchiveDataApplied()); }, + onReducersCommitted: options.onReducersCommitted, onArchiveSideEffectError: ({ op, processedCount, error }) => { OpLog.err( `OperationApplierService: Failed archive handling for operation ${op.id}. ` + diff --git a/src/app/op-log/backup/state-snapshot.service.spec.ts b/src/app/op-log/backup/state-snapshot.service.spec.ts index c0eddc9437..0421722ee5 100644 --- a/src/app/op-log/backup/state-snapshot.service.spec.ts +++ b/src/app/op-log/backup/state-snapshot.service.spec.ts @@ -20,6 +20,7 @@ import { selectPluginMetadataFeatureState } from '../../plugins/store/plugin-met import { selectReminderFeatureState } from '../../features/reminder/store/reminder.reducer'; import { ArchiveModel } from '../../features/time-tracking/time-tracking.model'; import { initialTimeTrackingState } from '../../features/time-tracking/store/time-tracking.reducer'; +import { TaskState } from '../../features/tasks/task.model'; describe('StateSnapshotService', () => { let service: StateSnapshotService; @@ -225,6 +226,32 @@ describe('StateSnapshotService', () => { expect(archiveDbAdapterSpy.loadArchiveYoung).toHaveBeenCalledTimes(1); expect(archiveDbAdapterSpy.loadArchiveOld).toHaveBeenCalledTimes(1); }); + + it('should capture NgRx state before awaiting archive I/O', async () => { + let releaseArchives!: () => void; + const archiveGate = new Promise((resolve) => { + releaseArchives = resolve; + }); + archiveDbAdapterSpy.loadArchiveYoung.and.callFake(async () => { + await archiveGate; + return mockArchiveYoung; + }); + archiveDbAdapterSpy.loadArchiveOld.and.callFake(async () => { + await archiveGate; + return mockArchiveOld; + }); + + const snapshotPromise = service.getStateSnapshotAsync(); + store.overrideSelector(selectTaskFeatureState, { + ...mockTaskState, + ids: ['later-task'], + } as unknown as TaskState); + store.refreshState(); + releaseArchives(); + + const snapshot = await snapshotPromise; + expect((snapshot.task as TaskState).ids).toEqual(['task1']); + }); }); describe('backward compatibility aliases', () => { diff --git a/src/app/op-log/backup/state-snapshot.service.ts b/src/app/op-log/backup/state-snapshot.service.ts index 119b92232d..ce3f125622 100644 --- a/src/app/op-log/backup/state-snapshot.service.ts +++ b/src/app/op-log/backup/state-snapshot.service.ts @@ -1,6 +1,5 @@ import { inject, Injectable } from '@angular/core'; import { Selector, Store } from '@ngrx/store'; -import { combineLatest, firstValueFrom } from 'rxjs'; import { first } from 'rxjs/operators'; import { selectBoardsState } from '../../features/boards/store/boards.selectors'; @@ -135,24 +134,18 @@ export class StateSnapshotService { * ⚠️ Returns live store references — never mutate (clone first). See class doc. */ async getStateSnapshotAsync(): Promise { + // Capture NgRx synchronously before the first await. Callers that hold the + // operation-log barrier can now establish an exact reducer-state cutoff: + // actions dispatched while archive I/O is pending are not half-included in + // the snapshot and will be persisted after it. + const ngRxData = this._getNgRxDataSync(); const [archiveYoung, archiveOld] = await Promise.all([ this._loadArchive('archiveYoung'), this._loadArchive('archiveOld'), ]); - const ngRxValues = await firstValueFrom( - combineLatest(SNAPSHOT_SELECTORS.map((s) => this._store.select(s.selector))).pipe( - first(), - ), - ); - - const result: Partial> = {}; - for (let i = 0; i < SNAPSHOT_SELECTORS.length; i++) { - result[SNAPSHOT_SELECTORS[i].key] = ngRxValues[i]; - } - return { - ...this._normalizeNgRxData(result), + ...ngRxData, archiveYoung, archiveOld, }; diff --git a/src/app/op-log/capture/operation-capture.meta-reducer.spec.ts b/src/app/op-log/capture/operation-capture.meta-reducer.spec.ts index 21d9b4d1ea..44a57dc990 100644 --- a/src/app/op-log/capture/operation-capture.meta-reducer.spec.ts +++ b/src/app/op-log/capture/operation-capture.meta-reducer.spec.ts @@ -249,7 +249,7 @@ describe('operationCaptureMetaReducer', () => { expect(buffered).toEqual([]); }); - it('should clear buffer after returning actions', () => { + it('should return a non-destructive snapshot until actions are acknowledged', () => { const action = createMockAction(); bufferDeferredAction(action); @@ -257,7 +257,7 @@ describe('operationCaptureMetaReducer', () => { const secondCall = getDeferredActions(); expect(firstCall).toEqual([action]); - expect(secondCall).toEqual([]); + expect(secondCall).toEqual([action]); }); }); @@ -329,7 +329,7 @@ describe('operationCaptureMetaReducer', () => { expect(getDeferredActions()).toEqual(actions); }); - it('should drop the oldest action only past the pathological hard cap', () => { + it('should retain every accepted action past the pathological warning cap', () => { const confirmSpy = spyNativeDialogs(); const actions = createManyActions(DEFERRED_ACTIONS_PATHOLOGICAL_HARD_CAP); const dropErrorCalls = (): unknown[][] => @@ -345,9 +345,8 @@ describe('operationCaptureMetaReducer', () => { expect(dropErrorCalls().length).toBe(1); const buffered = getDeferredActions(); - expect(buffered.length).toBe(DEFERRED_ACTIONS_PATHOLOGICAL_HARD_CAP); - // Oldest action was dropped, remaining order intact - expect(buffered[0]).toBe(actions[1]); + expect(buffered.length).toBe(DEFERRED_ACTIONS_PATHOLOGICAL_HARD_CAP + 1); + expect(buffered[0]).toBe(actions[0]); expect(buffered[buffered.length - 1]).toBe(extraAction); }); }); diff --git a/src/app/op-log/capture/operation-capture.meta-reducer.ts b/src/app/op-log/capture/operation-capture.meta-reducer.ts index 31a49d714c..eefd5476ee 100644 --- a/src/app/op-log/capture/operation-capture.meta-reducer.ts +++ b/src/app/op-log/capture/operation-capture.meta-reducer.ts @@ -148,13 +148,12 @@ const DEFERRED_ACTIONS_SOFT_WARNING_THRESHOLD = 10; export const DEFERRED_ACTIONS_RELOAD_WARNING_THRESHOLD = 100; /** - * Pathological hard cap for the deferred actions buffer — purely a memory - * backstop against a runaway dispatch loop. ~100 buffered actions are + * Pathological high-water mark for the deferred actions buffer. ~100 buffered actions are * plausibly reachable by a real user during a stuck/very long remote-apply * window, so we must never drop there (see reload-warning threshold above, - * which tells the user to reload long before this cap can be hit through - * real interaction). Only past this cap is drop-oldest the lesser evil vs - * unbounded memory growth. + * which tells the user to reload long before this mark can be hit through + * real interaction). Crossing it logs loudly but still cannot discard an + * action whose reducer change has already been accepted. * Exported for tests. */ export const DEFERRED_ACTIONS_PATHOLOGICAL_HARD_CAP = 5000; @@ -164,19 +163,15 @@ export const DEFERRED_ACTIONS_PATHOLOGICAL_HARD_CAP = 5000; * Called by the meta-reducer when a persistent action arrives during sync. */ export const bufferDeferredAction = (action: PersistentAction): void => { - // Pathological cap only: dropping an accepted persistent action means its - // state mutation survives in NgRx while no operation will ever represent - // it → permanent, unsyncable divergence. So this drop exists solely as a - // memory backstop against a runaway dispatch loop, never as flow control. - // NOTE: The shifted action remains in deferredActionSet (WeakSet has no - // delete-by-value). The effect filters it via isDeferredAction(), and - // getDeferredActions() won't return it, so it is silently lost. + // Never drop an accepted persistent action: its reducer mutation already + // exists in NgRx, so removal here would create permanent unsyncable state. + // The cap remains a loud runaway-loop diagnostic, not a lossy flow-control + // mechanism. if (deferredActions.length >= DEFERRED_ACTIONS_PATHOLOGICAL_HARD_CAP) { devError( `[operationCaptureMetaReducer] Deferred actions buffer exceeded pathological cap of ${DEFERRED_ACTIONS_PATHOLOGICAL_HARD_CAP} items. ` + - `Dropping oldest action - this local change will NOT sync. Likely a runaway dispatch loop; reload the app.`, + 'Retaining all actions to preserve sync correctness. Likely a runaway dispatch loop; reload the app.', ); - deferredActions.shift(); } deferredActions.push(action); @@ -195,25 +190,32 @@ export const bufferDeferredAction = (action: PersistentAction): void => { }; /** - * Gets and clears the deferred actions buffer. - * Called after sync completes to process buffered actions. + * Returns a stable snapshot of the deferred actions buffer. Entries remain queued + * until their operation is durably written and explicitly acknowledged. */ export const getDeferredActions = (): PersistentAction[] => { - const actions = deferredActions; - deferredActions = []; - return actions; + return [...deferredActions]; +}; + +export const acknowledgeDeferredAction = (action: PersistentAction): void => { + const index = deferredActions.indexOf(action); + if (index === -1) { + return; + } + deferredActions.splice(index, 1); + deferredActionSet.delete(action); }; /** * Clears the deferred actions buffer without processing. * Used for cleanup during testing or error recovery. * - * Note: deferredActionSet (WeakSet) is not cleared here because WeakSet has - * no .clear() method. Entries are garbage-collected when action references are - * released. In practice, cleared actions are not reused by reference, so stale - * entries in the WeakSet are harmless. + * WeakSet has no clear(), so delete the known buffered references individually. */ export const clearDeferredActions = (): void => { + for (const action of deferredActions) { + deferredActionSet.delete(action); + } deferredActions = []; }; diff --git a/src/app/op-log/capture/operation-log.effects.spec.ts b/src/app/op-log/capture/operation-log.effects.spec.ts index 59a88e40c9..b8881bb659 100644 --- a/src/app/op-log/capture/operation-log.effects.spec.ts +++ b/src/app/op-log/capture/operation-log.effects.spec.ts @@ -16,6 +16,7 @@ import { COMPACTION_THRESHOLD } from '../core/operation-log.const'; import { bufferDeferredAction, clearDeferredActions, + getDeferredActions, } from './operation-capture.meta-reducer'; import { ClientIdService } from '../../core/util/client-id.service'; import { OperationCaptureService } from './operation-capture.service'; @@ -876,8 +877,31 @@ describe('OperationLogEffects', () => { // Should not throw - errors are logged but don't stop processing await expectAsync(effects.processDeferredActions()).toBeResolved(); - // Both actions should have been attempted - expect(mockOpLogStore.appendWithVectorClockUpdate).toHaveBeenCalledTimes(2); + // The failed write is retried once, then processing continues to action 2. + expect(mockOpLogStore.appendWithVectorClockUpdate).toHaveBeenCalledTimes(3); + }); + + it('should keep an exhausted deferred write queued while acknowledging later successes', async () => { + const failedAction = createPersistentAction(ActionType.TASK_SHARED_ADD); + const successfulAction = createPersistentAction(ActionType.TASK_SHARED_UPDATE); + bufferDeferredAction(failedAction); + bufferDeferredAction(successfulAction); + let attempts = 0; + mockOpLogStore.appendWithVectorClockUpdate.and.callFake(() => { + attempts++; + return attempts <= 3 + ? Promise.reject(new Error('persistent failure')) + : Promise.resolve(1); + }); + + await effects.processDeferredActions(); + + expect(getDeferredActions()).toEqual([failedAction]); + + mockOpLogStore.appendWithVectorClockUpdate.and.resolveTo(2); + await effects.processDeferredActions(); + + expect(getDeferredActions()).toEqual([]); }); it('should use fresh vector clock for deferred actions', async () => { diff --git a/src/app/op-log/capture/operation-log.effects.ts b/src/app/op-log/capture/operation-log.effects.ts index a7097573fa..8b68de3511 100644 --- a/src/app/op-log/capture/operation-log.effects.ts +++ b/src/app/op-log/capture/operation-log.effects.ts @@ -28,7 +28,11 @@ import { import { CURRENT_SCHEMA_VERSION } from '../persistence/schema-migration.service'; import { OperationCaptureService } from './operation-capture.service'; import { ImmediateUploadService } from '../sync/immediate-upload.service'; -import { getDeferredActions, isDeferredAction } from './operation-capture.meta-reducer'; +import { + acknowledgeDeferredAction, + getDeferredActions, + isDeferredAction, +} from './operation-capture.meta-reducer'; import { ClientIdService } from '../../core/util/client-id.service'; import { SuperSyncStatusService } from '../sync/super-sync-status.service'; @@ -181,6 +185,9 @@ export class OperationLogEffects implements DeferredLocalActionsPort { devError( `[OperationLogEffects] Action ${action.type} has invalid entityId/entityIds (${action.meta.entityId}) - skipping persistence`, ); + if (isDeferredWrite) { + throw new Error(`Deferred action ${action.type} has invalid entity identifiers.`); + } return; } @@ -292,6 +299,9 @@ export class OperationLogEffects implements DeferredLocalActionsPort { window.location.reload(); }, }); + if (isDeferredWrite) { + throw new Error(`Deferred action ${action.type} has an invalid payload.`); + } return; // Skip persisting invalid operation } @@ -378,10 +388,12 @@ export class OperationLogEffects implements DeferredLocalActionsPort { this.notifyUserAndTriggerRollback(); } else { await this.handleQuotaExceeded(action, isDeferredWrite, options); + return; } } else { this.notifyUserAndTriggerRollback(); } + throw e; } } @@ -512,6 +524,7 @@ export class OperationLogEffects implements DeferredLocalActionsPort { // Use lock for cross-tab coordination - only one tab handles quota at a time let bailReason: Error | null = null; + let recovered = false; await this.lockService.request('sp_quota_exceeded', async () => { if (options.callerHoldsOperationLogLock) { OpLog.err( @@ -541,6 +554,7 @@ export class OperationLogEffects implements DeferredLocalActionsPort { // extractEntityChanges() inside writeOperation, so the retry simply // re-extracts the same changes. Pass isDeferredWrite through unchanged. await this.writeOperation(action, isDeferredWrite, options); + recovered = true; this.snackService.open({ type: 'SUCCESS', msg: T.F.SYNC.S.STORAGE_RECOVERED_AFTER_COMPACTION, @@ -562,6 +576,10 @@ export class OperationLogEffects implements DeferredLocalActionsPort { if (bailReason !== null) { throw bailReason; } + if (recovered) { + return; + } + throw new Error('Storage quota recovery failed; operation was not persisted.'); } private showStorageQuotaExceededError(): void { @@ -646,8 +664,13 @@ export class OperationLogEffects implements DeferredLocalActionsPort { // Log error after all retries exhausted, continue processing remaining actions OpLog.err( `OperationLogEffects: Failed to process deferred action after ${MAX_RETRIES} retries`, - { actionType: action.type, error: lastError }, + { + actionType: action.type, + errorName: (lastError as Error | undefined)?.name, + }, ); + } else { + acknowledgeDeferredAction(action); } } diff --git a/src/app/op-log/core/types/apply.types.ts b/src/app/op-log/core/types/apply.types.ts index 7c2e72f24b..d27fc3cf2c 100644 --- a/src/app/op-log/core/types/apply.types.ts +++ b/src/app/op-log/core/types/apply.types.ts @@ -41,4 +41,11 @@ export interface ApplyOperationsOptions { * increaseSimpleCounterCounterToday. */ skipReducerDispatch?: boolean; + + /** + * Called after the bulk reducer dispatch commits and before archive side effects. + * Remote apply uses this to persist its reducer/archive checkpoint and merge the + * causal frontier before deferred local actions can be written. + */ + onReducersCommitted?: (ops: Operation[]) => Promise; } diff --git a/src/app/op-log/core/types/sync-results.types.ts b/src/app/op-log/core/types/sync-results.types.ts index f684fe756b..1ef8e4ea7b 100644 --- a/src/app/op-log/core/types/sync-results.types.ts +++ b/src/app/op-log/core/types/sync-results.types.ts @@ -1,4 +1,4 @@ -import { Operation, VectorClock } from '../operation.types'; +import { Operation, OperationLogEntry, VectorClock } from '../operation.types'; import { OperationSyncProviderMode } from '../../sync-providers/provider.interface'; /** @@ -106,6 +106,13 @@ export interface UploadResult { piggybackedOps: Operation[]; rejectedCount: number; rejectedOps: RejectedOpInfo[]; + /** Exact in-lock pending set considered by this upload round. */ + selectedPendingOps?: OperationLogEntry[]; + /** + * Accepted/local-only operation sequences whose acknowledgement was deliberately + * deferred until the caller has resolved and applied piggybacked operations. + */ + pendingAcknowledgementSeqs?: number[]; /** * Number of local-win update ops created during LWW conflict resolution. * These ops need to be uploaded to propagate local state to other clients. @@ -171,6 +178,13 @@ export interface UploadOptions { */ preUploadCallback?: () => Promise; + /** + * Return accepted sequence numbers to the caller instead of marking them synced + * immediately. Required when piggybacked full-state operations may need user + * resolution before the upload round is considered committed locally. + */ + deferAcknowledgement?: boolean; + /** * If true, instructs server to delete all existing user data before accepting uploaded operations. * Used for clean slate operations like encryption password changes or full imports. @@ -254,6 +268,10 @@ export type DownloadOutcome = | { /** User cancelled a SYNC_IMPORT conflict dialog. */ kind: 'cancelled'; + } + | { + /** Processing stopped at an op this app version cannot interpret safely. */ + kind: 'blocked_incompatible'; }; /** @@ -284,4 +302,8 @@ export type UploadOutcome = | { /** User cancelled a piggybacked SYNC_IMPORT conflict dialog. */ kind: 'cancelled'; + } + | { + /** Piggyback processing stopped at an incompatible operation. */ + kind: 'blocked_incompatible'; }; diff --git a/src/app/op-log/persistence/compact/compact-operation.types.ts b/src/app/op-log/persistence/compact/compact-operation.types.ts index d51976ced4..52ee6eba6a 100644 --- a/src/app/op-log/persistence/compact/compact-operation.types.ts +++ b/src/app/op-log/persistence/compact/compact-operation.types.ts @@ -65,6 +65,6 @@ export interface CompactOperationLogEntry { source: 'local' | 'remote'; syncedAt?: number; rejectedAt?: number; - applicationStatus?: 'pending' | 'applied' | 'failed'; + applicationStatus?: 'pending' | 'archive_pending' | 'applied' | 'failed'; retryCount?: number; } diff --git a/src/app/op-log/persistence/operation-log-hydrator.service.spec.ts b/src/app/op-log/persistence/operation-log-hydrator.service.spec.ts index 6c77d85278..308be9126e 100644 --- a/src/app/op-log/persistence/operation-log-hydrator.service.spec.ts +++ b/src/app/op-log/persistence/operation-log-hydrator.service.spec.ts @@ -1450,7 +1450,7 @@ describe('OperationLogHydratorService', () => { expect(mockOpLogStore.markApplied).toHaveBeenCalledWith([40, 41, 42]); }); - it('should mark the failed op and every op after it (in seq order) as still-failing', async () => { + it('should charge retry budget only to the attempted archive failure', async () => { const entries = [ failedEntry(40, 'op-a'), failedEntry(41, 'op-b'), @@ -1469,10 +1469,9 @@ describe('OperationLogHydratorService', () => { await service.retryFailedRemoteOps(); expect(mockOpLogStore.markApplied).toHaveBeenCalledWith([40]); - // op-b (failed) and op-c (after it) are both marked failed, with the - // retry-cap so a permanently-failing op is eventually rejected. + // op-c remains archive-pending and has not consumed a retry attempt. expect(mockOpLogStore.markFailed).toHaveBeenCalledWith( - ['op-b', 'op-c'], + ['op-b'], MAX_CONFLICT_RETRY_ATTEMPTS, ); }); diff --git a/src/app/op-log/persistence/operation-log-hydrator.service.ts b/src/app/op-log/persistence/operation-log-hydrator.service.ts index e35345841e..e972905af4 100644 --- a/src/app/op-log/persistence/operation-log-hydrator.service.ts +++ b/src/app/op-log/persistence/operation-log-hydrator.service.ts @@ -22,7 +22,6 @@ import { ValidateStateService } from '../validation/validate-state.service'; import { OperationApplierService } from '../apply/operation-applier.service'; import { HydrationStateService } from '../apply/hydration-state.service'; import { bulkApplyOperations } from '../apply/bulk-hydration.action'; -import { getFailedOpIdsFromBatch } from '../apply/failed-op-ids.util'; import { VectorClockService } from '../sync/vector-clock.service'; import { MAX_CONFLICT_RETRY_ATTEMPTS } from '../core/operation-log.const'; import { AppDataComplete } from '../model/model-config'; @@ -629,22 +628,20 @@ export class OperationLogHydratorService { ); } - // On a partial failure the batch applier stops at the first op whose archive - // side effect throws and returns it; that op and every op after it in seq - // order stay unapplied (slice-from-failure, shared with the primary path). - // markFailed bumps the retry count (rejecting ops past - // MAX_CONFLICT_RETRY_ATTEMPTS), so a permanently-failing op can't be retried - // forever. + // On a partial failure the batch applier stops at the first archive error. + // Charge only that attempted operation: successors remain archive-pending + // without consuming retry budget and will run after the blocker succeeds or + // becomes terminally rejected. if (result.failedOp) { - const stillFailedOpIds = getFailedOpIdsFromBatch(opsToApply, result.failedOp.op); + const failedOpIds = [result.failedOp.op.id]; OpLog.warn( `OperationLogHydratorService: Failed to retry op ${result.failedOp.op.id}`, result.failedOp.error, ); - await this.opLogStore.markFailed(stillFailedOpIds, MAX_CONFLICT_RETRY_ATTEMPTS); + await this.opLogStore.markFailed(failedOpIds, MAX_CONFLICT_RETRY_ATTEMPTS); OpLog.warn( - `OperationLogHydratorService: ${stillFailedOpIds.length} ops still failing after retry`, + 'OperationLogHydratorService: Archive operation still failing after retry', ); } } diff --git a/src/app/op-log/persistence/operation-log-recovery.service.spec.ts b/src/app/op-log/persistence/operation-log-recovery.service.spec.ts index 61be77bb5f..76a9bc50fa 100644 --- a/src/app/op-log/persistence/operation-log-recovery.service.spec.ts +++ b/src/app/op-log/persistence/operation-log-recovery.service.spec.ts @@ -26,6 +26,7 @@ describe('OperationLogRecoveryService', () => { 'getPendingRemoteOps', 'markRejected', 'markApplied', + 'markArchivePending', 'getUnsynced', ]); mockOpLogStore.setVectorClock.and.resolveTo(undefined); @@ -183,22 +184,23 @@ describe('OperationLogRecoveryService', () => { await service.recoverPendingRemoteOps(); - expect(mockOpLogStore.markApplied).not.toHaveBeenCalled(); + expect(mockOpLogStore.markArchivePending).not.toHaveBeenCalled(); expect(mockOpLogStore.markRejected).not.toHaveBeenCalled(); }); - it('should mark valid pending ops as applied', async () => { + it('should mark valid crash-interrupted ops as archive-pending', async () => { const now = Date.now(); const pendingOps = [ { seq: 1, op: { id: 'op1' }, appliedAt: now - 1000, source: 'remote' }, { seq: 2, op: { id: 'op2' }, appliedAt: now - 2000, source: 'remote' }, ] as any; mockOpLogStore.getPendingRemoteOps.and.resolveTo(pendingOps); - mockOpLogStore.markApplied.and.resolveTo(undefined); + mockOpLogStore.markArchivePending.and.resolveTo(undefined); await service.recoverPendingRemoteOps(); - expect(mockOpLogStore.markApplied).toHaveBeenCalledWith([1, 2]); + expect(mockOpLogStore.markArchivePending).toHaveBeenCalledWith([1, 2]); + expect(mockOpLogStore.markApplied).not.toHaveBeenCalled(); }); it('should reject ops that exceed PENDING_OPERATION_EXPIRY_MS', async () => { @@ -213,12 +215,12 @@ describe('OperationLogRecoveryService', () => { }, // Expired ] as any; mockOpLogStore.getPendingRemoteOps.and.resolveTo(pendingOps); - mockOpLogStore.markApplied.and.resolveTo(undefined); + mockOpLogStore.markArchivePending.and.resolveTo(undefined); mockOpLogStore.markRejected.and.resolveTo(undefined); await service.recoverPendingRemoteOps(); - expect(mockOpLogStore.markApplied).toHaveBeenCalledWith([1]); + expect(mockOpLogStore.markArchivePending).toHaveBeenCalledWith([1]); expect(mockOpLogStore.markRejected).toHaveBeenCalledWith(['expired']); }); @@ -235,7 +237,7 @@ describe('OperationLogRecoveryService', () => { await service.recoverPendingRemoteOps(); expect(mockOpLogStore.markRejected).toHaveBeenCalledWith(['old1', 'old2']); - expect(mockOpLogStore.markApplied).not.toHaveBeenCalled(); + expect(mockOpLogStore.markArchivePending).not.toHaveBeenCalled(); }); it('should handle mixed valid and expired ops correctly', async () => { @@ -257,12 +259,12 @@ describe('OperationLogRecoveryService', () => { }, ] as any; mockOpLogStore.getPendingRemoteOps.and.resolveTo(pendingOps); - mockOpLogStore.markApplied.and.resolveTo(undefined); + mockOpLogStore.markArchivePending.and.resolveTo(undefined); mockOpLogStore.markRejected.and.resolveTo(undefined); await service.recoverPendingRemoteOps(); - expect(mockOpLogStore.markApplied).toHaveBeenCalledWith([1, 3]); + expect(mockOpLogStore.markArchivePending).toHaveBeenCalledWith([1, 3]); expect(mockOpLogStore.markRejected).toHaveBeenCalledWith(['expired1', 'expired2']); }); }); diff --git a/src/app/op-log/persistence/operation-log-recovery.service.ts b/src/app/op-log/persistence/operation-log-recovery.service.ts index 0c66bc0daf..47d40c8020 100644 --- a/src/app/op-log/persistence/operation-log-recovery.service.ts +++ b/src/app/op-log/persistence/operation-log-recovery.service.ts @@ -136,8 +136,10 @@ export class OperationLogRecoveryService { /** * Recovers from pending remote ops that were stored but not applied (crash recovery). - * These ops are in the log and will be replayed during normal hydration, so we just - * need to mark them as applied to prevent them appearing as orphaned. + * These ops are replayed through reducers during normal hydration, but a crash may + * have happened before their archive side effects completed. Move them to the + * archive-pending checkpoint so hydration retries archive work without double-applying + * reducers. * * Operations pending for longer than PENDING_OPERATION_EXPIRY_MS are considered * superseded (likely due to data corruption or repeated failures) and are rejected @@ -169,13 +171,13 @@ export class OperationLogRecoveryService { ); } - // Mark valid ops as applied - they'll be replayed during normal hydration + // Reducers are replayed status-blind during hydration; archive work is retried after. if (validOps.length > 0) { const seqs = validOps.map((e) => e.seq); - await this.opLogStore.markApplied(seqs); + await this.opLogStore.markArchivePending(seqs); OpLog.warn( `OperationLogRecoveryService: Found ${validOps.length} pending remote ops from previous crash. ` + - `Marking as applied (they will be replayed during hydration).`, + 'Marking archive work pending (reducers will replay during hydration).', ); } diff --git a/src/app/op-log/persistence/operation-log-store.service.spec.ts b/src/app/op-log/persistence/operation-log-store.service.spec.ts index b505812618..2f1da31953 100644 --- a/src/app/op-log/persistence/operation-log-store.service.spec.ts +++ b/src/app/op-log/persistence/operation-log-store.service.spec.ts @@ -30,6 +30,7 @@ import { } from '../core/operation-log.const'; import { IndexedDBOpenError } from '../core/errors/indexed-db-open.error'; import { FULL_STATE_OPS_META_KEY, SINGLETON_KEY, STORE_NAMES } from './db-keys.const'; +import { ArchiveModel } from '../../features/time-tracking/time-tracking.model'; describe('OperationLogStoreService', () => { let service: OperationLogStoreService; @@ -1423,6 +1424,20 @@ describe('OperationLogStoreService', () => { }); describe('markApplied', () => { + it('should checkpoint reducer-committed operations as archive-pending', async () => { + const op = createTestOperation(); + const seq = await service.append(op, 'remote', { pendingApply: true }); + + await service.markArchivePending([seq]); + + const [stored] = await service.getOpsAfterSeq(0); + expect(stored.applicationStatus).toBe('archive_pending'); + expect((await service.getPendingRemoteOps()).length).toBe(0); + expect((await service.getFailedRemoteOps()).map((entry) => entry.op.id)).toEqual([ + op.id, + ]); + }); + it('should update applicationStatus from pending to applied', async () => { const op = createTestOperation(); const seq = await service.append(op, 'remote', { pendingApply: true }); @@ -1478,6 +1493,17 @@ describe('OperationLogStoreService', () => { expect(afterMarkApplied[0].applicationStatus).toBe('applied'); }); + it('should update applicationStatus from archive-pending to applied', async () => { + const op = createTestOperation(); + const seq = await service.append(op, 'remote', { pendingApply: true }); + await service.markArchivePending([seq]); + + await service.markApplied([seq]); + + const [stored] = await service.getOpsAfterSeq(0); + expect(stored.applicationStatus).toBe('applied'); + }); + it('should remove failed ops from getFailedRemoteOps after markApplied is called', async () => { // Create an op and mark it as pending const op = createTestOperation(); @@ -2301,6 +2327,139 @@ describe('OperationLogStoreService', () => { }); }); + describe('runRemoteStateReplacement', () => { + const createArchive = (taskId: string): ArchiveModel => + ({ + task: { + ids: [taskId], + entities: { [taskId]: { id: taskId, title: taskId } }, + }, + timeTracking: { project: {}, tag: {} }, + lastTimeTrackingFlush: 0, + }) as unknown as ArchiveModel; + + it('atomically replaces ops, cache, clock, metadata, and both archives', async () => { + await service.append( + createTestOperation({ + opType: OpType.SyncImport, + entityType: 'ALL' as EntityType, + }), + ); + const baselineState = { task: { ids: [], entities: {} } }; + const archiveYoung = createArchive('remote-young'); + const archiveOld = createArchive('remote-old'); + + await service.runRemoteStateReplacement({ + baselineState, + vectorClock: { remote: 4 }, + schemaVersion: 4, + snapshotEntityKeys: ['TASK:remote-task'], + archiveYoung, + archiveOld, + }); + + expect(await service.getOpsAfterSeq(0)).toEqual([]); + expect(await service.getLatestFullStateOpEntry()).toBeUndefined(); + expect(await service.loadStateCache()).toEqual( + jasmine.objectContaining({ + state: baselineState, + lastAppliedOpSeq: 0, + vectorClock: { remote: 4 }, + schemaVersion: 4, + snapshotEntityKeys: ['TASK:remote-task'], + }), + ); + expect(await service.getVectorClock()).toEqual({ remote: 4 }); + + const db = ( + service as unknown as { + db: IDBPDatabase; + } + ).db; + expect((await db.get(STORE_NAMES.ARCHIVE_YOUNG, SINGLETON_KEY)).data).toEqual( + archiveYoung, + ); + expect((await db.get(STORE_NAMES.ARCHIVE_OLD, SINGLETON_KEY)).data).toEqual( + archiveOld, + ); + }); + + it('rolls back every store if one archive write fails', async () => { + const priorOp = createTestOperation({ entityId: 'prior-task' }); + const priorState = { sentinel: 'prior-state' }; + const priorYoung = createArchive('prior-young'); + const priorOld = createArchive('prior-old'); + await service.append(priorOp); + await service.saveStateCache({ + state: priorState, + lastAppliedOpSeq: 1, + vectorClock: { testClient: 1 }, + compactedAt: Date.now(), + }); + await service.setVectorClock({ testClient: 1 }); + + const db = ( + service as unknown as { + db: IDBPDatabase; + } + ).db; + await db.put(STORE_NAMES.ARCHIVE_YOUNG, { + id: SINGLETON_KEY, + data: priorYoung, + lastModified: 1, + }); + await db.put(STORE_NAMES.ARCHIVE_OLD, { + id: SINGLETON_KEY, + data: priorOld, + lastModified: 1, + }); + + const realTransaction = db.transaction.bind(db); + spyOn(db, 'transaction').and.callFake((( + stores: Parameters[0], + mode: Parameters[1], + ) => { + const tx = realTransaction(stores, mode); + if (Array.isArray(stores) && stores.includes(STORE_NAMES.ARCHIVE_OLD)) { + const realObjectStore = tx.objectStore.bind(tx); + tx.objectStore = ((storeName: string) => { + const store = realObjectStore(storeName); + if (storeName === STORE_NAMES.ARCHIVE_OLD) { + store.put = async () => { + throw new Error('Simulated archive write failure'); + }; + } + return store; + }) as typeof tx.objectStore; + } + return tx; + }) as typeof db.transaction); + + await expectAsync( + service.runRemoteStateReplacement({ + baselineState: { sentinel: 'new-state' }, + vectorClock: { remote: 2 }, + schemaVersion: 4, + snapshotEntityKeys: [], + archiveYoung: createArchive('new-young'), + archiveOld: createArchive('new-old'), + }), + ).toBeRejected(); + + expect((await service.getOpsAfterSeq(0)).map((entry) => entry.op.id)).toEqual([ + priorOp.id, + ]); + expect((await service.loadStateCache())!.state).toEqual(priorState); + expect(await service.getVectorClock()).toEqual({ testClient: 1 }); + expect((await db.get(STORE_NAMES.ARCHIVE_YOUNG, SINGLETON_KEY)).data).toEqual( + priorYoung, + ); + expect((await db.get(STORE_NAMES.ARCHIVE_OLD, SINGLETON_KEY)).data).toEqual( + priorOld, + ); + }); + }); + describe('appendWithVectorClockUpdate', () => { it('should append operation and update vector clock atomically for local ops', async () => { const op = createTestOperation({ diff --git a/src/app/op-log/persistence/operation-log-store.service.ts b/src/app/op-log/persistence/operation-log-store.service.ts index 3e88d9e685..0db53f6a65 100644 --- a/src/app/op-log/persistence/operation-log-store.service.ts +++ b/src/app/op-log/persistence/operation-log-store.service.ts @@ -90,7 +90,7 @@ interface StoredOperationLogEntry { source: 'local' | 'remote'; syncedAt?: number; rejectedAt?: number; - applicationStatus?: 'pending' | 'applied' | 'failed'; + applicationStatus?: 'pending' | 'archive_pending' | 'applied' | 'failed'; retryCount?: number; } @@ -679,6 +679,24 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort { + await this._ensureInit(); + await this._adapter.transaction([STORE_NAMES.OPS], 'readwrite', async (tx) => { + for (const seq of seqs) { + const entry = await tx.get(STORE_NAMES.OPS, seq); + if (entry?.applicationStatus === 'pending') { + entry.applicationStatus = 'archive_pending'; + await tx.put(STORE_NAMES.OPS, entry); + } + } + }); + } + /** * Marks operations as successfully applied. * Called after remote operations have been dispatched to NgRx. @@ -689,11 +707,12 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort { for (const seq of seqs) { const entry = await tx.get(STORE_NAMES.OPS, seq); - // Allow transitioning from 'pending' or 'failed' to 'applied' - // 'failed' ops can be retried and need to be cleared when successful + // Failed/archive-pending ops can be retried and cleared when successful. if ( entry && - (entry.applicationStatus === 'pending' || entry.applicationStatus === 'failed') + (entry.applicationStatus === 'pending' || + entry.applicationStatus === 'archive_pending' || + entry.applicationStatus === 'failed') ) { entry.applicationStatus = 'applied'; await tx.put(STORE_NAMES.OPS, entry); @@ -1100,20 +1119,31 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort { await this._ensureInit(); let storedEntries: StoredOperationLogEntry[]; try { - // Exact compound-key match expressed as a degenerate [k, k] range. - storedEntries = await this._adapter.getAllFromIndex( - STORE_NAMES.OPS, - OPS_INDEXES.BY_SOURCE_AND_STATUS, - { lower: ['remote', 'failed'], upper: ['remote', 'failed'] }, - ); + const [archivePendingEntries, failedEntries] = await Promise.all([ + this._adapter.getAllFromIndex( + STORE_NAMES.OPS, + OPS_INDEXES.BY_SOURCE_AND_STATUS, + { + lower: ['remote', 'archive_pending'], + upper: ['remote', 'archive_pending'], + }, + ), + this._adapter.getAllFromIndex( + STORE_NAMES.OPS, + OPS_INDEXES.BY_SOURCE_AND_STATUS, + { lower: ['remote', 'failed'], upper: ['remote', 'failed'] }, + ), + ]); + storedEntries = [...archivePendingEntries, ...failedEntries]; } catch (e) { // Fallback for databases created before version 3 index migration Log.warn( @@ -1121,7 +1151,10 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort(STORE_NAMES.OPS); storedEntries = allOps.filter( - (entry) => entry.source === 'remote' && entry.applicationStatus === 'failed', + (entry) => + entry.source === 'remote' && + (entry.applicationStatus === 'archive_pending' || + entry.applicationStatus === 'failed'), ); } // Decode and filter out rejected ops @@ -1504,6 +1537,83 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort { + await this._ensureInit(); + + const now = Date.now(); + try { + await this._adapter.transaction( + [ + STORE_NAMES.OPS, + STORE_NAMES.META, + STORE_NAMES.STATE_CACHE, + STORE_NAMES.VECTOR_CLOCK, + STORE_NAMES.ARCHIVE_YOUNG, + STORE_NAMES.ARCHIVE_OLD, + ], + 'readwrite', + async (tx) => { + await tx.clear(STORE_NAMES.OPS); + await tx.put( + STORE_NAMES.META, + buildFullStateOpsMeta([]), + FULL_STATE_OPS_META_KEY, + ); + await tx.put(STORE_NAMES.STATE_CACHE, { + id: SINGLETON_KEY, + state: opts.baselineState, + lastAppliedOpSeq: 0, + vectorClock: opts.vectorClock, + compactedAt: now, + schemaVersion: opts.schemaVersion, + snapshotEntityKeys: opts.snapshotEntityKeys, + }); + await tx.put( + STORE_NAMES.VECTOR_CLOCK, + { clock: opts.vectorClock, lastUpdate: now }, + SINGLETON_KEY, + ); + await tx.put(STORE_NAMES.ARCHIVE_YOUNG, { + id: SINGLETON_KEY, + data: opts.archiveYoung, + lastModified: now, + }); + await tx.put(STORE_NAMES.ARCHIVE_OLD, { + id: SINGLETON_KEY, + data: opts.archiveOld, + lastModified: now, + }); + }, + ); + + this._invalidateAppliedAndUnsyncedCaches(); + this._vectorClockCache = { ...opts.vectorClock }; + } catch (e) { + if (e instanceof DOMException && e.name === 'QuotaExceededError') { + throw new StorageQuotaExceededError(); + } + throw e; + } + } + // ============================================================ // Vector Clock Management (Performance Optimization) // ============================================================ diff --git a/src/app/op-log/persistence/schema-migration.service.spec.ts b/src/app/op-log/persistence/schema-migration.service.spec.ts index 409998fd9c..bc722b9abb 100644 --- a/src/app/op-log/persistence/schema-migration.service.spec.ts +++ b/src/app/op-log/persistence/schema-migration.service.spec.ts @@ -147,6 +147,13 @@ describe('SchemaMigrationService', () => { // Should return the operation (with undefined treated as version 1) expect(result).not.toBeNull(); }); + + it('should reject a present non-integer schema version at the migration boundary', () => { + const op = createMockOperation('malformed-op'); + op.schemaVersion = '2' as unknown as number; + + expect(() => service.migrateOperation(op)).toThrowError(/schemaVersion/); + }); }); describe('migrateOperations', () => { @@ -181,6 +188,33 @@ describe('SchemaMigrationService', () => { expect(result.map((op) => op.id)).toEqual(['op-a', 'op-b', 'op-c']); }); + + it('should preserve unique migrated identities when a v1 config operation splits', () => { + const op = createMockOperation('config-op', 1); + op.actionType = ActionType.GLOBAL_CONFIG_UPDATE_SECTION; + op.entityType = 'GLOBAL_CONFIG'; + op.entityId = 'misc'; + op.entityIds = ['misc']; + op.payload = { + actionPayload: { + sectionKey: 'misc', + sectionCfg: { + isConfirmBeforeTaskDelete: true, + unrelatedMiscSetting: 'keep-me', + }, + }, + entityChanges: [], + }; + + const result = service.migrateOperations([op]); + + expect(result.map((migrated) => migrated.id)).toEqual([ + 'config-op_misc', + 'config-op_tasks', + ]); + expect(result.map((migrated) => migrated.entityId)).toEqual(['misc', 'tasks']); + expect(result.map((migrated) => migrated.entityIds)).toEqual([['misc'], ['tasks']]); + }); }); describe('migrateIfNeeded (deprecated)', () => { diff --git a/src/app/op-log/persistence/schema-migration.service.ts b/src/app/op-log/persistence/schema-migration.service.ts index f3a1ebdcbd..4fbaaafc5a 100644 --- a/src/app/op-log/persistence/schema-migration.service.ts +++ b/src/app/op-log/persistence/schema-migration.service.ts @@ -23,6 +23,22 @@ export const MIN_SUPPORTED_SCHEMA_VERSION = SHARED_MIN_SUPPORTED_SCHEMA_VERSION; // Re-export types export type { SchemaMigration }; +export const getOperationSchemaVersion = (op: { schemaVersion?: unknown }): number => { + if (op.schemaVersion === undefined) { + return 1; + } + if ( + typeof op.schemaVersion !== 'number' || + !Number.isInteger(op.schemaVersion) || + op.schemaVersion < 0 + ) { + throw new Error( + 'Operation schemaVersion must be a non-negative integer when present.', + ); + } + return op.schemaVersion; +}; + /** * Interface for state cache that may need migration. */ @@ -133,7 +149,7 @@ export class SchemaMigrationService { * @returns The migrated operation(s), or null if it should be dropped */ migrateOperation(op: Operation): Operation | Operation[] | null { - const opVersion = op.schemaVersion ?? 1; + const opVersion = getOperationSchemaVersion(op); if (opVersion >= CURRENT_SCHEMA_VERSION) { return op; @@ -164,6 +180,7 @@ export class SchemaMigrationService { if (Array.isArray(result.data)) { return result.data.map((migratedOpLike) => ({ ...op, + id: migratedOpLike.id, opType: migratedOpLike.opType as Operation['opType'], entityType: migratedOpLike.entityType as Operation['entityType'], entityId: migratedOpLike.entityId, @@ -176,9 +193,11 @@ export class SchemaMigrationService { // Merge migrated fields back into the original operation return { ...op, + id: result.data.id, opType: result.data.opType as Operation['opType'], entityType: result.data.entityType as Operation['entityType'], entityId: result.data.entityId, + entityIds: result.data.entityIds, payload: result.data.payload, schemaVersion: result.data.schemaVersion, }; @@ -225,13 +244,14 @@ export class SchemaMigrationService { * Returns true if the operation needs migration. */ operationNeedsMigration(op: Operation): boolean { + const schemaVersion = getOperationSchemaVersion(op); return sharedOperationNeedsMigration( { id: op.id, opType: op.opType, entityType: op.entityType, payload: op.payload, - schemaVersion: op.schemaVersion ?? 1, + schemaVersion, }, CURRENT_SCHEMA_VERSION, ); diff --git a/src/app/op-log/sync/conflict-resolution.service.spec.ts b/src/app/op-log/sync/conflict-resolution.service.spec.ts index 669b6dd0af..64690bf83d 100644 --- a/src/app/op-log/sync/conflict-resolution.service.spec.ts +++ b/src/app/op-log/sync/conflict-resolution.service.spec.ts @@ -55,6 +55,7 @@ describe('ConflictResolutionService', () => { 'append', 'appendBatchSkipDuplicates', 'appendWithVectorClockUpdate', + 'markArchivePending', 'markApplied', 'markRejected', 'markFailed', @@ -1787,8 +1788,9 @@ describe('ConflictResolutionService', () => { mockOpLogStore.hasOp.and.resolveTo(false); mockOpLogStore.append.and.resolveTo(1); - mockOperationApplier.applyOperations.and.resolveTo({ - appliedOps: [remoteOp], + mockOperationApplier.applyOperations.and.callFake(async (ops, options) => { + await options?.onReducersCommitted?.(ops); + return { appliedOps: [remoteOp] }; }); mockOpLogStore.markApplied.and.resolveTo(undefined); mockOpLogStore.markRejected.and.resolveTo(undefined); @@ -1812,10 +1814,14 @@ describe('ConflictResolutionService', () => { const callOrder: string[] = []; mockOpLogStore.hasOp.and.resolveTo(false); - mockOperationApplier.applyOperations.and.callFake(async () => { + mockOperationApplier.applyOperations.and.callFake(async (ops, options) => { callOrder.push('applyOperations'); + await options?.onReducersCommitted?.(ops); return { appliedOps: [remoteOp] }; }); + mockOpLogStore.markArchivePending.and.callFake(async () => { + callOrder.push('markArchivePending'); + }); mockOpLogStore.markApplied.and.callFake(async () => { callOrder.push('markApplied'); }); @@ -1831,16 +1837,21 @@ describe('ConflictResolutionService', () => { callerHoldsOperationLogLock: true, }); - expect(mockOperationApplier.applyOperations).toHaveBeenCalledWith([remoteOp], { - skipDeferredLocalActions: true, - }); + expect(mockOperationApplier.applyOperations).toHaveBeenCalledWith( + [remoteOp], + jasmine.objectContaining({ + skipDeferredLocalActions: true, + onReducersCommitted: jasmine.any(Function), + }), + ); expect(mockOperationLogEffects.processDeferredActions).toHaveBeenCalledWith({ callerHoldsOperationLogLock: true, }); expect(callOrder).toEqual([ 'applyOperations', - 'markApplied', + 'markArchivePending', 'mergeRemoteOpClocks', + 'markApplied', 'processDeferredActions', ]); }); @@ -1865,8 +1876,9 @@ describe('ConflictResolutionService', () => { mockOpLogStore.hasOp.and.resolveTo(false); mockOpLogStore.append.and.resolveTo(1); - mockOperationApplier.applyOperations.and.resolveTo({ - appliedOps: [conflictRemoteOp, nonConflictingOp], + mockOperationApplier.applyOperations.and.callFake(async (ops, options) => { + await options?.onReducersCommitted?.(ops); + return { appliedOps: [conflictRemoteOp, nonConflictingOp] }; }); mockOpLogStore.markApplied.and.resolveTo(undefined); mockOpLogStore.markRejected.and.resolveTo(undefined); diff --git a/src/app/op-log/sync/conflict-resolution.service.ts b/src/app/op-log/sync/conflict-resolution.service.ts index 4603755a33..1d7e93a747 100644 --- a/src/app/op-log/sync/conflict-resolution.service.ts +++ b/src/app/op-log/sync/conflict-resolution.service.ts @@ -35,7 +35,6 @@ import { } from '../core/operation.types'; import { toLwwUpdateActionType } from '../core/lww-update-action-types'; import { OperationApplierService } from '../apply/operation-applier.service'; -import { getFailedOpIdsFromBatch } from '../apply/failed-op-ids.util'; import { OperationLogStoreService } from '../persistence/operation-log-store.service'; import { OpLog } from '../../core/log'; import { toEntityKey } from '../util/entity-key.util'; @@ -430,6 +429,18 @@ export class ConflictResolutionService { const opIdToSeq = new Map(allStoredOps.map((o) => [o.id, o.seq])); const applyResult = await this.operationApplier.applyOperations(allOpsToApply, { skipDeferredLocalActions: true, + onReducersCommitted: async (reducerCommittedOps) => { + const reducerCommittedSeqs = reducerCommittedOps + .map((op) => opIdToSeq.get(op.id)) + .filter((seq): seq is number => seq !== undefined); + if (reducerCommittedSeqs.length !== reducerCommittedOps.length) { + throw new Error( + 'ConflictResolutionService: reducer commit contained an unknown operation.', + ); + } + await this.opLogStore.markArchivePending(reducerCommittedSeqs); + await this.opLogStore.mergeRemoteOpClocks(reducerCommittedOps); + }, }); const appliedSeqs = applyResult.appliedOps @@ -439,28 +450,17 @@ export class ConflictResolutionService { if (appliedSeqs.length > 0) { await this.opLogStore.markApplied(appliedSeqs); - // CRITICAL: Merge remote ops' vector clocks into local clock. - // This ensures subsequent local operations have clocks that "dominate" - // the applied remote ops (GREATER_THAN instead of CONCURRENT). - // Without this, ops created after conflict resolution would have clocks - // that are CONCURRENT with the applied ops, causing them to be incorrectly - // filtered by SyncImportFilterService or rejected as conflicts on next sync. - await this.opLogStore.mergeRemoteOpClocks(applyResult.appliedOps); - OpLog.normal( `ConflictResolutionService: Successfully applied ${appliedSeqs.length} ops`, ); } if (applyResult.failedOp) { - const failedOpIds = getFailedOpIdsFromBatch( - allOpsToApply, - applyResult.failedOp.op, - ); + const failedOpIds = [applyResult.failedOp.op.id]; OpLog.err( `ConflictResolutionService: ${applyResult.appliedOps.length} ops applied before failure. ` + - `Marking ${failedOpIds.length} ops as failed.`, + 'Marking the attempted archive operation as failed.', applyResult.failedOp.error, ); await this.opLogStore.markFailed(failedOpIds, MAX_CONFLICT_RETRY_ATTEMPTS); diff --git a/src/app/op-log/sync/immediate-upload.service.spec.ts b/src/app/op-log/sync/immediate-upload.service.spec.ts index 62cf2a5cff..6d18e1ce4a 100644 --- a/src/app/op-log/sync/immediate-upload.service.spec.ts +++ b/src/app/op-log/sync/immediate-upload.service.spec.ts @@ -179,6 +179,19 @@ describe('ImmediateUploadService', () => { expect(mockProviderManager.setSyncStatus).not.toHaveBeenCalled(); }); + it('should report ERROR when piggyback processing is blocked by an incompatible op', fakeAsync(() => { + mockSyncService.uploadPendingOps.and.resolveTo({ + kind: 'blocked_incompatible', + }); + + service.initialize(); + service.trigger(); + tick(2000); + flush(); + + expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith('ERROR'); + })); + it('should NOT show checkmark when piggybacked ops exist (multiple)', fakeAsync(() => { mockSyncService.uploadPendingOps.and.returnValue( Promise.resolve(completedResult({ uploadedCount: 5, piggybackedOpsCount: 3 })), diff --git a/src/app/op-log/sync/immediate-upload.service.ts b/src/app/op-log/sync/immediate-upload.service.ts index 9f3cd1d2de..22c0fc0c9b 100644 --- a/src/app/op-log/sync/immediate-upload.service.ts +++ b/src/app/op-log/sync/immediate-upload.service.ts @@ -237,6 +237,14 @@ export class ImmediateUploadService implements OnDestroy { return; } + if (result.kind === 'blocked_incompatible') { + OpLog.warn( + 'ImmediateUploadService: Piggyback processing blocked by an incompatible operation', + ); + this._providerManager.setSyncStatus('ERROR'); + return; + } + // result.kind === 'completed' from here // If LWW local-wins created new update ops from piggybacked ops, diff --git a/src/app/op-log/sync/operation-log-download.service.spec.ts b/src/app/op-log/sync/operation-log-download.service.spec.ts index 1572a1d60e..23e78e857b 100644 --- a/src/app/op-log/sync/operation-log-download.service.spec.ts +++ b/src/app/op-log/sync/operation-log-download.service.spec.ts @@ -718,6 +718,51 @@ describe('OperationLogDownloadService', () => { expect(mockApiProvider.downloadOps).toHaveBeenCalledTimes(2); }); + it('should reject an empty page that claims more data', async () => { + mockApiProvider.downloadOps.and.resolveTo({ + ops: [], + hasMore: true, + latestSeq: 10, + }); + + const result = await service.downloadRemoteOps(mockApiProvider); + + expect(result.success).toBeFalse(); + expect(result.newOps).toEqual([]); + expect(mockSuperSyncStatusService.markRemoteChecked).not.toHaveBeenCalled(); + }); + + it('should reject a page that does not advance its cursor while claiming more data', async () => { + mockApiProvider.getLastServerSeq.and.resolveTo(5); + mockApiProvider.downloadOps.and.resolveTo({ + ops: [ + { + serverSeq: 5, + receivedAt: Date.now(), + op: { + id: 'op-stuck', + clientId: 'c1', + actionType: '[Task] Update' as ActionType, + opType: OpType.Update, + entityType: 'TASK', + payload: {}, + vectorClock: {}, + timestamp: Date.now(), + schemaVersion: 1, + }, + }, + ], + hasMore: true, + latestSeq: 10, + }); + + const result = await service.downloadRemoteOps(mockApiProvider); + + expect(result.success).toBeFalse(); + expect(result.newOps).toEqual([]); + expect(mockApiProvider.downloadOps).toHaveBeenCalledTimes(1); + }); + it('should filter already applied operations', async () => { mockOpLogStore.getAppliedOpIds.and.returnValue( Promise.resolve(new Set(['op-1'])), diff --git a/src/app/op-log/sync/operation-log-download.service.ts b/src/app/op-log/sync/operation-log-download.service.ts index daf055fb21..0034b2d8aa 100644 --- a/src/app/op-log/sync/operation-log-download.service.ts +++ b/src/app/op-log/sync/operation-log-download.service.ts @@ -229,6 +229,12 @@ export class OperationLogDownloadService implements OnDestroy { } if (response.ops.length === 0) { + if (response.hasMore) { + OpLog.error( + 'OperationLogDownloadService: Server returned an empty page with hasMore=true. Aborting to avoid accepting a partial download.', + ); + downloadFailed = true; + } // No ops to download - caller will persist latestServerSeq after this method returns break; } @@ -316,8 +322,18 @@ export class OperationLogDownloadService implements OnDestroy { break; } - // Update cursors - sinceSeq = response.ops[response.ops.length - 1].serverSeq; + // Update cursors. A page that claims more data must advance the cursor; + // otherwise accepting the accumulated prefix would silently skip the + // unseen suffix (or spin until the iteration cap). + const nextSinceSeq = response.ops[response.ops.length - 1].serverSeq; + if (response.hasMore && nextSinceSeq <= sinceSeq) { + OpLog.error( + `OperationLogDownloadService: Non-progressing page cursor (${nextSinceSeq} <= ${sinceSeq}) with hasMore=true. Aborting partial download.`, + ); + downloadFailed = true; + break; + } + sinceSeq = nextSinceSeq; hasMore = response.hasMore; // Monotonicity check: warn if server seq decreased (indicates potential server bug) diff --git a/src/app/op-log/sync/operation-log-sync.service.spec.ts b/src/app/op-log/sync/operation-log-sync.service.spec.ts index f0cc485793..30ea0d7307 100644 --- a/src/app/op-log/sync/operation-log-sync.service.spec.ts +++ b/src/app/op-log/sync/operation-log-sync.service.spec.ts @@ -37,6 +37,7 @@ import { BackupService } from '../backup/backup.service'; import { T } from '../../t.const'; import { INBOX_PROJECT } from '../../features/project/project.const'; import { TODAY_TAG, SYSTEM_TAG_IDS } from '../../features/tag/tag.const'; +import { OperationSyncCapable } from '../sync-providers/provider.interface'; describe('OperationLogSyncService', () => { let service: OperationLogSyncService; @@ -50,6 +51,9 @@ describe('OperationLogSyncService', () => { let stateSnapshotServiceSpy: jasmine.SpyObj; let backupServiceSpy: jasmine.SpyObj; let syncImportConflictDialogServiceSpy: jasmine.SpyObj; + let schemaMigrationServiceSpy: jasmine.SpyObj; + let validateStateServiceSpy: jasmine.SpyObj; + let lockServiceSpy: jasmine.SpyObj; beforeEach(() => { snackServiceSpy = jasmine.createSpyObj('SnackService', ['open']); @@ -58,14 +62,17 @@ describe('OperationLogSyncService', () => { 'loadStateCache', 'getLastSeq', 'getOpById', + 'markSynced', 'markRejected', 'setVectorClock', 'clearFullStateOps', 'getVectorClock', 'appendBatchSkipDuplicates', 'hasSyncedOps', + 'runRemoteStateReplacement', ]); opLogStoreSpy.hasSyncedOps.and.resolveTo(true); + opLogStoreSpy.markSynced.and.resolveTo(); opLogStoreSpy.setVectorClock.and.resolveTo(); opLogStoreSpy.clearFullStateOps.and.resolveTo(); opLogStoreSpy.getVectorClock.and.resolveTo(null); @@ -74,6 +81,26 @@ describe('OperationLogSyncService', () => { writtenOps: [], skippedCount: 0, }); + opLogStoreSpy.runRemoteStateReplacement.and.resolveTo(); + + schemaMigrationServiceSpy = jasmine.createSpyObj('SchemaMigrationService', [ + 'getCurrentVersion', + 'migrateOperation', + 'migrateOperations', + ]); + schemaMigrationServiceSpy.migrateOperations.and.callFake((ops) => ops); + + validateStateServiceSpy = jasmine.createSpyObj('ValidateStateService', [ + 'validateAndRepair', + 'validateAndRepairCurrentState', + ]); + validateStateServiceSpy.validateAndRepair.and.resolveTo({ + isValid: true, + wasRepaired: false, + }); + + lockServiceSpy = jasmine.createSpyObj('LockService', ['request']); + lockServiceSpy.request.and.callFake(async (_name, callback) => callback()); serverMigrationServiceSpy = jasmine.createSpyObj('ServerMigrationService', [ 'checkAndHandleMigration', 'handleServerMigration', @@ -137,13 +164,7 @@ describe('OperationLogSyncService', () => { providers: [ OperationLogSyncService, provideMockStore(), - { - provide: SchemaMigrationService, - useValue: jasmine.createSpyObj('SchemaMigrationService', [ - 'getCurrentVersion', - 'migrateOperation', - ]), - }, + { provide: SchemaMigrationService, useValue: schemaMigrationServiceSpy }, { provide: SnackService, useValue: snackServiceSpy }, { provide: OperationLogStoreService, useValue: opLogStoreSpy }, { @@ -166,13 +187,7 @@ describe('OperationLogSyncService', () => { 'checkOpForConflicts', ]), }, - { - provide: ValidateStateService, - useValue: jasmine.createSpyObj('ValidateStateService', [ - 'validateAndRepair', - 'validateAndRepairCurrentState', - ]), - }, + { provide: ValidateStateService, useValue: validateStateServiceSpy }, { provide: RepairOperationService, useValue: jasmine.createSpyObj('RepairOperationService', [ @@ -191,10 +206,7 @@ describe('OperationLogSyncService', () => { 'downloadRemoteOps', ]), }, - { - provide: LockService, - useValue: jasmine.createSpyObj('LockService', ['request']), - }, + { provide: LockService, useValue: lockServiceSpy }, { provide: OperationLogCompactionService, useValue: jasmine.createSpyObj('OperationLogCompactionService', ['compact']), @@ -747,6 +759,48 @@ describe('OperationLogSyncService', () => { expect(rejectedOpsHandlerServiceSpy.handleRejectedOps).not.toHaveBeenCalled(); }); + it('should return a terminal outcome and keep acknowledgements pending when piggyback processing is incompatible', async () => { + const piggybackedOp = { + id: 'future-op', + clientId: 'client-B', + actionType: 'test' as ActionType, + opType: OpType.Update, + entityType: 'TASK' as const, + entityId: 'task-1', + payload: {}, + vectorClock: { clientB: 1 }, + timestamp: Date.now(), + schemaVersion: 99, + }; + uploadServiceSpy.uploadPendingOps.and.resolveTo({ + uploadedCount: 1, + piggybackedOps: [piggybackedOp], + rejectedCount: 0, + rejectedOps: [], + pendingAcknowledgementSeqs: [1], + lastServerSeqToPersist: 9, + }); + remoteOpsProcessingServiceSpy.processRemoteOps.and.resolveTo({ + localWinOpsCreated: 0, + allOpsFilteredBySyncImport: false, + filteredOpCount: 0, + isLocalUnsyncedImport: false, + blockedByIncompatibleOp: true, + }); + const setLastServerSeq = jasmine.createSpy('setLastServerSeq').and.resolveTo(); + const provider = { + ...mockProvider, + setLastServerSeq, + } as unknown as OperationSyncCapable; + + const result = await service.uploadPendingOps(provider); + + expect(result.kind).toBe('blocked_incompatible'); + expect(opLogStoreSpy.markSynced).not.toHaveBeenCalled(); + expect(setLastServerSeq).not.toHaveBeenCalled(); + expect(rejectedOpsHandlerServiceSpy.handleRejectedOps).not.toHaveBeenCalled(); + }); + it('should not call handleRejectedOps when there are no rejected ops', async () => { uploadServiceSpy.uploadPendingOps.and.returnValue( Promise.resolve({ @@ -942,7 +996,7 @@ describe('OperationLogSyncService', () => { // Cursor stays behind the blocked op so it is re-downloaded and retried // after an app update instead of skipped forever. - expect(result.kind).toBe('ops_processed'); + expect(result.kind).toBe('blocked_incompatible'); expect(setLastServerSeqSpy).not.toHaveBeenCalled(); }); @@ -1190,10 +1244,7 @@ describe('OperationLogSyncService', () => { ]); }); - it('should NOT throw LocalDataConflictError for clients with only system/config ops (no user data)', async () => { - // Clients with only system/config ops (no tasks/projects/tags) should NOT see conflict dialog. - // They should just download the remote data. - + it('should protect unsynced user config from file-snapshot replacement', async () => { const unsyncedEntry: OperationLogEntry = { seq: 1, op: { @@ -1239,9 +1290,10 @@ describe('OperationLogSyncService', () => { setLastServerSeq: jasmine.createSpy('setLastServerSeq').and.resolveTo(), } as any; - // Should NOT throw - fresh client should proceed with download - await expectAsync(service.downloadRemoteOps(mockProvider)).toBeResolved(); - expect(syncHydrationServiceSpy.hydrateFromRemoteSync).toHaveBeenCalled(); + await expectAsync( + service.downloadRemoteOps(mockProvider), + ).toBeRejectedWithError(LocalDataConflictError); + expect(syncHydrationServiceSpy.hydrateFromRemoteSync).not.toHaveBeenCalled(); }); it('should throw LocalDataConflictError when only config ops but store has meaningful data (provider switch)', async () => { @@ -2289,7 +2341,7 @@ describe('OperationLogSyncService', () => { const mockProvider = { supportsOperationSync: true, setLastServerSeq: jasmine.createSpy('setLastServerSeq').and.resolveTo(), - } as any; + } as unknown as OperationSyncCapable; await service.forceUploadLocalState(mockProvider); @@ -2318,7 +2370,7 @@ describe('OperationLogSyncService', () => { const mockProvider = { supportsOperationSync: true, setLastServerSeq: jasmine.createSpy('setLastServerSeq').and.resolveTo(), - } as any; + } as unknown as OperationSyncCapable; await service.forceUploadLocalState(mockProvider); @@ -2331,7 +2383,7 @@ describe('OperationLogSyncService', () => { const mockProvider = { supportsOperationSync: true, - } as any; + } as unknown as OperationSyncCapable; await expectAsync(service.forceUploadLocalState(mockProvider)).toBeRejectedWith( error, @@ -2345,7 +2397,7 @@ describe('OperationLogSyncService', () => { const mockProvider = { supportsOperationSync: true, setLastServerSeq: jasmine.createSpy('setLastServerSeq').and.resolveTo(), - } as any; + } as unknown as OperationSyncCapable; await expectAsync(service.forceUploadLocalState(mockProvider)).toBeRejectedWith( error, @@ -2391,16 +2443,13 @@ describe('OperationLogSyncService', () => { OperationLogDownloadService, ) as jasmine.SpyObj; - opLogStoreSpy.clearAllOperations = jasmine - .createSpy('clearAllOperations') - .and.resolveTo(); - opLogStoreSpy.saveStateCache = jasmine.createSpy('saveStateCache').and.resolveTo(); + opLogStoreSpy.runRemoteStateReplacement.calls.reset(); }); it('should download BEFORE any destructive local mutation', async () => { const callOrder: string[] = []; - opLogStoreSpy.clearAllOperations.and.callFake(async () => { - callOrder.push('clearAllOperations'); + opLogStoreSpy.runRemoteStateReplacement.and.callFake(async () => { + callOrder.push('runRemoteStateReplacement'); }); downloadServiceSpy.downloadRemoteOps.and.callFake(async () => { callOrder.push('downloadRemoteOps'); @@ -2421,25 +2470,31 @@ describe('OperationLogSyncService', () => { await service.forceDownloadRemoteState(mockProvider); - expect(callOrder).toEqual(['downloadRemoteOps', 'clearAllOperations']); + expect(callOrder).toEqual(['downloadRemoteOps', 'runRemoteStateReplacement']); }); - it('should capture a safety backup BEFORE clearing local data (#8107)', async () => { + it('should capture a safety backup after download but before replacement (#8107)', async () => { const callOrder: string[] = []; + downloadServiceSpy.downloadRemoteOps.and.callFake(async () => { + callOrder.push('downloadRemoteOps'); + return { + newOps: [makeRemoteOp()], + needsFullStateUpload: false, + success: true, + providerMode: 'superSyncOps', + failedFileCount: 0, + latestServerSeq: 1, + }; + }); backupServiceSpy.captureImportBackup.and.callFake(async () => { callOrder.push('captureImportBackup'); return 1; }); - opLogStoreSpy.clearAllOperations.and.callFake(async () => { - callOrder.push('clearAllOperations'); + writeFlushServiceSpy.flushPendingWrites.and.callFake(async () => { + callOrder.push('flushPendingWrites'); }); - downloadServiceSpy.downloadRemoteOps.and.resolveTo({ - newOps: [makeRemoteOp()], - needsFullStateUpload: false, - success: true, - providerMode: 'superSyncOps', - failedFileCount: 0, - latestServerSeq: 1, + opLogStoreSpy.runRemoteStateReplacement.and.callFake(async () => { + callOrder.push('runRemoteStateReplacement'); }); const mockProvider = { supportsOperationSync: true, @@ -2448,10 +2503,23 @@ describe('OperationLogSyncService', () => { await service.forceDownloadRemoteState(mockProvider); - expect(callOrder).toEqual(['captureImportBackup', 'clearAllOperations']); + expect(callOrder).toEqual([ + 'downloadRemoteOps', + 'flushPendingWrites', + 'captureImportBackup', + 'runRemoteStateReplacement', + ]); }); it('should ABORT without wiping local data if the safety backup fails (#8107)', async () => { + downloadServiceSpy.downloadRemoteOps.and.resolveTo({ + newOps: [makeRemoteOp()], + needsFullStateUpload: false, + success: true, + providerMode: 'superSyncOps', + failedFileCount: 0, + latestServerSeq: 1, + }); backupServiceSpy.captureImportBackup.and.rejectWith(new Error('disk full')); const mockProvider = { supportsOperationSync: true, @@ -2460,8 +2528,9 @@ describe('OperationLogSyncService', () => { await expectAsync(service.forceDownloadRemoteState(mockProvider)).toBeRejected(); - expect(opLogStoreSpy.clearAllOperations).not.toHaveBeenCalled(); - expect(downloadServiceSpy.downloadRemoteOps).not.toHaveBeenCalled(); + expect(opLogStoreSpy.runRemoteStateReplacement).not.toHaveBeenCalled(); + expect(downloadServiceSpy.downloadRemoteOps).toHaveBeenCalled(); + expect(backupServiceSpy.captureImportBackup).toHaveBeenCalled(); }); it('should offer to restore the previous data after replacing (#8107)', async () => { @@ -2510,6 +2579,33 @@ describe('OperationLogSyncService', () => { expect(setLastServerSeqSpy).toHaveBeenCalledWith(0); }); + it('should reset the external cursor before committing the local replacement', async () => { + const callOrder: string[] = []; + downloadServiceSpy.downloadRemoteOps.and.resolveTo({ + newOps: [makeRemoteOp()], + needsFullStateUpload: false, + success: true, + providerMode: 'superSyncOps', + failedFileCount: 0, + latestServerSeq: 1, + }); + opLogStoreSpy.runRemoteStateReplacement.and.callFake(async () => { + callOrder.push('replace'); + }); + const mockProvider = { + supportsOperationSync: true, + setLastServerSeq: jasmine + .createSpy('setLastServerSeq') + .and.callFake(async (seq: number) => { + if (seq === 0) callOrder.push('cursor-zero'); + }), + } as unknown as OperationSyncCapable; + + await service.forceDownloadRemoteState(mockProvider); + + expect(callOrder.slice(0, 2)).toEqual(['cursor-zero', 'replace']); + }); + it('should download raw history: forceFromSeq0 AND includeOwnAndAppliedOps', async () => { downloadServiceSpy.downloadRemoteOps.and.resolveTo({ newOps: [makeRemoteOp()], @@ -2551,7 +2647,7 @@ describe('OperationLogSyncService', () => { service.forceDownloadRemoteState(mockProvider), ).toBeRejectedWithError(/Download failed/); expect(remoteOpsProcessingServiceSpy.processRemoteOps).not.toHaveBeenCalled(); - expect(opLogStoreSpy.clearAllOperations).not.toHaveBeenCalled(); + expect(opLogStoreSpy.runRemoteStateReplacement).not.toHaveBeenCalled(); expect(setLastServerSeqSpy).not.toHaveBeenCalled(); }); @@ -2573,10 +2669,95 @@ describe('OperationLogSyncService', () => { await expectAsync( service.forceDownloadRemoteState(mockProvider), ).toBeRejectedWithError(/newer schema version/); - expect(opLogStoreSpy.clearAllOperations).not.toHaveBeenCalled(); + expect(opLogStoreSpy.runRemoteStateReplacement).not.toHaveBeenCalled(); expect(remoteOpsProcessingServiceSpy.processRemoteOps).not.toHaveBeenCalled(); }); + it('should run all operation migrations before backup or replacement', async () => { + const remoteOp = { ...makeRemoteOp(), schemaVersion: 1 }; + const migratedOp = { ...remoteOp, schemaVersion: 4 }; + downloadServiceSpy.downloadRemoteOps.and.resolveTo({ + newOps: [remoteOp], + needsFullStateUpload: false, + success: true, + providerMode: 'superSyncOps', + failedFileCount: 0, + latestServerSeq: 1, + }); + schemaMigrationServiceSpy.migrateOperations.and.returnValue([migratedOp]); + const mockProvider = { + supportsOperationSync: true, + setLastServerSeq: jasmine.createSpy('setLastServerSeq').and.resolveTo(), + } as unknown as OperationSyncCapable; + + await service.forceDownloadRemoteState(mockProvider); + + expect(schemaMigrationServiceSpy.migrateOperations).toHaveBeenCalledOnceWith([ + remoteOp, + ]); + expect(remoteOpsProcessingServiceSpy.processRemoteOps).toHaveBeenCalledWith( + [migratedOp], + { + skipConflictDetection: true, + callerHoldsOperationLogLock: true, + }, + ); + }); + + it('should abort before backup and replacement when operation migration fails', async () => { + downloadServiceSpy.downloadRemoteOps.and.resolveTo({ + newOps: [makeRemoteOp()], + needsFullStateUpload: false, + success: true, + providerMode: 'superSyncOps', + failedFileCount: 0, + latestServerSeq: 1, + }); + schemaMigrationServiceSpy.migrateOperations.and.throwError('bad migration'); + const mockProvider = { + supportsOperationSync: true, + setLastServerSeq: jasmine.createSpy('setLastServerSeq').and.resolveTo(), + } as unknown as OperationSyncCapable; + + await expectAsync( + service.forceDownloadRemoteState(mockProvider), + ).toBeRejectedWithError(/migration failed/); + + expect(backupServiceSpy.captureImportBackup).not.toHaveBeenCalled(); + expect(opLogStoreSpy.runRemoteStateReplacement).not.toHaveBeenCalled(); + }); + + it('should validate a file snapshot before backup and replacement', async () => { + const snapshotState = { task: { ids: ['remote-task'] } }; + downloadServiceSpy.downloadRemoteOps.and.resolveTo({ + newOps: [], + needsFullStateUpload: false, + success: true, + providerMode: 'fileSnapshotOps', + failedFileCount: 0, + snapshotState, + latestServerSeq: 1, + }); + validateStateServiceSpy.validateAndRepair.and.resolveTo({ + isValid: false, + wasRepaired: false, + }); + const mockProvider = { + supportsOperationSync: true, + setLastServerSeq: jasmine.createSpy('setLastServerSeq').and.resolveTo(), + } as unknown as OperationSyncCapable; + + await expectAsync( + service.forceDownloadRemoteState(mockProvider), + ).toBeRejectedWithError(/snapshot is invalid/); + + expect(validateStateServiceSpy.validateAndRepair).toHaveBeenCalledOnceWith( + snapshotState, + ); + expect(backupServiceSpy.captureImportBackup).not.toHaveBeenCalled(); + expect(opLogStoreSpy.runRemoteStateReplacement).not.toHaveBeenCalled(); + }); + it('should NOT advance the cursor past 0 when replay blocks on a migration failure', async () => { downloadServiceSpy.downloadRemoteOps.and.resolveTo({ newOps: [makeRemoteOp()], @@ -2641,7 +2822,10 @@ describe('OperationLogSyncService', () => { expect(remoteOpsProcessingServiceSpy.processRemoteOps).toHaveBeenCalledWith( mockOps, - { skipConflictDetection: true }, + { + skipConflictDetection: true, + callerHoldsOperationLogLock: true, + }, ); }); @@ -2703,7 +2887,7 @@ describe('OperationLogSyncService', () => { service.forceDownloadRemoteState(mockProvider), ).toBeRejectedWithError(/no data to rebuild from/); expect(remoteOpsProcessingServiceSpy.processRemoteOps).not.toHaveBeenCalled(); - expect(opLogStoreSpy.clearAllOperations).not.toHaveBeenCalled(); + expect(opLogStoreSpy.runRemoteStateReplacement).not.toHaveBeenCalled(); expect(setLastServerSeqSpy).not.toHaveBeenCalled(); }); @@ -2753,9 +2937,9 @@ describe('OperationLogSyncService', () => { expect(setLastServerSeqSpy).toHaveBeenCalledWith(1); }); - it('should propagate errors from clearAllOperations', async () => { + it('should propagate errors from the atomic replacement', async () => { const error = new Error('Failed to clear ops'); - opLogStoreSpy.clearAllOperations.and.rejectWith(error); + opLogStoreSpy.runRemoteStateReplacement.and.rejectWith(error); downloadServiceSpy.downloadRemoteOps.and.resolveTo({ newOps: [makeRemoteOp()], needsFullStateUpload: false, @@ -2816,7 +3000,10 @@ describe('OperationLogSyncService', () => { await service.forceDownloadRemoteState(mockProvider); expect(remoteOpsProcessingServiceSpy.processRemoteOps).toHaveBeenCalledWith( mockOps, - { skipConflictDetection: true }, + { + skipConflictDetection: true, + callerHoldsOperationLogLock: true, + }, ); }); }); @@ -3504,7 +3691,7 @@ describe('OperationLogSyncService', () => { expect(result.kind).toBe('ops_processed'); }); - it('should process incoming SYNC_IMPORT when pending ops are config-only', async () => { + it('should prompt before replacing pending user config with an incoming SYNC_IMPORT', async () => { const incomingSyncImport = createIncomingSyncImport(); downloadServiceSpy.downloadRemoteOps.and.resolveTo({ @@ -3542,19 +3729,14 @@ describe('OperationLogSyncService', () => { const result = await service.downloadRemoteOps(mockProvider); - expect( - syncImportConflictDialogServiceSpy.showConflictDialog, - ).not.toHaveBeenCalled(); - // No example-task ops pending -> nothing is rejected (empty-array guard). + expect(syncImportConflictDialogServiceSpy.showConflictDialog).toHaveBeenCalled(); expect(opLogStoreSpy.markRejected).not.toHaveBeenCalled(); - expect(remoteOpsProcessingServiceSpy.processRemoteOps).toHaveBeenCalledWith([ - incomingSyncImport, - ]); - expect(mockProvider.setLastServerSeq).toHaveBeenCalledWith(42); - expect(result.kind).toBe('ops_processed'); + expect(remoteOpsProcessingServiceSpy.processRemoteOps).not.toHaveBeenCalled(); + expect(mockProvider.setLastServerSeq).not.toHaveBeenCalled(); + expect(result.kind).toBe('cancelled'); }); - it('should process incoming SYNC_IMPORT when pending ops are only config and startup example tasks', async () => { + it('should prompt when pending user config exists alongside startup example tasks', async () => { const incomingSyncImport = createIncomingSyncImport(); downloadServiceSpy.downloadRemoteOps.and.resolveTo({ @@ -3619,20 +3801,11 @@ describe('OperationLogSyncService', () => { const result = await service.downloadRemoteOps(mockProvider); - expect( - syncImportConflictDialogServiceSpy.showConflictDialog, - ).not.toHaveBeenCalled(); - expect(opLogStoreSpy.markRejected).toHaveBeenCalledWith([ - 'local-example-task-op-1', - 'local-example-task-op-2', - 'local-example-task-op-3', - 'local-example-task-op-4', - ]); - expect(remoteOpsProcessingServiceSpy.processRemoteOps).toHaveBeenCalledWith([ - incomingSyncImport, - ]); - expect(mockProvider.setLastServerSeq).toHaveBeenCalledWith(42); - expect(result.kind).toBe('ops_processed'); + expect(syncImportConflictDialogServiceSpy.showConflictDialog).toHaveBeenCalled(); + expect(opLogStoreSpy.markRejected).not.toHaveBeenCalled(); + expect(remoteOpsProcessingServiceSpy.processRemoteOps).not.toHaveBeenCalled(); + expect(mockProvider.setLastServerSeq).not.toHaveBeenCalled(); + expect(result.kind).toBe('cancelled'); }); }); @@ -3673,6 +3846,7 @@ describe('OperationLogSyncService', () => { piggybackedOps: [piggybackedSyncImport], rejectedCount: 0, rejectedOps: [], + pendingAcknowledgementSeqs: [1], }); // Client has pending ops @@ -3709,6 +3883,7 @@ describe('OperationLogSyncService', () => { syncImportReason: 'SERVER_MIGRATION', }), ); + expect(opLogStoreSpy.markSynced).not.toHaveBeenCalled(); expect(result.kind).toBe('cancelled'); }); @@ -3725,13 +3900,6 @@ describe('OperationLogSyncService', () => { schemaVersion: 1, }; - uploadServiceSpy.uploadPendingOps.and.resolveTo({ - uploadedCount: 1, - piggybackedOps: [piggybackedSyncImport], - rejectedCount: 0, - rejectedOps: [], - }); - const pendingEntry: OperationLogEntry = { seq: 1, op: { @@ -3749,14 +3917,17 @@ describe('OperationLogSyncService', () => { appliedAt: Date.now(), source: 'local', }; - // The op is pending BEFORE the upload but marked synced DURING it (server - // accepted it in the same round that piggybacked the import) — a live - // post-upload read no longer sees it. - let getUnsyncedCalls = 0; - opLogStoreSpy.getUnsynced.and.callFake(async () => { - getUnsyncedCalls++; - return getUnsyncedCalls === 1 ? [pendingEntry] : []; + uploadServiceSpy.uploadPendingOps.and.resolveTo({ + uploadedCount: 1, + piggybackedOps: [piggybackedSyncImport], + rejectedCount: 0, + rejectedOps: [], + selectedPendingOps: [pendingEntry], + pendingAcknowledgementSeqs: [pendingEntry.seq], }); + // The accepted operation is represented by the exact in-lock upload snapshot; + // it no longer needs to remain live-unsynced for the gate to protect it. + opLogStoreSpy.getUnsynced.and.resolveTo([]); syncImportConflictDialogServiceSpy.showConflictDialog.and.resolveTo('CANCEL'); @@ -3771,6 +3942,7 @@ describe('OperationLogSyncService', () => { expect(syncImportConflictDialogServiceSpy.showConflictDialog).toHaveBeenCalledWith( jasmine.objectContaining({ scenario: 'INCOMING_IMPORT' }), ); + expect(opLogStoreSpy.markSynced).not.toHaveBeenCalled(); expect(result.kind).toBe('cancelled'); }); @@ -3819,7 +3991,7 @@ describe('OperationLogSyncService', () => { expect(result.kind).toBe('completed'); }); - it('should not flush again before checking piggybacked SYNC_IMPORT conflicts', async () => { + it('should flush again before checking piggybacked SYNC_IMPORT conflicts', async () => { const piggybackedSyncImport: Operation = { id: 'import-1', clientId: 'client-B', @@ -3846,7 +4018,7 @@ describe('OperationLogSyncService', () => { const result = await service.uploadPendingOps(mockProvider); - expect(writeFlushServiceSpy.flushPendingWrites).toHaveBeenCalledTimes(1); + expect(writeFlushServiceSpy.flushPendingWrites).toHaveBeenCalledTimes(2); expect(opLogStoreSpy.getUnsynced).toHaveBeenCalled(); expect(result.kind).toBe('completed'); }); @@ -3898,6 +4070,7 @@ describe('OperationLogSyncService', () => { }); it('should process piggybacked ops normally when no SYNC_IMPORT present', async () => { + const events: string[] = []; const piggybackedOp: Operation = { id: 'op-1', clientId: 'client-B', @@ -3916,9 +4089,23 @@ describe('OperationLogSyncService', () => { piggybackedOps: [piggybackedOp], rejectedCount: 0, rejectedOps: [], + pendingAcknowledgementSeqs: [1], }); opLogStoreSpy.getUnsynced.and.resolveTo([]); + remoteOpsProcessingServiceSpy.processRemoteOps.and.callFake(async () => { + events.push('processRemoteOps'); + return { + localWinOpsCreated: 0, + allOpsFilteredBySyncImport: false, + filteredOpCount: 0, + isLocalUnsyncedImport: false, + blockedByIncompatibleOp: false, + }; + }); + opLogStoreSpy.markSynced.and.callFake(async () => { + events.push('markSynced'); + }); const mockProvider = { isReady: () => Promise.resolve(true), @@ -3934,6 +4121,8 @@ describe('OperationLogSyncService', () => { expect(remoteOpsProcessingServiceSpy.processRemoteOps).toHaveBeenCalledWith([ piggybackedOp, ]); + expect(opLogStoreSpy.markSynced).toHaveBeenCalledOnceWith([1]); + expect(events).toEqual(['processRemoteOps', 'markSynced']); expect(result.kind).not.toBe('cancelled'); }); diff --git a/src/app/op-log/sync/operation-log-sync.service.ts b/src/app/op-log/sync/operation-log-sync.service.ts index 7fd655e5f8..6d97e0d7ce 100644 --- a/src/app/op-log/sync/operation-log-sync.service.ts +++ b/src/app/op-log/sync/operation-log-sync.service.ts @@ -38,13 +38,23 @@ import { SyncImportConflictResolution, } from './dialog-sync-import-conflict/dialog-sync-import-conflict.component'; import { SyncImportConflictGateService } from './sync-import-conflict-gate.service'; -import { CURRENT_SCHEMA_VERSION } from '../persistence/schema-migration.service'; +import { + CURRENT_SCHEMA_VERSION, + MIN_SUPPORTED_SCHEMA_VERSION, + SchemaMigrationService, + getOperationSchemaVersion, +} from '../persistence/schema-migration.service'; import { SyncProviderManager } from '../sync-providers/provider-manager.service'; -import { getDefaultMainModelData } from '../model/model-config'; +import { getDefaultMainModelData, MODEL_CONFIGS } from '../model/model-config'; import { loadAllData } from '../../root-store/meta/load-all-data.action'; import { SyncLocalStateService } from './sync-local-state.service'; import { SyncImportConflictCoordinatorService } from './sync-import-conflict-coordinator.service'; import { isExampleTaskCreateOp } from '../validation/is-example-task-op.util'; +import { Operation } from '../core/operation.types'; +import { LockService } from './lock.service'; +import { LOCK_NAMES } from '../core/operation-log.const'; +import { ValidateStateService } from '../validation/validate-state.service'; +import { extractEntityKeysFromState } from '../persistence/extract-entity-keys'; /** * Orchestrates synchronization of the Operation Log with remote storage. @@ -122,6 +132,9 @@ export class OperationLogSyncService { private superSyncStatusService = inject(SuperSyncStatusService); private serverMigrationService = inject(ServerMigrationService); private writeFlushService = inject(OperationWriteFlushService); + private lockService = inject(LockService); + private schemaMigrationService = inject(SchemaMigrationService); + private validateStateService = inject(ValidateStateService); // Extracted services private remoteOpsProcessingService = inject(RemoteOpsProcessingService); @@ -189,11 +202,9 @@ export class OperationLogSyncService { // we would upload an incomplete set. This flush waits for all queued writes. await this.writeFlushService.flushPendingWrites(); - // Capture never-synced status BEFORE the upload runs: uploadService.uploadPendingOps - // marks accepted ops synced, which flips hasSyncedOps() and would defeat the piggyback - // conflict gate's never-synced guard if it read live state afterwards. The orchestrator - // passes a value captured even earlier (pre-download, since download also persists - // synced ops); fall back to a local read for standalone upload callers. + // Capture never-synced status before the upload runs. The orchestrator passes a + // value captured even earlier (pre-download, since download persists synced ops); + // fall back to a local read for standalone upload callers. const isNeverSyncedAtSyncStart = options?.isNeverSynced ?? !(await this.opLogStore.hasSyncedOps()); @@ -209,14 +220,6 @@ export class OperationLogSyncService { return { kind: 'blocked_fresh_client' }; } - // Capture pending ops BEFORE the upload round (the flush at the top of this - // method already drained in-flight writes). The piggyback conflict gate below - // must judge "would this import discard local work?" against this snapshot: - // ops accepted by the server during the upload are marked synced per chunk, - // so a post-upload getUnsynced() read no longer sees local work that a - // piggybacked import is about to replace. - const pendingOpsAtUploadStart = await this.opLogStore.getUnsynced(); - // SERVER MIGRATION CHECK: Passed as callback to execute INSIDE the upload lock. // This prevents race conditions where multiple tabs could both detect migration // and create duplicate SYNC_IMPORT operations. @@ -227,6 +230,9 @@ export class OperationLogSyncService { ? undefined : () => this.serverMigrationService.checkAndHandleMigration(syncProvider), skipPiggybackProcessing: options?.skipPiggybackProcessing, + // Keep accepted operations pending until piggyback processing commits. This + // preserves the conflict gate across cancellation and crash/retry boundaries. + deferAcknowledgement: true, }); // STEP 1: Process piggybacked ops FIRST @@ -258,7 +264,8 @@ export class OperationLogSyncService { result.piggybackedOps, { isNeverSynced: isNeverSyncedAtSyncStart, - preCapturedPendingOps: pendingOpsAtUploadStart, + flushPendingWrites: true, + preCapturedPendingOps: result.selectedPendingOps ?? [], }, ); if (piggybackedConflict.fullStateOp) { @@ -319,6 +326,10 @@ export class OperationLogSyncService { localWinOpsCreated = processResult.localWinOpsCreated; // Validation failure (if any) is on the session-validation latch. + if (processResult.blockedByIncompatibleOp) { + return { kind: 'blocked_incompatible' }; + } + // #8304: Persist lastServerSeq ONLY now that the piggybacked ops have been applied // above. The upload service deferred this (see UploadResult.lastServerSeqToPersist) // so that a crash — or a cancelled/USE_REMOTE/USE_LOCAL SYNC_IMPORT dialog, all of @@ -326,14 +337,16 @@ export class OperationLogSyncService { // that were never stored. Mirrors the download path's invariant. // A version/migration block keeps the cursor behind the blocked op so it is // re-downloaded and retried after an app update instead of skipped forever. - if ( - result.lastServerSeqToPersist !== undefined && - !processResult.blockedByIncompatibleOp - ) { + if (result.lastServerSeqToPersist !== undefined) { await syncProvider.setLastServerSeq(result.lastServerSeqToPersist); } } + const pendingAcknowledgementSeqs = result.pendingAcknowledgementSeqs ?? []; + if (pendingAcknowledgementSeqs.length > 0) { + await this.opLogStore.markSynced(pendingAcknowledgementSeqs); + } + // STEP 2: Handle server-rejected operations // handleRejectedOps may create merged ops for concurrent modifications. // These need to be uploaded, so we add them to localWinOpsCreated. @@ -367,6 +380,8 @@ export class OperationLogSyncService { case 'server_migration_handled': case 'cancelled': return { newOpsCount: 0 }; + case 'blocked_incompatible': + throw new Error('Nested download blocked by an incompatible remote operation.'); } }; try { @@ -878,6 +893,10 @@ export class OperationLogSyncService { result.newOps, ); + if (processResult.blockedByIncompatibleOp) { + return { kind: 'blocked_incompatible' }; + } + // ───────────────────────────────────────────────────────────────────────── // Handle SYNC_IMPORT conflict: all remote ops filtered by STORED local import. // This happens when user imports/restores data locally, and other devices @@ -933,7 +952,7 @@ export class OperationLogSyncService { // This is the correct behavior - better to re-download than to skip ops. // A version/migration block keeps the cursor behind the blocked op so it is // re-downloaded and retried after an app update instead of skipped forever. - if (result.latestServerSeq !== undefined && !processResult.blockedByIncompatibleOp) { + if (result.latestServerSeq !== undefined) { await syncProvider.setLastServerSeq(result.latestServerSeq); } @@ -1076,10 +1095,14 @@ export class OperationLogSyncService { result.newOps, ); + if (processResult.blockedByIncompatibleOp) { + return { kind: 'blocked_incompatible' }; + } + // Persist the cursor only AFTER the ops are applied, matching the normal // incremental path's crash-safety ordering. A version/migration block keeps // the cursor behind the blocked op (retried after an app update). - if (result.latestServerSeq !== undefined && !processResult.blockedByIncompatibleOp) { + if (result.latestServerSeq !== undefined) { await syncProvider.setLastServerSeq(result.latestServerSeq); } @@ -1156,24 +1179,6 @@ export class OperationLogSyncService { 'OperationLogSyncService: Force downloading remote state for a full rebuild.', ); - // Safety net (#8107): snapshot current local state before we destroy it, so an - // unintended "Use Server Data" — which can roll the user back to a stale server - // snapshot — is reversible via the Undo affordance shown on success below. - // Fail-safe: if the backup can't be written, abort rather than wipe without a - // recovery point (mirrors BackupService._persistImportToOperationLog). - let backupSavedAt: number; - try { - backupSavedAt = await this.backupService.captureImportBackup(); - } catch (e) { - OpLog.warn( - 'OperationLogSyncService: Pre-replace safety backup failed; aborting force download.', - { name: (e as Error | undefined)?.name }, - ); - throw new Error( - 'Pre-replace safety backup failed; aborting to preserve local state.', - ); - } - // ───────────────────────────────────────────────────────────────────────── // PHASE 1 — download and validate. Nothing local is mutated until the // complete server history is in memory: a network failure here must leave @@ -1207,23 +1212,19 @@ export class OperationLogSyncService { throw new Error('USE_REMOTE aborted: remote returned no data to rebuild from.'); } - // Refuse BEFORE destroying anything if the remote history contains ops this - // client cannot interpret (newer schema) — replay would block midway and - // leave a partial rebuild. Migration exceptions can still surface during - // replay; those are handled after processRemoteOps below. - const tooNewOp = result.newOps.find( - (op) => (op.schemaVersion ?? 1) > CURRENT_SCHEMA_VERSION, - ); - if (tooNewOp) { - this.snackService.open({ - type: 'ERROR', - msg: T.F.SYNC.S.VERSION_TOO_OLD, - actionStr: T.PS.UPDATE_APP, - actionFn: () => window.open('https://super-productivity.com/download', '_blank'), - }); - throw new Error( - 'USE_REMOTE aborted: remote history contains ops from a newer schema version — update the app first.', - ); + const migratedRemoteOps = this._preflightRemoteOperations(result.newOps); + + let snapshotState = result.snapshotState as Record | undefined; + if (hasSnapshotState && snapshotState) { + const validation = await this.validateStateService.validateAndRepair(snapshotState); + if (!validation.isValid) { + throw new Error( + 'USE_REMOTE aborted: remote snapshot is invalid and could not be repaired.', + ); + } + if (validation.wasRepaired && validation.repairedState) { + snapshotState = validation.repairedState; + } } // ───────────────────────────────────────────────────────────────────────── @@ -1234,9 +1235,6 @@ export class OperationLogSyncService { // variant made the duplicate filter skip them, so a "rebuild" replayed only // an unseen suffix onto a defaults reset. // ───────────────────────────────────────────────────────────────────────── - await this.opLogStore.clearAllOperations(); - await syncProvider.setLastServerSeq(0); - // Reset the vector clock to the remote's causal knowledge (snapshot clock // merged with every downloaded op clock). This also drops entries from // rejected local ops that would otherwise pollute conflict detection. @@ -1244,113 +1242,202 @@ export class OperationLogSyncService { for (const opClock of result.allOpClocks ?? []) { rebuiltClock = mergeVectorClocks(rebuiltClock, opClock); } - await this.opLogStore.setVectorClock(rebuiltClock); - OpLog.normal('OperationLogSyncService: Reset vector clock to rebuilt remote clock.'); - - // FILE-BASED SYNC: Handle snapshot state from force download. - // When downloading from seq 0 on file-based providers, we may receive a - // snapshotState instead of incremental ops. This happens when the remote - // has a SYNC_IMPORT (full state snapshot) with empty recentOps. - // hydrateFromRemoteSync persists its own state cache + vector clock. - if (result.providerMode === 'fileSnapshotOps' && result.snapshotState) { - OpLog.normal( - 'OperationLogSyncService: Force download received snapshotState. Hydrating...', - ); - - // Hydrate from snapshot - DON'T create SYNC_IMPORT since we're - // accepting remote state, not uploading local state. - await this.syncHydrationService.hydrateFromRemoteSync( - result.snapshotState as Record, - result.snapshotVectorClock, - false, // Don't create SYNC_IMPORT - ); - - // CRITICAL FIX: Write recentOps to IndexedDB after snapshot hydration. - // Same rationale as downloadRemoteOps: file-based providers return ALL - // recentOps on every download and rely on getAppliedOpIds() to filter them. - if (result.newOps.length > 0) { - const appendResult = await this.opLogStore.appendBatchSkipDuplicates( - result.newOps, - 'remote', - ); - OpLog.normal( - `OperationLogSyncService: Wrote ${appendResult.writtenOps.length} snapshot ops to IndexedDB ` + - 'after force-download hydration.' + - (appendResult.skippedCount > 0 - ? ` Skipped ${appendResult.skippedCount} duplicate(s).` - : ''), - ); - } - - // Update lastServerSeq after hydration - if (result.latestServerSeq !== undefined) { - await syncProvider.setLastServerSeq(result.latestServerSeq); - } - - OpLog.normal( - 'OperationLogSyncService: Force download snapshot hydration complete.', - ); - this._showRestorePreviousDataSnack(backupSavedAt); - return; - } - - // Crash safety: persist a defaults baseline so an interrupted replay - // hydrates as defaults + appended prefix on next boot (a consistent prefix - // of server state, completed by the next sync from cursor 0) instead of - // layering the new history onto the stale pre-replace snapshot. - await this.opLogStore.saveStateCache({ - state: getDefaultMainModelData(), - lastAppliedOpSeq: 0, - vectorClock: rebuiltClock, - compactedAt: Date.now(), - snapshotEntityKeys: [], - }); - - // Reset live state to defaults, then replay the COMPLETE server history on - // top. A full-state op in the history replaces state again by its own - // semantics; a purely incremental history rebuilds from this baseline. const defaultData = getDefaultMainModelData(); - this.store.dispatch( - loadAllData({ - appDataComplete: defaultData as Parameters< - typeof loadAllData - >[0]['appDataComplete'], - }), - ); - // Brief yield to let NgRx process the state reset - await new Promise((resolve) => setTimeout(resolve, 0)); + const baselineState = snapshotState ?? defaultData; + const archiveYoung = + (snapshotState?.[ + 'archiveYoung' + ] as typeof MODEL_CONFIGS.archiveYoung.defaultData) ?? + MODEL_CONFIGS.archiveYoung.defaultData!; + const archiveOld = + (snapshotState?.['archiveOld'] as typeof MODEL_CONFIGS.archiveOld.defaultData) ?? + MODEL_CONFIGS.archiveOld.defaultData!; - // Process all remote ops (no confirmation needed - user already chose USE_REMOTE). - // Skip conflict detection because the NgRx store was just reset to empty state, - // which causes all entities to appear missing and CONCURRENT ops to be discarded. - // Validation failure is surfaced via the session-validation latch. (#7330) - const processResult = await this.remoteOpsProcessingService.processRemoteOps( - result.newOps, - { skipConflictDetection: true }, - ); + let capturedBackupSavedAt: number | undefined; + let replacementCommitted = false; + let backupSavedAt: number; + try { + backupSavedAt = await this.lockService.request( + LOCK_NAMES.OPERATION_LOG, + async () => { + // Include actions dispatched while the network request and preflight were + // in flight in the reversible safety backup. + await this.writeFlushService.flushPendingWrites(); + let savedAt: number; + try { + savedAt = await this.backupService.captureImportBackup(); + capturedBackupSavedAt = savedAt; + } catch (e) { + OpLog.warn( + 'OperationLogSyncService: Pre-replace safety backup failed; aborting force download.', + { name: (e as Error | undefined)?.name }, + ); + throw new Error( + 'Pre-replace safety backup failed; aborting to preserve local state.', + ); + } - if (processResult.blockedByIncompatibleOp) { - // Version blocks were pre-checked above; only a migration exception lands - // here. The rebuild is partial: keep the cursor at 0 so the next sync - // retries the remainder, and surface the failure — the Undo snack still - // offers the pre-replace backup. - this._showRestorePreviousDataSnack(backupSavedAt); - throw new Error( - 'USE_REMOTE incomplete: an op failed schema migration during replay.', + // The provider cursor lives outside SUP_OPS, so it cannot join the IDB + // transaction. Reset it first: a crash/failure before the transaction + // merely causes a safe re-download onto intact local state, while a + // commit can never become visible with a stale cursor that skips the + // remote history required to rebuild the baseline. + await syncProvider.setLastServerSeq(0); + await this.opLogStore.runRemoteStateReplacement({ + baselineState, + vectorClock: rebuiltClock, + schemaVersion: CURRENT_SCHEMA_VERSION, + snapshotEntityKeys: extractEntityKeysFromState( + baselineState as Parameters[0], + ), + archiveYoung, + archiveOld, + }); + replacementCommitted = true; + + OpLog.normal( + 'OperationLogSyncService: Replaced local persistence with remote baseline.', + ); + + // FILE-BASED SYNC: Handle snapshot state from force download. + // When downloading from seq 0 on file-based providers, we may receive a + // snapshotState instead of incremental ops. This happens when the remote + // has a SYNC_IMPORT (full state snapshot) with empty recentOps. + // hydrateFromRemoteSync persists its own state cache + vector clock. + if (result.providerMode === 'fileSnapshotOps' && snapshotState) { + OpLog.normal( + 'OperationLogSyncService: Force download received snapshotState. Hydrating...', + ); + + // Hydrate from snapshot - DON'T create SYNC_IMPORT since we're + // accepting remote state, not uploading local state. + await this.syncHydrationService.hydrateFromRemoteSync( + snapshotState, + result.snapshotVectorClock, + false, // Don't create SYNC_IMPORT + ); + + // CRITICAL FIX: Write recentOps to IndexedDB after snapshot hydration. + // Same rationale as downloadRemoteOps: file-based providers return ALL + // recentOps on every download and rely on getAppliedOpIds() to filter them. + if (migratedRemoteOps.length > 0) { + const appendResult = await this.opLogStore.appendBatchSkipDuplicates( + migratedRemoteOps, + 'remote', + ); + OpLog.normal( + `OperationLogSyncService: Wrote ${appendResult.writtenOps.length} snapshot ops to IndexedDB ` + + 'after force-download hydration.' + + (appendResult.skippedCount > 0 + ? ` Skipped ${appendResult.skippedCount} duplicate(s).` + : ''), + ); + } + + // Update lastServerSeq after hydration + if (result.latestServerSeq !== undefined) { + await syncProvider.setLastServerSeq(result.latestServerSeq); + } + + OpLog.normal( + 'OperationLogSyncService: Force download snapshot hydration complete.', + ); + return savedAt; + } + + // Reset live state to defaults, then replay the COMPLETE server history on + // top. A full-state op in the history replaces state again by its own + // semantics; a purely incremental history rebuilds from this baseline. + this.store.dispatch( + loadAllData({ + appDataComplete: defaultData as Parameters< + typeof loadAllData + >[0]['appDataComplete'], + }), + ); + // Brief yield to let NgRx process the state reset + await new Promise((resolve) => setTimeout(resolve, 0)); + + // Process all remote ops (no confirmation needed - user already chose USE_REMOTE). + // Skip conflict detection because the NgRx store was just reset to empty state, + // which causes all entities to appear missing and CONCURRENT ops to be discarded. + // Validation failure is surfaced via the session-validation latch. (#7330) + const processResult = await this.remoteOpsProcessingService.processRemoteOps( + migratedRemoteOps, + { + skipConflictDetection: true, + callerHoldsOperationLogLock: true, + }, + ); + + if (processResult.blockedByIncompatibleOp) { + // Version blocks were pre-checked above; only a migration exception lands + // here. The rebuild is partial: keep the cursor at 0 so the next sync + // retries the remainder, and surface the failure — the Undo snack still + // offers the pre-replace backup. + throw new Error( + 'USE_REMOTE incomplete: an op failed schema migration during replay.', + ); + } + + // Update lastServerSeq + if (result.latestServerSeq !== undefined) { + await syncProvider.setLastServerSeq(result.latestServerSeq); + } + + OpLog.normal( + `OperationLogSyncService: Force download complete. Rebuilt from ${migratedRemoteOps.length} ops.`, + ); + return savedAt; + }, ); + } catch (e) { + if (replacementCommitted && capturedBackupSavedAt !== undefined) { + this._showRestorePreviousDataSnack(capturedBackupSavedAt); + } + throw e; } - // Update lastServerSeq - if (result.latestServerSeq !== undefined) { - await syncProvider.setLastServerSeq(result.latestServerSeq); - } - - OpLog.normal( - `OperationLogSyncService: Force download complete. Rebuilt from ${result.newOps.length} ops.`, - ); this._showRestorePreviousDataSnack(backupSavedAt); } + private _preflightRemoteOperations(remoteOps: Operation[]): Operation[] { + for (const op of remoteOps) { + let version: number; + try { + version = getOperationSchemaVersion(op as { schemaVersion?: unknown }); + } catch { + throw new Error( + 'USE_REMOTE aborted: remote history has an invalid schema version.', + ); + } + + if (version < MIN_SUPPORTED_SCHEMA_VERSION) { + throw new Error( + 'USE_REMOTE aborted: remote history contains an unsupported schema version.', + ); + } + if (version > CURRENT_SCHEMA_VERSION) { + this.snackService.open({ + type: 'ERROR', + msg: T.F.SYNC.S.VERSION_TOO_OLD, + actionStr: T.PS.UPDATE_APP, + actionFn: () => + window.open('https://super-productivity.com/download', '_blank'), + }); + throw new Error( + 'USE_REMOTE aborted: remote history contains ops from a newer schema version — update the app first.', + ); + } + } + + try { + return this.schemaMigrationService.migrateOperations(remoteOps); + } catch { + throw new Error('USE_REMOTE aborted: remote operation migration failed.'); + } + } + /** * Shows a non-blocking snack after a destructive "Use Server Data" replace, * offering to restore the local snapshot captured before the wipe — making the diff --git a/src/app/op-log/sync/operation-log-upload.service.spec.ts b/src/app/op-log/sync/operation-log-upload.service.spec.ts index 8a9a5fac18..ea4a810525 100644 --- a/src/app/op-log/sync/operation-log-upload.service.spec.ts +++ b/src/app/op-log/sync/operation-log-upload.service.spec.ts @@ -320,6 +320,30 @@ describe('OperationLogUploadService', () => { expect(mockOpLogStore.markSynced).toHaveBeenCalledWith([1, 2]); }); + it('should defer acknowledgements and return the exact selected batch for piggyback resolution', async () => { + const pendingOps = [ + createMockEntry(1, 'op-1', 'client-1'), + createMockEntry(2, 'op-2', 'client-1'), + ]; + mockOpLogStore.getUnsynced.and.resolveTo(pendingOps); + mockApiProvider.uploadOps.and.resolveTo({ + results: [ + { opId: 'op-1', accepted: true }, + { opId: 'op-2', accepted: true }, + ], + latestSeq: 10, + newOps: [], + }); + + const result = await service.uploadPendingOps(mockApiProvider, { + deferAcknowledgement: true, + }); + + expect(mockOpLogStore.markSynced).not.toHaveBeenCalled(); + expect(result.selectedPendingOps).toEqual(pendingOps); + expect(result.pendingAcknowledgementSeqs).toEqual([1, 2]); + }); + it('should mark accepted seqs correctly when server results are out of order', async () => { const pendingOps = [ createMockEntry(1, 'op-1', 'client-1'), diff --git a/src/app/op-log/sync/operation-log-upload.service.ts b/src/app/op-log/sync/operation-log-upload.service.ts index 93bef79ca4..29beba2f14 100644 --- a/src/app/op-log/sync/operation-log-upload.service.ts +++ b/src/app/op-log/sync/operation-log-upload.service.ts @@ -86,6 +86,24 @@ export class OperationLogUploadService { let uploadedCount = 0; let rejectedCount = 0; let hasMorePiggyback = false; + let selectedPendingOps: OperationLogEntry[] = []; + const pendingAcknowledgementSeqs: number[] = []; + const pendingAcknowledgementSeqSet = new Set(); + const acknowledge = async (seqs: number[]): Promise => { + if (seqs.length === 0) { + return; + } + if (!options?.deferAcknowledgement) { + await this.opLogStore.markSynced(seqs); + return; + } + for (const seq of seqs) { + if (!pendingAcknowledgementSeqSet.has(seq)) { + pendingAcknowledgementSeqSet.add(seq); + pendingAcknowledgementSeqs.push(seq); + } + } + }; // Track encryption state of piggybacked operations for detecting encryption config mismatch. // When another client disables encryption, all piggybacked ops will be unencrypted. // We track this BEFORE decryption to detect the server's actual encryption state. @@ -107,6 +125,7 @@ export class OperationLogUploadService { } const pendingOps = await this.opLogStore.getUnsynced(); + selectedPendingOps = pendingOps; if (pendingOps.length === 0) { OpLog.normal('OperationLogUploadService: No pending operations to upload.'); @@ -198,7 +217,7 @@ export class OperationLogUploadService { isCleanSlateForOp, ); if (result.accepted) { - await this.opLogStore.markSynced([entry.seq]); + await acknowledge([entry.seq]); uploadedCount++; if (result.serverSeq !== undefined) { await syncProvider.setLastServerSeq(result.serverSeq); @@ -258,7 +277,7 @@ export class OperationLogUploadService { if (opsIncludedInSnapshot.length > 0) { const seqs = opsIncludedInSnapshot.map((entry) => entry.seq); - await this.opLogStore.markSynced(seqs); + await acknowledge(seqs); uploadedCount += seqs.length; OpLog.normal( `OperationLogUploadService: Marked ${seqs.length} regular ops as synced ` + @@ -293,7 +312,7 @@ export class OperationLogUploadService { } } if (localOnlySeqs.length > 0) { - await this.opLogStore.markSynced(localOnlySeqs); + await acknowledge(localOnlySeqs); uploadedCount += localOnlySeqs.length; OpLog.normal( `OperationLogUploadService: Marked ${localOnlySeqs.length} local-only op(s) as synced without upload`, @@ -339,7 +358,7 @@ export class OperationLogUploadService { .filter((seq): seq is number => seq !== undefined); if (acceptedSeqs.length > 0) { - await this.opLogStore.markSynced(acceptedSeqs); + await acknowledge(acceptedSeqs); uploadedCount += acceptedSeqs.length; } @@ -468,6 +487,9 @@ export class OperationLogUploadService { ...(piggybackHasOnlyUnencryptedData ? { piggybackHasOnlyUnencryptedData } : {}), ...(lastServerSeqToPersist !== undefined ? { lastServerSeqToPersist } : {}), ...(encryptionRequiredKeyMissing ? { encryptionRequiredKeyMissing: true } : {}), + ...(options?.deferAcknowledgement + ? { selectedPendingOps, pendingAcknowledgementSeqs } + : {}), }; } diff --git a/src/app/op-log/sync/remote-ops-processing.service.spec.ts b/src/app/op-log/sync/remote-ops-processing.service.spec.ts index c59653fc0f..2c77b7527f 100644 --- a/src/app/op-log/sync/remote-ops-processing.service.spec.ts +++ b/src/app/op-log/sync/remote-ops-processing.service.spec.ts @@ -73,6 +73,7 @@ describe('RemoteOpsProcessingService', () => { 'append', 'appendBatchSkipDuplicates', 'appendWithVectorClockUpdate', + 'markArchivePending', 'markApplied', 'markFailed', 'mergeRemoteOpClocks', @@ -369,6 +370,57 @@ describe('RemoteOpsProcessingService', () => { ]); }); + it('should log conflict identities without logging operation payloads', async () => { + const localOp = { + id: 'local-op', + entityType: 'TASK', + entityId: 'task-1', + payload: { title: 'private local title' }, + } as Operation; + const remoteOp = { + id: 'remote-op', + entityType: 'TASK', + entityId: 'task-1', + payload: { title: 'private remote title' }, + schemaVersion: 1, + } as Operation; + spyOn(service, 'detectConflicts').and.resolveTo({ + nonConflicting: [], + conflicts: [ + { + entityType: 'TASK', + entityId: 'task-1', + localOps: [localOp], + remoteOps: [remoteOp], + suggestedResolution: 'manual', + }, + ], + }); + conflictResolutionServiceSpy.autoResolveConflictsLWW.and.resolveTo({ + localWinOpsCreated: 0, + }); + vectorClockServiceSpy.getEntityFrontier.and.resolveTo(new Map()); + const warnSpy = spyOn(OpLog, 'warn'); + + await service.processRemoteOps([remoteOp]); + + const summary = warnSpy.calls + .allArgs() + .find(([message]) => String(message).includes('Detected 1 conflicts'))?.[1]; + expect(summary).toEqual({ + conflicts: [ + { + entityType: 'TASK', + entityId: 'task-1', + localOpIds: ['local-op'], + remoteOpIds: ['remote-op'], + suggestedResolution: 'manual', + }, + ], + }); + expect(JSON.stringify(summary)).not.toContain('private'); + }); + it('should drop operations if migrateOperation returns null', async () => { const remoteOps: Operation[] = [ { id: 'op1', schemaVersion: 1 } as Operation, @@ -516,6 +568,21 @@ describe('RemoteOpsProcessingService', () => { expect(result.blockedByIncompatibleOp).toBe(true); }); + for (const invalidVersion of [null, '2', 1.5, {}, Number.NaN]) { + it(`should block malformed schemaVersion ${String(invalidVersion)}`, async () => { + const remoteOp = { + id: 'malformed-version', + schemaVersion: invalidVersion, + } as unknown as Operation; + + const result = await service.processRemoteOps([remoteOp]); + + expect(result.blockedByIncompatibleOp).toBeTrue(); + expect(schemaMigrationServiceSpy.migrateOperation).not.toHaveBeenCalled(); + expect(opLogStoreSpy.appendBatchSkipDuplicates).not.toHaveBeenCalled(); + }); + } + it('should process the prefix before a too-new op but flag the block', async () => { const remoteOps: Operation[] = [ { id: 'op1', schemaVersion: 1 } as Operation, @@ -1337,9 +1404,10 @@ describe('RemoteOpsProcessingService', () => { opLogStoreSpy.hasOp.and.returnValue(Promise.resolve(false)); opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ appliedOps: remoteOps }), - ); + operationApplierServiceSpy.applyOperations.and.callFake(async (ops, options) => { + await options?.onReducersCommitted?.(ops); + return { appliedOps: remoteOps }; + }); await service.applyNonConflictingOps(remoteOps); @@ -1352,10 +1420,14 @@ describe('RemoteOpsProcessingService', () => { ]; const callOrder: string[] = []; - operationApplierServiceSpy.applyOperations.and.callFake(async () => { + operationApplierServiceSpy.applyOperations.and.callFake(async (ops, options) => { callOrder.push('applyOperations'); + await options?.onReducersCommitted?.(ops); return { appliedOps: remoteOps }; }); + opLogStoreSpy.markArchivePending.and.callFake(async () => { + callOrder.push('markArchivePending'); + }); opLogStoreSpy.markApplied.and.callFake(async () => { callOrder.push('markApplied'); }); @@ -1368,16 +1440,21 @@ describe('RemoteOpsProcessingService', () => { await service.applyNonConflictingOps(remoteOps, true); - expect(operationApplierServiceSpy.applyOperations).toHaveBeenCalledWith(remoteOps, { - skipDeferredLocalActions: true, - }); + expect(operationApplierServiceSpy.applyOperations).toHaveBeenCalledWith( + remoteOps, + jasmine.objectContaining({ + skipDeferredLocalActions: true, + onReducersCommitted: jasmine.any(Function), + }), + ); expect(operationLogEffectsSpy.processDeferredActions).toHaveBeenCalledWith({ callerHoldsOperationLogLock: true, }); expect(callOrder).toEqual([ 'applyOperations', - 'markApplied', + 'markArchivePending', 'mergeRemoteOpClocks', + 'markApplied', 'processDeferredActions', ]); }); @@ -1397,7 +1474,7 @@ describe('RemoteOpsProcessingService', () => { expect(opLogStoreSpy.mergeRemoteOpClocks).not.toHaveBeenCalled(); }); - it('should mark failed ops and run validation on partial failure', async () => { + it('should charge only the attempted archive failure and run validation', async () => { const remoteOps: Operation[] = [ createFullOp({ id: 'op-1' }), createFullOp({ id: 'op-2' }), @@ -1407,17 +1484,19 @@ describe('RemoteOpsProcessingService', () => { opLogStoreSpy.append.and.returnValue(Promise.resolve(1)); opLogStoreSpy.markApplied.and.returnValue(Promise.resolve()); opLogStoreSpy.markFailed.and.returnValue(Promise.resolve()); - operationApplierServiceSpy.applyOperations.and.returnValue( - Promise.resolve({ + operationApplierServiceSpy.applyOperations.and.callFake(async (ops, options) => { + await options?.onReducersCommitted?.(ops); + return { appliedOps: [remoteOps[0]], failedOp: { op: remoteOps[1], error: new Error('Test error') }, - }), - ); + }; + }); await expectAsync(service.applyNonConflictingOps(remoteOps)).toBeRejected(); - // Should mark op-2 and op-3 as failed - expect(opLogStoreSpy.markFailed).toHaveBeenCalledWith(['op-2', 'op-3']); + expect(opLogStoreSpy.markArchivePending).toHaveBeenCalledWith([1, 2, 3]); + expect(opLogStoreSpy.mergeRemoteOpClocks).toHaveBeenCalledWith(remoteOps); + expect(opLogStoreSpy.markFailed).toHaveBeenCalledWith(['op-2']); // Should run validation after partial failure expect(validateStateServiceSpy.validateAndRepairCurrentState).toHaveBeenCalledWith( 'partial-apply-failure', @@ -1486,7 +1565,10 @@ describe('RemoteOpsProcessingService', () => { // Should apply only the non-duplicate op expect(operationApplierServiceSpy.applyOperations).toHaveBeenCalledWith( [remoteOps[1]], - { skipDeferredLocalActions: true }, + jasmine.objectContaining({ + skipDeferredLocalActions: true, + onReducersCommitted: jasmine.any(Function), + }), ); }); diff --git a/src/app/op-log/sync/remote-ops-processing.service.ts b/src/app/op-log/sync/remote-ops-processing.service.ts index d1e910f3c9..8d64f02ad2 100644 --- a/src/app/op-log/sync/remote-ops-processing.service.ts +++ b/src/app/op-log/sync/remote-ops-processing.service.ts @@ -21,6 +21,7 @@ import { VectorClockService } from './vector-clock.service'; import { MIN_SUPPORTED_SCHEMA_VERSION, SchemaMigrationService, + getOperationSchemaVersion, } from '../persistence/schema-migration.service'; import { SnackService } from '../../core/snack/snack.service'; import { T } from '../../t.const'; @@ -92,7 +93,10 @@ export class RemoteOpsProcessingService { */ async processRemoteOps( remoteOps: Operation[], - options?: { skipConflictDetection?: boolean }, + options?: { + skipConflictDetection?: boolean; + callerHoldsOperationLogLock?: boolean; + }, ): Promise<{ localWinOpsCreated: number; allOpsFilteredBySyncImport: boolean; @@ -125,12 +129,22 @@ export class RemoteOpsProcessingService { const currentVersion = this.schemaMigrationService.getCurrentVersion(); const migratedOps: Operation[] = []; const droppedEntityIds = new Set(); - let blockReason: 'VERSION_UNSUPPORTED' | 'VERSION_TOO_NEW' | 'MIGRATION_FAILED' = - 'MIGRATION_FAILED'; + let blockReason: + | 'VERSION_UNSUPPORTED' + | 'VERSION_TOO_NEW' + | 'INVALID_SCHEMA_VERSION' + | 'MIGRATION_FAILED' = 'MIGRATION_FAILED'; let blockedOp: Operation | null = null; for (const op of remoteOps) { - const opVersion = op.schemaVersion ?? 1; + let opVersion: number; + try { + opVersion = getOperationSchemaVersion(op as { schemaVersion?: unknown }); + } catch { + blockedOp = op; + blockReason = 'INVALID_SCHEMA_VERSION'; + break; + } // Op below minimum supported version: no migration path exists. if (opVersion < MIN_SUPPORTED_SCHEMA_VERSION) { @@ -281,8 +295,11 @@ export class RemoteOpsProcessingService { 'RemoteOpsProcessingService: Skipping conflict detection (skipConflictDetection=true). ' + `Applying ${validOps.length} ops directly.`, ); - await this.applyNonConflictingOps(validOps); - await this.validateAfterSync(); + await this.applyNonConflictingOps( + validOps, + options.callerHoldsOperationLogLock ?? false, + ); + await this.validateAfterSync(options.callerHoldsOperationLogLock ?? false); return { localWinOpsCreated: 0, allOpsFilteredBySyncImport: false, @@ -322,7 +339,15 @@ export class RemoteOpsProcessingService { if (conflicts.length > 0) { OpLog.warn( `RemoteOpsProcessingService: Detected ${conflicts.length} conflicts. Auto-resolving with LWW.`, - conflicts, + { + conflicts: conflicts.map((conflict) => ({ + entityType: conflict.entityType, + entityId: conflict.entityId, + localOpIds: conflict.localOps.map((op) => op.id), + remoteOpIds: conflict.remoteOps.map((op) => op.id), + suggestedResolution: conflict.suggestedResolution, + })), + }, ); // Auto-resolve conflicts using Last-Write-Wins strategy. // Piggyback non-conflicting ops so they're applied with resolved conflicts. @@ -359,9 +384,13 @@ export class RemoteOpsProcessingService { * until an app update or migration fix, and every retry re-hits it). */ private _notifyBlockedOp( - reason: 'VERSION_UNSUPPORTED' | 'VERSION_TOO_NEW' | 'MIGRATION_FAILED', + reason: + | 'VERSION_UNSUPPORTED' + | 'VERSION_TOO_NEW' + | 'INVALID_SCHEMA_VERSION' + | 'MIGRATION_FAILED', ): void { - if (reason === 'MIGRATION_FAILED') { + if (reason === 'MIGRATION_FAILED' || reason === 'INVALID_SCHEMA_VERSION') { if (!this._hasWarnedMigrationFailureThisSession) { this._hasWarnedMigrationFailureThisSession = true; this.snackService.open({ @@ -423,9 +452,10 @@ export class RemoteOpsProcessingService { ops: locallyReplayableOps, store: this.opLogStore, applier: { - applyOperations: (opsToApply) => + applyOperations: (opsToApply, applyOptions) => this.operationApplier.applyOperations(opsToApply, { skipDeferredLocalActions: true, + onReducersCommitted: applyOptions?.onReducersCommitted, }), }, isFullStateOperation: this._isFullStateOperation, diff --git a/src/app/op-log/sync/server-migration.service.spec.ts b/src/app/op-log/sync/server-migration.service.spec.ts index 08861775a8..2eb1041845 100644 --- a/src/app/op-log/sync/server-migration.service.spec.ts +++ b/src/app/op-log/sync/server-migration.service.spec.ts @@ -7,7 +7,7 @@ import { ServerMigrationService } from './server-migration.service'; import { OperationLogStoreService } from '../persistence/operation-log-store.service'; import { VectorClockService } from './vector-clock.service'; import { ValidateStateService } from '../validation/validate-state.service'; -import { StateSnapshotService } from '../backup/state-snapshot.service'; +import { AppStateSnapshot, StateSnapshotService } from '../backup/state-snapshot.service'; import { SnackService } from '../../core/snack/snack.service'; import { UserInputWaitStateService } from '../../imex/sync/user-input-wait-state.service'; import { @@ -20,6 +20,10 @@ import { SYSTEM_TAG_IDS } from '../../features/tag/tag.const'; import { INBOX_PROJECT } from '../../features/project/project.const'; import { loadAllData } from '../../root-store/meta/load-all-data.action'; import { CLIENT_ID_PROVIDER, ClientIdProvider } from '../util/client-id.provider'; +import { LockService } from './lock.service'; +import { OperationWriteFlushService } from './operation-write-flush.service'; +import { LOCK_NAMES } from '../core/operation-log.const'; +import { OperationCaptureService } from '../capture/operation-capture.service'; describe('ServerMigrationService', () => { let service: ServerMigrationService; @@ -32,6 +36,9 @@ describe('ServerMigrationService', () => { let clientIdProviderSpy: jasmine.SpyObj; let matDialogSpy: jasmine.SpyObj; let userInputWaitStateSpy: jasmine.SpyObj; + let lockServiceSpy: jasmine.SpyObj; + let writeFlushServiceSpy: jasmine.SpyObj; + let operationCaptureServiceSpy: jasmine.SpyObj; let defaultProvider: OperationSyncProvider; // Type for operation-sync-capable provider @@ -86,6 +93,18 @@ describe('ServerMigrationService', () => { 'startWaiting', ]); userInputWaitStateSpy.startWaiting.and.returnValue(() => {}); + lockServiceSpy = jasmine.createSpyObj('LockService', ['request']); + lockServiceSpy.request.and.callFake(async (_name: string, fn: () => Promise) => + fn(), + ); + writeFlushServiceSpy = jasmine.createSpyObj('OperationWriteFlushService', [ + 'flushPendingWrites', + ]); + writeFlushServiceSpy.flushPendingWrites.and.resolveTo(); + operationCaptureServiceSpy = jasmine.createSpyObj('OperationCaptureService', [ + 'getPendingCount', + ]); + operationCaptureServiceSpy.getPendingCount.and.returnValue(0); // Default mock returns opLogStoreSpy.hasSyncedOps.and.returnValue(Promise.resolve(true)); @@ -130,6 +149,9 @@ describe('ServerMigrationService', () => { { provide: CLIENT_ID_PROVIDER, useValue: clientIdProviderSpy }, { provide: MatDialog, useValue: matDialogSpy }, { provide: UserInputWaitStateService, useValue: userInputWaitStateSpy }, + { provide: LockService, useValue: lockServiceSpy }, + { provide: OperationWriteFlushService, useValue: writeFlushServiceSpy }, + { provide: OperationCaptureService, useValue: operationCaptureServiceSpy }, ], }); @@ -471,6 +493,52 @@ describe('ServerMigrationService', () => { }); }); + it('should capture and append the full-state operation inside one operation-log barrier', async () => { + const events: string[] = []; + writeFlushServiceSpy.flushPendingWrites.and.callFake(async () => { + events.push('flush'); + }); + lockServiceSpy.request.and.callFake(async (name: string, fn: () => Promise) => { + events.push(`lock:${name}:start`); + const result = await fn(); + events.push(`lock:${name}:end`); + return result; + }); + stateSnapshotServiceSpy.getStateSnapshotAsync.and.callFake(async () => { + events.push('snapshot'); + return { + task: { ids: ['task-1'], entities: { 'task-1': { id: 'task-1' } } }, + project: { ids: [], entities: {} }, + tag: { ids: [], entities: {} }, + } as unknown as AppStateSnapshot; + }); + opLogStoreSpy.append.and.callFake(async () => { + events.push('append'); + return 1; + }); + + await service.handleServerMigration(defaultProvider); + + expect(events).toEqual([ + 'flush', + `lock:${LOCK_NAMES.OPERATION_LOG}:start`, + 'snapshot', + 'append', + `lock:${LOCK_NAMES.OPERATION_LOG}:end`, + ]); + }); + + it('should release, flush, and retry when an action lands before snapshot capture', async () => { + operationCaptureServiceSpy.getPendingCount.and.returnValues(1, 0); + + await service.handleServerMigration(defaultProvider); + + expect(writeFlushServiceSpy.flushPendingWrites).toHaveBeenCalledTimes(2); + expect(lockServiceSpy.request).toHaveBeenCalledTimes(2); + expect(stateSnapshotServiceSpy.getStateSnapshotAsync).toHaveBeenCalledTimes(1); + expect(opLogStoreSpy.append).toHaveBeenCalledTimes(1); + }); + describe('system-tag empty-state detection (tested via handleServerMigration)', () => { it('should identify system tags correctly', async () => { for (const systemTagId of SYSTEM_TAG_IDS) { diff --git a/src/app/op-log/sync/server-migration.service.ts b/src/app/op-log/sync/server-migration.service.ts index 12acfaa95a..5e8330b3aa 100644 --- a/src/app/op-log/sync/server-migration.service.ts +++ b/src/app/op-log/sync/server-migration.service.ts @@ -24,7 +24,11 @@ import { OpLog } from '../../core/log'; import { CLIENT_ID_PROVIDER } from '../util/client-id.provider'; import { DialogServerMigrationConfirmComponent } from './dialog-server-migration-confirm/dialog-server-migration-confirm.component'; import { hasMeaningfulStateData } from '../validation/has-meaningful-state-data.util'; -import { MODEL_CONFIGS } from '../model/model-config'; +import { AppDataComplete, MODEL_CONFIGS } from '../model/model-config'; +import { LockService } from './lock.service'; +import { OperationWriteFlushService } from './operation-write-flush.service'; +import { LOCK_NAMES } from '../core/operation-log.const'; +import { OperationCaptureService } from '../capture/operation-capture.service'; const MEANINGFUL_ENTITY_STATE_KEYS = new Set(['task', 'project', 'tag', 'note']); @@ -86,6 +90,9 @@ export class ServerMigrationService { private clientIdProvider = inject(CLIENT_ID_PROVIDER); private _matDialog = inject(MatDialog); private _userInputWaitState = inject(UserInputWaitStateService); + private lockService = inject(LockService); + private writeFlushService = inject(OperationWriteFlushService); + private operationCaptureService = inject(OperationCaptureService); /** * Checks if we're connecting to a new/empty server and handles migration if needed. @@ -198,104 +205,129 @@ export class ServerMigrationService { 'ServerMigrationService: Server migration detected. Creating full state SYNC_IMPORT.', ); - // Get current full state from NgRx store (async to include archives from IndexedDB) - // Cast to Record for validation compatibility - let currentState: Record = - (await this.stateSnapshotService.getStateSnapshotAsync()) as unknown as Record< - string, - unknown - >; + // Drain already-captured writes, then keep snapshot capture, validation, clock + // construction, and append behind the same mutation barrier. This makes the + // full-state operation's local seq an exact cutoff: every earlier op is in the + // snapshot, and any action captured while this runs is appended afterwards. + let retryForPendingCapture: boolean; + do { + retryForPendingCapture = false; + await this.writeFlushService.flushPendingWrites(); + await this.lockService.request(LOCK_NAMES.OPERATION_LOG, async () => { + // A reducer action can land in the tiny gap between flush() releasing + // this lock and our acquisition. Its pending counter increments + // synchronously, before the persistence effect waits for the lock. Do + // not take a snapshot that includes that reducer state but precedes its + // operation; release, drain it, and retry the cutoff instead. + if (this.operationCaptureService.getPendingCount() > 0) { + retryForPendingCapture = true; + return; + } - // Skip if local state is effectively empty - if (!hasServerMigrationStateData(currentState)) { - OpLog.warn('ServerMigrationService: Skipping SYNC_IMPORT - local state is empty.'); - return; - } + // Get current full state from NgRx store (async to include archives from IndexedDB) + // Cast to Record for validation compatibility + let currentState: Record = + (await this.stateSnapshotService.getStateSnapshotAsync()) as unknown as Record< + string, + unknown + >; - // Validate and repair state before creating SYNC_IMPORT - // This prevents corrupted state (e.g., orphaned menuTree references) from - // propagating to other clients via the full state import. - const validationResult = - await this.validateStateService.validateAndRepair(currentState); + // Skip if local state is effectively empty + if (!hasServerMigrationStateData(currentState)) { + OpLog.warn( + 'ServerMigrationService: Skipping SYNC_IMPORT - local state is empty.', + ); + return; + } - // If state is invalid and couldn't be repaired, abort - don't propagate corruption - if (!validationResult.isValid) { - OpLog.err( - 'ServerMigrationService: Cannot create SYNC_IMPORT - state validation failed.', - validationResult.error || validationResult.crossModelError, - ); - this.snackService.open({ - type: 'ERROR', - msg: T.F.SYNC.S.SERVER_MIGRATION_VALIDATION_FAILED, + // Validate and repair state before creating SYNC_IMPORT + // This prevents corrupted state (e.g., orphaned menuTree references) from + // propagating to other clients via the full state import. + const validationResult = + await this.validateStateService.validateAndRepair(currentState); + + // If state is invalid and couldn't be repaired, abort - don't propagate corruption + if (!validationResult.isValid) { + OpLog.err( + 'ServerMigrationService: Cannot create SYNC_IMPORT - state validation failed.', + validationResult.error || validationResult.crossModelError, + ); + this.snackService.open({ + type: 'ERROR', + msg: T.F.SYNC.S.SERVER_MIGRATION_VALIDATION_FAILED, + }); + return; + } + + // If state was repaired, use the repaired version + if (validationResult.repairedState) { + OpLog.warn( + 'ServerMigrationService: State repaired before creating SYNC_IMPORT', + validationResult.repairSummary, + ); + currentState = validationResult.repairedState; + + // Also update NgRx store with repaired state so local client is consistent + this.store.dispatch( + loadAllData({ + appDataComplete: validationResult.repairedState as AppDataComplete, + }), + ); + } + + // Get client ID + const clientId = await this.clientIdProvider.loadClientId(); + if (!clientId) { + OpLog.err( + 'ServerMigrationService: Cannot create SYNC_IMPORT - no client ID available.', + ); + return; + } + + // Build vector clock by merging ALL local operation clocks. + // This ensures the SYNC_IMPORT's clock dominates all pre-import ops, + // so when SyncImportFilterService compares them, all prior ops are + // LESS_THAN (not CONCURRENT) and can be properly filtered. + const allLocalOps = await this.opLogStore.getOpsAfterSeq(0); + let mergedClock = await this.vectorClockService.getCurrentVectorClock(); + for (const entry of allLocalOps) { + mergedClock = mergeVectorClocks(mergedClock, entry.op.vectorClock); + } + const newClock = limitVectorClockSize( + incrementVectorClock(mergedClock, clientId), + clientId, + ); + + OpLog.normal( + `ServerMigrationService: Merged ${allLocalOps.length} local op clocks into SYNC_IMPORT vector clock.`, + ); + + // Create SYNC_IMPORT operation with full state + // NOTE: Use raw state directly (not wrapped in appDataComplete). + // The snapshot endpoint expects raw state, and the hydrator handles + // both formats on extraction. + const op: Operation = { + id: uuidv7(), + actionType: ActionType.LOAD_ALL_DATA, + opType: OpType.SyncImport, + entityType: 'ALL', + payload: currentState, + clientId, + vectorClock: newClock, + timestamp: Date.now(), + schemaVersion: CURRENT_SCHEMA_VERSION, + syncImportReason: options?.syncImportReason ?? 'SERVER_MIGRATION', + }; + + // Append to operation log - will be uploaded via snapshot endpoint + await this.opLogStore.append(op, 'local'); + + OpLog.normal( + 'ServerMigrationService: Created SYNC_IMPORT operation for server migration. ' + + 'Will be uploaded immediately via follow-up upload.', + ); }); - return; - } - - // If state was repaired, use the repaired version - if (validationResult.repairedState) { - OpLog.warn( - 'ServerMigrationService: State repaired before creating SYNC_IMPORT', - validationResult.repairSummary, - ); - currentState = validationResult.repairedState; - - // Also update NgRx store with repaired state so local client is consistent - this.store.dispatch( - loadAllData({ appDataComplete: validationResult.repairedState as any }), - ); - } - - // Get client ID - const clientId = await this.clientIdProvider.loadClientId(); - if (!clientId) { - OpLog.err( - 'ServerMigrationService: Cannot create SYNC_IMPORT - no client ID available.', - ); - return; - } - - // Build vector clock by merging ALL local operation clocks. - // This ensures the SYNC_IMPORT's clock dominates all pre-import ops, - // so when SyncImportFilterService compares them, all prior ops are - // LESS_THAN (not CONCURRENT) and can be properly filtered. - const allLocalOps = await this.opLogStore.getOpsAfterSeq(0); - let mergedClock = await this.vectorClockService.getCurrentVectorClock(); - for (const entry of allLocalOps) { - mergedClock = mergeVectorClocks(mergedClock, entry.op.vectorClock); - } - const newClock = limitVectorClockSize( - incrementVectorClock(mergedClock, clientId), - clientId, - ); - - OpLog.normal( - `ServerMigrationService: Merged ${allLocalOps.length} local op clocks into SYNC_IMPORT vector clock.`, - ); - - // Create SYNC_IMPORT operation with full state - // NOTE: Use raw state directly (not wrapped in appDataComplete). - // The snapshot endpoint expects raw state, and the hydrator handles - // both formats on extraction. - const op: Operation = { - id: uuidv7(), - actionType: ActionType.LOAD_ALL_DATA, - opType: OpType.SyncImport, - entityType: 'ALL', - payload: currentState, - clientId, - vectorClock: newClock, - timestamp: Date.now(), - schemaVersion: CURRENT_SCHEMA_VERSION, - syncImportReason: options?.syncImportReason ?? 'SERVER_MIGRATION', - }; - - // Append to operation log - will be uploaded via snapshot endpoint - await this.opLogStore.append(op, 'local'); - - OpLog.normal( - 'ServerMigrationService: Created SYNC_IMPORT operation for server migration. ' + - 'Will be uploaded immediately via follow-up upload.', - ); + } while (retryForPendingCapture); } /** diff --git a/src/app/op-log/sync/sync-import-conflict-gate.service.spec.ts b/src/app/op-log/sync/sync-import-conflict-gate.service.spec.ts index 2718eeddb4..0adea0c7d2 100644 --- a/src/app/op-log/sync/sync-import-conflict-gate.service.spec.ts +++ b/src/app/op-log/sync/sync-import-conflict-gate.service.spec.ts @@ -94,7 +94,7 @@ describe('SyncImportConflictGateService', () => { }); }); - it('should not produce dialog data when pending ops are config-only', async () => { + it('should produce dialog data when pending ops contain user config changes', async () => { const incomingSyncImport = createOperation(); const pendingConfigEntry = createEntry( createOperation({ @@ -113,8 +113,8 @@ describe('SyncImportConflictGateService', () => { const result = await service.checkIncomingFullStateConflict([incomingSyncImport]); expect(result.fullStateOp).toBe(incomingSyncImport); - expect(result.hasMeaningfulPending).toBeFalse(); - expect(result.dialogData).toBeUndefined(); + expect(result.hasMeaningfulPending).toBeTrue(); + expect(result.dialogData).toBeDefined(); }); it('should not produce dialog data when pending task creates are startup example tasks', async () => { @@ -272,19 +272,27 @@ describe('SyncImportConflictGateService', () => { expect(opLogStoreSpy.hasSyncedOps).not.toHaveBeenCalled(); }); - it('should not consult sync history when there are no meaningful pending ops', async () => { + it('should not consult sync history when only startup example tasks are pending', async () => { const incomingSyncImport = createOperation(); - const pendingConfigEntry = createEntry( + const pendingExampleTaskEntry = createEntry( createOperation({ - id: 'local-config-update', - opType: OpType.Update, - entityType: 'GLOBAL_CONFIG', - entityId: 'sync', + id: 'local-example-task-create', + actionType: ActionType.TASK_SHARED_ADD, + opType: OpType.Create, + entityType: 'TASK', + entityId: 'example-task-1', + payload: { + actionPayload: { + task: { id: 'example-task-1' }, + isExampleTask: true, + }, + entityChanges: [], + }, clientId: 'client-A', vectorClock: { clientA: 1 }, }), ); - opLogStoreSpy.getUnsynced.and.resolveTo([pendingConfigEntry]); + opLogStoreSpy.getUnsynced.and.resolveTo([pendingExampleTaskEntry]); await service.checkIncomingFullStateConflict([incomingSyncImport]); @@ -383,25 +391,41 @@ describe('SyncImportConflictGateService', () => { expect(service.hasMeaningfulPendingOps([pendingCounter])).toBeTrue(); }); - it('should still treat MIGRATION/RECOVERY bookkeeping ops as not meaningful', async () => { + it('should treat MIGRATION/RECOVERY genesis batches as meaningful recovered user data', async () => { const pendingMigration = createEntry( createOperation({ id: 'local-genesis', actionType: '[Migration] Genesis' as ActionType, - opType: OpType.Create, + opType: OpType.Batch, entityType: 'MIGRATION', entityId: 'genesis', + payload: { task: { ids: ['recovered-task'] } }, clientId: 'client-A', vectorClock: { clientA: 1 }, }), ); - expect(service.hasMeaningfulPendingOps([pendingMigration])).toBeFalse(); + const pendingRecovery = createEntry( + createOperation({ + id: 'local-recovery', + actionType: '[Recovery] Data Import' as ActionType, + opType: OpType.Batch, + entityType: 'RECOVERY', + entityId: 'genesis', + payload: { project: { ids: ['recovered-project'] } }, + clientId: 'client-A', + vectorClock: { clientA: 2 }, + }), + { seq: 2 }, + ); + + expect(service.hasMeaningfulPendingOps([pendingMigration])).toBeTrue(); + expect(service.hasMeaningfulPendingOps([pendingRecovery])).toBeTrue(); }); }); describe('preCapturedPendingOps (piggyback-upload race)', () => { - it('should judge meaningfulness against the pre-upload snapshot, not the live pending set', async () => { + it('should judge meaningfulness against the union of the upload snapshot and live pending set', async () => { // Live pending set is empty — the op was accepted and marked synced during // the same upload round that piggybacked the import. opLogStoreSpy.getUnsynced.and.resolveTo([]); @@ -426,6 +450,54 @@ describe('SyncImportConflictGateService', () => { expect(result.dialogData).toBeDefined(); }); + it('should include meaningful work created after the upload snapshot', async () => { + const createdDuringUpload = createEntry( + createOperation({ + id: 'created-during-upload', + actionType: '[SimpleCounter] Increase Counter Today' as ActionType, + opType: OpType.Update, + entityType: 'SIMPLE_COUNTER', + entityId: 'counter-1', + clientId: 'client-A', + vectorClock: { clientA: 2 }, + }), + { seq: 2 }, + ); + opLogStoreSpy.getUnsynced.and.resolveTo([createdDuringUpload]); + + const result = await service.checkIncomingFullStateConflict([createOperation()], { + flushPendingWrites: true, + preCapturedPendingOps: [], + }); + + expect(writeFlushServiceSpy.flushPendingWrites).toHaveBeenCalled(); + expect(result.pendingOps).toEqual([createdDuringUpload]); + expect(result.hasMeaningfulPending).toBeTrue(); + expect(result.dialogData).toBeDefined(); + }); + + it('should de-duplicate operations present in both upload and live snapshots', async () => { + const selectedForUpload = createEntry( + createOperation({ + id: 'same-op', + actionType: '[Task] Update task' as ActionType, + opType: OpType.Update, + entityType: 'TASK', + entityId: 'task-1', + clientId: 'client-A', + vectorClock: { clientA: 1 }, + }), + ); + opLogStoreSpy.getUnsynced.and.resolveTo([selectedForUpload]); + + const result = await service.checkIncomingFullStateConflict([createOperation()], { + preCapturedPendingOps: [selectedForUpload], + }); + + expect(result.pendingOps).toEqual([selectedForUpload]); + expect(result.dialogData?.filteredOpCount).toBe(1); + }); + it('should derive discardable example-task ids from the LIVE pending set', async () => { const exampleCreate = (id: string): OperationLogEntry => createEntry( diff --git a/src/app/op-log/sync/sync-import-conflict-gate.service.ts b/src/app/op-log/sync/sync-import-conflict-gate.service.ts index bc255685de..5a530864fa 100644 --- a/src/app/op-log/sync/sync-import-conflict-gate.service.ts +++ b/src/app/op-log/sync/sync-import-conflict-gate.service.ts @@ -10,17 +10,6 @@ import { OperationWriteFlushService } from './operation-write-flush.service'; import { SyncImportConflictData } from './dialog-sync-import-conflict/dialog-sync-import-conflict.component'; import { isExampleTaskCreateOp } from '../validation/is-example-task-op.util'; -/** - * Pending ops on these entity types are startup/system noise, not user work: - * config writes happen automatically (defaults, migrations) and sync settings - * are intentionally local; MIGRATION/RECOVERY are bookkeeping genesis ops. - * Everything else — including MOV/BATCH ops and entities like TIME_TRACKING, - * SIMPLE_COUNTER, TASK_REPEAT_CFG, PLANNER, BOARD — is user work an incoming - * full-state import would silently discard (imports drop concurrent ops by - * design), so it must count as meaningful and trigger the conflict dialog. - */ -const DISCARDABLE_ENTITY_TYPES = new Set(['GLOBAL_CONFIG', 'MIGRATION', 'RECOVERY']); - export interface IncomingFullStateConflictGateResult { fullStateOp?: Operation; pendingOps: OperationLogEntry[]; @@ -45,10 +34,11 @@ export class SyncImportConflictGateService { private writeFlushService = inject(OperationWriteFlushService); /** - * Every pending op is user work unless it is on a DISCARDABLE_ENTITY_TYPES - * entity or is an onboarding example-task create. Full-state ops are always - * meaningful because applying a newer full-state op can invalidate their - * local import/repair semantics. + * Every pending op is user work unless it is an onboarding example-task create. + * Entity-wide exemptions are unsafe: GLOBAL_CONFIG contains synced preferences, + * while MIGRATION and RECOVERY genesis operations contain the user's full recovered + * database. Full-state ops are always meaningful because applying a newer full-state + * op can invalidate their local import/repair semantics. */ hasMeaningfulPendingOps(ops: OperationLogEntry[]): boolean { return ops.some((entry) => { @@ -58,16 +48,15 @@ export class SyncImportConflictGateService { if (isExampleTaskCreateOp(entry)) { return false; } - return !DISCARDABLE_ENTITY_TYPES.has(entry.op.entityType); + return true; }); } /** - * @param options.preCapturedPendingOps - Pending ops captured BEFORE the upload - * round started. The piggyback-upload path MUST pass this: by the time - * its gate runs, ops accepted in the same round were already marked - * synced, so a live getUnsynced() read would no longer see local work - * that the piggybacked import is about to discard. + * @param options.preCapturedPendingOps - Exact pending ops selected by the upload + * round. The piggyback path unions this snapshot with a live read so it + * protects both accepted work from that upload and work created while the + * network request was in flight. */ async checkIncomingFullStateConflict( incomingOps: Operation[], @@ -94,8 +83,10 @@ export class SyncImportConflictGateService { await this.writeFlushService.flushPendingWrites(); } - const pendingOps = - options.preCapturedPendingOps ?? (await this.opLogStore.getUnsynced()); + const livePendingOps = await this.opLogStore.getUnsynced(); + const pendingOps = options.preCapturedPendingOps + ? this._mergePendingOps(options.preCapturedPendingOps, livePendingOps) + : livePendingOps; const hasMeaningfulPending = this.hasMeaningfulPendingOps(pendingOps); // Example-task ops that the caller may reject when it accepts the import silently. // When `hasMeaningfulPending` is true (real work pending alongside example tasks), @@ -105,9 +96,6 @@ export class SyncImportConflictGateService { // These must come from a LIVE read: with a pre-captured snapshot, example ops // accepted earlier in the same upload round are already marked synced and must // not be re-marked rejected by the caller. - const livePendingOps = options.preCapturedPendingOps - ? await this.opLogStore.getUnsynced() - : pendingOps; const discardablePendingOpIds = livePendingOps .filter(isExampleTaskCreateOp) .map((entry) => entry.op.id); @@ -148,4 +136,21 @@ export class SyncImportConflictGateService { }, }; } + + private _mergePendingOps( + uploadSnapshot: OperationLogEntry[], + livePendingOps: OperationLogEntry[], + ): OperationLogEntry[] { + const merged = [...uploadSnapshot]; + const seenOpIds = new Set(uploadSnapshot.map((entry) => entry.op.id)); + + for (const entry of livePendingOps) { + if (!seenOpIds.has(entry.op.id)) { + merged.push(entry); + seenOpIds.add(entry.op.id); + } + } + + return merged; + } } diff --git a/src/app/op-log/sync/ws-triggered-download.service.spec.ts b/src/app/op-log/sync/ws-triggered-download.service.spec.ts index 8309919a51..df86aa1f47 100644 --- a/src/app/op-log/sync/ws-triggered-download.service.spec.ts +++ b/src/app/op-log/sync/ws-triggered-download.service.spec.ts @@ -39,7 +39,7 @@ describe('WsTriggeredDownloadService', () => { mockProviderManager = jasmine.createSpyObj( 'SyncProviderManager', - ['getActiveProvider'], + ['getActiveProvider', 'setSyncStatus'], { isSyncInProgress: false, }, @@ -244,9 +244,6 @@ describe('WsTriggeredDownloadService', () => { // reset clears them) or leak into the next session. The service must // be its own session boundary. it('sets sync status ERROR when the download flips the validation latch', fakeAsync(() => { - if (mockProviderManager.setSyncStatus === undefined) { - mockProviderManager.setSyncStatus = jasmine.createSpy('setSyncStatus'); - } const latch = TestBed.inject(SyncSessionValidationService); mockSyncService.downloadRemoteOps.and.callFake(async () => { latch.setFailed(); @@ -262,9 +259,6 @@ describe('WsTriggeredDownloadService', () => { })); it('does not flag ERROR when the download leaves the latch reset', fakeAsync(() => { - if (mockProviderManager.setSyncStatus === undefined) { - mockProviderManager.setSyncStatus = jasmine.createSpy('setSyncStatus'); - } const latch = TestBed.inject(SyncSessionValidationService); latch._resetForTest(); @@ -276,6 +270,19 @@ describe('WsTriggeredDownloadService', () => { expect(mockProviderManager.setSyncStatus).not.toHaveBeenCalledWith('ERROR'); })); + it('sets sync status ERROR when processing is blocked by an incompatible op', fakeAsync(() => { + mockSyncService.downloadRemoteOps.and.resolveTo({ + kind: 'blocked_incompatible', + }); + + service.start(); + notification$.next({ latestSeq: 1 }); + tick(500); + flushMicrotasks(); + + expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith('ERROR'); + })); + // Defense against stale latch from a prior path: the WS service opens its // own session, which resets the latch up front so the read at the end // reflects only this session's outcome. diff --git a/src/app/op-log/sync/ws-triggered-download.service.ts b/src/app/op-log/sync/ws-triggered-download.service.ts index 35b607fb65..a2ecfed4a7 100644 --- a/src/app/op-log/sync/ws-triggered-download.service.ts +++ b/src/app/op-log/sync/ws-triggered-download.service.ts @@ -151,6 +151,14 @@ export class WsTriggeredDownloadService implements OnDestroy { SyncLog.log(`WsTriggeredDownloadService: Download complete. kind=${result.kind}`); + if (result.kind === 'blocked_incompatible') { + SyncLog.warn( + 'WsTriggeredDownloadService: Download blocked by an incompatible operation', + ); + this._providerManager.setSyncStatus('ERROR'); + return; + } + if (this._sessionValidation.hasFailed()) { SyncLog.err( 'WsTriggeredDownloadService: Post-sync validation failed during WS download — reporting ERROR', diff --git a/src/app/op-log/testing/integration/remote-apply-store-port.integration.spec.ts b/src/app/op-log/testing/integration/remote-apply-store-port.integration.spec.ts index e56b8f87af..269f19ceed 100644 --- a/src/app/op-log/testing/integration/remote-apply-store-port.integration.spec.ts +++ b/src/app/op-log/testing/integration/remote-apply-store-port.integration.spec.ts @@ -56,7 +56,10 @@ const defineStorePortContract = ( } => { const applyOperationsSpy = jasmine .createSpy('applyOperations') - .and.resolveTo(result); + .and.callFake(async (ops: Operation[], options) => { + await options?.onReducersCommitted?.(ops); + return result; + }); return { applier: { applyOperations: applyOperationsSpy }, @@ -97,7 +100,10 @@ const defineStorePortContract = ( applier, }); - expect(applyOperationsSpy).toHaveBeenCalledOnceWith([newOp]); + expect(applyOperationsSpy).toHaveBeenCalledOnceWith( + [newOp], + jasmine.objectContaining({ onReducersCommitted: jasmine.any(Function) }), + ); expect(result.appendedOps).toEqual([newOp]); expect(result.skippedCount).toBe(1); expect(result.appliedOps).toEqual([newOp]); @@ -141,7 +147,7 @@ const defineStorePortContract = ( }); expect(result.failedOp).toEqual({ op: failedOp, error }); - expect(result.failedOpIds).toEqual([failedOp.id, remainingOp.id]); + expect(result.failedOpIds).toEqual([failedOp.id]); const appliedEntry = await store.getOpById(appliedOp.id); const failedEntry = await store.getOpById(failedOp.id); @@ -149,12 +155,16 @@ const defineStorePortContract = ( expect(appliedEntry?.applicationStatus).toBe('applied'); expect(failedEntry?.applicationStatus).toBe('failed'); expect(failedEntry?.retryCount).toBe(1); - expect(remainingEntry?.applicationStatus).toBe('failed'); - expect(remainingEntry?.retryCount).toBe(1); + expect(remainingEntry?.applicationStatus).toBe('archive_pending'); + expect(remainingEntry?.retryCount).toBeUndefined(); expect( (await store.getFailedRemoteOps()).map((entry) => entry.op.id).sort(), ).toEqual([failedOp.id, remainingOp.id].sort()); - expect(await store.getVectorClock()).toEqual({ clientA: 2 }); + expect(await store.getVectorClock()).toEqual({ + clientA: 2, + clientB: 4, + clientC: 6, + }); }); it('should clear older full-state ops and reset the vector clock after an applied remote import', async () => {