diff --git a/docs/sync-and-op-log/operation-log-architecture.md b/docs/sync-and-op-log/operation-log-architecture.md index 5a595f92b3..ae7c458c2a 100644 --- a/docs/sync-and-op-log/operation-log-architecture.md +++ b/docs/sync-and-op-log/operation-log-architecture.md @@ -215,7 +215,28 @@ Downloaded operations use a durable status transition so reducer state, archive 3. `failed` — an archive side-effect attempt failed. `retryCount` is charged only to the attempted row, so later rows in the same batch do not consume retry budget. 4. `applied` — reducer and archive work both completed. -Startup hydration replays persisted state history, quarantines surviving `pending` rows, and retries `archive_pending`/`failed` rows with reducer dispatch disabled. Ordinary sync refuses to download, upload, or advance its cursor while any incomplete rows remain. Database version 8 is a downgrade barrier: released readers that do not understand the distinct reducer checkpoint cannot open the newer store and silently overlook outstanding archive work. +Bulk replay isolates conversion/reducer exceptions per operation. The reducer-successful +subsequence is checkpointed and receives archive side effects; ordinary reducer-failed remote +rows are marked with terminal `rejectedAt` plus `reducerRejectedAt` metadata in the **same transaction**. +`reducerRejectedAt` is distinct from an ordinary sync rejection: hydration excludes that row +because its migrated reducer effect never entered state. Pending rows deliberately removed by a +schema migration receive the same terminal marker. This prevents one malformed operation from +terminating NgRx's state pipeline or receiving archive work, without creating a crash window +where startup could mistake it for incomplete reducer work. + +Full-state and local operations are exceptions: a full-state failure discards the entire +speculative bulk batch, while a local replay failure aborts hydration without rejecting the user +intent. Neither case is terminally acknowledged when its state never entered NgRx. + +Startup recovery leaves surviving `pending` rows pending, then hydration replays them through the +same per-operation reducer-failure collector. Successful rows and reducer failures are durably +partitioned before archive retry or snapshot creation. A pending full-state operation never uses +the direct-load shortcut. During live sync, a full-state reducer failure aborts before the reducer +checkpoint and server cursor advance, so the pending row remains recoverable. Hydration retries +`archive_pending`/`failed` rows with reducer dispatch disabled. Ordinary sync refuses to download, +upload, or advance its cursor while any incomplete rows remain. Version 8 introduced a downgrade +barrier for the reducer/archive checkpoint; version 9 similarly prevents older readers from replaying +operations quarantined with `reducerRejectedAt`. Local actions buffered during a remote-apply window stay ordered until each operation is durable. Transient persistence failures keep the failed suffix queued and block the current sync so a later sync can retry. A deterministically invalid buffered action also remains queued, but requires reload: its reducer already changed live state, so discarding it would let live state diverge from the durable operation log. @@ -399,12 +420,15 @@ Without compaction, the op log grows unbounded. Compaction: ```typescript async compact(): Promise { - // 1. Acquire lock - await this.lockService.request('sp_op_log_compact', async () => { - // 2. Read current state from NgRx (via delegate) + // 1. Acquire the operation-log lock + await this.lockService.request('sp_op_log', async () => { + // 2. Never advance a snapshot past reducer-uncommitted remote rows + if ((await this.opLogStore.getPendingRemoteOps()).length > 0) return; + + // 3. Read current state from NgRx (via delegate) const currentState = await this.storeDelegate.getAllSyncModelDataFromStore(); - // 3. Save new snapshot + // 4. Save new snapshot const lastSeq = await this.opLogStore.getLastSeq(); await this.opLogStore.saveStateCache({ state: currentState, @@ -414,16 +438,25 @@ async compact(): Promise { schemaVersion: CURRENT_SCHEMA_VERSION }); - // 4. Delete old ops (sync-aware) - // Only delete ops that have been synced AND are older than retention window + // 5. Delete old terminal ops only const retentionWindowMs = 7 * 24 * 60 * 60 * 1000; // 7 days const cutoff = Date.now() - retentionWindowMs; await this.opLogStore.deleteOpsWhere( - (entry) => - !!entry.syncedAt && // never drop unsynced ops - entry.appliedAt < cutoff && - entry.seq <= lastSeq + (entry) => { + const rejected = entry.rejectedAt !== undefined; + const complete = + rejected || + entry.applicationStatus === undefined || + entry.applicationStatus === 'applied'; + const terminalAt = entry.rejectedAt ?? entry.appliedAt; + return ( + (!!entry.syncedAt || rejected) && + complete && + terminalAt < cutoff && + entry.seq <= lastSeq + ); + } ); }); } @@ -438,7 +471,9 @@ async compact(): Promise { | Emergency retention | 1 day | Shorter retention for quota exceeded | | Compaction timeout | 25 sec | Abort if exceeds (prevents lock expiration) | | Max compaction failures | 3 | Failures before user notification | -| Unsynced ops | ∞ | Never delete unsynced ops | +| Unsynced non-rejected ops | ∞ | Never delete unsynced active ops | +| Incomplete remote ops | ∞ | Never delete pending/archive_pending/failed | +| Rejected ops retention | 7 days | Delete after terminal rejection retention | | Max download ops in memory | 50,000 | Bounds memory during API download | | Remote file retention | 14 days | Server-side operation file retention | | Max remote files to keep | 100 | Minimum recent files on server | @@ -566,13 +601,19 @@ export class MyEffects { ``` 1. Detect: Hydration fails or returns empty/invalid state -2. Check legacy 'pf' database for data -3. If found: Run recovery migration with that data -4. If not: Check remote sync for data -5. If remote has data: Force sync download -6. If all else fails: User must restore from backup +2. Verify SUP_OPS has neither a snapshot nor any operation rows +3. Only when SUP_OPS is provably empty, check legacy 'pf' database for data +4. If found: Run recovery migration with that data +5. Otherwise: restore through sync or a user-selected backup ``` +Automatic legacy recovery runs the emptiness check and legacy write under the operation-log +lock, and fails closed. A present snapshot, a non-empty operation log, or an inspection error +prevents the legacy write and propagates the hydration failure. The generic hydration catch +must never place an older `pf` copy at the current SUP_OPS sequence frontier. When recovery is +allowed, the recovery operation, state-cache snapshot, and vector clock commit in one IndexedDB +transaction; an interrupted write cannot leave a snapshot claiming an operation that rolled back. + ### Implementation ```typescript @@ -580,11 +621,12 @@ async hydrateStore(): Promise { try { const snapshot = await this.opLogStore.loadStateCache(); if (!snapshot || !this.isValidSnapshot(snapshot)) { - await this.attemptRecovery(); - return; + // attemptRecovery() proceeds only if snapshot === null and lastSeq === 0 + return await this.attemptRecovery(); } // Normal hydration... } catch (e) { + // This rethrows when SUP_OPS is non-empty or cannot be inspected. await this.attemptRecovery(); } } @@ -1163,6 +1205,16 @@ async hydrateFromRemoteSync(downloadedMainModelData?: Record): Archive data (`archiveYoung`, `archiveOld`) is included in the state snapshot for file-based sync. Archives are written directly to IndexedDB via `ArchiveDbAdapter` (bypassing the operation log for performance). +Remote full-state replay holds the archive mutex while it loads, preflights, and writes both +archive halves. When both halves are available, `archiveYoung` and `archiveOld` commit through +one `saveArchivesAtomic()` transaction; a protected or omitted half is carried forward unchanged. +`BACKUP_IMPORT` does not prompt on receiving clients: the originating restore was already an +explicit user decision, so replay is deterministic across devices. + +Archive compression uses the same `TASK_ARCHIVE` mutex for its entire read-compress-write cycle. +This prevents compression from overwriting a concurrent full-state replacement with archives it +read before the replacement. The final young/old pair is still committed atomically. + ### Why Archives Bypass Operation Log 1. **Size**: Archived tasks can grow to tens of thousands of entries over years @@ -1406,27 +1458,12 @@ Conflicts are automatically resolved using Last-Write-Wins (LWW) strategy via `C 2. **Newer wins**: The side with the newer timestamp wins 3. **Tie-breaker**: When timestamps are equal, remote wins (server-authoritative) -```typescript -async autoResolveConflictsLWW(conflicts: EntityConflict[], nonConflictingOps: Operation[]): Promise { - for (const conflict of conflicts) { - const localMaxTimestamp = Math.max(...conflict.localOps.map(op => op.timestamp)); - const remoteMaxTimestamp = Math.max(...conflict.remoteOps.map(op => op.timestamp)); - - if (localMaxTimestamp > remoteMaxTimestamp) { - // Local wins - create new UPDATE op with current entity state - const localWinOp = await this._createLocalWinUpdateOp(conflict); - // Reject both old local and remote ops - await this.opLogStore.markRejected([...localOpIds, ...remoteOpIds]); - // New op will sync local state on next upload - await this.opLogStore.append(localWinOp, 'local'); - } else { - // Remote wins (including tie) - await this.operationApplier.applyOperations(conflict.remoteOps); - await this.opLogStore.markRejected(localOpIds); - } - } -} -``` +The selected operation or synthetic merge is persisted and applied before either original side +is rejected. Only after reducer and archive work succeeds does the service finalize rejections +and emit the conflict journal entry. A failed resolution therefore leaves the originals eligible +for retry instead of recording a winner that never entered state. If a synthetic disjoint merge +cannot pass reducer replay, that synthetic row is quarantined and the same conflict immediately +falls back to ordinary LWW; a `merged` journal entry is emitted only when the merge itself succeeds. ### When Local Wins @@ -1871,9 +1908,12 @@ fresh id. **Status:** Handled via Locks -- Compaction acquires `sp_op_log_compact` lock -- Sync operations use separate locks -- **Verified safe**: Compaction only deletes ops with `syncedAt` set, so unsynced ops from active sync are preserved +- Compaction and sync serialize on the operation-log lock +- Compaction aborts before snapshotting while any non-rejected `pending` remote row exists +- Deletion requires terminal status: synced applied/legacy-complete rows or old rejected rows +- `archive_pending` and `failed` quarantine rows survive regardless of age +- Emergency compaction returns `false` when it skips for pending reducer work or an empty/degraded + live state; callers only treat an actually written snapshot/prune pass as success --- diff --git a/docs/sync-and-op-log/operation-rules.md b/docs/sync-and-op-log/operation-rules.md index 3fa400feda..8e921c5a52 100644 --- a/docs/sync-and-op-log/operation-rules.md +++ b/docs/sync-and-op-log/operation-rules.md @@ -1,6 +1,6 @@ # Operation Log: Design Rules & Guidelines -**Last Updated:** December 2025 +**Last Updated:** July 2026 **Related:** [Operation Log Architecture](./operation-log-architecture.md) This document establishes the core rules and principles for designing the Operation Log store and defining new Operations. Adherence to these rules ensures data integrity, synchronization reliability, and system performance. @@ -9,17 +9,18 @@ This document establishes the core rules and principles for designing the Operat ### 1.1 Append-Only Persistence -- **Rule:** The `ops` table in the store must be strictly **append-only** for active operations. +- **Rule:** Operation payloads in the `ops` table are append-only. Lifecycle metadata may advance in place as described in 1.2. - **Reasoning:** History preservation is critical for event sourcing and conflict resolution. -- **Exception:** Operations can only be deleted by the **Compaction Service**, and only if they are: - 1. Older than the retention window. - 2. Successfully synced (`syncedAt` is set). - 3. "Baked" into a secure snapshot. +- **Exception:** Operations can only be deleted by the **Compaction Service**, and only if they are older than the retention window, baked into the current snapshot (`seq <= lastAppliedOpSeq`), and either: + 1. Successfully synced and application-complete; or + 2. Terminally rejected. +- **Never compact:** Non-rejected `pending`, `archive_pending`, and `failed` remote work is retained regardless of age. ### 1.2 Immutable History -- **Rule:** Once an operation is written to `SUP_OPS`, it **MUST NOT** be modified. +- **Rule:** Once an operation is written to `SUP_OPS`, its operation ID, payload, action, vector clock, source, and sequence **MUST NOT** be modified. - **Reasoning:** Modifying history breaks the cryptographic chain (if implemented later) and confuses sync peers who have already received the operation. +- **Lifecycle exception:** Local bookkeeping metadata advances in place: `syncedAt`, `rejectedAt`, `reducerRejectedAt`, `applicationStatus`, `retryCount`, `lastRetryAt`, and `error`. These fields describe local delivery/application state; they do not rewrite the operation peers exchange. - **Correction:** If an operation was incorrect, append a new _compensating operation_ (e.g., an undo or correction op) rather than editing the old one. ### 1.3 Single Source of Truth diff --git a/docs/wiki/3.06-User-Data.md b/docs/wiki/3.06-User-Data.md index 79be42cc52..89290a0f0e 100644 --- a/docs/wiki/3.06-User-Data.md +++ b/docs/wiki/3.06-User-Data.md @@ -77,7 +77,7 @@ The Windows Store build uses a different path that includes the Windows Store pa ### IndexedDB Databases -- **SUP_OPS** (current, version 8): Main database. Object stores: `ops`, `state_cache`, `import_backup`, `vector_clock`, `archive_young`, `archive_old`. Holds operations log, state snapshots, vector clocks, and archives. +- **SUP_OPS** (current, version 9): Main database. Object stores: `ops`, `state_cache`, `import_backup`, `vector_clock`, `archive_young`, `archive_old`. Holds operations log, state snapshots, vector clocks, and archives. - **pf:** Legacy database used for migration and recovery. - **SUPPluginCache:** Plugin cache. - **SUP_CONFLICT_JOURNAL** (version 1): Sync **conflict journal** — a device-local record of automatic sync-conflict resolutions, reviewable under `/sync-conflicts`. One object store (`conflicts`). Deliberately separate from `SUP_OPS`, **never synced or uploaded** (entries capture the discarded side of conflicts verbatim), pruned on start to 14 days / newest 200 entries, and cleared whenever the full dataset is replaced (backup import/restore, profile switch). @@ -177,7 +177,7 @@ On mobile (Android/iOS), if the app launches with no local data but a usable on- ## Configuration and Versioning - There are no separate config files in the User Data Folder. Configuration is stored in IndexedDB (`SUP_OPS`, `state_cache`) and in localStorage (`SUP_*` and related keys). -- **Database version:** `SUP_OPS` is at schema version 8. +- **Database version:** `SUP_OPS` is at schema version 9. - **Application version:** Defined in the build (e.g. `src/environments/versions.ts`), not stored in the User Data Folder. - **Backup files:** Each JSON file carries full state; desktop automatic backup naming includes date and time. diff --git a/packages/sync-core/src/apply.types.ts b/packages/sync-core/src/apply.types.ts index 7c79608c03..a33d44be22 100644 --- a/packages/sync-core/src/apply.types.ts +++ b/packages/sync-core/src/apply.types.ts @@ -1,5 +1,10 @@ import type { Operation } from './operation.types'; +export interface OperationApplyFailure = Operation> { + op: TOperation; + error: Error; +} + /** * Result of applying a batch of operations. * @@ -10,13 +15,14 @@ export interface ApplyOperationsResult = Op /** Operations that were successfully applied. */ appliedOps: TOperation[]; + /** Operations skipped because their host reducer/conversion threw. */ + reducerFailures?: OperationApplyFailure[]; + /** * If an error occurred, this contains the failed operation and the error. - * The failed op and the operations after it in the batch did NOT complete - * their archive side effects — but their reducer effects DID commit: the - * bulk dispatch is all-or-nothing and runs before archive handling, so - * archive side effects are the only per-op failure point. Retry paths must - * therefore use `skipReducerDispatch` to avoid double-applying reducers. + * The failed op and the successfully reducer-applied operations after it did + * NOT complete their archive side effects. Their reducer effects DID commit, + * so retry paths must use `skipReducerDispatch` to avoid double-applying them. */ failedOp?: { op: TOperation; diff --git a/packages/sync-core/src/index.ts b/packages/sync-core/src/index.ts index d967d45040..8bbdafdc55 100644 --- a/packages/sync-core/src/index.ts +++ b/packages/sync-core/src/index.ts @@ -75,7 +75,11 @@ export { createLwwUpdateActionTypeHelpers } from './lww-update-action-types'; export type { LwwUpdateActionTypeHelpers } from './lww-update-action-types'; // Apply-operation result and option types. -export type { ApplyOperationsResult, ApplyOperationsOptions } from './apply.types'; +export type { + ApplyOperationsResult, + ApplyOperationsOptions, + OperationApplyFailure, +} from './apply.types'; // Generic operation replay coordinator. export { replayOperationBatch } from './replay-coordinator'; diff --git a/packages/sync-core/src/operation.types.ts b/packages/sync-core/src/operation.types.ts index c356f98064..29e90d8c8b 100644 --- a/packages/sync-core/src/operation.types.ts +++ b/packages/sync-core/src/operation.types.ts @@ -147,6 +147,14 @@ export interface OperationLogEntry = Operat */ rejectedAt?: number; + /** + * Timestamp (epoch ms) when replay skipped this operation because conversion, + * schema migration, or reducer application could not produce state. Unlike + * ordinary conflict rejection, this operation must never be replayed during + * hydration. + */ + reducerRejectedAt?: number; + /** * For remote ops only: tracks whether the op was successfully applied to * local state. `archive_pending` means reducers and clocks committed but diff --git a/packages/sync-core/src/ports.ts b/packages/sync-core/src/ports.ts index da02c6fece..a8fcf8ed27 100644 --- a/packages/sync-core/src/ports.ts +++ b/packages/sync-core/src/ports.ts @@ -1,4 +1,8 @@ -import type { ApplyOperationsOptions, ApplyOperationsResult } from './apply.types'; +import type { + ApplyOperationsOptions, + ApplyOperationsResult, + OperationApplyFailure, +} from './apply.types'; import type { Operation } from './operation.types'; export type SyncPortMeta = Record; @@ -22,7 +26,10 @@ export interface OperationApplyPort = Opera ops: TOperation[], options?: ApplyOperationsOptions & { /** Persist reducer-commit bookkeeping before post-dispatch side effects start. */ - onReducersCommitted?: (ops: TOperation[]) => Promise; + onReducersCommitted?: ( + ops: TOperation[], + failures?: OperationApplyFailure[], + ) => Promise; }, ): Promise>; } @@ -40,7 +47,10 @@ export interface ReducerCommitAwareOperationApplyPort< applyOperations( ops: TOperation[], options: ApplyOperationsOptions & { - onReducersCommitted: (ops: TOperation[]) => Promise; + onReducersCommitted: ( + ops: TOperation[], + failures?: OperationApplyFailure[], + ) => Promise; }, ): Promise>; } diff --git a/packages/sync-core/src/remote-apply.ts b/packages/sync-core/src/remote-apply.ts index 50a18ad0b8..f962d27f71 100644 --- a/packages/sync-core/src/remote-apply.ts +++ b/packages/sync-core/src/remote-apply.ts @@ -1,4 +1,4 @@ -import type { ApplyOperationsResult } from './apply.types'; +import type { ApplyOperationsResult, OperationApplyFailure } from './apply.types'; import type { Operation } from './operation.types'; import type { ReducerCommitAwareOperationApplyPort } from './ports'; @@ -25,7 +25,11 @@ export interface RemoteOperationApplyStorePort< options: { pendingApply: true }, ): Promise>; mergeRemoteOpClocks(ops: TOperation[]): Promise; - markReducersCommittedAndMergeClocks(seqs: number[], ops: TOperation[]): Promise; + markReducersCommittedAndMergeClocks( + seqs: number[], + ops: TOperation[], + rejectedOpIds?: string[], + ): Promise; markApplied(seqs: number[]): Promise; markFailed(opIds: string[]): Promise; clearFullStateOpsExcept(excludeIds: string[]): Promise; @@ -52,6 +56,8 @@ export interface RemoteApplyOperationsResult< clearedFullStateOpCount: number; failedOp?: ApplyOperationsResult['failedOp']; failedOpIds: string[]; + reducerFailures?: OperationApplyFailure[]; + reducerFailedOpIds?: string[]; } const emptyRemoteApplyResult = < @@ -76,9 +82,9 @@ const emptyRemoteApplyResult = < * 2. durably merge all newly appended remote clocks before reducers or a * deferred-local-action window can start; * 3. bulk-apply only the newly appended ops through the host applier; - * 4. at reducer commit, atomically checkpoint the whole batch as - * `archive_pending` and merge all reducer-committed vector clocks before - * archive side effects begin; + * 4. at reducer commit, atomically checkpoint reducer-successful ops as + * `archive_pending`, reject reducer-failed ops, and merge successful clocks + * before archive side effects begin; * 5. mark archive-complete seqs applied; * 6. after applying a full-state op, clear older full-state ops while * retaining every full-state op of this batch (incl. quarantined ones); @@ -129,6 +135,8 @@ export const applyRemoteOperations = async < let reducerCommitCallbackCount = 0; let reducerCommitCallbackError: Error | undefined; let reducerCommitPromise: Promise | undefined; + let reducerCommittedOps: TOperation[] | undefined; + let reducerFailures: OperationApplyFailure[] | undefined; let applyResult: ApplyOperationsResult; const observeReducerCommitPromisePreservingPrimaryError = async (): Promise => { const promise = reducerCommitPromise; @@ -145,7 +153,7 @@ export const applyRemoteOperations = async < }; try { applyResult = await applier.applyOperations(appendResult.writtenOps, { - onReducersCommitted: (reducerCommittedOps) => { + onReducersCommitted: (reportedCommittedOps, reportedFailures = []) => { reducerCommitCallbackCount++; if (reducerCommitCallbackCount !== 1) { reducerCommitCallbackError = new Error( @@ -154,31 +162,63 @@ export const applyRemoteOperations = async < throw reducerCommitCallbackError; } - const isEntireAppendedBatch = - reducerCommittedOps.length === appendResult.writtenOps.length && - reducerCommittedOps.every( - (op, index) => op.id === appendResult.writtenOps[index]?.id, - ); - if (!isEntireAppendedBatch) { + const authoritativeReducerFailures = getAuthoritativeReducerFailures( + appendResult.writtenOps, + reportedFailures, + ); + const failedFullStateOp = authoritativeReducerFailures.find((failure) => + isFullStateOperation(failure.op), + ); + if (failedFullStateOp) { reducerCommitCallbackError = new Error( - 'applyRemoteOperations: reducer-commit callback must contain the entire appended batch in order.', + `applyRemoteOperations: full-state operation ${failedFullStateOp.op.id} failed during reducer replay.`, ); throw reducerCommitCallbackError; } + reducerFailures = authoritativeReducerFailures; + const reducerFailedIds = new Set( + authoritativeReducerFailures.map((failure) => failure.op.id), + ); + const expectedCommittedOps = appendResult.writtenOps.filter( + (op) => !reducerFailedIds.has(op.id), + ); + const isExactSuccessfulSubset = + reportedCommittedOps.length === expectedCommittedOps.length && + reportedCommittedOps.every( + (op, index) => op.id === expectedCommittedOps[index]?.id, + ); + if (!isExactSuccessfulSubset) { + reducerCommitCallbackError = new Error( + 'applyRemoteOperations: reducer-commit callback and failures must partition the entire appended batch in order.', + ); + throw reducerCommitCallbackError; + } + reducerCommittedOps = expectedCommittedOps; reducerCommitPromise = (async () => { - const reducerCommittedSeqs = appendResult.writtenOps + const reducerCommittedSeqs = expectedCommittedOps .map((op) => opIdToSeq.get(op.id)) .filter((seq): seq is number => seq !== undefined); - if (reducerCommittedSeqs.length !== appendResult.writtenOps.length) { + if (reducerCommittedSeqs.length !== expectedCommittedOps.length) { throw new Error( 'applyRemoteOperations: reducer commit contained an operation outside the appended batch.', ); } - await store.markReducersCommittedAndMergeClocks( - reducerCommittedSeqs, - appendResult.writtenOps, + const reducerFailedOpIds = authoritativeReducerFailures.map( + (failure) => failure.op.id, ); + if (reducerFailedOpIds.length > 0) { + await store.markReducersCommittedAndMergeClocks( + reducerCommittedSeqs, + expectedCommittedOps, + reducerFailedOpIds, + ); + } else { + await store.markReducersCommittedAndMergeClocks( + reducerCommittedSeqs, + expectedCommittedOps, + ); + } })(); return reducerCommitPromise; }, @@ -194,7 +234,12 @@ export const applyRemoteOperations = async < await observeReducerCommitPromisePreservingPrimaryError(); throw reducerCommitCallbackError; } - if (reducerCommitCallbackCount !== 1 || reducerCommitPromise === undefined) { + if ( + reducerCommitCallbackCount !== 1 || + reducerCommitPromise === undefined || + reducerCommittedOps === undefined || + reducerFailures === undefined + ) { throw new Error( 'applyRemoteOperations: applier did not invoke the reducer-commit callback.', ); @@ -203,20 +248,37 @@ export const applyRemoteOperations = async < // awaiting its returned promise. Pending ops must never be marked applied // before the reducer/archive checkpoint is durable. await reducerCommitPromise; + const authoritativeReducerCommittedOps = reducerCommittedOps; + const authoritativeReducerFailures = reducerFailures; - const appliedOpCount = applyResult.appliedOps.length; - const hasExactAppliedPrefix = - appliedOpCount <= appendResult.writtenOps.length && - applyResult.appliedOps.every( - (op, index) => op.id === appendResult.writtenOps[index]?.id, + const resultReducerFailures = getAuthoritativeReducerFailures( + appendResult.writtenOps, + applyResult.reducerFailures ?? [], + ); + const hasMatchingReducerFailures = + resultReducerFailures.length === authoritativeReducerFailures.length && + resultReducerFailures.every( + (failure, index) => failure.op.id === authoritativeReducerFailures[index]?.op.id, ); - if (!hasExactAppliedPrefix) { + if (!hasMatchingReducerFailures) { throw new Error( - 'applyRemoteOperations: applied operations must be the exact ordered prefix of the appended batch.', + 'applyRemoteOperations: applier result must report the same reducer failures as the reducer-commit callback.', ); } - const expectedFailedOp = appendResult.writtenOps[appliedOpCount]; + const appliedOpCount = applyResult.appliedOps.length; + const hasExactAppliedPrefix = + appliedOpCount <= authoritativeReducerCommittedOps.length && + applyResult.appliedOps.every( + (op, index) => op.id === authoritativeReducerCommittedOps[index]?.id, + ); + if (!hasExactAppliedPrefix) { + throw new Error( + 'applyRemoteOperations: applied operations must be the exact ordered prefix of the reducer-committed operations.', + ); + } + + const expectedFailedOp = authoritativeReducerCommittedOps[appliedOpCount]; if ( (applyResult.failedOp && applyResult.failedOp.op.id !== expectedFailedOp?.id) || (!applyResult.failedOp && expectedFailedOp !== undefined) @@ -229,7 +291,10 @@ export const applyRemoteOperations = async < ); } - const authoritativeAppliedOps = appendResult.writtenOps.slice(0, appliedOpCount); + const authoritativeAppliedOps = authoritativeReducerCommittedOps.slice( + 0, + appliedOpCount, + ); const authoritativeFailedOp = applyResult.failedOp ? { op: expectedFailedOp!, @@ -264,6 +329,7 @@ export const applyRemoteOperations = async < } const failedOpIds = authoritativeFailedOp ? [authoritativeFailedOp.op.id] : []; + const reducerFailedOpIds = authoritativeReducerFailures.map((failure) => failure.op.id); if (failedOpIds.length > 0) { await store.markFailed(failedOpIds); @@ -277,5 +343,31 @@ export const applyRemoteOperations = async < clearedFullStateOpCount, failedOp: authoritativeFailedOp, failedOpIds, + ...(authoritativeReducerFailures.length > 0 + ? { reducerFailures: authoritativeReducerFailures, reducerFailedOpIds } + : {}), }; }; + +const getAuthoritativeReducerFailures = >( + writtenOps: TOperation[], + reportedFailures: OperationApplyFailure[], +): OperationApplyFailure[] => { + const writtenOpById = new Map(writtenOps.map((op) => [op.id, op])); + const seenIds = new Set(); + + return reportedFailures.map((failure) => { + const authoritativeOp = writtenOpById.get(failure.op.id); + if (!authoritativeOp || seenIds.has(failure.op.id)) { + throw new Error( + `applyRemoteOperations: invalid reducer failure for operation ${failure.op.id}.`, + ); + } + seenIds.add(failure.op.id); + return { + op: authoritativeOp, + error: + failure.error instanceof Error ? failure.error : new Error(String(failure.error)), + }; + }); +}; diff --git a/packages/sync-core/src/replay-coordinator.ts b/packages/sync-core/src/replay-coordinator.ts index 57792e3d47..54a7fb23e1 100644 --- a/packages/sync-core/src/replay-coordinator.ts +++ b/packages/sync-core/src/replay-coordinator.ts @@ -1,4 +1,8 @@ -import type { ApplyOperationsOptions, ApplyOperationsResult } from './apply.types'; +import type { + ApplyOperationsOptions, + ApplyOperationsResult, + OperationApplyFailure, +} from './apply.types'; import type { Operation } from './operation.types'; import type { ActionDispatchPort, @@ -29,13 +33,18 @@ export interface OperationReplayCoordinatorOptions< applyOptions?: ApplyOperationsOptions; dispatcher: ActionDispatchPort; createBulkApplyAction: (ops: TOperation[]) => TBulkAction; + /** Reads reducer failures captured synchronously during the bulk dispatch. */ + getReducerFailures?: () => OperationApplyFailure[]; remoteApplyWindow: RemoteApplyWindowPort; deferredLocalActions: DeferredLocalActionsPort; archiveSideEffects?: ArchiveSideEffectPort; operationToAction?: (op: TOperation) => TReplayAction; isArchiveAffectingAction?: (action: TReplayAction) => boolean; onRemoteArchiveDataApplied?: () => void; - onReducersCommitted?: (ops: TOperation[]) => Promise; + onReducersCommitted?: ( + ops: TOperation[], + failures: OperationApplyFailure[], + ) => Promise; onArchiveSideEffectError?: ( context: OperationReplayArchiveFailureContext, ) => void; @@ -75,8 +84,8 @@ export const yieldToEventLoop = (): Promise => * 2. dispatch the host's bulk replay action (skipped when the caller marks * reducers as already committed via `skipReducerDispatch` — retry paths * must not double-apply additive reducers); - * 3. yield once so host state reducers finish before side effects; - * 4. run remote archive side effects after dispatch, when configured; + * 3. yield once, then exclude any host-reported reducer failures; + * 4. checkpoint and run archive side effects only for reducer-successful ops; * 5. start post-sync cooldown before closing the remote-apply window; * 6. close the window and flush deferred local actions. */ @@ -89,6 +98,7 @@ export const replayOperationBatch = async < applyOptions = {}, dispatcher, createBulkApplyAction, + getReducerFailures, remoteApplyWindow, deferredLocalActions, archiveSideEffects, @@ -116,16 +126,21 @@ export const replayOperationBatch = async < } remoteApplyWindow.startApplyingRemoteOps(); + let reducerCommittedOps = ops; + let reducerFailures: OperationApplyFailure[] = []; try { if (!skipReducerDispatch) { dispatcher.dispatch(createBulkApplyAction(ops)); await waitForEventLoop(); - await onReducersCommitted?.(ops); + reducerFailures = validateReducerFailures(ops, getReducerFailures?.() ?? []); + const reducerFailedIds = new Set(reducerFailures.map((failure) => failure.op.id)); + reducerCommittedOps = ops.filter((op) => !reducerFailedIds.has(op.id)); + await onReducersCommitted?.(reducerCommittedOps, reducerFailures); } if (!isLocalHydration && archiveSideEffects !== undefined && operationToAction) { const archiveResult = await processArchiveSideEffects({ - ops, + ops: reducerCommittedOps, archiveSideEffects, operationToAction, isArchiveAffectingAction, @@ -137,6 +152,7 @@ export const replayOperationBatch = async < return { appliedOps: archiveResult.appliedOps, failedOp: archiveResult.failedOp, + ...(reducerFailures.length > 0 ? { reducerFailures } : {}), }; } @@ -157,7 +173,37 @@ export const replayOperationBatch = async < await deferredLocalActions.processDeferredActions(); } - return { appliedOps: ops }; + return { + appliedOps: reducerCommittedOps, + ...(reducerFailures.length > 0 ? { reducerFailures } : {}), + }; +}; + +const validateReducerFailures = >( + ops: TOperation[], + reportedFailures: OperationApplyFailure[], +): OperationApplyFailure[] => { + const opById = new Map(ops.map((op) => [op.id, op])); + const seenIds = new Set(); + + return reportedFailures.map((failure) => { + const authoritativeOp = opById.get(failure.op.id); + if (!authoritativeOp) { + throw new Error( + `replayOperationBatch: reducer failure references unknown operation ${failure.op.id}.`, + ); + } + if (seenIds.has(failure.op.id)) { + throw new Error( + `replayOperationBatch: reducer failure reported more than once for ${failure.op.id}.`, + ); + } + seenIds.add(failure.op.id); + return { + op: authoritativeOp, + error: toError(failure.error), + }; + }); }; const processArchiveSideEffects = async < diff --git a/packages/sync-core/tests/remote-apply.spec.ts b/packages/sync-core/tests/remote-apply.spec.ts index d5e149365c..fc070206f8 100644 --- a/packages/sync-core/tests/remote-apply.spec.ts +++ b/packages/sync-core/tests/remote-apply.spec.ts @@ -180,6 +180,78 @@ describe('applyRemoteOperations', () => { expect(result.failedOpIds).toEqual(['op-2']); }); + it('rejects reducer-failed ops while checkpointing and applying successful successors', async () => { + const op1 = createOperation('op-1'); + const op2 = createOperation('op-2'); + const op3 = createOperation('op-3'); + const reducerError = new Error('reducer failure'); + const store = createStore({ + seqs: [1, 2, 3], + writtenOps: [op1, op2, op3], + skippedCount: 0, + }); + const applier: ReducerCommitAwareOperationApplyPort> = { + applyOperations: vi.fn(async (_ops, options) => { + await options.onReducersCommitted([op1, op3], [{ op: op2, error: reducerError }]); + return { + appliedOps: [op1, op3], + reducerFailures: [{ op: op2, error: reducerError }], + }; + }), + }; + + const result = await applyRemoteOperations({ + ops: [op1, op2, op3], + store, + applier, + }); + + expect(store.markReducersCommittedAndMergeClocks).toHaveBeenCalledWith( + [1, 3], + [op1, op3], + ['op-2'], + ); + expect(store.markApplied).toHaveBeenCalledWith([1, 3]); + expect(store.markFailed).not.toHaveBeenCalled(); + expect(result.appliedOps).toEqual([op1, op3]); + expect(result.reducerFailedOpIds).toEqual(['op-2']); + expect(result.reducerFailures).toEqual([{ op: op2, error: reducerError }]); + }); + + it('fails closed before checkpointing when a full-state reducer fails', async () => { + const fullStateOp = { + ...createOperation('sync-import'), + opType: 'SYNC_IMPORT', + }; + const reducerError = new Error('full-state reducer failure'); + const store = createStore({ + seqs: [1], + writtenOps: [fullStateOp], + skippedCount: 0, + }); + const applier: ReducerCommitAwareOperationApplyPort> = { + applyOperations: vi.fn(async (_ops, options) => { + await options.onReducersCommitted([], [{ op: fullStateOp, error: reducerError }]); + return { + appliedOps: [], + reducerFailures: [{ op: fullStateOp, error: reducerError }], + }; + }), + }; + + await expect( + applyRemoteOperations({ + ops: [fullStateOp], + store, + applier, + isFullStateOperation: (op) => op.opType === 'SYNC_IMPORT', + }), + ).rejects.toThrow(/full-state.*reducer/i); + + expect(store.markReducersCommittedAndMergeClocks).not.toHaveBeenCalled(); + expect(store.markApplied).not.toHaveBeenCalled(); + }); + 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'); diff --git a/packages/sync-core/tests/replay-coordinator.spec.ts b/packages/sync-core/tests/replay-coordinator.spec.ts index 9d738f8796..e090689744 100644 --- a/packages/sync-core/tests/replay-coordinator.spec.ts +++ b/packages/sync-core/tests/replay-coordinator.spec.ts @@ -269,6 +269,52 @@ describe('replayOperationBatch', () => { ]); }); + it('skips reducer-failed operations during checkpointing and archive handling', async () => { + const op1 = createOperation('op-1'); + const op2 = createOperation('op-2'); + const op3 = createOperation('op-3'); + const reducerError = new Error('reducer failed'); + const onReducersCommitted = vi.fn(async () => undefined); + const archiveSideEffects: ArchiveSideEffectPort = { + handleOperation: vi.fn(async () => undefined), + }; + + const result = await replayOperationBatch({ + ops: [op1, op2, op3], + dispatcher: { dispatch: vi.fn() }, + createBulkApplyAction: (operations) => ({ + type: '[Test] Bulk Apply', + operations, + }), + getReducerFailures: () => [{ op: op2, error: reducerError }], + remoteApplyWindow: createRemoteApplyWindow([]), + deferredLocalActions: createDeferredLocalActions([]), + archiveSideEffects, + operationToAction: (op) => ({ type: '[Test] Action', opId: op.id }), + onReducersCommitted, + yieldToEventLoop: vi.fn(async () => undefined), + }); + + expect(onReducersCommitted).toHaveBeenCalledOnce(); + expect(onReducersCommitted).toHaveBeenCalledWith( + [op1, op3], + [{ op: op2, error: reducerError }], + ); + expect(archiveSideEffects.handleOperation).toHaveBeenCalledTimes(2); + expect(archiveSideEffects.handleOperation).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ opId: 'op-1' }), + ); + expect(archiveSideEffects.handleOperation).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ opId: 'op-3' }), + ); + expect(result).toEqual({ + appliedOps: [op1, op3], + reducerFailures: [{ op: op2, error: reducerError }], + }); + }); + it('runs only archive side effects when skipReducerDispatch is set (retry path)', async () => { const callOrder: string[] = []; const ops = [createOperation('op-1'), createOperation('op-2')]; diff --git a/src/app/features/archive/archive-compression.service.spec.ts b/src/app/features/archive/archive-compression.service.spec.ts index a1a42ecb87..a19ee41594 100644 --- a/src/app/features/archive/archive-compression.service.spec.ts +++ b/src/app/features/archive/archive-compression.service.spec.ts @@ -2,6 +2,8 @@ import { TestBed } from '@angular/core/testing'; import { ArchiveCompressionService } from './archive-compression.service'; import { ArchiveDbAdapter } from '../../core/persistence/archive-db-adapter.service'; import { ArchiveModel } from './archive.model'; +import { LockService } from '../../op-log/sync/lock.service'; +import { LOCK_NAMES } from '../../op-log/core/operation-log.const'; const emptyArchive = (): ArchiveModel => ({ task: { ids: [], entities: {} }, @@ -12,6 +14,7 @@ const emptyArchive = (): ArchiveModel => ({ describe('ArchiveCompressionService', () => { let service: ArchiveCompressionService; let archiveDbAdapterMock: jasmine.SpyObj; + let lockServiceMock: jasmine.SpyObj; beforeEach(() => { archiveDbAdapterMock = jasmine.createSpyObj('ArchiveDbAdapter', [ @@ -26,11 +29,14 @@ describe('ArchiveCompressionService', () => { archiveDbAdapterMock.saveArchiveYoung.and.resolveTo(undefined); archiveDbAdapterMock.saveArchiveOld.and.resolveTo(undefined); archiveDbAdapterMock.saveArchivesAtomic.and.resolveTo(undefined); + lockServiceMock = jasmine.createSpyObj('LockService', ['request']); + lockServiceMock.request.and.callFake(async (_name, callback) => callback()); TestBed.configureTestingModule({ providers: [ ArchiveCompressionService, { provide: ArchiveDbAdapter, useValue: archiveDbAdapterMock }, + { provide: LockService, useValue: lockServiceMock }, ], }); @@ -59,5 +65,40 @@ describe('ArchiveCompressionService', () => { expect(archiveDbAdapterMock.saveArchiveYoung).not.toHaveBeenCalled(); expect(archiveDbAdapterMock.saveArchiveOld).not.toHaveBeenCalled(); }); + + it('serializes the complete read-modify-write behind TASK_ARCHIVE', async () => { + const callOrder: string[] = []; + lockServiceMock.request.and.callFake( + async (_name: string, callback: () => Promise): Promise => { + callOrder.push('lock-start'); + const result = await callback(); + callOrder.push('lock-end'); + return result; + }, + ); + archiveDbAdapterMock.loadArchiveYoung.and.callFake(async () => { + callOrder.push('load-young'); + return emptyArchive(); + }); + archiveDbAdapterMock.loadArchiveOld.and.callFake(async () => { + callOrder.push('load-old'); + return emptyArchive(); + }); + archiveDbAdapterMock.saveArchivesAtomic.and.callFake(async () => { + callOrder.push('save'); + }); + + await service.compressArchive(Date.now()); + + expect(lockServiceMock.request).toHaveBeenCalledOnceWith( + LOCK_NAMES.TASK_ARCHIVE, + jasmine.any(Function), + ); + expect(callOrder[0]).toBe('lock-start'); + expect(callOrder.at(-1)).toBe('lock-end'); + expect(callOrder.indexOf('load-young')).toBeGreaterThan(0); + expect(callOrder.indexOf('load-old')).toBeGreaterThan(0); + expect(callOrder.indexOf('save')).toBeLessThan(callOrder.indexOf('lock-end')); + }); }); }); diff --git a/src/app/features/archive/archive-compression.service.ts b/src/app/features/archive/archive-compression.service.ts index 575ae2a8a2..0f6899b096 100644 --- a/src/app/features/archive/archive-compression.service.ts +++ b/src/app/features/archive/archive-compression.service.ts @@ -2,6 +2,8 @@ import { inject, Injectable } from '@angular/core'; import { ArchiveDbAdapter } from '../../core/persistence/archive-db-adapter.service'; import { ArchiveTask, TimeSpentOnDayCopy } from '../tasks/task.model'; import { ArchiveModel } from './archive.model'; +import { LockService } from '../../op-log/sync/lock.service'; +import { LOCK_NAMES } from '../../op-log/core/operation-log.const'; export interface CompressionPreview { subtasksToDelete: number; @@ -21,6 +23,7 @@ const DEFAULT_ARCHIVE: ArchiveModel = { }) export class ArchiveCompressionService { private _archiveDbAdapter = inject(ArchiveDbAdapter); + private _lockService = inject(LockService); /** * Calculate compression preview statistics for UI display. @@ -74,19 +77,24 @@ export class ArchiveCompressionService { * DETERMINISTIC: Same input produces same output across all clients. */ async compressArchive(oneYearAgoTimestamp: number): Promise { - const [archiveYoung, archiveOld] = await Promise.all([ - this._archiveDbAdapter.loadArchiveYoung().then((a) => a ?? DEFAULT_ARCHIVE), - this._archiveDbAdapter.loadArchiveOld().then((a) => a ?? DEFAULT_ARCHIVE), - ]); + await this._lockService.request(LOCK_NAMES.TASK_ARCHIVE, async () => { + const [archiveYoung, archiveOld] = await Promise.all([ + this._archiveDbAdapter.loadArchiveYoung().then((a) => a ?? DEFAULT_ARCHIVE), + this._archiveDbAdapter.loadArchiveOld().then((a) => a ?? DEFAULT_ARCHIVE), + ]); - const newArchiveYoung = this._compressArchiveData(archiveYoung, oneYearAgoTimestamp); - const newArchiveOld = this._compressArchiveData(archiveOld, oneYearAgoTimestamp); + const newArchiveYoung = this._compressArchiveData( + archiveYoung, + oneYearAgoTimestamp, + ); + const newArchiveOld = this._compressArchiveData(archiveOld, oneYearAgoTimestamp); - // Atomic write: both archives written in a single IndexedDB transaction. - // Two independent writes could tear on a crash between them, and since - // compression is op-replayed on other clients a half-compressed local - // result would diverge from replicas (#8843). - await this._archiveDbAdapter.saveArchivesAtomic(newArchiveYoung, newArchiveOld); + // Keep the complete read-modify-write cycle under the same cross-tab lock + // used by archive replacement and ordinary archive mutations. The atomic + // database write prevents a torn pair; the lock prevents overwriting a + // concurrent replacement with data read before that replacement. + await this._archiveDbAdapter.saveArchivesAtomic(newArchiveYoung, newArchiveOld); + }); } private _compressArchiveData( diff --git a/src/app/features/archive/task-archive.service.ts b/src/app/features/archive/task-archive.service.ts index e69aa7ccc9..5bec285164 100644 --- a/src/app/features/archive/task-archive.service.ts +++ b/src/app/features/archive/task-archive.service.ts @@ -119,9 +119,9 @@ export class TaskArchiveService { * and silently drop one side's archive write. * * Known paths still OUTSIDE the lock (tracked in #8941): - * ArchiveCompressionService.compressArchive, the remote loadAllData archive - * import (holds no lock across its user-confirmation guard by design), and - * TimeTrackingService's project/tag archive cleanups. + * TimeTrackingService's project/tag archive cleanups. Compression and remote + * full-state archive replacement use the same lock for their complete + * read-modify-write cycles. */ private _runTaskArchiveMutation(mutation: () => Promise): Promise { return this._lockService.request(LOCK_NAMES.TASK_ARCHIVE, mutation); diff --git a/src/app/op-log/apply/archive-operation-handler.service.spec.ts b/src/app/op-log/apply/archive-operation-handler.service.spec.ts index da7215e984..971d8f5914 100644 --- a/src/app/op-log/apply/archive-operation-handler.service.spec.ts +++ b/src/app/op-log/apply/archive-operation-handler.service.spec.ts @@ -16,6 +16,8 @@ import { TimeTrackingService } from '../../features/time-tracking/time-tracking. import { loadAllData } from '../../root-store/meta/load-all-data.action'; import { ArchiveDbAdapter } from '../../core/persistence/archive-db-adapter.service'; import { OpType } from '../core/operation.types'; +import { LockService } from '../sync/lock.service'; +import { LOCK_NAMES } from '../core/operation-log.const'; describe('isArchiveAffectingAction', () => { it('should return true for moveToArchive action', () => { @@ -95,6 +97,7 @@ describe('ArchiveOperationHandler', () => { let mockTaskArchiveService: jasmine.SpyObj; let mockArchiveDbAdapter: jasmine.SpyObj; let mockTimeTrackingService: jasmine.SpyObj; + let mockLockService: jasmine.SpyObj; const createMockTaskWithSubTasks = ( id: string, @@ -140,6 +143,8 @@ describe('ArchiveOperationHandler', () => { 'cleanupDataEverywhereForProject', 'cleanupArchiveDataForTag', ]); + mockLockService = jasmine.createSpyObj('LockService', ['request']); + mockLockService.request.and.callFake(async (_lockName, callback) => callback()); mockArchiveDbAdapter = jasmine.createSpyObj('ArchiveDbAdapter', [ 'loadArchiveYoung', 'loadArchiveOld', @@ -188,6 +193,7 @@ describe('ArchiveOperationHandler', () => { { provide: TaskArchiveService, useValue: mockTaskArchiveService }, { provide: ArchiveDbAdapter, useValue: mockArchiveDbAdapter }, { provide: TimeTrackingService, useValue: mockTimeTrackingService }, + { provide: LockService, useValue: mockLockService }, ], }); @@ -873,9 +879,10 @@ describe('ArchiveOperationHandler', () => { await service.handleOperation(action); - expect(mockArchiveDbAdapter.saveArchiveYoung).toHaveBeenCalledWith( - archiveYoungData, - ); + const [savedYoung, savedOld] = + mockArchiveDbAdapter.saveArchivesAtomic.calls.mostRecent().args; + expect(savedYoung).toBe(archiveYoungData); + expect(savedOld.task.ids).toEqual([]); }); it('should write archiveOld to IndexedDB for remote operations', async () => { @@ -889,7 +896,10 @@ describe('ArchiveOperationHandler', () => { await service.handleOperation(action); - expect(mockArchiveDbAdapter.saveArchiveOld).toHaveBeenCalledWith(archiveOldData); + const [savedYoung, savedOld] = + mockArchiveDbAdapter.saveArchivesAtomic.calls.mostRecent().args; + expect(savedYoung.task.ids).toEqual([]); + expect(savedOld).toBe(archiveOldData); }); it('should write both archiveYoung and archiveOld for remote operations', async () => { @@ -904,10 +914,33 @@ describe('ArchiveOperationHandler', () => { await service.handleOperation(action); - expect(mockArchiveDbAdapter.saveArchiveYoung).toHaveBeenCalledWith( + expect(mockArchiveDbAdapter.saveArchivesAtomic).toHaveBeenCalledOnceWith( archiveYoungData, + archiveOldData, ); - expect(mockArchiveDbAdapter.saveArchiveOld).toHaveBeenCalledWith(archiveOldData); + expect(mockLockService.request).toHaveBeenCalledWith( + LOCK_NAMES.TASK_ARCHIVE, + jasmine.any(Function), + ); + }); + + it('should commit both remote archive halves in one transaction', async () => { + const archiveYoungData = createArchiveModelForTest(['young-1']); + const archiveOldData = createArchiveModelForTest(['old-1']); + const action = { + type: loadAllData.type, + appDataComplete: { archiveYoung: archiveYoungData, archiveOld: archiveOldData }, + meta: { isPersistent: true, isRemote: true, opType: OpType.SyncImport }, + } as unknown as PersistentAction; + + await service.handleOperation(action); + + expect(mockArchiveDbAdapter.saveArchivesAtomic).toHaveBeenCalledOnceWith( + archiveYoungData, + archiveOldData, + ); + expect(mockArchiveDbAdapter.saveArchiveYoung).not.toHaveBeenCalled(); + expect(mockArchiveDbAdapter.saveArchiveOld).not.toHaveBeenCalled(); }); it('should NOT write archive for local operations (already done by PfapiService)', async () => { @@ -923,6 +956,7 @@ describe('ArchiveOperationHandler', () => { expect(mockArchiveDbAdapter.saveArchiveYoung).not.toHaveBeenCalled(); expect(mockArchiveDbAdapter.saveArchiveOld).not.toHaveBeenCalled(); + expect(mockArchiveDbAdapter.saveArchivesAtomic).not.toHaveBeenCalled(); }); it('should NOT write archive when isRemote is undefined (treated as local)', async () => { @@ -938,6 +972,7 @@ describe('ArchiveOperationHandler', () => { expect(mockArchiveDbAdapter.saveArchiveYoung).not.toHaveBeenCalled(); expect(mockArchiveDbAdapter.saveArchiveOld).not.toHaveBeenCalled(); + expect(mockArchiveDbAdapter.saveArchivesAtomic).not.toHaveBeenCalled(); }); it('should handle missing archiveYoung gracefully', async () => { @@ -952,7 +987,10 @@ describe('ArchiveOperationHandler', () => { await service.handleOperation(action); expect(mockArchiveDbAdapter.saveArchiveYoung).not.toHaveBeenCalled(); - expect(mockArchiveDbAdapter.saveArchiveOld).toHaveBeenCalledWith(archiveOldData); + const [savedYoung, savedOld] = + mockArchiveDbAdapter.saveArchivesAtomic.calls.mostRecent().args; + expect(savedYoung.task.ids).toEqual([]); + expect(savedOld).toBe(archiveOldData); }); it('should handle missing archiveOld gracefully', async () => { @@ -966,10 +1004,11 @@ describe('ArchiveOperationHandler', () => { await service.handleOperation(action); - expect(mockArchiveDbAdapter.saveArchiveYoung).toHaveBeenCalledWith( - archiveYoungData, - ); expect(mockArchiveDbAdapter.saveArchiveOld).not.toHaveBeenCalled(); + const [savedYoung, savedOld] = + mockArchiveDbAdapter.saveArchivesAtomic.calls.mostRecent().args; + expect(savedYoung).toBe(archiveYoungData); + expect(savedOld.task.ids).toEqual([]); }); it('should handle empty appDataComplete gracefully', async () => { @@ -983,6 +1022,7 @@ describe('ArchiveOperationHandler', () => { expect(mockArchiveDbAdapter.saveArchiveYoung).not.toHaveBeenCalled(); expect(mockArchiveDbAdapter.saveArchiveOld).not.toHaveBeenCalled(); + expect(mockArchiveDbAdapter.saveArchivesAtomic).not.toHaveBeenCalled(); }); it('should preserve timeTracking data in archive', async () => { @@ -1004,7 +1044,7 @@ describe('ArchiveOperationHandler', () => { await service.handleOperation(action); const savedData = - mockArchiveDbAdapter.saveArchiveYoung.calls.mostRecent().args[0]; + mockArchiveDbAdapter.saveArchivesAtomic.calls.mostRecent().args[0]; expect(savedData.timeTracking.project.proj1.date20240115.s).toBe(3600000); expect(savedData.timeTracking.tag.tag1.date20240115.s).toBe(1800000); expect(savedData.lastTimeTrackingFlush).toBe(1234567890); @@ -1052,6 +1092,27 @@ describe('ArchiveOperationHandler', () => { }); describe('SYNC_IMPORT', () => { + it('should preflight both guards before atomically updating the allowed half', async () => { + const newArchiveYoung = createArchiveModelForTest(['new-young']); + const action = { + type: loadAllData.type, + appDataComplete: { + archiveYoung: newArchiveYoung, + archiveOld: emptyArchive, + }, + meta: { isPersistent: true, isRemote: true, opType: OpType.SyncImport }, + } as unknown as PersistentAction; + + await service.handleOperation(action); + + expect(mockArchiveDbAdapter.saveArchivesAtomic).toHaveBeenCalledOnceWith( + newArchiveYoung, + existingArchiveOld, + ); + expect(mockArchiveDbAdapter.saveArchiveYoung).not.toHaveBeenCalled(); + expect(mockArchiveDbAdapter.saveArchiveOld).not.toHaveBeenCalled(); + }); + it('should preserve local archiveYoung when SYNC_IMPORT has empty archive (likely bug)', async () => { const action = { type: loadAllData.type, @@ -1090,8 +1151,9 @@ describe('ArchiveOperationHandler', () => { await service.handleOperation(action); - expect(mockArchiveDbAdapter.saveArchiveYoung).toHaveBeenCalledWith( + expect(mockArchiveDbAdapter.saveArchivesAtomic).toHaveBeenCalledOnceWith( newArchive, + existingArchiveOld, ); }); }); @@ -1134,8 +1196,9 @@ describe('ArchiveOperationHandler', () => { await service.handleOperation(action); - expect(mockArchiveDbAdapter.saveArchiveYoung).toHaveBeenCalledWith( + expect(mockArchiveDbAdapter.saveArchivesAtomic).toHaveBeenCalledOnceWith( newArchive, + existingArchiveOld, ); }); }); @@ -1152,61 +1215,50 @@ describe('ArchiveOperationHandler', () => { window.confirm = originalConfirm; }); - it('should show confirm dialog and throw if user cancels (archiveYoung)', async () => { + it('should replay the originating backup decision without prompting remotely', async () => { (window.confirm as jasmine.Spy).and.returnValue(false); - const action = { type: loadAllData.type, - appDataComplete: { archiveYoung: emptyArchive }, - meta: { isPersistent: true, isRemote: true, opType: OpType.BackupImport }, - } as unknown as PersistentAction; - - await expectAsync(service.handleOperation(action)).toBeRejectedWithError( - /User cancelled backup import/, - ); - expect(window.confirm).toHaveBeenCalledWith( - jasmine.stringContaining('2 archived tasks'), - ); - expect(mockArchiveDbAdapter.saveArchiveYoung).not.toHaveBeenCalled(); - }); - - it('should show confirm dialog and proceed if user confirms (archiveYoung)', async () => { - (window.confirm as jasmine.Spy).and.returnValue(true); - - const action = { - type: loadAllData.type, - appDataComplete: { archiveYoung: emptyArchive }, - meta: { isPersistent: true, isRemote: true, opType: OpType.BackupImport }, + appDataComplete: { + archiveYoung: emptyArchive, + archiveOld: emptyArchive, + }, + meta: { + isPersistent: true, + isRemote: true, + opType: OpType.BackupImport, + }, } as unknown as PersistentAction; await service.handleOperation(action); - expect(window.confirm).toHaveBeenCalled(); - expect(mockArchiveDbAdapter.saveArchiveYoung).toHaveBeenCalledWith( + expect(window.confirm).not.toHaveBeenCalled(); + expect(mockArchiveDbAdapter.saveArchivesAtomic).toHaveBeenCalledOnceWith( + emptyArchive, emptyArchive, ); }); - it('should show confirm dialog and throw if user cancels (archiveOld)', async () => { + it('should apply an empty archiveYoung without another device-local decision', async () => { (window.confirm as jasmine.Spy).and.returnValue(false); const action = { type: loadAllData.type, - appDataComplete: { archiveOld: emptyArchive }, + appDataComplete: { archiveYoung: emptyArchive }, meta: { isPersistent: true, isRemote: true, opType: OpType.BackupImport }, } as unknown as PersistentAction; - await expectAsync(service.handleOperation(action)).toBeRejectedWithError( - /User cancelled backup import/, + await service.handleOperation(action); + + expect(window.confirm).not.toHaveBeenCalled(); + expect(mockArchiveDbAdapter.saveArchivesAtomic).toHaveBeenCalledOnceWith( + emptyArchive, + existingArchiveOld, ); - expect(window.confirm).toHaveBeenCalledWith( - jasmine.stringContaining('1 old archived tasks'), - ); - expect(mockArchiveDbAdapter.saveArchiveOld).not.toHaveBeenCalled(); }); - it('should show confirm dialog and proceed if user confirms (archiveOld)', async () => { - (window.confirm as jasmine.Spy).and.returnValue(true); + it('should apply an empty archiveOld without another device-local decision', async () => { + (window.confirm as jasmine.Spy).and.returnValue(false); const action = { type: loadAllData.type, @@ -1216,8 +1268,9 @@ describe('ArchiveOperationHandler', () => { await service.handleOperation(action); - expect(window.confirm).toHaveBeenCalled(); - expect(mockArchiveDbAdapter.saveArchiveOld).toHaveBeenCalledWith( + expect(window.confirm).not.toHaveBeenCalled(); + expect(mockArchiveDbAdapter.saveArchivesAtomic).toHaveBeenCalledOnceWith( + existingArchiveYoung, emptyArchive, ); }); @@ -1234,8 +1287,9 @@ describe('ArchiveOperationHandler', () => { await service.handleOperation(action); expect(window.confirm).not.toHaveBeenCalled(); - expect(mockArchiveDbAdapter.saveArchiveYoung).toHaveBeenCalledWith( + expect(mockArchiveDbAdapter.saveArchivesAtomic).toHaveBeenCalledOnceWith( newArchive, + existingArchiveOld, ); }); }); diff --git a/src/app/op-log/apply/archive-operation-handler.service.ts b/src/app/op-log/apply/archive-operation-handler.service.ts index 2ff5d14421..08e45d76de 100644 --- a/src/app/op-log/apply/archive-operation-handler.service.ts +++ b/src/app/op-log/apply/archive-operation-handler.service.ts @@ -24,7 +24,6 @@ import { ArchiveDbAdapter } from '../../core/persistence/archive-db-adapter.serv import { OpType } from '../core/operation.types'; import { LockService } from '../sync/lock.service'; import { LOCK_NAMES } from '../core/operation-log.const'; -import { confirmDialog } from '../../util/native-dialogs'; /** * Creates an empty ArchiveModel with default values. @@ -106,7 +105,11 @@ export const isArchiveAffectingAction = (action: Action): action is PersistentAc * * ## Important Notes * - * - Remote archive mutations serialize behind the TASK_ARCHIVE mutex (separate from the OPERATION_LOG lock sync holds) + * - Task/archive model writes, full-state archive replacement, flush, and + * compression serialize behind the TASK_ARCHIVE mutex (separate from the + * OPERATION_LOG lock sync holds) + * - TimeTrackingService's project/tag cleanup paths are not yet covered by that + * mutex (tracked in #8941) * - All operations are idempotent - safe to run multiple times * - Use `isArchiveAffectingAction()` helper to check if an action needs archive handling */ @@ -143,9 +146,11 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort { const projectId = (action as ReturnType) @@ -390,7 +395,7 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort { const tagIdsToRemove = @@ -465,8 +470,8 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort { + // Load both originals before evaluating either guard. A refused/missing half + // is preserved in the same transaction as an accepted incoming half. + const originalArchiveYoung = await this._archiveDbAdapter.loadArchiveYoung(); + const originalArchiveOld = await this._archiveDbAdapter.loadArchiveOld(); - // Write archiveYoung if present in the import data - if (archiveYoung !== undefined) { - if ( - await this._guardArchiveOverwrite({ - label: 'archiveYoung', - existing: originalArchiveYoung, - incoming: archiveYoung, - opType: action.meta.opType, - }) - ) { - await this._archiveDbAdapter.saveArchiveYoung(archiveYoung); - } - } + const shouldWriteYoung = + archiveYoung !== undefined && + (await this._guardArchiveOverwrite({ + label: 'archiveYoung', + existing: originalArchiveYoung, + incoming: archiveYoung, + opType: action.meta.opType, + })); + const shouldWriteOld = + archiveOld !== undefined && + (await this._guardArchiveOverwrite({ + label: 'archiveOld', + existing: originalArchiveOld, + incoming: archiveOld, + opType: action.meta.opType, + })); - // Write archiveOld if present in the import data - if (archiveOld !== undefined) { - if ( - await this._guardArchiveOverwrite({ - label: 'archiveOld', - existing: originalArchiveOld, - incoming: archiveOld, - opType: action.meta.opType, - }) - ) { - try { - await this._archiveDbAdapter.saveArchiveOld(archiveOld); - } catch (e) { - // Attempt rollback: restore archiveYoung to original state - OpLog.err( - '[ArchiveOperationHandler] archiveOld write failed, attempting rollback...', - e, - ); - try { - if (originalArchiveYoung !== undefined) { - await this._archiveDbAdapter.saveArchiveYoung(originalArchiveYoung); - OpLog.log('[ArchiveOperationHandler] Rollback successful'); - } - } catch (rollbackErr) { - OpLog.err( - '[ArchiveOperationHandler] Rollback FAILED - archive may be inconsistent', - rollbackErr, - ); - } - throw e; // Re-throw original error + if (!shouldWriteYoung && !shouldWriteOld) { + return false; } - } - } - OpLog.log( - '[ArchiveOperationHandler] Wrote archive data from SYNC_IMPORT/BACKUP_IMPORT', + const nextArchiveYoung = shouldWriteYoung ? archiveYoung : originalArchiveYoung; + const nextArchiveOld = shouldWriteOld ? archiveOld : originalArchiveOld; + + if (nextArchiveYoung !== undefined && nextArchiveOld !== undefined) { + await this._archiveDbAdapter.saveArchivesAtomic( + nextArchiveYoung, + nextArchiveOld, + ); + } else if (shouldWriteYoung && nextArchiveYoung !== undefined) { + await this._archiveDbAdapter.saveArchiveYoung(nextArchiveYoung); + } else if (shouldWriteOld && nextArchiveOld !== undefined) { + await this._archiveDbAdapter.saveArchiveOld(nextArchiveOld); + } + return true; + }, ); + + if (didWrite) { + OpLog.log( + '[ArchiveOperationHandler] Wrote archive data from SYNC_IMPORT/BACKUP_IMPORT', + ); + } } /** * Safety guard: prevents overwriting a non-empty local archive with an empty * incoming one. Returns true if the caller should proceed with the write. * - * Handles three opType paths: + * Handles two opType paths: * - SYNC_IMPORT / REPAIR: silently preserves local (empty incoming is likely a bug) - * - BACKUP_IMPORT: asks user for confirmation (explicit restore action) - * - default: allows the write + * - BACKUP_IMPORT / default: allows the write. BACKUP_IMPORT is already an + * explicit user decision on the originating client, so remote replay must + * be deterministic and cannot prompt independently on every other client. */ private async _guardArchiveOverwrite(opts: { label: 'archiveYoung' | 'archiveOld'; @@ -572,18 +574,6 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort +export const isTaskArchiveOrDeleteOp = (op: Operation): boolean => op.actionType === ActionType.TASK_SHARED_MOVE_TO_ARCHIVE || op.actionType === ActionType.TASK_SHARED_DELETE || op.actionType === ActionType.TASK_SHARED_DELETE_MULTIPLE; diff --git a/src/app/op-log/apply/bulk-hydration.action.ts b/src/app/op-log/apply/bulk-hydration.action.ts index 633781ce5e..1f927d204c 100644 --- a/src/app/op-log/apply/bulk-hydration.action.ts +++ b/src/app/op-log/apply/bulk-hydration.action.ts @@ -25,7 +25,16 @@ import { Operation } from '../core/operation.types'; */ export const bulkApplyOperations = createAction( '[OperationLog] Bulk Apply Operations', - props<{ operations: Operation[]; localClientId?: string }>(), + props<{ + operations: Operation[]; + localClientId?: string; + /** + * Ephemeral replay groups whose operations came from one durable source op. + * If one member fails, the meta-reducer excludes the whole group so a split + * schema migration cannot leave a state that the durable log cannot rebuild. + */ + atomicReplayGroups?: string[][]; + }>(), ); /** diff --git a/src/app/op-log/apply/bulk-hydration.meta-reducer.spec.ts b/src/app/op-log/apply/bulk-hydration.meta-reducer.spec.ts index c91f16f018..301b77ffdb 100644 --- a/src/app/op-log/apply/bulk-hydration.meta-reducer.spec.ts +++ b/src/app/op-log/apply/bulk-hydration.meta-reducer.spec.ts @@ -13,6 +13,7 @@ import { Task } from '../../features/tasks/task.model'; import { Project } from '../../features/project/project.model'; import { Tag } from '../../features/tag/tag.model'; import { toLwwUpdateActionType } from '../core/lww-update-action-types'; +import { runWithBulkReplayFailureCollector } from './bulk-replay-failure-collector'; // Set to true to run stress tests (10k+ operations) // These tests take 1-2 seconds each and are skipped by default to speed up test runs @@ -581,19 +582,26 @@ describe('bulkHydrationMetaReducer', () => { }); describe('error scenarios', () => { - it('should propagate errors from reducer', () => { - const errorReducer = jasmine - .createSpy('errorReducer') - .and.throwError('Reducer error'); + it('should isolate errors from reducer', () => { + const reducerError = new Error('Reducer error'); + const errorReducer = jasmine.createSpy('errorReducer').and.throwError(reducerError); const reducer = bulkHydrationMetaReducer(errorReducer); const state = createMockState(); const operation = createMockOperation(); const action = bulkApplyHydrationOperations({ operations: [operation] }); + const failures: Array<{ op: Operation; error: Error }> = []; - expect(() => reducer(state, action)).toThrowError('Reducer error'); + expect(() => + runWithBulkReplayFailureCollector( + (failure) => failures.push(failure), + () => reducer(state, action), + ), + ).not.toThrow(); + expect(errorReducer).toHaveBeenCalledTimes(1); + expect(failures).toEqual([{ op: operation, error: reducerError }]); }); - it('should stop processing on first error', () => { + it('should continue processing after a per-operation error', () => { let callCount = 0; const errorReducer = jasmine.createSpy('errorReducer').and.callFake(() => { callCount++; @@ -611,9 +619,85 @@ describe('bulkHydrationMetaReducer', () => { ]; const action = bulkApplyHydrationOperations({ operations }); - expect(() => reducer(state, action)).toThrowError('Second operation failed'); - // Should have called reducer twice (second call threw) - expect(callCount).toBe(2); + expect(() => reducer(state, action)).not.toThrow(); + expect(callCount).toBe(3); + }); + + it('should roll back every migrated child when one child reducer fails', () => { + const state = { applied: [] as string[] }; + const firstChild = createMockOperation({ + id: 'split-op-1', + actionType: '[Test] Split Child 1' as ActionType, + }); + const failedChild = createMockOperation({ + id: 'split-op-2', + actionType: '[Test] Split Child 2' as ActionType, + }); + const laterOp = createMockOperation({ + id: 'later-op', + actionType: '[Test] Later Operation' as ActionType, + }); + const reducerError = new Error('second migrated child failed'); + const atomicReducer = jasmine + .createSpy('atomicReducer') + .and.callFake( + (currentState: typeof state, reducerAction: Action): typeof state => { + if (reducerAction.type === failedChild.actionType) { + throw reducerError; + } + return { + applied: [...currentState.applied, reducerAction.type], + }; + }, + ); + const reducer = bulkHydrationMetaReducer(atomicReducer); + const failures: Array<{ op: Operation; error: Error }> = []; + + const result = runWithBulkReplayFailureCollector( + (failure) => failures.push(failure), + () => + reducer( + state, + bulkApplyHydrationOperations({ + operations: [firstChild, failedChild, laterOp], + atomicReplayGroups: [[firstChild.id, failedChild.id]], + }), + ), + ); + + expect(result.applied).toEqual([laterOp.actionType]); + expect(failures).toEqual([{ op: failedChild, error: reducerError }]); + }); + + it('should roll back the whole batch when a full-state reducer fails', () => { + const state = { value: 'before-import' }; + const fullStateOp = createMockOperation({ + id: 'sync-import', + opType: OpType.SyncImport, + entityType: 'ALL', + actionType: ActionType.LOAD_ALL_DATA, + payload: { appDataComplete: { task: {}, project: {} } }, + }); + const laterOp = createMockOperation({ id: 'later-op' }); + const errorReducer = jasmine + .createSpy('errorReducer') + .and.callFake( + (currentState: typeof state, reducerAction: Action): typeof state => { + if (reducerAction.type === ActionType.LOAD_ALL_DATA) { + throw new Error('full-state reducer failed'); + } + return { ...currentState, value: 'later-op-applied' }; + }, + ); + const reducer = bulkHydrationMetaReducer(errorReducer); + + const result = reducer( + state, + bulkApplyHydrationOperations({ operations: [fullStateOp, laterOp] }), + ); + + expect(result).toBe(state); + expect(errorReducer).toHaveBeenCalledTimes(1); }); }); @@ -725,6 +809,34 @@ describe('bulkHydrationMetaReducer', () => { expect(reducerCalls[0].action.type).toBe(ActionType.TASK_SHARED_MOVE_TO_ARCHIVE); }); + it('should reapply an earlier LWW Update when the later archive reducer fails', () => { + const reducerError = new Error('archive reducer failed'); + mockReducer.and.callFake((state, action) => { + reducerCalls.push({ state, action }); + if (action.type === ActionType.TASK_SHARED_MOVE_TO_ARCHIVE) { + throw reducerError; + } + return state; + }); + const reducer = bulkHydrationMetaReducer(mockReducer); + const state = createMockState(); + const lwwUpdate = createLwwUpdateOp(TASK_ID); + const archiveOp = createMoveToArchiveOp([TASK_ID]); + const failures: Array<{ op: Operation; error: Error }> = []; + + runWithBulkReplayFailureCollector( + (failure) => failures.push(failure), + () => + reducer( + state, + bulkApplyHydrationOperations({ operations: [lwwUpdate, archiveOp] }), + ), + ); + + expect(reducerCalls.map((call) => call.action.type)).toContain(TASK_LWW_TYPE); + expect(failures).toEqual([{ op: archiveOp, error: reducerError }]); + }); + it('should skip multiple LWW Updates when multi-entity archive is in same batch', () => { const reducer = bulkHydrationMetaReducer(mockReducer); const state = createMockState(); diff --git a/src/app/op-log/apply/bulk-hydration.meta-reducer.ts b/src/app/op-log/apply/bulk-hydration.meta-reducer.ts index 38822759d8..c4d66c1f20 100644 --- a/src/app/op-log/apply/bulk-hydration.meta-reducer.ts +++ b/src/app/op-log/apply/bulk-hydration.meta-reducer.ts @@ -4,10 +4,13 @@ import { convertOpToAction } from './operation-converter.util'; import { isLwwUpdateActionType } from '../core/lww-update-action-types'; import { collectArchivingOrDeletingEntityIdsFromBatch, + isTaskArchiveOrDeleteOp, stripBatchArchivedTaskIdsFromLwwPayload, } from './bulk-archive-filter.util'; import { OpLog } from '../../core/log'; import { runWithBulkReplayLoggingSuppressed } from '../../util/bulk-replay-log-guard'; +import { reportBulkReplayReducerFailure } from './bulk-replay-failure-collector'; +import { isFullStateOpType } from '../core/operation.types'; /** * Meta-reducer that applies multiple operations in a single reducer pass. @@ -46,69 +49,144 @@ export const bulkOperationsMetaReducer = ( ): ActionReducer => { return (state: T | undefined, action: Action): T => { if (action.type === bulkApplyOperations.type) { - const { operations, localClientId } = action as ReturnType< - typeof bulkApplyOperations - >; - - // Pre-scan: collect entity IDs being archived or deleted in this batch. - // LWW Update ops for these entities must be skipped to prevent - // lwwUpdateMetaReducer.addOne() from resurrecting archived/deleted tasks. - const archivingOrDeletingEntityIds = collectArchivingOrDeletingEntityIdsFromBatch( + const { operations, - state, - ); + localClientId, + atomicReplayGroups = [], + } = action as ReturnType; - const hasArchives = archivingOrDeletingEntityIds.size > 0; // Apply every op in one synchronous reducer pass. Suppress the action // logger's per-op console line for the duration (see bulk-replay-log-guard): // this is a single dispatch, and the caller (hydrator / applier) already // logs an "applying N ops" summary, so per-op `[a]` lines are just noise. const finalState = runWithBulkReplayLoggingSuppressed(() => { - let currentState = state; - for (const op of operations) { - const isLww = hasArchives && isLwwUpdateActionType(op.actionType); - // Skip LWW Updates whose entityId itself is archived/deleted in this batch - // (covers TASK; for TAG/PROJECT entityId is the tag/project id, not a task). - if (isLww && op.entityId && archivingOrDeletingEntityIds.has(op.entityId)) { - OpLog.normal( - `bulkOperationsMetaReducer: Skipping LWW Update for ` + - `${op.entityType}:${op.entityId} — entity archived/deleted in same batch`, + const failedOpIds = new Set(); + const atomicGroupByOpId = new Map(); + for (const group of atomicReplayGroups) { + for (const opId of group) { + atomicGroupByOpId.set(opId, group); + } + } + const reportFailure = ( + op: (typeof operations)[number], + error: unknown, + ): boolean => { + if (failedOpIds.has(op.id)) { + return false; + } + failedOpIds.add(op.id); + reportBulkReplayReducerFailure(op, error); + OpLog.err( + `bulkOperationsMetaReducer: Skipping reducer-failed operation ${op.id}`, + { name: error instanceof Error ? error.name : 'UnknownError' }, + ); + return true; + }; + const excludeAtomicGroup = (opId: string): boolean => { + const group = atomicGroupByOpId.get(opId); + if (!group) { + return false; + } + for (const groupedOpId of group) { + failedOpIds.add(groupedOpId); + } + return true; + }; + + // An archive/delete op affects how earlier LWW updates are replayed. If + // that archive reducer fails, discard the speculative pass and replay + // from the original state without the failed archive intent. Reducers + // are pure, and retries occur only on the exceptional failure path. + while (true) { + const candidateOps = operations.filter((op) => !failedOpIds.has(op.id)); + let archivingOrDeletingEntityIds: Set; + try { + archivingOrDeletingEntityIds = collectArchivingOrDeletingEntityIdsFromBatch( + candidateOps, + state, ); + } catch (error) { + const unsafeArchiveOps = candidateOps.filter(isTaskArchiveOrDeleteOp); + for (const op of unsafeArchiveOps) { + reportFailure(op, error); + excludeAtomicGroup(op.id); + } continue; } - const opForApply = hasArchives - ? stripBatchArchivedTaskIdsFromLwwPayload( - op, - isLww, - archivingOrDeletingEntityIds, - ) - : op; - const opAction = convertOpToAction(opForApply); - // Mark ops authored by a DIFFERENT client so reducers can preserve - // per-device "local-only" settings against remote overwrites — while - // replaying the device's OWN ops faithfully. - // - // When localClientId is unknown we leave the flag unset (own-op - // semantics: apply faithfully, don't preserve). In practice this only - // happens before the clientId cache is warm — i.e. a never-synced or - // cold-booting device. A device that actually has foreign ops to apply - // has already resolved its clientId (download/upload/vector-clock all - // require it), so genuine remote applies always carry it and stay - // protected. The unset fallback deliberately favours own-op fidelity: - // the alternative (blanket-preserve, the old `isRemote` gate) is what - // silently nulled the device's own syncProvider on replay. The - // residual risk — a foreign op adopting another device's provider/ - // isEnabled/isEncryptionEnabled — needs a transient IndexedDB failure - // on a cold boot and is user-recoverable, strictly narrower than the - // own-settings data-loss this replaces. - const isApplyingFromOtherClient = - !!localClientId && op.clientId !== localClientId; - const finalAction = isApplyingFromOtherClient - ? { ...opAction, meta: { ...opAction.meta, isApplyingFromOtherClient: true } } - : opAction; - currentState = reducer(currentState, finalAction); + + const hasArchives = archivingOrDeletingEntityIds.size > 0; + let currentState = state; + let shouldReplayWithoutFailedOperations = false; + for (const op of candidateOps) { + try { + const isLww = hasArchives && isLwwUpdateActionType(op.actionType); + // Skip LWW Updates whose entityId itself is archived/deleted in this batch + // (covers TASK; for TAG/PROJECT entityId is the tag/project id, not a task). + if (isLww && op.entityId && archivingOrDeletingEntityIds.has(op.entityId)) { + OpLog.normal( + `bulkOperationsMetaReducer: Skipping LWW Update for ` + + `${op.entityType}:${op.entityId} — entity archived/deleted in same batch`, + ); + continue; + } + const opForApply = hasArchives + ? stripBatchArchivedTaskIdsFromLwwPayload( + op, + isLww, + archivingOrDeletingEntityIds, + ) + : op; + const opAction = convertOpToAction(opForApply); + // Mark ops authored by a DIFFERENT client so reducers can preserve + // per-device "local-only" settings against remote overwrites — while + // replaying the device's OWN ops faithfully. + // + // When localClientId is unknown we leave the flag unset (own-op + // semantics: apply faithfully, don't preserve). In practice this only + // happens before the clientId cache is warm — i.e. a never-synced or + // cold-booting device. A device that actually has foreign ops to apply + // has already resolved its clientId (download/upload/vector-clock all + // require it), so genuine remote applies always carry it and stay + // protected. The unset fallback deliberately favours own-op fidelity: + // the alternative (blanket-preserve, the old `isRemote` gate) is what + // silently nulled the device's own syncProvider on replay. The + // residual risk — a foreign op adopting another device's provider/ + // isEnabled/isEncryptionEnabled — needs a transient IndexedDB failure + // on a cold boot and is user-recoverable, strictly narrower than the + // own-settings data-loss this replaces. + const isApplyingFromOtherClient = + !!localClientId && op.clientId !== localClientId; + const finalAction = isApplyingFromOtherClient + ? { + ...opAction, + meta: { ...opAction.meta, isApplyingFromOtherClient: true }, + } + : opAction; + currentState = reducer(currentState, finalAction); + } catch (error) { + const isNewFailure = reportFailure(op, error); + if (isFullStateOpType(op.opType)) { + // A full-state operation replaces the entire model. Continuing + // from the pre-import state would expose a projection that never + // existed in the log, so discard the speculative batch. + return state; + } + if (excludeAtomicGroup(op.id)) { + // The children of a split migration represent one durable + // intent. Discard this speculative pass and replay without all + // siblings so the resulting state is reconstructible. + shouldReplayWithoutFailedOperations = true; + break; + } + if (isNewFailure && isTaskArchiveOrDeleteOp(op)) { + shouldReplayWithoutFailedOperations = true; + } + } + } + if (!shouldReplayWithoutFailedOperations) { + return currentState; + } } - return currentState; }); return finalState as T; } diff --git a/src/app/op-log/apply/bulk-replay-failure-collector.ts b/src/app/op-log/apply/bulk-replay-failure-collector.ts new file mode 100644 index 0000000000..624f90bd9d --- /dev/null +++ b/src/app/op-log/apply/bulk-replay-failure-collector.ts @@ -0,0 +1,37 @@ +import type { Operation } from '../core/operation.types'; + +export interface BulkReplayReducerFailure { + op: Operation; + error: Error; +} + +type FailureCollector = (failure: BulkReplayReducerFailure) => void; + +let activeFailureCollector: FailureCollector | undefined; + +/** + * Scopes reducer-failure reporting to one synchronous NgRx bulk dispatch. + * + * Keeping the collector outside the action preserves NgRx action + * serializability and immutability. NgRx reducers run synchronously, so the + * collector is always restored before dispatch returns. + */ +export const runWithBulkReplayFailureCollector = ( + collector: FailureCollector, + run: () => T, +): T => { + const previousCollector = activeFailureCollector; + activeFailureCollector = collector; + try { + return run(); + } finally { + activeFailureCollector = previousCollector; + } +}; + +export const reportBulkReplayReducerFailure = (op: Operation, error: unknown): void => { + activeFailureCollector?.({ + op, + error: error instanceof Error ? error : new Error(String(error)), + }); +}; diff --git a/src/app/op-log/apply/operation-applier.service.spec.ts b/src/app/op-log/apply/operation-applier.service.spec.ts index c923934728..e5edd7a98c 100644 --- a/src/app/op-log/apply/operation-applier.service.spec.ts +++ b/src/app/op-log/apply/operation-applier.service.spec.ts @@ -13,6 +13,7 @@ import { remoteArchiveDataApplied } from '../../features/archive/store/archive.a import { bulkApplyOperations } from './bulk-hydration.action'; import { CLIENT_ID_PROVIDER, ClientIdProvider } from '../util/client-id.provider'; import { OperationLogEffects } from '../capture/operation-log.effects'; +import { reportBulkReplayReducerFailure } from './bulk-replay-failure-collector'; describe('OperationApplierService', () => { let service: OperationApplierService; @@ -229,6 +230,31 @@ describe('OperationApplierService', () => { }); describe('error paths', () => { + it('should report and skip reducer-failed operations before archive handling', async () => { + const op1 = createMockOperation('op-1'); + const op2 = createMockOperation('op-2'); + const op3 = createMockOperation('op-3'); + const reducerError = new Error('Reducer failed'); + const onReducersCommitted = jasmine + .createSpy('onReducersCommitted') + .and.resolveTo(); + mockStore.dispatch.and.callFake((() => { + reportBulkReplayReducerFailure(op2, reducerError); + }) as never); + + const result = await service.applyOperations([op1, op2, op3], { + onReducersCommitted, + }); + + expect(onReducersCommitted).toHaveBeenCalledOnceWith( + [op1, op3], + [{ op: op2, error: reducerError }], + ); + expect(mockArchiveOperationHandler.handleOperation).toHaveBeenCalledTimes(2); + expect(result.appliedOps).toEqual([op1, op3]); + expect(result.reducerFailures).toEqual([{ op: op2, error: reducerError }]); + }); + it('should return failed op when archiveOperationHandler throws', async () => { const op = createMockOperation('op-1', 'TASK', OpType.Update, { title: 'Test' }); const archiveError = new Error('Archive write failed'); @@ -728,6 +754,19 @@ describe('OperationApplierService', () => { expect(mockOperationLogEffects.processDeferredActions).not.toHaveBeenCalled(); }); + it('should leave an already-open remote apply window to its caller', async () => { + const op = createMockOperation('op-1'); + + await service.applyOperations([op], { + remoteApplyWindowAlreadyOpen: true, + skipDeferredLocalActions: true, + }); + + expect(mockHydrationState.startApplyingRemoteOps).not.toHaveBeenCalled(); + expect(mockHydrationState.startPostSyncCooldown).not.toHaveBeenCalled(); + expect(mockHydrationState.endApplyingRemoteOps).not.toHaveBeenCalled(); + }); + it('should call processDeferredActions after endApplyingRemoteOps', async () => { const callOrder: string[] = []; diff --git a/src/app/op-log/apply/operation-applier.service.ts b/src/app/op-log/apply/operation-applier.service.ts index 186b235e4c..4601d6fe64 100644 --- a/src/app/op-log/apply/operation-applier.service.ts +++ b/src/app/op-log/apply/operation-applier.service.ts @@ -4,6 +4,7 @@ import { replayOperationBatch } from '@sp/sync-core'; import type { ActionDispatchPort, OperationApplyPort, + RemoteApplyWindowPort, SyncActionLike, } from '@sp/sync-core'; import { Operation } from '../core/operation.types'; @@ -19,6 +20,10 @@ import { bulkApplyOperations } from './bulk-hydration.action'; import { CLIENT_ID_PROVIDER } from '../util/client-id.provider'; import { OperationLogEffects } from '../capture/operation-log.effects'; import { ApplyOperationsResult, ApplyOperationsOptions } from '../core/types/apply.types'; +import { + BulkReplayReducerFailure, + runWithBulkReplayFailureCollector, +} from './bulk-replay-failure-collector'; // Re-export for consumers that import from this service export type { @@ -26,6 +31,12 @@ export type { ApplyOperationsOptions, } from '../core/types/apply.types'; +const CALLER_MANAGED_REMOTE_APPLY_WINDOW: RemoteApplyWindowPort = { + startApplyingRemoteOps: () => undefined, + startPostSyncCooldown: () => undefined, + endApplyingRemoteOps: () => undefined, +}; + /** * Service responsible for applying operations to the local NgRx store. * @@ -104,16 +115,26 @@ export class OperationApplierService implements OperationApplyPort { // the unset flag's own-op default is the safe direction. See meta-reducer. const localClientId = (await this.clientIdProvider.loadClientId()) ?? undefined; + const reducerFailures: BulkReplayReducerFailure[] = []; const result = await replayOperationBatch({ ops, applyOptions: { isLocalHydration, skipReducerDispatch: options.skipReducerDispatch, }, - dispatcher: this.store, + dispatcher: { + dispatch: (action) => + runWithBulkReplayFailureCollector( + (failure) => reducerFailures.push(failure), + () => this.store.dispatch(action), + ), + }, createBulkApplyAction: (operations) => bulkApplyOperations({ operations, localClientId }), - remoteApplyWindow: this.hydrationState, + getReducerFailures: () => reducerFailures, + remoteApplyWindow: options.remoteApplyWindowAlreadyOpen + ? CALLER_MANAGED_REMOTE_APPLY_WINDOW + : this.hydrationState, deferredLocalActions: { processDeferredActions: () => options.skipDeferredLocalActions @@ -146,6 +167,12 @@ export class OperationApplierService implements OperationApplyPort { OpLog.normal('OperationApplierService: Finished applying operations.'); } + if (result.reducerFailures?.length) { + OpLog.err( + `OperationApplierService: Skipped ${result.reducerFailures.length} reducer-failed operation(s).`, + ); + } + return result; } } 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 a330c80f8a..1140ba1045 100644 --- a/src/app/op-log/capture/operation-log.effects.spec.ts +++ b/src/app/op-log/capture/operation-log.effects.spec.ts @@ -91,7 +91,7 @@ describe('OperationLogEffects', () => { mockVectorClockService.getCurrentVectorClock.and.returnValue( Promise.resolve({ testClient: 5 }), ); - mockCompactionService.compact.and.returnValue(Promise.resolve()); + mockCompactionService.compact.and.returnValue(Promise.resolve(true)); mockCompactionService.emergencyCompact.and.returnValue(Promise.resolve(true)); mockStore.select.and.returnValue(of({})); // Return empty state observable mockClientIdService.getOrGenerateClientId.and.returnValue( @@ -760,7 +760,7 @@ describe('OperationLogEffects', () => { mockOpLogStore.getCompactionCounter.and.returnValue( Promise.resolve(COMPACTION_THRESHOLD - 1), ); - mockCompactionService.compact.and.returnValue(Promise.resolve()); + mockCompactionService.compact.and.returnValue(Promise.resolve(true)); const action = createPersistentAction(ActionType.TASK_SHARED_UPDATE); actions$ = of(action); @@ -778,6 +778,20 @@ describe('OperationLogEffects', () => { }); expect(errorCalls.length).toBe(0); })); + + it('should keep the threshold counter when compaction safely skips', fakeAsync(() => { + mockOpLogStore.getCompactionCounter.and.resolveTo(COMPACTION_THRESHOLD - 1); + mockCompactionService.compact.and.returnValue(Promise.resolve(false)); + actions$ = of(createPersistentAction(ActionType.TASK_SHARED_UPDATE)); + + effects.persistOperation$.subscribe(); + tick(100); + + const internalState = effects as unknown as { + inMemoryCompactionCounter: number | null; + }; + expect(internalState.inMemoryCompactionCounter).toBe(COMPACTION_THRESHOLD); + })); }); describe('processDeferredActions', () => { diff --git a/src/app/op-log/capture/operation-log.effects.ts b/src/app/op-log/capture/operation-log.effects.ts index fa2c83248b..a7c30065ef 100644 --- a/src/app/op-log/capture/operation-log.effects.ts +++ b/src/app/op-log/capture/operation-log.effects.ts @@ -446,7 +446,10 @@ export class OperationLogEffects implements DeferredLocalActionsPort { OpLog.normal('OperationLogEffects: Triggering compaction...'); this.compactionService .compact() - .then(() => { + .then((didCompact) => { + if (!didCompact) { + return; + } this.compactionFailures = 0; // Reset in-memory counter on successful compaction this.inMemoryCompactionCounter = 0; diff --git a/src/app/op-log/core/types/apply.types.ts b/src/app/op-log/core/types/apply.types.ts index d27fc3cf2c..d9638124e6 100644 --- a/src/app/op-log/core/types/apply.types.ts +++ b/src/app/op-log/core/types/apply.types.ts @@ -7,10 +7,15 @@ import type { Operation } from '../operation.types'; export interface ApplyOperationsResult { /** Operations that were successfully applied. */ appliedOps: Operation[]; + /** Operations skipped because conversion or reducer application threw. */ + reducerFailures?: Array<{ + op: Operation; + error: Error; + }>; /** * First op whose archive side effect threw. Its reducer effect (and that of - * every op after it) DID commit — the bulk dispatch is all-or-nothing and - * precedes archive handling — so retry paths must pass `skipReducerDispatch`. + * every reducer-successful op after it) DID commit before archive handling, + * so retry paths must pass `skipReducerDispatch`. */ failedOp?: { op: Operation; @@ -33,6 +38,12 @@ export interface ApplyOperationsOptions { */ skipDeferredLocalActions?: boolean; + /** + * The caller already owns the remote-apply window and will close it after a + * larger multi-pass replay. The applier must not create a gap between passes. + */ + remoteApplyWindowAlreadyOpen?: boolean; + /** * When true, skip the bulk reducer dispatch and run only the archive side * effects. Used to retry ops marked `failed`, whose reducer effects already @@ -47,5 +58,8 @@ export interface ApplyOperationsOptions { * 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; + onReducersCommitted?: ( + ops: Operation[], + failures?: NonNullable, + ) => Promise; } diff --git a/src/app/op-log/persistence/db-keys.const.ts b/src/app/op-log/persistence/db-keys.const.ts index c582dc6cdc..d835eeded0 100644 --- a/src/app/op-log/persistence/db-keys.const.ts +++ b/src/app/op-log/persistence/db-keys.const.ts @@ -14,12 +14,13 @@ export const DB_NAME = 'SUP_OPS'; /** * Current database schema version. * - * Version 8 is a deliberate downgrade barrier for the distinct - * `archive_pending` reducer checkpoint. Released v7 readers only understand - * `failed`; IndexedDB must reject their attempt to open a newer database rather - * than let outstanding archive work become invisible. + * Versions 8 and 9 are deliberate downgrade barriers. Version 8 protects the + * `archive_pending` reducer checkpoint from v7 readers; version 9 protects the + * `reducerRejectedAt` replay quarantine from v8 readers. IndexedDB must reject + * older readers rather than let incomplete work become invisible or replay a + * terminal poison operation. */ -export const DB_VERSION = 8; +export const DB_VERSION = 9; /** Object store names */ export const STORE_NAMES = { diff --git a/src/app/op-log/persistence/db-upgrade.spec.ts b/src/app/op-log/persistence/db-upgrade.spec.ts index cad46872c1..cb94bea017 100644 --- a/src/app/op-log/persistence/db-upgrade.spec.ts +++ b/src/app/op-log/persistence/db-upgrade.spec.ts @@ -1,5 +1,10 @@ import { runDbUpgrade } from './db-upgrade'; -import { FULL_STATE_OPS_META_KEY, STORE_NAMES, OPS_INDEXES } from './db-keys.const'; +import { + DB_VERSION, + FULL_STATE_OPS_META_KEY, + STORE_NAMES, + OPS_INDEXES, +} from './db-keys.const'; import { deleteDB, openDB } from 'idb'; import { OpType } from '../core/operation.types'; @@ -298,7 +303,26 @@ describe('runDbUpgrade', () => { }); }); - describe('full upgrade path (version 0 to 7)', () => { + describe('version 9 downgrade barrier', () => { + it('should reject a v8 reader after the v9 database has been opened', async () => { + const dbName = `SUP_OPS_v9_barrier_${Date.now()}_${Math.random()}`; + + try { + const currentDb = await openDB(dbName, DB_VERSION, { + upgrade: (db, oldVersion, _newVersion, tx) => runDbUpgrade(db, oldVersion, tx), + }); + currentDb.close(); + + await expectAsync(openDB(dbName, 8)).toBeRejectedWith( + jasmine.objectContaining({ name: 'VersionError' }), + ); + } finally { + await deleteDB(dbName); + } + }); + }); + + describe('full upgrade path (from version 0)', () => { it('should create all stores and indexes when upgrading from version 0', () => { const { db, tx } = createMocks(); diff --git a/src/app/op-log/persistence/db-upgrade.ts b/src/app/op-log/persistence/db-upgrade.ts index 965a3c3e9a..60d24dacd8 100644 --- a/src/app/op-log/persistence/db-upgrade.ts +++ b/src/app/op-log/persistence/db-upgrade.ts @@ -150,4 +150,7 @@ export const runDbUpgrade = ( // Version 8: no shape change. The version itself is a downgrade barrier for // `archive_pending`; v7 readers must fail closed instead of silently skipping // reducer-committed operations whose archive work is still outstanding. + + // Version 9: no shape change. This downgrade barrier prevents v8 readers from + // replaying rows quarantined with `reducerRejectedAt`. }; diff --git a/src/app/op-log/persistence/op-log-db-schema.ts b/src/app/op-log/persistence/op-log-db-schema.ts index 38570c305f..84b271696a 100644 --- a/src/app/op-log/persistence/op-log-db-schema.ts +++ b/src/app/op-log/persistence/op-log-db-schema.ts @@ -40,7 +40,7 @@ export interface OpLogDbSchema { } /** - * Current `SUP_OPS` schema, mirroring db-upgrade.ts (currently v8). + * Current `SUP_OPS` schema, mirroring db-upgrade.ts. * * `name`/`version` are reused from `db-keys.const.ts` (not re-literaled) so the * adapter opens at exactly the version `runDbUpgrade` migrates to — a future diff --git a/src/app/op-log/persistence/operation-log-compaction.service.spec.ts b/src/app/op-log/persistence/operation-log-compaction.service.spec.ts index 0120037633..57506d68c8 100644 --- a/src/app/op-log/persistence/operation-log-compaction.service.spec.ts +++ b/src/app/op-log/persistence/operation-log-compaction.service.spec.ts @@ -41,6 +41,7 @@ describe('OperationLogCompactionService', () => { 'saveStateCache', 'resetCompactionCounter', 'deleteOpsWhere', + 'getPendingRemoteOps', ]); mockLockService = jasmine.createSpyObj('LockService', ['request']); mockStateSnapshot = jasmine.createSpyObj('StateSnapshotService', [ @@ -64,6 +65,7 @@ describe('OperationLogCompactionService', () => { mockOpLogStore.saveStateCache.and.returnValue(Promise.resolve()); mockOpLogStore.resetCompactionCounter.and.returnValue(Promise.resolve()); mockOpLogStore.deleteOpsWhere.and.returnValue(Promise.resolve()); + mockOpLogStore.getPendingRemoteOps.and.resolveTo([]); mockStateSnapshot.getStateSnapshot.and.returnValue(mockState); mockVectorClockService.getCurrentVectorClock.and.returnValue( Promise.resolve(mockVectorClock), @@ -277,6 +279,69 @@ describe('OperationLogCompactionService', () => { expect(capturedFilter!(oldSyncedEntry)).toBeTrue(); }); + it('should preserve old remote operations with incomplete application status', async () => { + let capturedFilter: ((entry: OperationLogEntry) => boolean) | undefined; + mockOpLogStore.deleteOpsWhere.and.callFake(async (filterFn) => { + capturedFilter = filterFn; + }); + + await service.compact(); + + for (const applicationStatus of ['pending', 'archive_pending', 'failed'] as const) { + const quarantinedEntry: OperationLogEntry = { + seq: 50, + op: {} as any, + appliedAt: Date.now() - COMPACTION_RETENTION_MS - 1000, + source: 'remote', + syncedAt: Date.now() - COMPACTION_RETENTION_MS - 500, + applicationStatus, + }; + + expect(capturedFilter!(quarantinedEntry)) + .withContext(applicationStatus) + .toBeFalse(); + } + }); + + it('should delete old rejected operations that were never synced', async () => { + let capturedFilter: ((entry: OperationLogEntry) => boolean) | undefined; + mockOpLogStore.deleteOpsWhere.and.callFake(async (filterFn) => { + capturedFilter = filterFn; + }); + + await service.compact(); + + const rejectedEntry: OperationLogEntry = { + seq: 50, + op: {} as any, + appliedAt: Date.now() - COMPACTION_RETENTION_MS - 1000, + source: 'remote', + rejectedAt: Date.now() - COMPACTION_RETENTION_MS - 500, + applicationStatus: 'pending', + }; + + expect(capturedFilter!(rejectedEntry)).toBeTrue(); + }); + + it('should skip compaction when reducer-uncommitted remote operations exist', async () => { + mockOpLogStore.getPendingRemoteOps.and.resolveTo([ + { + seq: 50, + op: {} as any, + appliedAt: Date.now(), + source: 'remote', + syncedAt: Date.now(), + applicationStatus: 'pending', + }, + ]); + + await service.compact(); + + expect(mockOpLogStore.saveStateCache).not.toHaveBeenCalled(); + expect(mockOpLogStore.resetCompactionCounter).not.toHaveBeenCalled(); + expect(mockOpLogStore.deleteOpsWhere).not.toHaveBeenCalled(); + }); + it('should not delete operations with seq greater than lastSeq', async () => { mockOpLogStore.getLastSeq.and.returnValue(Promise.resolve(100)); @@ -311,6 +376,12 @@ describe('OperationLogCompactionService', () => { expect(mockOpLogStore.resetCompactionCounter).not.toHaveBeenCalled(); }); + it('should report when regular compaction is skipped', async () => { + mockStateSnapshot.getStateSnapshot.and.returnValue({} as never); + + expect(await service.compact()).toBeFalse(); + }); + it('should handle empty vector clock', async () => { mockVectorClockService.getCurrentVectorClock.and.returnValue(Promise.resolve({})); @@ -432,6 +503,26 @@ describe('OperationLogCompactionService', () => { expect(result).toBeFalse(); }); + it('should return false when pending reducer work makes compaction skip', async () => { + mockOpLogStore.getPendingRemoteOps.and.resolveTo([ + { + seq: 1, + op: {} as never, + appliedAt: Date.now(), + source: 'remote', + applicationStatus: 'pending', + }, + ]); + + expect(await service.emergencyCompact()).toBeFalse(); + }); + + it('should return false when the empty-state safety guard skips compaction', async () => { + mockStateSnapshot.getStateSnapshot.and.returnValue({} as never); + + expect(await service.emergencyCompact()).toBeFalse(); + }); + it('should use shorter retention window than regular compaction', async () => { let capturedFilter: ((entry: OperationLogEntry) => boolean) | undefined; mockOpLogStore.deleteOpsWhere.and.callFake(async (filterFn) => { diff --git a/src/app/op-log/persistence/operation-log-compaction.service.ts b/src/app/op-log/persistence/operation-log-compaction.service.ts index 81622143f5..0f9f80714c 100644 --- a/src/app/op-log/persistence/operation-log-compaction.service.ts +++ b/src/app/op-log/persistence/operation-log-compaction.service.ts @@ -33,8 +33,8 @@ export class OperationLogCompactionService { private vectorClockService = inject(VectorClockService); private clientIdProvider: ClientIdProvider = inject(CLIENT_ID_PROVIDER); - async compact(): Promise { - await this._doCompact(COMPACTION_RETENTION_MS, false); + async compact(): Promise { + return this._doCompact(COMPACTION_RETENTION_MS, false); } /** @@ -44,8 +44,7 @@ export class OperationLogCompactionService { */ async emergencyCompact(): Promise { try { - await this._doCompact(EMERGENCY_COMPACTION_RETENTION_MS, true); - return true; + return await this._doCompact(EMERGENCY_COMPACTION_RETENTION_MS, true); } catch (e) { OpLog.err('OperationLogCompactionService: Emergency compaction failed', e); return false; @@ -57,11 +56,23 @@ export class OperationLogCompactionService { * @param retentionMs - How long to keep synced operations (in ms) * @param isEmergency - Whether this is an emergency compaction (for logging) */ - private async _doCompact(retentionMs: number, isEmergency: boolean): Promise { - await this.lockService.request(LOCK_NAMES.OPERATION_LOG, async () => { + private async _doCompact(retentionMs: number, isEmergency: boolean): Promise { + return this.lockService.request(LOCK_NAMES.OPERATION_LOG, async () => { const startTime = Date.now(); const label = isEmergency ? 'emergency ' : ''; + // A snapshot must never advance past remote operations whose reducers have + // not committed yet. Otherwise restart hydration would treat those ops as + // covered by the snapshot even though their state is missing from it. + const pendingRemoteOps = await this.opLogStore.getPendingRemoteOps(); + this.checkCompactionTimeout(startTime, `${label}pending operation check`); + if (pendingRemoteOps.length > 0) { + OpLog.warn( + 'OperationLogCompactionService: Skipping compaction — remote reducer work is pending', + ); + return false; + } + // 1. Get current state from NgRx store const currentState = this.stateSnapshot.getStateSnapshot(); this.checkCompactionTimeout(startTime, `${label}state snapshot`); @@ -82,7 +93,7 @@ export class OperationLogCompactionService { 'OperationLogCompactionService: Skipping compaction — current state has no ' + 'meaningful data (refusing to overwrite cache and prune ops against empty state)', ); - return; + return false; } // 2. Get current vector clock (max of all ops) @@ -120,16 +131,24 @@ export class OperationLogCompactionService { // 6. Reset compaction counter (persistent across tabs/restarts) await this.opLogStore.resetCompactionCounter(); - // 7. Delete old operations (keep recent for conflict resolution window) - // Only delete ops that have been synced to remote + // 7. Delete old terminal operations (keep recent for conflict resolution) const cutoff = Date.now() - retentionMs; - await this.opLogStore.deleteOpsWhere( - (entry) => - !!entry.syncedAt && // never drop unsynced ops - entry.appliedAt < cutoff && - entry.seq <= lastSeq, // keep tail for conflict frontier - ); + await this.opLogStore.deleteOpsWhere((entry) => { + const isRejected = entry.rejectedAt !== undefined; + const isApplicationComplete = + isRejected || + entry.applicationStatus === undefined || + entry.applicationStatus === 'applied'; + const terminalAt = entry.rejectedAt ?? entry.appliedAt; + + return ( + (entry.syncedAt !== undefined || isRejected) && + isApplicationComplete && + terminalAt < cutoff && + entry.seq <= lastSeq // keep tail for conflict frontier + ); + }); // Log metrics for slow compaction or emergency compaction const totalDuration = Date.now() - startTime; @@ -140,6 +159,8 @@ export class OperationLogCompactionService { isEmergency, }); } + + return true; }); } diff --git a/src/app/op-log/persistence/operation-log-hydrator.retry.integration.spec.ts b/src/app/op-log/persistence/operation-log-hydrator.retry.integration.spec.ts index 32ff621cdd..d461f6dba7 100644 --- a/src/app/op-log/persistence/operation-log-hydrator.retry.integration.spec.ts +++ b/src/app/op-log/persistence/operation-log-hydrator.retry.integration.spec.ts @@ -4,6 +4,7 @@ import { OperationLogHydratorService } from './operation-log-hydrator.service'; import { OperationLogStoreService } from './operation-log-store.service'; import { OperationLogMigrationService } from './operation-log-migration.service'; import { SchemaMigrationService } from './schema-migration.service'; +import { CURRENT_SCHEMA_VERSION } from './schema-migration.service'; import { OperationLogSnapshotService } from './operation-log-snapshot.service'; import { OperationLogRecoveryService } from './operation-log-recovery.service'; import { SyncHydrationService } from './sync-hydration.service'; @@ -18,6 +19,9 @@ import { CLIENT_ID_PROVIDER, ClientIdProvider } from '../util/client-id.provider import { ActionType, EntityType, Operation, OpType } from '../core/operation.types'; import { ApplyOperationsResult } from '../core/types/apply.types'; import { uuidv7 } from '../../util/uuid-v7'; +import { bulkApplyOperations } from '../apply/bulk-hydration.action'; +import { bulkOperationsMetaReducer } from '../apply/bulk-hydration.meta-reducer'; +import { reportBulkReplayReducerFailure } from '../apply/bulk-replay-failure-collector'; /** * Integration coverage for retryFailedRemoteOps (#8305 fix (b)) against the REAL @@ -31,7 +35,9 @@ import { uuidv7 } from '../../util/uuid-v7'; describe('OperationLogHydratorService retryFailedRemoteOps (integration, real store)', () => { let hydrator: OperationLogHydratorService; let store: OperationLogStoreService; + let ngrxStore: jasmine.SpyObj; let applier: jasmine.SpyObj; + let recovery: jasmine.SpyObj; const mockClientIdProvider: ClientIdProvider = { loadClientId: () => Promise.resolve('testClient'), @@ -69,9 +75,16 @@ describe('OperationLogHydratorService retryFailedRemoteOps (integration, real st }; beforeEach(async () => { + ngrxStore = jasmine.createSpyObj('Store', ['dispatch']); applier = jasmine.createSpyObj('OperationApplierService', [ 'applyOperations', ]); + recovery = jasmine.createSpyObj( + 'OperationLogRecoveryService', + ['recoverPendingRemoteOps', 'cleanupCorruptOps', 'attemptRecovery'], + ); + recovery.cleanupCorruptOps.and.resolveTo(); + recovery.attemptRecovery.and.resolveTo(); TestBed.configureTestingModule({ providers: [ @@ -82,17 +95,44 @@ describe('OperationLogHydratorService retryFailedRemoteOps (integration, real st { provide: OperationApplierService, useValue: applier }, // retryFailedRemoteOps touches only the store + applier; the remaining // hydrator deps must exist for DI but are never called on this path. - { provide: Store, useValue: jasmine.createSpyObj('Store', ['dispatch']) }, - { provide: OperationLogMigrationService, useValue: {} }, - { provide: SchemaMigrationService, useValue: {} }, - { provide: OperationLogSnapshotService, useValue: {} }, - { provide: OperationLogRecoveryService, useValue: {} }, + { provide: Store, useValue: ngrxStore }, + { + provide: OperationLogMigrationService, + useValue: { checkAndMigrate: () => Promise.resolve() }, + }, + SchemaMigrationService, + { + provide: OperationLogSnapshotService, + useValue: { + isValidSnapshot: () => true, + migrateSnapshotWithBackup: (snapshot: unknown) => Promise.resolve(snapshot), + saveCurrentStateAsSnapshot: () => Promise.resolve(), + }, + }, + { provide: OperationLogRecoveryService, useValue: recovery }, { provide: SyncHydrationService, useValue: {} }, - { provide: ArchiveMigrationService, useValue: {} }, - { provide: StateSnapshotService, useValue: {} }, + { + provide: ArchiveMigrationService, + useValue: { migrateArchivesIfNeeded: () => Promise.resolve() }, + }, + { + provide: StateSnapshotService, + useValue: { getStateSnapshot: () => ({}) }, + }, { provide: SnackService, useValue: {} }, - { provide: ValidateStateService, useValue: {} }, - { provide: HydrationStateService, useValue: {} }, + { + provide: ValidateStateService, + useValue: { + validateState: () => Promise.resolve({ isValid: true, typiaErrors: [] }), + }, + }, + { + provide: HydrationStateService, + useValue: { + startApplyingRemoteOps: () => {}, + endApplyingRemoteOps: () => {}, + }, + }, ], }); @@ -100,6 +140,197 @@ describe('OperationLogHydratorService retryFailedRemoteOps (integration, real st store = TestBed.inject(OperationLogStoreService); await store.init(); await store._clearAllDataForTesting(); + recovery.recoverPendingRemoteOps.and.callFake(() => store.getPendingRemoteOps()); + }); + + it('replays a split pending migration from its original durable row on two boots', async () => { + const legacyConfigOp = createOp({ + id: 'legacy-config-op', + actionType: ActionType.GLOBAL_CONFIG_UPDATE_SECTION, + entityType: 'GLOBAL_CONFIG', + entityId: 'misc', + entityIds: ['misc'], + payload: { + actionPayload: { + sectionKey: 'misc', + sectionCfg: { + isConfirmBeforeTaskDelete: true, + unrelatedMiscSetting: 'keep-me', + }, + }, + entityChanges: [], + }, + schemaVersion: 1, + }); + await store.saveStateCache({ + state: {}, + lastAppliedOpSeq: 0, + vectorClock: {}, + compactedAt: Date.now(), + schemaVersion: CURRENT_SCHEMA_VERSION, + }); + await store.appendBatchSkipDuplicates([legacyConfigOp], 'remote', { + pendingApply: true, + }); + applier.applyOperations.and.callFake(async (ops) => ({ appliedOps: ops })); + const expectedMigratedIds = ['legacy-config-op_misc', 'legacy-config-op_tasks']; + + await hydrator.hydrateStore(); + + expect(ngrxStore.dispatch.calls.mostRecent().args[0]).toEqual( + jasmine.objectContaining({ + type: bulkApplyOperations.type, + operations: jasmine.arrayWithExactContents([ + jasmine.objectContaining({ id: expectedMigratedIds[0] }), + jasmine.objectContaining({ id: expectedMigratedIds[1] }), + ]), + }), + ); + const durableAfterFirstBoot = await store.getOpById(legacyConfigOp.id); + expect(durableAfterFirstBoot?.applicationStatus).toBe('applied'); + expect(durableAfterFirstBoot?.reducerRejectedAt).toBeUndefined(); + + ngrxStore.dispatch.calls.reset(); + const rebootedHydrator = TestBed.runInInjectionContext( + () => new OperationLogHydratorService(), + ); + await rebootedHydrator.hydrateStore(); + + expect(ngrxStore.dispatch.calls.mostRecent().args[0]).toEqual( + jasmine.objectContaining({ + type: bulkApplyOperations.type, + operations: jasmine.arrayWithExactContents([ + jasmine.objectContaining({ id: expectedMigratedIds[0] }), + jasmine.objectContaining({ id: expectedMigratedIds[1] }), + ]), + }), + ); + }); + + it('keeps a partially failing split migration absent across two boots', async () => { + const legacyConfigOp = createOp({ + id: 'legacy-config-op-with-failing-child', + actionType: ActionType.GLOBAL_CONFIG_UPDATE_SECTION, + entityType: 'GLOBAL_CONFIG', + entityId: 'misc', + entityIds: ['misc'], + payload: { + actionPayload: { + sectionKey: 'misc', + sectionCfg: { + isConfirmBeforeTaskDelete: true, + unrelatedMiscSetting: 'keep-me', + }, + }, + entityChanges: [], + }, + schemaVersion: 1, + }); + await store.saveStateCache({ + state: {}, + lastAppliedOpSeq: 0, + vectorClock: {}, + compactedAt: Date.now(), + schemaVersion: CURRENT_SCHEMA_VERSION, + }); + await store.appendBatchSkipDuplicates([legacyConfigOp], 'remote', { + pendingApply: true, + }); + type ReplayState = { sections: string[] }; + let replayState: ReplayState = { sections: [] }; + let bulkDispatchCount = 0; + const replayReducer = bulkOperationsMetaReducer( + (state = { sections: [] }, action) => { + const sectionKey = (action as { sectionKey?: string }).sectionKey; + if (sectionKey === 'tasks') { + throw new Error('tasks migration child failed'); + } + return sectionKey ? { sections: [...state.sections, sectionKey] } : state; + }, + ); + ngrxStore.dispatch.and.callFake(((action: { type: string }) => { + if (action.type === bulkApplyOperations.type) { + bulkDispatchCount++; + replayState = replayReducer(replayState, action); + } + }) as never); + + await hydrator.hydrateStore(); + + expect(replayState.sections).toEqual([]); + const durableAfterFirstBoot = await store.getOpById(legacyConfigOp.id); + expect(durableAfterFirstBoot?.reducerRejectedAt).toBeDefined(); + + ngrxStore.dispatch.calls.reset(); + bulkDispatchCount = 0; + const rebootedHydrator = TestBed.runInInjectionContext( + () => new OperationLogHydratorService(), + ); + await rebootedHydrator.hydrateStore(); + + expect(replayState.sections).toEqual([]); + expect(bulkDispatchCount).toBe(0); + }); + + it('keeps a failed full-state row pending until a healthy boot applies it', async () => { + const fullStateOp = createOp({ + id: 'pending-sync-import', + actionType: ActionType.LOAD_ALL_DATA, + opType: OpType.SyncImport, + entityType: 'ALL', + entityId: undefined, + payload: { appDataComplete: {} }, + schemaVersion: CURRENT_SCHEMA_VERSION, + }); + await store.saveStateCache({ + state: {}, + lastAppliedOpSeq: 0, + vectorClock: {}, + compactedAt: Date.now(), + schemaVersion: CURRENT_SCHEMA_VERSION, + }); + await store.appendBatchSkipDuplicates([fullStateOp], 'remote', { + pendingApply: true, + }); + let shouldFailReducer = true; + ngrxStore.dispatch.and.callFake(((action: { type: string }) => { + if (shouldFailReducer && action.type === bulkApplyOperations.type) { + reportBulkReplayReducerFailure( + fullStateOp, + new Error('full-state reducer failed'), + ); + } + }) as never); + + await hydrator.hydrateStore(); + + const failedEntry = await store.getOpById(fullStateOp.id); + expect(failedEntry?.applicationStatus).toBe('pending'); + expect(failedEntry?.rejectedAt).toBeUndefined(); + expect(failedEntry?.reducerRejectedAt).toBeUndefined(); + expect(recovery.attemptRecovery).toHaveBeenCalled(); + + shouldFailReducer = false; + applier.applyOperations.and.callFake(async (ops) => ({ appliedOps: ops })); + const rebootedHydrator = TestBed.runInInjectionContext( + () => new OperationLogHydratorService(), + ); + await rebootedHydrator.hydrateStore(); + + expect((await store.getOpById(fullStateOp.id))?.applicationStatus).toBe('applied'); + expect(applier.applyOperations).toHaveBeenCalledWith( + [ + jasmine.objectContaining({ + id: fullStateOp.id, + opType: OpType.SyncImport, + payload: fullStateOp.payload, + }), + ], + { + skipReducerDispatch: true, + skipDeferredLocalActions: true, + }, + ); }); it('clears all failed ops to applied when the whole batch succeeds', async () => { 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 abd3dfb501..3ca07602ce 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 @@ -32,6 +32,7 @@ import { IndexedDBOpenError } from '../core/errors/indexed-db-open.error'; import { IDB_OPEN_ERROR_RELOAD_KEY } from './operation-log-hydrator.service'; import { SyncProviderId } from '../sync-providers/provider.const'; import { OperationLogEffects } from '../capture/operation-log.effects'; +import { reportBulkReplayReducerFailure } from '../apply/bulk-replay-failure-collector'; describe('OperationLogHydratorService', () => { let service: OperationLogHydratorService; @@ -113,6 +114,7 @@ describe('OperationLogHydratorService', () => { 'getVectorClock', 'setVectorClock', 'mergeRemoteOpClocks', + 'markReducersCommittedAndMergeClocks', 'getLatestFullStateOp', ]); mockMigrationService = jasmine.createSpyObj('OperationLogMigrationService', [ @@ -122,6 +124,7 @@ describe('OperationLogHydratorService', () => { 'needsMigration', 'migrateStateIfNeeded', 'operationNeedsMigration', + 'migrateOperation', 'migrateOperations', ]); mockStateSnapshotService = jasmine.createSpyObj('StateSnapshotService', [ @@ -181,6 +184,7 @@ describe('OperationLogHydratorService', () => { mockOpLogStore.markApplied.and.returnValue(Promise.resolve()); mockOpLogStore.markFailed.and.returnValue(Promise.resolve()); mockOpLogStore.mergeRemoteOpClocks.and.returnValue(Promise.resolve()); + mockOpLogStore.markReducersCommittedAndMergeClocks.and.resolveTo(undefined); mockOpLogStore.getLatestFullStateOp.and.returnValue(Promise.resolve(undefined)); mockOperationApplierService.applyOperations.and.returnValue( Promise.resolve({ appliedOps: [] }), @@ -188,6 +192,7 @@ describe('OperationLogHydratorService', () => { mockMigrationService.checkAndMigrate.and.returnValue(Promise.resolve()); mockSchemaMigrationService.needsMigration.and.returnValue(false); mockSchemaMigrationService.operationNeedsMigration.and.returnValue(false); + mockSchemaMigrationService.migrateOperation.and.callFake((op) => op); mockSchemaMigrationService.migrateOperations.and.callFake((ops) => ops); mockValidateStateService.validateAndRepair.and.resolveTo({ isValid: true, @@ -204,7 +209,7 @@ describe('OperationLogHydratorService', () => { mockSnapshotService.isValidSnapshot.and.returnValue(true); mockSnapshotService.migrateSnapshotWithBackup.and.callFake(async (s) => s); mockSnapshotService.saveCurrentStateAsSnapshot.and.returnValue(Promise.resolve()); - mockRecoveryService.recoverPendingRemoteOps.and.returnValue(Promise.resolve()); + mockRecoveryService.recoverPendingRemoteOps.and.resolveTo([]); mockRecoveryService.cleanupCorruptOps.and.returnValue(Promise.resolve()); mockRecoveryService.attemptRecovery.and.returnValue(Promise.resolve()); mockSyncHydrationService.hydrateFromRemoteSync.and.returnValue(Promise.resolve()); @@ -389,6 +394,122 @@ describe('OperationLogHydratorService', () => { }); describe('tail operation replay', () => { + it('should not replay an operation whose reducer was durably rejected', async () => { + const snapshot = createMockSnapshot({ lastAppliedOpSeq: 5 }); + const rejectedEntry: OperationLogEntry = { + ...createMockEntry(6, createMockOperation('op-reducer-rejected')), + rejectedAt: Date.now(), + reducerRejectedAt: Date.now(), + }; + mockOpLogStore.loadStateCache.and.resolveTo(snapshot); + mockOpLogStore.getOpsAfterSeq.and.resolveTo([rejectedEntry]); + + await service.hydrateStore(); + + expect(mockStore.dispatch).toHaveBeenCalledOnceWith( + loadAllData({ appDataComplete: mockState }), + ); + expect(mockOpLogStore.mergeRemoteOpClocks).not.toHaveBeenCalled(); + }); + + it('should durably partition pending reducer failures before archive retry', async () => { + const snapshot = createMockSnapshot({ lastAppliedOpSeq: 5 }); + const failedOp = createMockOperation('op-failed'); + const successfulOp = createMockOperation('op-success'); + const pendingEntries: OperationLogEntry[] = [ + { + seq: 6, + op: failedOp, + appliedAt: Date.now(), + source: 'remote', + applicationStatus: 'pending', + }, + { + seq: 7, + op: successfulOp, + appliedAt: Date.now(), + source: 'remote', + applicationStatus: 'pending', + }, + ]; + mockOpLogStore.loadStateCache.and.resolveTo(snapshot); + mockOpLogStore.getOpsAfterSeq.and.resolveTo(pendingEntries); + mockRecoveryService.recoverPendingRemoteOps.and.returnValue( + Promise.resolve(pendingEntries) as never, + ); + const durabilityOrder: string[] = []; + mockOpLogStore.mergeRemoteOpClocks.and.callFake(async () => { + durabilityOrder.push('clock'); + }); + mockOpLogStore.markReducersCommittedAndMergeClocks.and.callFake(async () => { + durabilityOrder.push('checkpoint'); + }); + mockStore.dispatch.and.callFake(((action: { type: string }) => { + if (action.type === bulkApplyHydrationOperations.type) { + reportBulkReplayReducerFailure(failedOp, new Error('reducer failed')); + } + }) as never); + + await service.hydrateStore(); + + expect(mockOpLogStore.markReducersCommittedAndMergeClocks).toHaveBeenCalledWith( + [7], + [successfulOp], + ['op-failed'], + ); + expect(durabilityOrder).toEqual(['clock', 'checkpoint']); + }); + + it('should leave a reducer-failed pending full-state operation recoverable', async () => { + const snapshot = createMockSnapshot({ lastAppliedOpSeq: 5 }); + const fullStateOp = createMockOperation('pending-import', OpType.SyncImport, { + entityType: 'ALL', + payload: { appDataComplete: { task: {}, project: {} } }, + }); + const pendingEntry: OperationLogEntry = { + ...createMockEntry(6, fullStateOp), + source: 'remote', + applicationStatus: 'pending', + }; + mockOpLogStore.loadStateCache.and.resolveTo(snapshot); + mockOpLogStore.getOpsAfterSeq.and.resolveTo([pendingEntry]); + mockRecoveryService.recoverPendingRemoteOps.and.resolveTo([pendingEntry]); + mockStore.dispatch.and.callFake(((action: { type: string }) => { + if (action.type === bulkApplyHydrationOperations.type) { + reportBulkReplayReducerFailure( + fullStateOp, + new Error('full-state reducer failed'), + ); + } + }) as never); + + await service.hydrateStore(); + + expect(mockOpLogStore.markReducersCommittedAndMergeClocks).not.toHaveBeenCalled(); + expect(mockRecoveryService.attemptRecovery).toHaveBeenCalled(); + }); + + it('should fail closed without rejecting a local replay failure', async () => { + const snapshot = createMockSnapshot({ lastAppliedOpSeq: 5 }); + const localOp = createMockOperation('local-replay-failure'); + const localEntry: OperationLogEntry = { + ...createMockEntry(6, localOp), + source: 'local', + }; + mockOpLogStore.loadStateCache.and.resolveTo(snapshot); + mockOpLogStore.getOpsAfterSeq.and.resolveTo([localEntry]); + mockStore.dispatch.and.callFake(((action: { type: string }) => { + if (action.type === bulkApplyHydrationOperations.type) { + reportBulkReplayReducerFailure(localOp, new Error('local replay failed')); + } + }) as never); + + await service.hydrateStore(); + + expect(mockOpLogStore.markReducersCommittedAndMergeClocks).not.toHaveBeenCalled(); + expect(mockRecoveryService.attemptRecovery).toHaveBeenCalled(); + }); + it('should replay tail operations after snapshot', async () => { const snapshot = createMockSnapshot({ lastAppliedOpSeq: 5 }); const tailOps = [ @@ -607,6 +728,72 @@ describe('OperationLogHydratorService', () => { }); describe('full state operations optimization', () => { + it('should not direct-load a full-state op while an earlier replay row is pending', async () => { + const snapshot = createMockSnapshot({ lastAppliedOpSeq: 5 }); + const pendingOp = createMockOperation('pending-op'); + const pendingEntry: OperationLogEntry = { + ...createMockEntry(6, pendingOp), + source: 'remote', + applicationStatus: 'pending', + }; + const syncImportOp = createMockOperation('sync-op', OpType.SyncImport, { + payload: { appDataComplete: { task: {}, project: {} } }, + entityType: 'ALL', + }); + const syncImportEntry = createMockEntry(7, syncImportOp); + mockOpLogStore.loadStateCache.and.resolveTo(snapshot); + mockOpLogStore.getOpsAfterSeq.and.resolveTo([pendingEntry, syncImportEntry]); + mockRecoveryService.recoverPendingRemoteOps.and.resolveTo([pendingEntry]); + + await service.hydrateStore(); + + expect(mockStore.dispatch).toHaveBeenCalledWith( + bulkApplyHydrationOperations({ + operations: [pendingOp, syncImportOp], + localClientId: 'test-client', + }), + ); + expect(mockOpLogStore.markReducersCommittedAndMergeClocks).toHaveBeenCalledWith( + [6], + [pendingOp], + [], + ); + }); + + it('should replay a pending SyncImport through reducers before checkpointing it', async () => { + const snapshot = createMockSnapshot({ lastAppliedOpSeq: 5 }); + const syncImportPayload = { task: {}, project: {} }; + const syncImportOp = createMockOperation('pending-sync-op', OpType.SyncImport, { + payload: { appDataComplete: syncImportPayload }, + entityType: 'ALL', + }); + const pendingEntry: OperationLogEntry = { + ...createMockEntry(6, syncImportOp), + source: 'remote', + applicationStatus: 'pending', + }; + mockOpLogStore.loadStateCache.and.resolveTo(snapshot); + mockOpLogStore.getOpsAfterSeq.and.resolveTo([pendingEntry]); + mockRecoveryService.recoverPendingRemoteOps.and.resolveTo([pendingEntry]); + + await service.hydrateStore(); + + expect(mockStore.dispatch).toHaveBeenCalledWith( + bulkApplyHydrationOperations({ + operations: [syncImportOp], + localClientId: 'test-client', + }), + ); + expect(mockStore.dispatch).not.toHaveBeenCalledWith( + loadAllData({ appDataComplete: syncImportPayload as never }), + ); + expect(mockOpLogStore.markReducersCommittedAndMergeClocks).toHaveBeenCalledWith( + [6], + [syncImportOp], + [], + ); + }); + it('should load SyncImport operation directly without replay', async () => { const snapshot = createMockSnapshot({ lastAppliedOpSeq: 5 }); const syncImportPayload = { task: {}, project: {} }; @@ -954,7 +1141,68 @@ describe('OperationLogHydratorService', () => { await service.hydrateStore(); - expect(mockSchemaMigrationService.migrateOperations).toHaveBeenCalled(); + expect(mockSchemaMigrationService.migrateOperation).toHaveBeenCalled(); + }); + + it('should durably reject a pending row that migration drops', async () => { + const snapshot = createMockSnapshot({ lastAppliedOpSeq: 5 }); + const pendingOp = createMockOperation('dropped-pending-op', OpType.Update, { + schemaVersion: 0, + }); + const pendingEntry: OperationLogEntry = { + ...createMockEntry(6, pendingOp), + source: 'remote', + applicationStatus: 'pending', + }; + mockOpLogStore.loadStateCache.and.resolveTo(snapshot); + mockOpLogStore.getOpsAfterSeq.and.resolveTo([pendingEntry]); + mockRecoveryService.recoverPendingRemoteOps.and.resolveTo([pendingEntry]); + mockSchemaMigrationService.operationNeedsMigration.and.returnValue(true); + mockSchemaMigrationService.migrateOperation.and.returnValue(null); + + await service.hydrateStore(); + + expect(mockOpLogStore.markReducersCommittedAndMergeClocks).toHaveBeenCalledWith( + [], + [], + [pendingOp.id], + ); + }); + + it('should checkpoint the original pending row when migration splits it', async () => { + const snapshot = createMockSnapshot({ lastAppliedOpSeq: 5 }); + const pendingOp = createMockOperation('split-pending-op', OpType.Update, { + schemaVersion: 0, + }); + const pendingEntry: OperationLogEntry = { + ...createMockEntry(6, pendingOp), + source: 'remote', + applicationStatus: 'pending', + }; + const migratedOps = [ + createMockOperation('split-pending-op-1'), + createMockOperation('split-pending-op-2'), + ]; + mockOpLogStore.loadStateCache.and.resolveTo(snapshot); + mockOpLogStore.getOpsAfterSeq.and.resolveTo([pendingEntry]); + mockRecoveryService.recoverPendingRemoteOps.and.resolveTo([pendingEntry]); + mockSchemaMigrationService.operationNeedsMigration.and.returnValue(true); + mockSchemaMigrationService.migrateOperation.and.returnValue(migratedOps); + + await service.hydrateStore(); + + expect(mockStore.dispatch).toHaveBeenCalledWith( + bulkApplyHydrationOperations({ + operations: migratedOps, + localClientId: 'test-client', + atomicReplayGroups: [migratedOps.map((op) => op.id)], + }), + ); + expect(mockOpLogStore.markReducersCommittedAndMergeClocks).toHaveBeenCalledWith( + [pendingEntry.seq], + [pendingOp], + [], + ); }); // Additional version mismatch tests @@ -1008,13 +1256,16 @@ describe('OperationLogHydratorService', () => { ...e.op, schemaVersion: CURRENT_SCHEMA_VERSION, })); - mockSchemaMigrationService.migrateOperations.and.returnValue(migratedOps); + mockSchemaMigrationService.migrateOperation.and.callFake((op) => ({ + ...op, + schemaVersion: CURRENT_SCHEMA_VERSION, + })); await service.hydrateStore(); // Both snapshot and operations should be migrated expect(mockSnapshotService.migrateSnapshotWithBackup).toHaveBeenCalled(); - expect(mockSchemaMigrationService.migrateOperations).toHaveBeenCalled(); + expect(mockSchemaMigrationService.migrateOperation).toHaveBeenCalled(); // Operations should be applied via bulk dispatch expect(mockStore.dispatch).toHaveBeenCalledWith( bulkApplyHydrationOperations({ @@ -1060,8 +1311,8 @@ describe('OperationLogHydratorService', () => { await service.hydrateStore(); - // migrateOperations should be called since some ops need migration - expect(mockSchemaMigrationService.migrateOperations).toHaveBeenCalled(); + // migrateOperation should be called since some ops need migration + expect(mockSchemaMigrationService.migrateOperation).toHaveBeenCalled(); }); it('should not migrate operations if none need migration', async () => { @@ -1086,8 +1337,8 @@ describe('OperationLogHydratorService', () => { await service.hydrateStore(); - // migrateOperations should NOT be called - expect(mockSchemaMigrationService.migrateOperations).not.toHaveBeenCalled(); + // migrateOperation should NOT be called + expect(mockSchemaMigrationService.migrateOperation).not.toHaveBeenCalled(); }); it('should handle undefined schemaVersion in snapshot (legacy data)', async () => { 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 27386e7fd0..d2f29f8b80 100644 --- a/src/app/op-log/persistence/operation-log-hydrator.service.ts +++ b/src/app/op-log/persistence/operation-log-hydrator.service.ts @@ -15,7 +15,13 @@ import { SyncHydrationService } from './sync-hydration.service'; import { ArchiveMigrationService } from './archive-migration.service'; import { OpLog } from '../../core/log'; import { StateSnapshotService, AppStateSnapshot } from '../backup/state-snapshot.service'; -import { Operation, OpType, RepairPayload } from '../core/operation.types'; +import { + Operation, + OperationLogEntry, + OpType, + RepairPayload, + isFullStateOpType, +} from '../core/operation.types'; import { SnackService } from '../../core/snack/snack.service'; import { T } from '../../t.const'; import { IndexedDBOpenError } from '../core/errors/indexed-db-open.error'; @@ -29,6 +35,10 @@ import { AppDataComplete } from '../model/model-config'; import { CLIENT_ID_PROVIDER, ClientIdProvider } from '../util/client-id.provider'; import { limitVectorClockSize } from '../../core/util/vector-clock'; import { IS_ELECTRON } from '../../app.constants'; +import { + BulkReplayReducerFailure, + runWithBulkReplayFailureCollector, +} from '../apply/bulk-replay-failure-collector'; /** * sessionStorage key used to track auto-reload attempts after IndexedDB backing store errors. @@ -36,6 +46,14 @@ import { IS_ELECTRON } from '../../app.constants'; */ export const IDB_OPEN_ERROR_RELOAD_KEY = 'sp_idb_open_reload_attempt'; +interface HydrationReplayBatch { + operations: Operation[]; + atomicReplayGroups: string[][]; + sourceOpIdByReplayedOpId: Map; + sourceOpIdsWithReplay: Set; + sourceEntryByOpId: Map; +} + /** * Handles the hydration (loading) of the application state from the operation log * during application startup. It first attempts to load a saved state snapshot, @@ -73,7 +91,7 @@ export class OperationLogHydratorService { try { // PERF: Parallel startup operations - all access different IndexedDB stores // and don't depend on each other's results, so they can run concurrently. - const [, , hasBackup] = await Promise.all([ + const [pendingRemoteOps, , hasBackup] = await Promise.all([ // Check for pending remote ops from crashed sync (touches 'ops' store) this.recoveryService.recoverPendingRemoteOps(), // Legacy migration placeholder - kept for future DB migrations if needed @@ -210,8 +228,9 @@ export class OperationLogHydratorService { // 4. Replay tail operations (A.7.13: with operation migration) // - // Replay is deliberately status-blind (getOpsAfterSeq has no status - // filter) — every entry's reducer effect belongs in state exactly once: + // Replay is status-blind except for durable reducer rejections + // (getOpsAfterSeq has no status filter) — every other entry's reducer + // effect belongs in state exactly once: // - applied ops: their effect is state history by definition. // - failed ops (remote, archive side effect threw): their reducers DID // commit before the failure (bulk dispatch precedes archive handling), @@ -224,12 +243,27 @@ export class OperationLogHydratorService { // rejections never revert state), and LWW-losing remote ops are // followed by the local-win op that overwrites them // (ConflictResolutionService). - const tailOps = await this.opLogStore.getOpsAfterSeq(snapshot.lastAppliedOpSeq); + // - reducerRejectedAt ops: conversion, schema migration, or reducer + // application could not produce state, so replay must not try them + // again on every startup. + const tailOps = ( + await this.opLogStore.getOpsAfterSeq(snapshot.lastAppliedOpSeq) + ).filter((entry) => entry.reducerRejectedAt === undefined); if (tailOps.length > 0) { // Optimization: If last op is SyncImport or Repair, skip replay and load directly - const lastOp = tailOps[tailOps.length - 1].op; - const appData = this._extractFullStateFromOp(lastOp); + const lastEntry = tailOps[tailOps.length - 1]; + const lastOp = lastEntry.op; + // The shortcut is safe only when the entire replay range has a + // durable reducer outcome. An earlier pending row still needs bulk + // replay/checkpointing even if a later full-state op replaces its + // visible state; otherwise that row would quarantine sync forever. + const hasPendingReducerWork = tailOps.some( + (entry) => entry.applicationStatus === 'pending', + ); + const appData = hasPendingReducerWork + ? undefined + : this._extractFullStateFromOp(lastOp); if (appData) { OpLog.normal( `OperationLogHydratorService: Last of ${tailOps.length} tail ops is ${lastOp.opType}, loading directly`, @@ -258,9 +292,10 @@ export class OperationLogHydratorService { // Snapshot will be saved after next batch of regular operations } else { // A.7.13: Migrate tail operations before replay - const opsToReplay = this._migrateTailOps(tailOps.map((e) => e.op)); + const replayBatch = this._migrateTailOps(tailOps); + const opsToReplay = replayBatch.operations; - const droppedCount = tailOps.length - opsToReplay.length; + const droppedCount = tailOps.length - replayBatch.sourceOpIdsWithReplay.size; OpLog.normal( `OperationLogHydratorService: Replaying ${opsToReplay.length} tail ops ` + `(${droppedCount} dropped during migration).`, @@ -275,15 +310,12 @@ export class OperationLogHydratorService { // See bulkOperationsMetaReducer. const localClientId = (await this.clientIdProvider.loadClientId()) ?? undefined; - this.hydrationStateService.startApplyingRemoteOps(); - this.store.dispatch( - bulkApplyOperations({ operations: opsToReplay, localClientId }), + const tailOpIds = new Set(tailOps.map((entry) => entry.op.id)); + await this._dispatchHydrationReplay( + replayBatch, + localClientId, + pendingRemoteOps.filter((entry) => tailOpIds.has(entry.op.id)), ); - this.hydrationStateService.endApplyingRemoteOps(); - - // Merge replayed ops' clocks into local clock - // This ensures subsequent ops have clocks that dominate these tail ops - await this.opLogStore.mergeRemoteOpClocks(opsToReplay); // CHECKPOINT C: Validate state after replaying tail operations. // If invalid, we keep the data on screen but skip the snapshot save so @@ -309,9 +341,11 @@ export class OperationLogHydratorService { ); // No snapshot means we might be in a fresh install state or post-migration-check with no legacy data. // We must replay ALL operations from the beginning of the log. - // Status-blind on purpose — see the replay-policy note in the snapshot - // branch above. - const allOps = await this.opLogStore.getOpsAfterSeq(0); + // Status-blind except for durable reducer rejections — see the replay + // policy note in the snapshot branch above. + const allOps = (await this.opLogStore.getOpsAfterSeq(0)).filter( + (entry) => entry.reducerRejectedAt === undefined, + ); if (allOps.length === 0) { // Fresh install - no data at all @@ -323,8 +357,14 @@ export class OperationLogHydratorService { } // Optimization: If last op is SyncImport or Repair, skip replay and load directly - const lastOp = allOps[allOps.length - 1].op; - const appData = this._extractFullStateFromOp(lastOp); + const lastEntry = allOps[allOps.length - 1]; + const lastOp = lastEntry.op; + const hasPendingReducerWork = allOps.some( + (entry) => entry.applicationStatus === 'pending', + ); + const appData = hasPendingReducerWork + ? undefined + : this._extractFullStateFromOp(lastOp); if (appData) { OpLog.normal( `OperationLogHydratorService: Last of ${allOps.length} ops is ${lastOp.opType}, loading directly`, @@ -346,9 +386,10 @@ export class OperationLogHydratorService { // No snapshot save needed - full state ops already contain complete state } else { // A.7.13: Migrate all operations before replay - const opsToReplay = this._migrateTailOps(allOps.map((e) => e.op)); + const replayBatch = this._migrateTailOps(allOps); + const opsToReplay = replayBatch.operations; - const droppedCount = allOps.length - opsToReplay.length; + const droppedCount = allOps.length - replayBatch.sourceOpIdsWithReplay.size; OpLog.normal( `OperationLogHydratorService: Replaying all ${opsToReplay.length} ops ` + `(${droppedCount} dropped during migration).`, @@ -362,14 +403,12 @@ export class OperationLogHydratorService { // for the common case (replaying THIS device's own ops). See // bulkOperationsMetaReducer. const localClientId = (await this.clientIdProvider.loadClientId()) ?? undefined; - this.hydrationStateService.startApplyingRemoteOps(); - this.store.dispatch( - bulkApplyOperations({ operations: opsToReplay, localClientId }), + const allOpIds = new Set(allOps.map((entry) => entry.op.id)); + await this._dispatchHydrationReplay( + replayBatch, + localClientId, + pendingRemoteOps.filter((entry) => allOpIds.has(entry.op.id)), ); - this.hydrationStateService.endApplyingRemoteOps(); - - // Merge replayed ops' clocks into local clock - await this.opLogStore.mergeRemoteOpClocks(opsToReplay); // CHECKPOINT C: Validate state after replaying all operations. // If invalid, we still proceed but skip the snapshot save so we don't @@ -433,6 +472,93 @@ export class OperationLogHydratorService { } } + /** + * Replays a hydration batch and durably records its reducer outcome before + * startup can retry archive side effects or save a snapshot past the batch. + */ + private async _dispatchHydrationReplay( + replayBatch: HydrationReplayBatch, + localClientId: string | undefined, + pendingRemoteOps: OperationLogEntry[], + ): Promise { + const { + operations, + atomicReplayGroups, + sourceOpIdByReplayedOpId, + sourceOpIdsWithReplay, + sourceEntryByOpId, + } = replayBatch; + const reducerFailures: BulkReplayReducerFailure[] = []; + this.hydrationStateService.startApplyingRemoteOps(); + try { + runWithBulkReplayFailureCollector( + (failure) => reducerFailures.push(failure), + () => + this.store.dispatch( + bulkApplyOperations({ + operations, + localClientId, + ...(atomicReplayGroups.length > 0 ? { atomicReplayGroups } : {}), + }), + ), + ); + } finally { + this.hydrationStateService.endApplyingRemoteOps(); + } + + const failedFullStateOp = reducerFailures.find((failure) => + isFullStateOpType(failure.op.opType), + ); + if (failedFullStateOp) { + throw failedFullStateOp.error; + } + + const failedLocalOp = reducerFailures.find((failure) => { + const sourceOpId = sourceOpIdByReplayedOpId.get(failure.op.id) ?? failure.op.id; + return sourceEntryByOpId.get(sourceOpId)?.source === 'local'; + }); + if (failedLocalOp) { + throw failedLocalOp.error; + } + + const reducerFailedSourceOpIds = new Set( + reducerFailures.map( + (failure) => sourceOpIdByReplayedOpId.get(failure.op.id) ?? failure.op.id, + ), + ); + const committedPendingEntries = pendingRemoteOps.filter( + (entry) => + sourceOpIdsWithReplay.has(entry.op.id) && + !reducerFailedSourceOpIds.has(entry.op.id), + ); + const committedPendingOps = committedPendingEntries.map((entry) => entry.op); + const committedPendingSeqs = committedPendingEntries.map((entry) => entry.seq); + const migratedOutPendingEntries = pendingRemoteOps.filter( + (entry) => !sourceOpIdsWithReplay.has(entry.op.id), + ); + const migratedOutPendingOpIds = migratedOutPendingEntries.map((entry) => entry.op.id); + const rejectedOpIds = [ + ...new Set([...reducerFailedSourceOpIds, ...migratedOutPendingOpIds]), + ]; + + // Make the entire replay frontier durable before terminally marking any + // reducer failure. If startup crashes after the clock write, the rows stay + // pending and replay safely; the inverse order could filter a rejected row + // on the next boot before its clock was ever merged. + await this.opLogStore.mergeRemoteOpClocks([ + ...operations, + ...migratedOutPendingEntries.map((entry) => entry.op), + ]); + + if (committedPendingOps.length > 0 || rejectedOpIds.length > 0) { + await this.opLogStore.markReducersCommittedAndMergeClocks( + committedPendingSeqs, + committedPendingOps, + rejectedOpIds, + ); + } + } + /** * Extracts full application state from operations that contain complete state. * Returns undefined for operations that don't contain full state (normal CRUD ops). @@ -482,17 +608,17 @@ export class OperationLogHydratorService { * Migrates tail operations to current schema version (A.7.13). * Operations that should be dropped (e.g., for removed features) are filtered out. * - * @param ops - The operations to migrate - * @returns Array of migrated operations + * @param entries - The durable operation-log rows to migrate + * @returns Migrated operations plus their durable source-row lineage */ - private _migrateTailOps(ops: Operation[]): Operation[] { + private _migrateTailOps(entries: OperationLogEntry[]): HydrationReplayBatch { // Lenient boundary: a malformed stored schemaVersion (legacy or corrupt // entry) must not abort the WHOLE hydration into attemptRecovery() — that // trades one questionable op for possible tail-data loss on every boot. // Strict parsing stays on the receive/upload paths; locally we replay the // op verbatim as a best effort (stamping the current version so // migrateOperations passes it through unchanged, preserving order). - const sanitizedOps = ops.map((op) => { + const sanitizedOps = entries.map(({ op }) => { try { getOperationSchemaVersion(op); return op; @@ -510,15 +636,57 @@ export class OperationLogHydratorService { this.schemaMigrationService.operationNeedsMigration(op), ); + const sourceOpIdByReplayedOpId = new Map(); + const sourceOpIdsWithReplay = new Set(); + const sourceEntryByOpId = new Map(entries.map((entry) => [entry.op.id, entry])); + if (!needsMigration) { - return sanitizedOps; + for (const op of sanitizedOps) { + sourceOpIdByReplayedOpId.set(op.id, op.id); + sourceOpIdsWithReplay.add(op.id); + } + return { + operations: sanitizedOps, + atomicReplayGroups: [], + sourceOpIdByReplayedOpId, + sourceOpIdsWithReplay, + sourceEntryByOpId, + }; } OpLog.normal( `OperationLogHydratorService: Migrating ${sanitizedOps.length} tail ops to current schema version...`, ); - return this.schemaMigrationService.migrateOperations(sanitizedOps); + const atomicReplayGroups: string[][] = []; + const operations = sanitizedOps.flatMap((op) => { + const migrationResult = this.schemaMigrationService.operationNeedsMigration(op) + ? this.schemaMigrationService.migrateOperation(op) + : op; + const migratedOps = migrationResult + ? Array.isArray(migrationResult) + ? migrationResult + : [migrationResult] + : []; + if (migratedOps.length > 0) { + sourceOpIdsWithReplay.add(op.id); + } + if (migratedOps.length > 1) { + atomicReplayGroups.push(migratedOps.map((migratedOp) => migratedOp.id)); + } + for (const migratedOp of migratedOps) { + sourceOpIdByReplayedOpId.set(migratedOp.id, op.id); + } + return migratedOps; + }); + + return { + operations, + atomicReplayGroups, + sourceOpIdByReplayedOpId, + sourceOpIdsWithReplay, + sourceEntryByOpId, + }; } /** 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 3d332d0f46..0ed82b4d79 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 @@ -6,6 +6,8 @@ import { LegacyPfDbService } from '../../core/persistence/legacy-pf-db.service'; import { ClientIdService } from '../../core/util/client-id.service'; import { ActionType, OpType } from '../core/operation.types'; import { ValidateStateService } from '../validation/validate-state.service'; +import { LockService } from '../sync/lock.service'; +import { LOCK_NAMES } from '../core/operation-log.const'; describe('OperationLogRecoveryService', () => { let service: OperationLogRecoveryService; @@ -14,12 +16,15 @@ describe('OperationLogRecoveryService', () => { let mockLegacyPfDb: jasmine.SpyObj; let mockClientIdService: jasmine.SpyObj; let mockValidateStateService: jasmine.SpyObj; + let mockLockService: jasmine.SpyObj; beforeEach(() => { mockStore = jasmine.createSpyObj('Store', ['dispatch']); mockOpLogStore = jasmine.createSpyObj('OperationLogStoreService', [ 'append', + 'appendRecoveryOperationAndSnapshot', 'getLastSeq', + 'loadStateCache', 'saveStateCache', 'setVectorClock', 'getPendingRemoteOps', @@ -31,6 +36,8 @@ describe('OperationLogRecoveryService', () => { 'getUnsynced', ]); mockOpLogStore.setVectorClock.and.resolveTo(undefined); + mockOpLogStore.getLastSeq.and.resolveTo(0); + mockOpLogStore.loadStateCache.and.resolveTo(null); mockOpLogStore.recoverLegacyTerminalRemoteFailures.and.resolveTo(0); mockLegacyPfDb = jasmine.createSpyObj('LegacyPfDbService', [ 'hasUsableEntityData', @@ -40,6 +47,8 @@ describe('OperationLogRecoveryService', () => { mockValidateStateService = jasmine.createSpyObj('ValidateStateService', [ 'validateState', ]); + mockLockService = jasmine.createSpyObj('LockService', ['request']); + mockLockService.request.and.callFake(async (_lockName, callback) => callback()); mockValidateStateService.validateState.and.resolveTo({ isValid: true, typiaErrors: [], @@ -53,6 +62,7 @@ describe('OperationLogRecoveryService', () => { { provide: LegacyPfDbService, useValue: mockLegacyPfDb }, { provide: ClientIdService, useValue: mockClientIdService }, { provide: ValidateStateService, useValue: mockValidateStateService }, + { provide: LockService, useValue: mockLockService }, ], }); service = TestBed.inject(OperationLogRecoveryService); @@ -65,20 +75,34 @@ describe('OperationLogRecoveryService', () => { mockLegacyPfDb.loadAllEntityData.and.resolveTo(legacyData as any); mockClientIdService.loadClientId.and.resolveTo('testClient'); mockOpLogStore.append.and.resolveTo(undefined); - mockOpLogStore.getLastSeq.and.resolveTo(1); + mockOpLogStore.getLastSeq.and.returnValues(Promise.resolve(0), Promise.resolve(1)); mockOpLogStore.saveStateCache.and.resolveTo(undefined); await service.attemptRecovery(); - expect(mockOpLogStore.append).toHaveBeenCalledWith( + expect( + ( + mockOpLogStore as unknown as { + appendRecoveryOperationAndSnapshot: jasmine.Spy; + } + ).appendRecoveryOperationAndSnapshot, + ).toHaveBeenCalledWith( jasmine.objectContaining({ actionType: ActionType.RECOVERY_DATA_IMPORT, opType: OpType.Batch, entityType: 'RECOVERY', payload: legacyData, }), + legacyData, ); + expect(mockOpLogStore.append).not.toHaveBeenCalled(); + expect(mockOpLogStore.saveStateCache).not.toHaveBeenCalled(); + expect(mockOpLogStore.setVectorClock).not.toHaveBeenCalled(); expect(mockStore.dispatch).toHaveBeenCalled(); + expect(mockLockService.request).toHaveBeenCalledWith( + LOCK_NAMES.OPERATION_LOG, + jasmine.any(Function), + ); }); it('should not recover when no usable legacy data exists', async () => { @@ -91,11 +115,48 @@ describe('OperationLogRecoveryService', () => { expect(mockStore.dispatch).not.toHaveBeenCalled(); }); - it('should handle database access errors gracefully', async () => { + it('should refuse recovery when a SUP_OPS snapshot exists', async () => { + mockOpLogStore.loadStateCache.and.resolveTo({ state: {} } as any); + + await expectAsync(service.attemptRecovery()).toBeRejectedWithError( + /Refusing legacy recovery.*snapshot/i, + ); + + expect(mockLegacyPfDb.hasUsableEntityData).not.toHaveBeenCalled(); + expect(mockOpLogStore.append).not.toHaveBeenCalled(); + expect(mockOpLogStore.saveStateCache).not.toHaveBeenCalled(); + }); + + it('should refuse recovery when the SUP_OPS log is non-empty', async () => { + mockOpLogStore.getLastSeq.and.resolveTo(3); + + await expectAsync(service.attemptRecovery()).toBeRejectedWithError( + /Refusing legacy recovery.*operation log/i, + ); + + expect(mockLegacyPfDb.hasUsableEntityData).not.toHaveBeenCalled(); + expect(mockOpLogStore.append).not.toHaveBeenCalled(); + expect(mockOpLogStore.saveStateCache).not.toHaveBeenCalled(); + }); + + it('should propagate SUP_OPS inspection errors without attempting writes', async () => { + mockOpLogStore.loadStateCache.and.rejectWith(new Error('SUP_OPS unavailable')); + + await expectAsync(service.attemptRecovery()).toBeRejectedWithError( + 'SUP_OPS unavailable', + ); + + expect(mockLegacyPfDb.hasUsableEntityData).not.toHaveBeenCalled(); + expect(mockOpLogStore.append).not.toHaveBeenCalled(); + expect(mockOpLogStore.saveStateCache).not.toHaveBeenCalled(); + }); + + it('should propagate legacy database access errors', async () => { mockLegacyPfDb.hasUsableEntityData.and.rejectWith(new Error('Database error')); - // Should not throw - await expectAsync(service.attemptRecovery()).toBeResolved(); + await expectAsync(service.attemptRecovery()).toBeRejectedWithError( + 'Database error', + ); expect(mockOpLogStore.append).not.toHaveBeenCalled(); }); }); @@ -112,7 +173,7 @@ describe('OperationLogRecoveryService', () => { await service.recoverFromLegacyData(legacyData); - expect(mockOpLogStore.append).toHaveBeenCalledWith( + expect(mockOpLogStore.appendRecoveryOperationAndSnapshot).toHaveBeenCalledWith( jasmine.objectContaining({ actionType: ActionType.RECOVERY_DATA_IMPORT, opType: OpType.Batch, @@ -122,6 +183,7 @@ describe('OperationLogRecoveryService', () => { clientId: 'testClient', vectorClock: { testClient: 1 }, }), + legacyData, ); }); @@ -133,7 +195,7 @@ describe('OperationLogRecoveryService', () => { ); }); - it('should save state cache after recovery', async () => { + it('should pass recovered state to the atomic persistence boundary', async () => { const legacyData = { task: { ids: ['task1'] } }; mockClientIdService.loadClientId.and.resolveTo('testClient'); mockOpLogStore.append.and.resolveTo(undefined); @@ -142,16 +204,13 @@ describe('OperationLogRecoveryService', () => { await service.recoverFromLegacyData(legacyData); - expect(mockOpLogStore.saveStateCache).toHaveBeenCalledWith( - jasmine.objectContaining({ - state: legacyData, - lastAppliedOpSeq: 5, - vectorClock: { testClient: 1 }, - }), + expect(mockOpLogStore.appendRecoveryOperationAndSnapshot).toHaveBeenCalledWith( + jasmine.any(Object), + legacyData, ); }); - it('should persist vector clock to IndexedDB store after recovery', async () => { + it('should include the recovery clock in the atomically persisted operation', async () => { const legacyData = { task: { ids: ['task1'] } }; mockClientIdService.loadClientId.and.resolveTo('testClient'); mockOpLogStore.append.and.resolveTo(undefined); @@ -160,7 +219,10 @@ describe('OperationLogRecoveryService', () => { await service.recoverFromLegacyData(legacyData); - expect(mockOpLogStore.setVectorClock).toHaveBeenCalledWith({ testClient: 1 }); + expect(mockOpLogStore.appendRecoveryOperationAndSnapshot).toHaveBeenCalledWith( + jasmine.objectContaining({ vectorClock: { testClient: 1 } }), + legacyData, + ); }); it('should reject invalid legacy data before writing or dispatching it', async () => { @@ -191,28 +253,21 @@ describe('OperationLogRecoveryService', () => { expect(mockOpLogStore.markFailed).not.toHaveBeenCalled(); }); - it('should quarantine crash-interrupted ops for archive recovery', async () => { + it('should leave crash-interrupted ops pending until hydration replays their reducers', 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.markReducersCommittedAndMergeClocks.and.resolveTo(undefined); + const result = await service.recoverPendingRemoteOps(); - await service.recoverPendingRemoteOps(); - - expect(mockOpLogStore.markReducersCommittedAndMergeClocks).toHaveBeenCalledWith( - [1, 2], - pendingOps.map((entry) => entry.op), - ); + expect(result).toEqual(pendingOps); + expect(mockOpLogStore.markReducersCommittedAndMergeClocks).not.toHaveBeenCalled(); expect(mockOpLogStore.markApplied).not.toHaveBeenCalled(); }); - it('should quarantine regardless of age without charging retry budget', async () => { - // The former PENDING_OPERATION_EXPIRY_MS split changed nothing: every - // crash-interrupted op lands in the same quarantine, and retryCount is - // only ever bumped for an actually attempted archive failure. + it('should return every pending op regardless of age without changing status', async () => { const now = Date.now(); const weekMs = 7 * 24 * 60 * 60 * 1000; const pendingOps = [ @@ -225,14 +280,10 @@ describe('OperationLogRecoveryService', () => { }, ] as any; mockOpLogStore.getPendingRemoteOps.and.resolveTo(pendingOps); - mockOpLogStore.markReducersCommittedAndMergeClocks.and.resolveTo(undefined); + const result = await service.recoverPendingRemoteOps(); - await service.recoverPendingRemoteOps(); - - expect(mockOpLogStore.markReducersCommittedAndMergeClocks).toHaveBeenCalledWith( - [1, 2], - pendingOps.map((entry) => entry.op), - ); + expect(result).toEqual(pendingOps); + expect(mockOpLogStore.markReducersCommittedAndMergeClocks).not.toHaveBeenCalled(); expect(mockOpLogStore.markFailed).not.toHaveBeenCalled(); }); }); 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 f012b07983..13f96a003e 100644 --- a/src/app/op-log/persistence/operation-log-recovery.service.ts +++ b/src/app/op-log/persistence/operation-log-recovery.service.ts @@ -5,12 +5,19 @@ import { CURRENT_SCHEMA_VERSION } from './schema-migration.service'; import { LegacyPfDbService } from '../../core/persistence/legacy-pf-db.service'; import { ClientIdService } from '../../core/util/client-id.service'; import { loadAllData } from '../../root-store/meta/load-all-data.action'; -import { Operation, OpType, ActionType } from '../core/operation.types'; +import { + Operation, + OperationLogEntry, + OpType, + ActionType, +} from '../core/operation.types'; import { SINGLETON_ENTITY_ID } from '../core/entity-registry'; import { uuidv7 } from '../../util/uuid-v7'; import { OpLog } from '../../core/log'; import { AppDataComplete } from '../model/model-config'; import { ValidateStateService } from '../validation/validate-state.service'; +import { LockService } from '../sync/lock.service'; +import { LOCK_NAMES } from '../core/operation-log.const'; /** * Handles crash recovery and data restoration for the operation log system. @@ -30,45 +37,60 @@ export class OperationLogRecoveryService { private legacyPfDb = inject(LegacyPfDbService); private clientIdService = inject(ClientIdService); private validateStateService = inject(ValidateStateService); + private lockService = inject(LockService); /** * Attempts to recover from a corrupted or missing SUP_OPS database. * Recovery strategy: - * 1. Try to load data from legacy 'pf' database (IndexedDB) - * 2. If found, run genesis migration with that data - * 3. If no legacy data, log error (user will need to sync or restore from backup) + * 1. Prove SUP_OPS has neither a snapshot nor operations + * 2. Try to load data from legacy 'pf' database (IndexedDB) + * 3. If found, run genesis migration with that data + * 4. If no legacy data, log error (user will need to sync or restore from backup) + * + * Inspection and recovery errors intentionally propagate. Treating an + * unreadable SUP_OPS database as empty could overwrite newer data with a stale + * legacy copy and then advance the snapshot past the healthy operation log. */ async attemptRecovery(): Promise { OpLog.normal('OperationLogRecoveryService: Attempting disaster recovery...'); + await this.lockService.request(LOCK_NAMES.OPERATION_LOG, () => + this._attemptRecoveryWhileLocked(), + ); + } - try { - // 1. Try to load from legacy 'pf' database - const hasLegacyData = await this.legacyPfDb.hasUsableEntityData(); + private async _attemptRecoveryWhileLocked(): Promise { + const [stateCache, lastSeq] = await Promise.all([ + this.opLogStore.loadStateCache(), + this.opLogStore.getLastSeq(), + ]); - if (hasLegacyData) { - OpLog.normal( - 'OperationLogRecoveryService: Found data in legacy database. Recovering...', - ); - const legacyData = await this.legacyPfDb.loadAllEntityData(); - await this.recoverFromLegacyData( - legacyData as unknown as Record, - ); - return; - } - - // 2. No legacy data found - // App will start with NgRx initial state (empty). - // User can sync or import a backup to restore their data. - OpLog.warn( - 'OperationLogRecoveryService: No legacy data found. ' + - 'If you have sync enabled, please trigger a sync to restore your data. ' + - 'Otherwise, you may need to restore from a backup.', - ); - } catch (e) { - OpLog.err('OperationLogRecoveryService: Recovery failed', e); - // App will start with NgRx initial state (empty). - // User can sync or restore from backup. + if (stateCache !== null && stateCache !== undefined) { + throw new Error('Refusing legacy recovery because a SUP_OPS snapshot still exists'); } + if (lastSeq > 0) { + throw new Error( + 'Refusing legacy recovery because the SUP_OPS operation log is not empty', + ); + } + + const hasLegacyData = await this.legacyPfDb.hasUsableEntityData(); + + if (hasLegacyData) { + OpLog.normal( + 'OperationLogRecoveryService: Found data in legacy database. Recovering...', + ); + const legacyData = await this.legacyPfDb.loadAllEntityData(); + await this.recoverFromLegacyData(legacyData as unknown as Record); + return; + } + + // No legacy data found. App will start with NgRx initial state (empty). + // User can sync or import a backup to restore their data. + OpLog.warn( + 'OperationLogRecoveryService: No legacy data found. ' + + 'If you have sync enabled, please trigger a sync to restore your data. ' + + 'Otherwise, you may need to restore from a backup.', + ); } /** @@ -109,21 +131,7 @@ export class OperationLogRecoveryService { schemaVersion: CURRENT_SCHEMA_VERSION, }; - // Write recovery operation - await this.opLogStore.append(recoveryOp); - - // Create state cache - const lastSeq = await this.opLogStore.getLastSeq(); - await this.opLogStore.saveStateCache({ - state: legacyData, - lastAppliedOpSeq: lastSeq, - vectorClock: recoveryOp.vectorClock, - compactedAt: Date.now(), - }); - - // Persist vector clock to IndexedDB store for immediate availability - // Without this, getVectorClock() returns stale clock until cache is populated - await this.opLogStore.setVectorClock(recoveryOp.vectorClock); + await this.opLogStore.appendRecoveryOperationAndSnapshot(recoveryOp, legacyData); // Dispatch to NgRx this.store.dispatch(loadAllData({ appDataComplete: legacyData as AppDataComplete })); @@ -135,12 +143,11 @@ export class OperationLogRecoveryService { /** * Recovers from pending remote ops that were stored but not applied (crash recovery). - * 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; sync stays blocked until that recovery succeeds. + * The crash point is unknowable, so this method only returns the quarantine. + * Hydration replays the rows, persists their reducer outcome, and only then + * retries archive work; sync stays blocked until that recovery succeeds. */ - async recoverPendingRemoteOps(): Promise { + async recoverPendingRemoteOps(): Promise { const recoveredLegacyFailures = await this.opLogStore.recoverLegacyTerminalRemoteFailures(); if (recoveredLegacyFailures > 0) { @@ -151,21 +158,18 @@ export class OperationLogRecoveryService { const pendingOps = await this.opLogStore.getPendingRemoteOps(); if (pendingOps.length === 0) { - return; + return []; } - // Reducers are replayed status-blind during hydration; archive work is - // retried after. Age is irrelevant — every crash-interrupted op lands in - // the same quarantine, and retryCount stays untouched (no attempt was made). - const seqs = pendingOps.map((e) => e.seq); - await this.opLogStore.markReducersCommittedAndMergeClocks( - seqs, - pendingOps.map((entry) => entry.op), - ); + // Do not checkpoint these rows yet. A crash can occur before reducer + // dispatch, during a partially successful bulk dispatch, or immediately + // after it. Hydration must replay the reducers and durably partition their + // successes/failures before any archive-only retry can start. OpLog.warn( `OperationLogRecoveryService: Found ${pendingOps.length} pending remote ops from previous crash. ` + - 'Quarantined their archive work (reducers will replay during hydration).', + 'Reducers will be replayed before archive recovery.', ); + return pendingOps; } /** 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 6a7b83ed47..6c24286d95 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 @@ -307,6 +307,100 @@ describe('OperationLogStoreService', () => { expect(ops[0].source).toBe('remote'); expect(ops[0].syncedAt).toBeDefined(); }); + + it('should atomically append a legacy recovery with its snapshot and clock', async () => { + const op = createTestOperation({ + id: 'legacy-recovery-op', + vectorClock: { testClient: 7 }, + }); + const state = { task: { ids: ['task-1'] } }; + + const seq = await service.appendRecoveryOperationAndSnapshot(op, state); + + expect((await service.getOpById(op.id))?.seq).toBe(seq); + expect(await service.loadStateCache()).toEqual( + jasmine.objectContaining({ + state, + lastAppliedOpSeq: seq, + vectorClock: op.vectorClock, + schemaVersion: op.schemaVersion, + }), + ); + expect(await service.getVectorClock()).toEqual(op.vectorClock); + }); + + it('should roll back the recovery operation when its snapshot write fails', async () => { + const op = createTestOperation({ id: 'failed-legacy-recovery-op' }); + const adapter = ( + service as unknown as { + _adapter: OpLogDbAdapter; + } + )._adapter; + const originalTransaction = adapter.transaction.bind(adapter); + spyOn(adapter, 'transaction').and.callFake(async (stores, mode, callback) => + originalTransaction(stores, mode, async (tx) => { + const failingTx = new Proxy(tx, { + get: (target, property): unknown => { + if (property === 'put') { + return async (store: string, value: unknown, key?: string | number) => { + if (store === STORE_NAMES.STATE_CACHE) { + throw new Error('injected recovery snapshot failure'); + } + return target.put(store, value, key); + }; + } + const value = Reflect.get(target, property); + return typeof value === 'function' ? value.bind(target) : value; + }, + }); + return callback(failingTx); + }), + ); + + await expectAsync( + service.appendRecoveryOperationAndSnapshot(op, { task: {} }), + ).toBeRejectedWithError('injected recovery snapshot failure'); + + expect(await service.getOpsAfterSeq(0)).toEqual([]); + expect(await service.loadStateCache()).toBeNull(); + }); + + it('should roll back the recovery operation and snapshot when its clock write fails', async () => { + const op = createTestOperation({ id: 'failed-recovery-clock-op' }); + const adapter = ( + service as unknown as { + _adapter: OpLogDbAdapter; + } + )._adapter; + const originalTransaction = adapter.transaction.bind(adapter); + spyOn(adapter, 'transaction').and.callFake(async (stores, mode, callback) => + originalTransaction(stores, mode, async (tx) => { + const failingTx = new Proxy(tx, { + get: (target, property): unknown => { + if (property === 'put') { + return async (store: string, value: unknown, key?: string | number) => { + if (store === STORE_NAMES.VECTOR_CLOCK) { + throw new Error('injected recovery clock failure'); + } + return target.put(store, value, key); + }; + } + const value = Reflect.get(target, property); + return typeof value === 'function' ? value.bind(target) : value; + }, + }); + return callback(failingTx); + }), + ); + + await expectAsync( + service.appendRecoveryOperationAndSnapshot(op, { task: {} }), + ).toBeRejectedWithError('injected recovery clock failure'); + + expect(await service.getOpsAfterSeq(0)).toEqual([]); + expect(await service.loadStateCache()).toBeNull(); + expect(await service.getVectorClock()).toBeNull(); + }); }); describe('hasOp', () => { @@ -1601,9 +1695,72 @@ describe('OperationLogStoreService', () => { }); }); + it('should atomically reject reducer failures while checkpointing successful ops', async () => { + const successfulOp = createTestOperation({ id: 'successful-op' }); + const failedOp = createTestOperation({ + id: 'reducer-failed-op', + opType: OpType.SyncImport, + entityType: 'ALL' as EntityType, + entityId: undefined, + }); + const successfulSeq = await service.append(successfulOp, 'remote', { + pendingApply: true, + }); + await service.append(failedOp, 'remote', { pendingApply: true }); + expect((await service.getLatestFullStateOpEntry())?.op.id).toBe(failedOp.id); + + await service.markReducersCommittedAndMergeClocks( + [successfulSeq], + [successfulOp], + [failedOp.id], + ); + + const successfulEntry = await service.getOpById(successfulOp.id); + const failedEntry = await service.getOpById(failedOp.id); + expect(successfulEntry?.applicationStatus).toBe('archive_pending'); + expect(failedEntry?.applicationStatus).toBe('pending'); + expect(failedEntry?.rejectedAt).toBeDefined(); + expect(failedEntry?.reducerRejectedAt).toBeDefined(); + expect(await service.getPendingRemoteOps()).toEqual([]); + expect(await service.getLatestFullStateOpEntry()).toBeUndefined(); + }); + + it('should durably reject a local synthetic operation whose reducer fails', async () => { + const syntheticOp = createTestOperation({ id: 'synthetic-local-op' }); + await service.append(syntheticOp, 'local'); + + await service.markReducersCommittedAndMergeClocks([], [], [syntheticOp.id]); + + const storedEntry = await service.getOpById(syntheticOp.id); + expect(storedEntry?.rejectedAt).toBeDefined(); + expect(storedEntry?.reducerRejectedAt).toBeDefined(); + expect(await service.getUnsynced()).toEqual([]); + }); + + it('should not resurrect a reducer-rejected full-state op when metadata rebuilds', async () => { + const failedOp = createTestOperation({ + id: 'reducer-failed-full-state-op', + opType: OpType.SyncImport, + entityType: 'ALL' as EntityType, + entityId: undefined, + }); + await service.append(failedOp, 'remote', { pendingApply: true }); + await service.markReducersCommittedAndMergeClocks([], [], [failedOp.id]); + const db = ( + service as unknown as { + db: IDBPDatabase; + } + ).db; + await db.delete(STORE_NAMES.META, FULL_STATE_OPS_META_KEY); + + expect(await service.getLatestFullStateOpEntry()).toBeUndefined(); + }); + it('should roll back reducer checkpoint and clock when the atomic clock write fails', async () => { const op = createTestOperation(); + const reducerFailedOp = createTestOperation({ id: 'reducer-failed-op' }); const seq = await service.append(op, 'remote', { pendingApply: true }); + await service.append(reducerFailedOp, 'remote', { pendingApply: true }); await service.setVectorClock({ testClient: 2 }); const adapter = ( @@ -1636,11 +1793,15 @@ describe('OperationLogStoreService', () => { service.markReducersCommittedAndMergeClocks( [seq], [{ ...op, vectorClock: { remoteClient: 4 } }], + [reducerFailedOp.id], ), ).toBeRejectedWithError('injected vector-clock write failure'); const [stored] = await service.getOpsAfterSeq(0); expect(stored.applicationStatus).toBe('pending'); + const failedEntry = await service.getOpById(reducerFailedOp.id); + expect(failedEntry?.applicationStatus).toBe('pending'); + expect(failedEntry?.rejectedAt).toBeUndefined(); service.clearVectorClockCache(); expect(await service.getVectorClock()).toEqual({ testClient: 2 }); }); 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 1f57a6d278..6afe50f8d8 100644 --- a/src/app/op-log/persistence/operation-log-store.service.ts +++ b/src/app/op-log/persistence/operation-log-store.service.ts @@ -139,6 +139,7 @@ interface StoredOperationLogEntry { source: 'local' | 'remote'; syncedAt?: number; rejectedAt?: number; + reducerRejectedAt?: number; applicationStatus?: 'pending' | 'archive_pending' | 'applied' | 'failed'; retryCount?: number; } @@ -156,6 +157,7 @@ const decodeStoredEntry = (stored: StoredOperationLogEntry): OperationLogEntry = source: stored.source, syncedAt: stored.syncedAt, rejectedAt: stored.rejectedAt, + reducerRejectedAt: stored.reducerRejectedAt, applicationStatus: stored.applicationStatus, retryCount: stored.retryCount, }; @@ -586,7 +588,10 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort { const refs: FullStateOpRef[] = []; await tx.iterate(STORE_NAMES.OPS, {}, (value, key) => { - const ref = this._getFullStateRef(value.op, key as number); + const ref = + value.rejectedAt === undefined + ? this._getFullStateRef(value.op, key as number) + : undefined; if (ref) { refs.push(ref); } @@ -631,7 +636,10 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort { - const ref = this._getFullStateRef(value.op, key as number); + const ref = + value.rejectedAt === undefined + ? this._getFullStateRef(value.op, key as number) + : undefined; if (ref) { refs.push(ref); } @@ -680,6 +688,54 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort { + await this._ensureInit(); + const now = Date.now(); + try { + const seq = await this._adapter.transaction( + [STORE_NAMES.OPS, STORE_NAMES.STATE_CACHE, STORE_NAMES.VECTOR_CLOCK], + 'readwrite', + async (tx) => { + const writtenSeq = await tx.add( + STORE_NAMES.OPS, + this._buildStoredEntry(op, 'local'), + ); + await tx.put(STORE_NAMES.STATE_CACHE, { + id: SINGLETON_KEY, + state, + lastAppliedOpSeq: writtenSeq, + vectorClock: op.vectorClock, + compactedAt: now, + schemaVersion: op.schemaVersion, + } satisfies StateCacheEntry); + await tx.put( + STORE_NAMES.VECTOR_CLOCK, + { + clock: op.vectorClock, + lastUpdate: now, + } satisfies VectorClockEntry, + SINGLETON_KEY, + ); + return writtenSeq; + }, + ); + this._vectorClockCache = { ...op.vectorClock }; + this._invalidateUnsyncedCache(); + return seq; + } catch (e) { + this._handleAppendError(e); + } + } + async appendBatch( ops: Operation[], source: 'local' | 'remote' = 'local', @@ -891,22 +947,29 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort { if (seqs.length !== ops.length) { throw new Error( 'markReducersCommittedAndMergeClocks requires one sequence per operation.', ); } - if (ops.length === 0) { + if (ops.length === 0 && rejectedOpIds.length === 0) { return; } + const committedOpIds = new Set(ops.map((op) => op.id)); + if (rejectedOpIds.some((opId) => committedOpIds.has(opId))) { + throw new Error('Reducer checkpoint cannot commit and reject the same operation.'); + } await this._ensureInit(); const currentClientId = await this.clientIdProvider.loadClientId(); let committedClock: VectorClock | undefined; + const rejectedAt = Date.now(); + const rejectedFullStateOpIds = new Set(); await this._adapter.transaction( - [STORE_NAMES.OPS, STORE_NAMES.VECTOR_CLOCK], + [STORE_NAMES.OPS, STORE_NAMES.VECTOR_CLOCK, STORE_NAMES.META], 'readwrite', async (tx) => { const currentEntry = await tx.get( @@ -930,6 +993,32 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort( + STORE_NAMES.OPS, + OPS_INDEXES.BY_ID, + opId, + ); + if (!entry) { + throw new Error(`Reducer rejection requires persisted operation ${opId}.`); + } + entry.rejectedAt = rejectedAt; + entry.reducerRejectedAt = rejectedAt; + if (isFullStateOpType(getStoredOpType(entry.op))) { + rejectedFullStateOpIds.add(opId); + } + await tx.put(STORE_NAMES.OPS, entry); + } + + if (rejectedFullStateOpIds.size > 0) { + const meta = await this._getFullStateOpsMetaInTxOrRebuild(tx); + await tx.put( + STORE_NAMES.META, + this._withoutFullStateRefs(meta, rejectedFullStateOpIds), + FULL_STATE_OPS_META_KEY, + ); + } + await tx.put( STORE_NAMES.VECTOR_CLOCK, { clock: committedClock, lastUpdate: Date.now() } satisfies VectorClockEntry, @@ -939,6 +1028,9 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort 0) { + this._invalidateUnsyncedCache(); + } } /** 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 9a0e015d4d..2c19038835 100644 --- a/src/app/op-log/sync/conflict-resolution.service.spec.ts +++ b/src/app/op-log/sync/conflict-resolution.service.spec.ts @@ -3,6 +3,7 @@ import { SyncConflictBannerService } from './sync-conflict-banner.service'; import { ConflictResolutionService } from './conflict-resolution.service'; import { Store } from '@ngrx/store'; import { OperationApplierService } from '../apply/operation-applier.service'; +import { HydrationStateService } from '../apply/hydration-state.service'; import { OperationLogStoreService } from '../persistence/operation-log-store.service'; import { SnackService } from '../../core/snack/snack.service'; import { BannerService } from '../../core/banner/banner.service'; @@ -28,6 +29,7 @@ describe('ConflictResolutionService', () => { let mockOpLogStore: jasmine.SpyObj; let mockSnackService: jasmine.SpyObj; let mockValidateStateService: jasmine.SpyObj; + let mockHydrationStateService: jasmine.SpyObj; let mockClientIdProvider: { loadClientId: jasmine.Spy }; let mockEntityRegistry: ReturnType; let mockOperationLogEffects: jasmine.SpyObj; @@ -91,6 +93,7 @@ describe('ConflictResolutionService', () => { 'markRejected', 'markFailed', 'getUnsyncedByEntity', + 'getOpById', ]); mockOpLogStore.mergeRemoteOpClocks.and.resolveTo(undefined); mockOpLogStore.markReducersCommittedAndMergeClocks.and.resolveTo(undefined); @@ -102,6 +105,11 @@ describe('ConflictResolutionService', () => { mockValidateStateService = jasmine.createSpyObj('ValidateStateService', [ 'validateAndRepairCurrentState', ]); + mockHydrationStateService = jasmine.createSpyObj('HydrationStateService', [ + 'startApplyingRemoteOps', + 'startPostSyncCooldown', + 'endApplyingRemoteOps', + ]); mockOperationLogEffects = jasmine.createSpyObj('OperationLogEffects', [ 'processDeferredActions', ]); @@ -120,6 +128,7 @@ describe('ConflictResolutionService', () => { { provide: OperationLogStoreService, useValue: mockOpLogStore }, { provide: SnackService, useValue: mockSnackService }, { provide: ValidateStateService, useValue: mockValidateStateService }, + { provide: HydrationStateService, useValue: mockHydrationStateService }, { provide: OperationLogEffects, useValue: mockOperationLogEffects }, { provide: CLIENT_ID_PROVIDER, useValue: mockClientIdProvider }, { provide: ENTITY_REGISTRY, useValue: mockEntityRegistry }, @@ -132,6 +141,7 @@ describe('ConflictResolutionService', () => { mockOperationLogEffects.processDeferredActions.and.resolveTo(); mockValidateStateService.validateAndRepairCurrentState.and.resolveTo(true); mockOpLogStore.getUnsyncedByEntity.and.resolveTo(new Map()); + mockOpLogStore.getOpById.and.resolveTo(undefined); // By default, appendBatchSkipDuplicates writes all ops (no duplicates) mockOpLogStore.appendBatchSkipDuplicates.and.callFake((ops: Operation[]) => Promise.resolve({ @@ -1749,7 +1759,7 @@ describe('ConflictResolutionService', () => { expect(result).toEqual({ localWinOpsCreated: 1 }); }); - it('should persist local-win compensation before applying mixed remote winners', async () => { + it('should keep originals recoverable when applying a mixed resolution fails', async () => { const now = Date.now(); const remoteWinner = createOpWithTimestamp('remote-winner', 'client-b', now); const remoteLoser = createOpWithTimestamp( @@ -1809,12 +1819,7 @@ describe('ConflictResolutionService', () => { service.autoResolveConflictsLWW(conflicts), ).toBeRejectedWithError('remote archive apply failed'); - expect(callOrder).toEqual([ - 'persist-mixed-resolution', - 'mark-rejected', - 'mark-rejected', - 'apply-remote', - ]); + expect(callOrder).toEqual(['persist-mixed-resolution', 'apply-remote']); }); it('should not reject or apply remote rows when local-win compensation cannot persist', async () => { @@ -2015,6 +2020,276 @@ describe('ConflictResolutionService', () => { ); }); + it('should keep original operations recoverable when a remote winner reducer fails', async () => { + const now = Date.now(); + const remoteOp = createOpWithTimestamp('remote-1', 'client-b', now); + const reducerError = new Error('Reducer failed'); + const conflicts: EntityConflict[] = [ + createConflict( + 'task-1', + [createOpWithTimestamp('local-1', 'client-a', now - 1000)], + [remoteOp], + ), + ]; + + mockOpLogStore.hasOp.and.resolveTo(false); + mockOpLogStore.append.and.resolveTo(1); + mockOperationApplier.applyOperations.and.callFake(async (_ops, options) => { + const reducerFailures = [{ op: remoteOp, error: reducerError }]; + await options?.onReducersCommitted?.([], reducerFailures); + return { appliedOps: [], reducerFailures }; + }); + + await expectAsync( + service.autoResolveConflictsLWW(conflicts), + ).toBeRejectedWithError(IncompleteRemoteOperationsError); + + expect(mockOpLogStore.markReducersCommittedAndMergeClocks).not.toHaveBeenCalled(); + expect(mockOpLogStore.markRejected).not.toHaveBeenCalled(); + expect(mockOpLogStore.markFailed).not.toHaveBeenCalled(); + }); + + it('should retry an existing pending remote winner after its reducer recovers', async () => { + const now = Date.now(); + const localOp = createOpWithTimestamp('local-1', 'client-a', now - 1000); + const remoteOp = createOpWithTimestamp('remote-1', 'client-b', now); + const redeliveredRemoteOp = { + ...remoteOp, + payload: { changedAfterPersistence: true }, + vectorClock: { clientB: 99 }, + }; + const reducerError = new Error('Reducer failed'); + const conflicts: EntityConflict[] = [ + createConflict('task-1', [localOp], [remoteOp]), + ]; + const retryConflicts: EntityConflict[] = [ + createConflict('task-1', [localOp], [redeliveredRemoteOp]), + ]; + let appendAttempt = 0; + mockOpLogStore.appendBatchSkipDuplicates.and.callFake(async () => { + appendAttempt++; + return appendAttempt === 1 + ? { seqs: [7], writtenOps: [remoteOp], skippedCount: 0 } + : { seqs: [], writtenOps: [], skippedCount: 1 }; + }); + mockOpLogStore.getOpById.and.resolveTo({ + seq: 7, + op: remoteOp, + appliedAt: now, + source: 'remote', + applicationStatus: 'pending', + }); + let applyAttempt = 0; + mockOperationApplier.applyOperations.and.callFake(async (ops, options) => { + applyAttempt++; + if (applyAttempt === 1) { + const reducerFailures = [{ op: remoteOp, error: reducerError }]; + await options?.onReducersCommitted?.([], reducerFailures); + return { appliedOps: [], reducerFailures }; + } + await options?.onReducersCommitted?.(ops); + return { appliedOps: ops }; + }); + + await expectAsync( + service.autoResolveConflictsLWW(conflicts), + ).toBeRejectedWithError(IncompleteRemoteOperationsError); + const retryResult = await service.autoResolveConflictsLWW(retryConflicts); + + expect(retryResult).toEqual({ localWinOpsCreated: 0 }); + expect(mockOperationApplier.applyOperations).toHaveBeenCalledTimes(2); + expect(mockOperationApplier.applyOperations.calls.argsFor(1)[0]).toEqual([ + remoteOp, + ]); + expect(mockOpLogStore.markApplied).toHaveBeenCalledWith([7]); + expect(mockOpLogStore.markRejected).toHaveBeenCalledWith([localOp.id]); + }); + + it('should fall back to LWW when a synthetic merge reducer fails', async () => { + const now = Date.now(); + const localOp = createOpWithTimestamp('local-1', 'client-a', now - 1000); + const remoteOp = createOpWithTimestamp('remote-1', 'client-b', now); + const mergedOp = createOpWithTimestamp('merged-1', TEST_CLIENT_ID, now + 1); + const conflict = createConflict('task-1', [localOp], [remoteOp]); + const serviceInternals = service as unknown as { + _journalMergedResolution: () => Promise; + _journalResolution: () => Promise; + _resolveConflictsWithLWW: ( + conflicts: EntityConflict[], + disableDisjointMerge?: boolean, + ) => Promise; + }; + spyOn(serviceInternals, '_journalMergedResolution').and.resolveTo(); + spyOn(serviceInternals, '_journalResolution').and.resolveTo(); + spyOn(serviceInternals, '_resolveConflictsWithLWW').and.callFake( + async (_conflicts, disableDisjointMerge = false) => + disableDisjointMerge + ? { + lwwResolutions: [{ conflict, winner: 'remote' }], + mergedResolutions: [], + lwwPlans: [{ conflict }], + } + : { + lwwResolutions: [], + mergedResolutions: [ + { + conflict, + mergedOp, + plan: { conflict }, + }, + ], + lwwPlans: [], + }, + ); + mockOpLogStore.appendBatchSkipDuplicates.and.resolveTo({ + seqs: [], + writtenOps: [], + skippedCount: 1, + }); + mockOpLogStore.getOpById.and.resolveTo({ + seq: 7, + op: remoteOp, + appliedAt: now, + source: 'remote', + applicationStatus: 'pending', + }); + let applyAttempt = 0; + mockOperationApplier.applyOperations.and.callFake(async (ops, options) => { + applyAttempt++; + if (applyAttempt === 1) { + const reducerFailures = [ + { op: mergedOp, error: new Error('synthetic reducer failed') }, + ]; + await options?.onReducersCommitted?.([], reducerFailures); + return { appliedOps: [], reducerFailures }; + } + await options?.onReducersCommitted?.(ops); + return { appliedOps: ops }; + }); + + const result = await service.autoResolveConflictsLWW([conflict]); + + expect(result).toEqual({ localWinOpsCreated: 0 }); + const mergeRemoteBatch = + mockOpLogStore.appendMixedSourceBatchSkipDuplicates.calls.argsFor(0)[0][0]; + expect(mergeRemoteBatch).toEqual( + jasmine.objectContaining({ + source: 'remote', + options: { pendingApply: true }, + }), + ); + expect(mockOpLogStore.markReducersCommittedAndMergeClocks).toHaveBeenCalledWith( + [], + [], + [mergedOp.id], + ); + expect(mockOpLogStore.markReducersCommittedAndMergeClocks).toHaveBeenCalledWith( + [7], + [remoteOp], + ); + expect(mockOpLogStore.markRejected).toHaveBeenCalledWith([localOp.id]); + expect(serviceInternals._journalMergedResolution).not.toHaveBeenCalled(); + expect(serviceInternals._journalResolution).toHaveBeenCalled(); + }); + + it('should keep deferred user actions outside a failed merge fallback rejection', async () => { + const now = Date.now(); + const localOp = createOpWithTimestamp('local-1', 'client-a', now - 1000); + const remoteOp = createOpWithTimestamp('remote-1', 'client-b', now); + const mergedOp = createOpWithTimestamp('merged-1', TEST_CLIENT_ID, now + 1); + const deferredOp = createOpWithTimestamp( + 'deferred-user-op', + TEST_CLIENT_ID, + now + 2, + ); + const conflict = createConflict('task-1', [localOp], [remoteOp]); + const serviceInternals = service as unknown as { + _journalMergedResolution: () => Promise; + _journalResolution: () => Promise; + _resolveConflictsWithLWW: ( + conflicts: EntityConflict[], + disableDisjointMerge?: boolean, + ) => Promise; + }; + spyOn(serviceInternals, '_journalMergedResolution').and.resolveTo(); + spyOn(serviceInternals, '_journalResolution').and.resolveTo(); + spyOn(serviceInternals, '_resolveConflictsWithLWW').and.callFake( + async (_conflicts, disableDisjointMerge = false) => + disableDisjointMerge + ? { + lwwResolutions: [{ conflict, winner: 'remote' }], + mergedResolutions: [], + lwwPlans: [{ conflict }], + } + : { + lwwResolutions: [], + mergedResolutions: [{ conflict, mergedOp, plan: { conflict } }], + lwwPlans: [], + }, + ); + mockOpLogStore.appendBatchSkipDuplicates.and.resolveTo({ + seqs: [], + writtenOps: [], + skippedCount: 1, + }); + mockOpLogStore.getOpById.and.resolveTo({ + seq: 7, + op: remoteOp, + appliedAt: now, + source: 'remote', + applicationStatus: 'pending', + }); + const callOrder: string[] = []; + let deferredWasPersisted = false; + let applyAttempt = 0; + mockHydrationStateService.startApplyingRemoteOps.and.callFake(() => { + callOrder.push('window-start'); + }); + mockHydrationStateService.startPostSyncCooldown.and.callFake(() => { + callOrder.push('cooldown-start'); + }); + mockHydrationStateService.endApplyingRemoteOps.and.callFake(() => { + callOrder.push('window-end'); + }); + mockOpLogStore.getUnsyncedByEntity.and.callFake(async () => + deferredWasPersisted ? new Map([['TASK:task-1', [deferredOp]]]) : new Map(), + ); + mockOperationApplier.applyOperations.and.callFake(async (ops, options) => { + applyAttempt++; + callOrder.push(`apply-${applyAttempt}`); + if (applyAttempt === 1) { + const reducerFailures = [ + { op: mergedOp, error: new Error('synthetic reducer failed') }, + ]; + await options?.onReducersCommitted?.([], reducerFailures); + return { appliedOps: [], reducerFailures }; + } + await options?.onReducersCommitted?.(ops); + return { appliedOps: ops }; + }); + mockOperationLogEffects.processDeferredActions.and.callFake(async () => { + callOrder.push('process-deferred'); + deferredWasPersisted = true; + }); + + await service.autoResolveConflictsLWW([conflict]); + + const rejectedIds = mockOpLogStore.markRejected.calls + .allArgs() + .flatMap(([ids]) => ids); + expect(rejectedIds).not.toContain(deferredOp.id); + expect(callOrder).toEqual([ + 'window-start', + 'apply-1', + 'apply-2', + 'cooldown-start', + 'window-end', + 'process-deferred', + ]); + expect(mockHydrationStateService.startApplyingRemoteOps).toHaveBeenCalledTimes(1); + expect(mockHydrationStateService.endApplyingRemoteOps).toHaveBeenCalledTimes(1); + }); + it('should process deferred actions after merging remote clocks when caller holds lock', async () => { const now = Date.now(); const remoteOp = createOpWithTimestamp('remote-1', 'client-b', now); diff --git a/src/app/op-log/sync/conflict-resolution.service.ts b/src/app/op-log/sync/conflict-resolution.service.ts index 3381d92fc3..af1ef45ebb 100644 --- a/src/app/op-log/sync/conflict-resolution.service.ts +++ b/src/app/op-log/sync/conflict-resolution.service.ts @@ -36,6 +36,7 @@ import { } from '../core/operation.types'; import { toLwwUpdateActionType } from '../core/lww-update-action-types'; import { OperationApplierService } from '../apply/operation-applier.service'; +import { HydrationStateService } from '../apply/hydration-state.service'; import { OperationLogStoreService } from '../persistence/operation-log-store.service'; import { OpLog } from '../../core/log'; import { toEntityKey } from '../util/entity-key.util'; @@ -86,7 +87,7 @@ type LWWResolution = LwwResolvedConflict; interface MergedResolution { conflict: EntityConflict; mergedOp: Operation; - /** Kept so STEP 3b can journal the merge AFTER the merged op is durably appended. */ + /** Kept so the merge is journaled only after its reducer work succeeds. */ plan: LwwConflictResolutionPlan; } @@ -94,10 +95,13 @@ interface MergedResolution { interface ResolvedConflicts { lwwResolutions: LWWResolution[]; mergedResolutions: MergedResolution[]; + lwwPlans: LwwConflictResolutionPlan[]; } interface AutoResolveConflictsLwwOptions { callerHoldsOperationLogLock?: boolean; + disableDisjointMerge?: boolean; + remoteApplyLifecycleOwnedByCaller?: boolean; } /** @@ -130,6 +134,7 @@ interface AutoResolveConflictsLwwOptions { export class ConflictResolutionService { private store = inject(Store); private operationApplier = inject(OperationApplierService); + private hydrationState = inject(HydrationStateService); private opLogStore = inject(OperationLogStoreService); private snackService = inject(SnackService); private bannerService = inject(BannerService); @@ -338,13 +343,20 @@ export class ConflictResolutionService { // ───────────────────────────────────────────────────────────────────────── // STEP 1: Resolve each conflict using LWW // ───────────────────────────────────────────────────────────────────────── - const { lwwResolutions: resolutions, mergedResolutions } = - await this._resolveConflictsWithLWW(conflicts); + const { + lwwResolutions: resolutions, + mergedResolutions, + lwwPlans, + } = await this._resolveConflictsWithLWW( + conflicts, + options.disableDisjointMerge ?? false, + ); const allOpsToApply: Operation[] = []; const allStoredOps: Array<{ id: string; seq: number }> = []; // Synthetic local ops (disjoint merges) ride in the apply batch but are NOT - // pending remote rows — the reducer-commit checkpoint must never see them. + // pending remote rows. Successful ones are excluded from the remote reducer + // checkpoint; failed ones are quarantined before falling back to plain LWW. const checkpointExemptOpIds = new Set(); const lwwPartitions = partitionLwwResolutions( @@ -368,6 +380,7 @@ export class ConflictResolutionService { const localOpsToReject = [...lwwPartitions.localOpsToReject]; const localOpsToRejectSet = new Set(localOpsToReject); let writtenLocalWinOps: Operation[] = []; + const writtenMergedOpIds = new Set(); for (const resolution of resolutions) { // Note: localWinOp is undefined for archive-wins sibling conflicts @@ -495,6 +508,7 @@ export class ConflictResolutionService { { ops: mergedResolutions.flatMap((merged) => merged.conflict.remoteOps), source: 'remote', + options: { pendingApply: true }, }, { ops: mergedResolutions.map((merged) => merged.mergedOp), @@ -507,7 +521,6 @@ export class ConflictResolutionService { ); } - const writtenMergedOpIds = new Set(); for (const entry of mergeBatch.written) { if (entry.source !== 'local') { continue; @@ -522,39 +535,10 @@ export class ConflictResolutionService { `${entry.op.entityType}:${entry.op.entityId}`, ); } - - // Journal ONLY after the append: once persisted as a pending local op the - // merge is durable (it applies/uploads even across a crash), so a `merged` - // ("kept both") entry can never describe a merge that didn't happen. The - // inverse window is accepted: batch committed but crash before this loop - // means a durable merge without a journal entry (observe-only log; the - // remote originals are recorded-as-seen, so it never re-enters - // resolution and the entry stays absent). - for (const merged of mergedResolutions) { - if (writtenMergedOpIds.has(merged.mergedOp.id)) { - await this._journalMergedResolution(merged.plan); - } - } } // ───────────────────────────────────────────────────────────────────────── - // STEP 4: Mark rejected operations BEFORE applying (crash safety) - // ───────────────────────────────────────────────────────────────────────── - if (localOpsToReject.length > 0) { - await this.opLogStore.markRejected(localOpsToReject); - OpLog.normal( - `ConflictResolutionService: Marked ${localOpsToReject.length} local ops as rejected`, - ); - } - if (remoteOpsToReject.length > 0) { - await this.opLogStore.markRejected(remoteOpsToReject); - OpLog.normal( - `ConflictResolutionService: Marked ${remoteOpsToReject.length} remote ops as rejected`, - ); - } - - // ───────────────────────────────────────────────────────────────────────── - // STEP 5: Apply remote ops in a single batch. + // STEP 4: Apply remote ops in a single batch. // Merge their clocks before entering the reducer/deferred-action window. // Pending rows make this durable frontier crash-safe, and any subsequent // dispatch/checkpoint/bookkeeping failure can drain buffered local actions. @@ -562,6 +546,12 @@ export class ConflictResolutionService { // ───────────────────────────────────────────────────────────────────────── let canDrainDeferredActions = false; let hasPrimaryError = false; + let failedMergedResolutions: MergedResolution[] = []; + let fallbackLocalWinOpsCreated = 0; + let remoteApplyWindowStarted = false; + const ownsRemoteApplyLifecycle = !( + options.remoteApplyLifecycleOwnedByCaller ?? false + ); try { if (allOpsToApply.length > 0) { OpLog.normal( @@ -569,15 +559,20 @@ export class ConflictResolutionService { ); await this.opLogStore.mergeRemoteOpClocks(allOpsToApply); canDrainDeferredActions = true; + if (ownsRemoteApplyLifecycle) { + this.hydrationState.startApplyingRemoteOps(); + remoteApplyWindowStarted = true; + } const opIdToSeq = new Map(allStoredOps.map((o) => [o.id, o.seq])); const applyResult = await this.operationApplier.applyOperations(allOpsToApply, { skipDeferredLocalActions: true, - onReducersCommitted: async (reducerCommittedOps) => { - // Disjoint-merge ops are synthetic LOCAL rows in the apply batch; - // their durability contract is the mixed-source append + upload - // path. The checkpoint's pending-only assertion must only see rows - // appended with pendingApply. + remoteApplyWindowAlreadyOpen: true, + onReducersCommitted: async (reducerCommittedOps, reducerFailures = []) => { + // Disjoint-merge ops are synthetic LOCAL rows in the apply batch. + // Exclude successful ones from the checkpoint's pending-only seq + // assertion. Failed synthetic rows are quarantined; their remote + // originals stay pending for the LWW fallback below. const checkpointOps = reducerCommittedOps.filter( (op) => !checkpointExemptOpIds.has(op.id), ); @@ -589,13 +584,30 @@ export class ConflictResolutionService { 'ConflictResolutionService: reducer commit contained an unknown operation.', ); } - await this.opLogStore.markReducersCommittedAndMergeClocks( - reducerCommittedSeqs, - checkpointOps, - ); + const failedSyntheticOpIds = reducerFailures + .filter((failure) => checkpointExemptOpIds.has(failure.op.id)) + .map((failure) => failure.op.id); + if (failedSyntheticOpIds.length > 0) { + await this.opLogStore.markReducersCommittedAndMergeClocks( + reducerCommittedSeqs, + checkpointOps, + failedSyntheticOpIds, + ); + } else if (checkpointOps.length > 0) { + await this.opLogStore.markReducersCommittedAndMergeClocks( + reducerCommittedSeqs, + checkpointOps, + ); + } }, }); + if (applyResult.reducerFailures?.length) { + OpLog.err( + `ConflictResolutionService: ${applyResult.reducerFailures.length} resolution operation(s) failed reducer replay.`, + ); + } + const appliedSeqs = applyResult.appliedOps .map((op) => opIdToSeq.get(op.id)) .filter((seq): seq is number => seq !== undefined); @@ -640,12 +652,56 @@ export class ConflictResolutionService { // propagates. throw new IncompleteRemoteOperationsError(applyResult.failedOp.error); } + + if (applyResult.reducerFailures?.length) { + const failedSyntheticOpIds = new Set( + applyResult.reducerFailures + .filter((failure) => checkpointExemptOpIds.has(failure.op.id)) + .map((failure) => failure.op.id), + ); + failedMergedResolutions = mergedResolutions.filter((merged) => + failedSyntheticOpIds.has(merged.mergedOp.id), + ); + const nonSyntheticFailure = applyResult.reducerFailures.find( + (failure) => !failedSyntheticOpIds.has(failure.op.id), + ); + if (nonSyntheticFailure) { + throw new IncompleteRemoteOperationsError(nonSyntheticFailure.error); + } + } + } + + if (failedMergedResolutions.length > 0) { + OpLog.warn( + `ConflictResolutionService: Falling back to LWW for ${failedMergedResolutions.length} failed disjoint merge(s).`, + ); + const fallbackResult = await this.autoResolveConflictsLWW( + failedMergedResolutions.map((merged) => merged.conflict), + [], + { + ...options, + disableDisjointMerge: true, + remoteApplyLifecycleOwnedByCaller: true, + }, + ); + fallbackLocalWinOpsCreated = fallbackResult.localWinOpsCreated; } } catch (error) { hasPrimaryError = true; throw error; } finally { - if (canDrainDeferredActions) { + if (remoteApplyWindowStarted) { + try { + this.hydrationState.startPostSyncCooldown(); + } catch (error) { + OpLog.err( + 'ConflictResolutionService: Failed to start post-sync cooldown', + error, + ); + } + this.hydrationState.endApplyingRemoteOps(); + } + if (canDrainDeferredActions && ownsRemoteApplyLifecycle) { try { await processDeferredActionsAfterRemoteApply( this.injector, @@ -663,8 +719,48 @@ export class ConflictResolutionService { } } + const fallbackOriginalOpIds = new Set( + failedMergedResolutions.flatMap((merged) => [ + ...merged.conflict.localOps.map((op) => op.id), + ...merged.conflict.remoteOps.map((op) => op.id), + ]), + ); + const remainingLocalOpsToReject = localOpsToReject.filter( + (opId) => !fallbackOriginalOpIds.has(opId), + ); + const remainingRemoteOpsToReject = remoteOpsToReject.filter( + (opId) => !fallbackOriginalOpIds.has(opId), + ); + const successfulMergedResolutions = mergedResolutions.filter( + (merged) => !failedMergedResolutions.includes(merged), + ); + + // Finalize only after every chosen resolution entered state. If reducer or + // archive work fails, the originals stay eligible for a clean retry. + if (remainingLocalOpsToReject.length > 0) { + await this.opLogStore.markRejected(remainingLocalOpsToReject); + OpLog.normal( + `ConflictResolutionService: Marked ${remainingLocalOpsToReject.length} local ops as rejected`, + ); + } + if (remainingRemoteOpsToReject.length > 0) { + await this.opLogStore.markRejected(remainingRemoteOpsToReject); + OpLog.normal( + `ConflictResolutionService: Marked ${remainingRemoteOpsToReject.length} remote ops as rejected`, + ); + } + + for (const plan of lwwPlans) { + await this._journalResolution(plan); + } + for (const merged of successfulMergedResolutions) { + if (writtenMergedOpIds.has(merged.mergedOp.id)) { + await this._journalMergedResolution(merged.plan); + } + } + // ───────────────────────────────────────────────────────────────────────── - // STEP 6: Show non-blocking notification + // STEP 5: Show non-blocking notification // // Distinguish "routine" self-healing (reschedule/repeat/archive/done churn // that resolves correctly on its own) from resolutions that discarded a real @@ -677,7 +773,7 @@ export class ConflictResolutionService { } // ───────────────────────────────────────────────────────────────────────── - // STEP 7: Validate and repair state after resolution + // STEP 6: Validate and repair state after resolution // Validation failure flips the SyncSessionValidationService latch — the // wrapper reads it before deciding IN_SYNC vs ERROR. (#7330) // ───────────────────────────────────────────────────────────────────────── @@ -693,7 +789,10 @@ export class ConflictResolutionService { // writtenLocalWinOps (not newLocalWinOps) is the post-dedupe set the atomic // mixed-source batch actually persisted. return { - localWinOpsCreated: writtenLocalWinOps.length + mergedResolutions.length, + localWinOpsCreated: + writtenLocalWinOps.length + + successfulMergedResolutions.length + + fallbackLocalWinOpsCreated, }; } @@ -817,9 +916,11 @@ export class ConflictResolutionService { */ private async _resolveConflictsWithLWW( conflicts: EntityConflict[], + disableDisjointMerge: boolean = false, ): Promise { const resolutions: LWWResolution[] = []; const mergedResolutions: MergedResolution[] = []; + const lwwPlans: LwwConflictResolutionPlan[] = []; const plans = planLwwConflictResolutions(conflicts, { isArchiveAction: (op) => op.actionType === ActionType.TASK_SHARED_MOVE_TO_ARCHIVE, @@ -857,15 +958,13 @@ export class ConflictResolutionService { plan.conflict.entityId, ); const mergedOp = - (conflictCountByEntity.get(entityKey) ?? 0) > 1 + disableDisjointMerge || (conflictCountByEntity.get(entityKey) ?? 0) > 1 ? undefined : await this._tryCreateDisjointMergeOp(plan); if (mergedOp) { // NOT journaled here: a `merged` entry claims "both sides kept", which - // is only true once the merged op is durably appended — STEP 3b journals - // it right after the append, so a failure in between cannot leave a - // phantom "kept both" entry. (LWW entries below journal at plan time: - // they describe a pick, not a new op, so there is nothing to wait for.) + // is only true once the merged op enters state. All journal entries are + // emitted after the chosen reducer work succeeds. mergedResolutions.push({ conflict: plan.conflict, mergedOp, plan }); OpLog.normal( `ConflictResolutionService: Disjoint-field merge for ` + @@ -887,12 +986,7 @@ export class ConflictResolutionService { winner: plan.winner, localWinOp, }); - - // SPAP-13 (observe-only): journal this auto-resolution so the discarded - // side is preserved and reviewable. Reads `plan`/`conflict` only — never - // mutates the resolution. Journal failures are swallowed inside - // `_journalResolution`, so they cannot affect what LWW picked. - await this._journalResolution(plan); + lwwPlans.push(plan); if ( plan.reason === 'remote-archive' || @@ -917,7 +1011,7 @@ export class ConflictResolutionService { } } - return { lwwResolutions: resolutions, mergedResolutions }; + return { lwwResolutions: resolutions, mergedResolutions, lwwPlans }; } /** @@ -1082,9 +1176,8 @@ export class ConflictResolutionService { * SPAP-14 (observe-only): journal a disjoint-field merge as `merged` / * `disjoint-merge` / `info`. Nothing was discarded, so it must NOT count toward * the unreviewed count. Like `_journalResolution`, any failure is swallowed and - * can never affect resolution. Called from STEP 3b AFTER the merged op is - * appended — not at plan time — so the entry never describes a merge that was - * never persisted. + * can never affect resolution. It is called only after the merged op's reducer + * work succeeds, so the entry never describes a failed merge. */ private async _journalMergedResolution( plan: LwwConflictResolutionPlan, @@ -1644,6 +1737,36 @@ export class ConflictResolutionService { options?: { pendingApply?: boolean }, ): Promise<{ ops: Operation[]; seqs: number[] }> { const result = await this.opLogStore.appendBatchSkipDuplicates(ops, source, options); - return { ops: result.writtenOps, seqs: result.seqs }; + const writtenSeqByOpId = new Map( + result.writtenOps.map((op, index) => [op.id, result.seqs[index]]), + ); + const replayable = await Promise.all( + ops.map(async (op) => { + const writtenSeq = writtenSeqByOpId.get(op.id); + if (writtenSeq !== undefined) { + return { op, seq: writtenSeq }; + } + + // A reducer failure deliberately leaves the durable remote row pending. + // On the next sync, deduplication finds that row instead of inserting it; + // reuse its sequence so the recovered reducer can be retried and + // checkpointed. Applied, archive-pending, failed, or rejected rows must + // not be reducer-dispatched again. + const existing = await this.opLogStore.getOpById(op.id); + return existing?.source === source && + existing.applicationStatus === 'pending' && + existing.rejectedAt === undefined && + existing.reducerRejectedAt === undefined + ? { op: existing.op, seq: existing.seq } + : undefined; + }), + ); + const pendingOps = replayable.filter( + (entry): entry is { op: Operation; seq: number } => entry !== undefined, + ); + return { + ops: pendingOps.map((entry) => entry.op), + seqs: pendingOps.map((entry) => entry.seq), + }; } } 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 beb02ef103..02b52a3c1f 100644 --- a/src/app/op-log/sync/remote-ops-processing.service.ts +++ b/src/app/op-log/sync/remote-ops-processing.service.ts @@ -501,7 +501,7 @@ export class RemoteOpsProcessingService { }, }); - committedFullStateOpIds = result.appendedOps + committedFullStateOpIds = result.appliedOps .filter((op) => FULL_STATE_OP_TYPES.has(op.opType)) .map((op) => op.id); diff --git a/src/app/op-log/testing/integration/migration-handling.integration.spec.ts b/src/app/op-log/testing/integration/migration-handling.integration.spec.ts index aae2c542ab..8b5dc52e0b 100644 --- a/src/app/op-log/testing/integration/migration-handling.integration.spec.ts +++ b/src/app/op-log/testing/integration/migration-handling.integration.spec.ts @@ -88,7 +88,7 @@ describe('Migration Handling Integration', () => { const spy = jasmine.createSpyObj('OperationLogCompactionService', [ 'compact', ]); - spy.compact.and.returnValue(Promise.resolve()); + spy.compact.and.returnValue(Promise.resolve(true)); return spy; }, }, 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 054b4647e0..0e567781b9 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 @@ -206,6 +206,44 @@ const defineStorePortContract = ( }); }); + it('should keep a reducer-failed full-state operation pending and recoverable', async () => { + const importOp = createOperation('failed-import', { + opType: OpType.SyncImport, + clientId: 'importClient', + vectorClock: { importClient: 9 }, + }); + const reducerError = new Error('full-state reducer failed'); + const applier: OperationApplyPort = { + applyOperations: async (_ops, options) => { + await options?.onReducersCommitted?.( + [], + [{ op: importOp, error: reducerError }], + ); + return { + appliedOps: [], + reducerFailures: [{ op: importOp, error: reducerError }], + }; + }, + }; + + await expectAsync( + applyRemoteOperations({ + ops: [importOp], + store, + applier, + isFullStateOperation: (op) => op.opType === OpType.SyncImport, + }), + ).toBeRejectedWithError(/full-state operation.*failed/i); + + const stored = await store.getOpById(importOp.id); + expect(stored?.applicationStatus).toBe('pending'); + expect(stored?.rejectedAt).toBeUndefined(); + expect(stored?.reducerRejectedAt).toBeUndefined(); + expect((await store.getPendingRemoteOps()).map((entry) => entry.op.id)).toEqual([ + importOp.id, + ]); + }); + it('should retain a third-client clock from an operation after a full-state import', async () => { const importOp = createOperation('ordered-import', { opType: OpType.SyncImport, diff --git a/src/app/op-log/testing/integration/service-logic.integration.spec.ts b/src/app/op-log/testing/integration/service-logic.integration.spec.ts index fea41b000e..214656f9cc 100644 --- a/src/app/op-log/testing/integration/service-logic.integration.spec.ts +++ b/src/app/op-log/testing/integration/service-logic.integration.spec.ts @@ -367,7 +367,7 @@ describe('Service Logic Integration', () => { const compactionSpy = jasmine.createSpyObj('OperationLogCompactionService', [ 'compact', ]); - compactionSpy.compact.and.returnValue(Promise.resolve()); + compactionSpy.compact.and.returnValue(Promise.resolve(true)); // Use real SyncImportFilterService for SYNC_IMPORT filtering integration tests // Note: This must be the real service, not a mock, because we're testing the diff --git a/src/app/op-log/testing/integration/task-done-replay.integration.spec.ts b/src/app/op-log/testing/integration/task-done-replay.integration.spec.ts index 86c03b5465..5949ce5f27 100644 --- a/src/app/op-log/testing/integration/task-done-replay.integration.spec.ts +++ b/src/app/op-log/testing/integration/task-done-replay.integration.spec.ts @@ -137,7 +137,7 @@ describe('done task operation replay', () => { mockVectorClockService.getCurrentVectorClock.and.returnValue( Promise.resolve({ clientA: 1 }), ); - mockCompactionService.compact.and.returnValue(Promise.resolve()); + mockCompactionService.compact.and.returnValue(Promise.resolve(true)); mockCompactionService.emergencyCompact.and.returnValue(Promise.resolve(true)); mockOperationCaptureService.extractEntityChanges.and.returnValue([]); mockClientIdService.getOrGenerateClientId.and.returnValue(Promise.resolve('clientA'));