From 610ca64894acb1188fc64ba59eefcc299bc0220c Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Wed, 8 Jul 2026 16:45:16 +0200 Subject: [PATCH] fix(sync): harden file-based .bak recovery, split gap detection & conflict counts (#8857) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sync): harden file-based .bak recovery and split gap detection Post-merge review of the SPAP-8/9/10/11 series found five defects in the file-based sync adapter; all fixed here with regression tests proven red/green against the previous code. Design cross-checked by a 7-reviewer multi-agent pass; its findings are folded in. - Split gap detection suppressed a syncVersion reset when the remote clock was EQUAL OR GREATER_THAN the last-seen clock — the exact bug the SPAP-9 review follow-up removed from the single-file path. A dominating client's snapshot reset (which compacts ops this client never saw) was treated as cosmetic, skipping snapshot hydration and silently diverging. Now EQUAL only, matching the single-file path. - .bak recovery staged the CORRUPT primary's rev, which setLastServerSeq then promoted to _lastSeenRevs: every later poll's SPAP-10 pre-check read "unchanged" and skipped the re-download while the upload path (no .bak recovery) kept failing on the corrupt primary — sync wedged until another client rewrote the file. A recovery download now never stages/promotes the rev (each poll re-recovers and re-seeds the heal cache), and every site that rewrites _lastSeenRevs drops any stale staged rev via the shared _commitLastSeenRev. - Snapshot uploads (force-upload / "Use Local" / E2EE re-encryption) left the pre-snapshot .bak behind. After a password rotation the stale old-key .bak was silently "recovered" by a still-old-key client (suppressing its wrong-password prompt) and heal-uploaded back over the new-key primary, reverting the rotation. Snapshot uploads now write the same payload to .bak FIRST, then the primary (_forceUploadWithBakFirst; deliberately FATAL — aborting pre-primary leaves the remote consistent for a retry). Applies to sync-data.json.bak and sync-ops.json.bak; sync-state.json.bak is deliberately exempt: its adoption is ref-validated (EQUAL clock vs snapshotRef), so a stale copy is inert, and it must keep serving the compaction crash window it exists for. - Recovery additionally refuses a PLAINTEXT .bak when encryption is expected: decoding trusts the file's own prefix flags, so a plaintext .bak decodes even under a wrong/rotated key — the same wrong-password-suppression class via mode (rather than key) mismatch. - The split migration wrote the v3 tombstone BEFORE neutralizing the legacy .bak (best-effort): a crash between the writes left a live v2 .bak that an OFF client's recovery would resurrect over the tombstone, forking the folder. Neutralize-first, fatal. Residual: a step-3 failure after the migration's ops-file commit leaves a live v2 file until the next snapshot upload (documented at the call site). - sync-ops.json — the hot file, rewritten on every op-bearing sync — had no backup at all, so a torn write wedged split sync until a manual force-upload. It now gets the same backup-before-overwrite + recovery + heal treatment as the single-file format. deleteAllData deletes the split files BEFORE the tombstone and treats source-of-truth deletion failures as errors (success:false) — it previously left every split file behind and reported success. The three backup/recover pairs are collapsed into shared _writeBakFile / _readBakFile helpers (the EQUAL||GREATER_THAN divergence above is exactly the copy-drift failure mode this prevents); .bak file names move into FILE_BASED_SYNC_CONSTANTS as remote-format surface. * fix(sync): show the exact pending-op count in the conflict dialog Compaction can fold still-unsynced ops into the snapshot baseline clock, so the dialog's vector-clock delta could report "0 changes" right next to "N local changes pending" — and the false 0 skipped the secondary USE_REMOTE overwrite confirmation. The dialog now prefers the EXACT pending-op count carried on LocalDataConflictError (new optional ConflictData.localUnsyncedOpsCount): it is precisely "what USE_REMOTE would discard", so both the displayed count and the >= 20-difference confirmation threshold work from a truthful figure; the clock delta remains the fallback when no measured count is supplied. Display/confirmation-only — no clock or op-log semantics touched. Also asserts the explicit-null lastSyncedVectorClock contract at the fresh-client conflict throw sites (test gap from SPAP-7). * chore(lint): enforce tx-handle-only access in op-log transactions The SQLite op-log adapter serializes every entry point through a per-connection FIFO queue; awaiting an adapter method inside a .transaction() callback enqueues behind the transaction's own slot and silently deadlocks all op-log persistence. The port contract documents the precondition and #8849 promised a lint rule — this adds it (no-adapter-in-tx, scoped to src/app/op-log) with RuleTester specs. Matching is rename-proof: it flags access on the SAME receiver the transaction was opened on (plain identifiers and any `this.`), so it does not depend on the field being named `_adapter`; known heuristic gaps (extracted callbacks, aliasing, method indirection) are documented. Also corrects the port doc: IndexedDB serializes only overlapping-scope transactions; SQLite provides the stronger whole-connection exclusion. --- eslint-local-rules/index.js | 1 + eslint-local-rules/rules/no-adapter-in-tx.js | 134 +++++ .../rules/no-adapter-in-tx.spec.js | 197 +++++++ eslint.config.js | 19 +- .../src/file-based-sync-data.ts | 4 + .../dialog-sync-conflict.component.spec.ts | 73 +++ .../dialog-sync-conflict.component.ts | 23 +- src/app/imex/sync/sync-wrapper.service.ts | 1 + src/app/op-log/core/types/sync.types.ts | 9 + .../op-log/persistence/op-log-db-adapter.ts | 12 +- .../file-based-sync-adapter.service.spec.ts | 362 +++++++++++- .../file-based-sync-adapter.service.ts | 520 ++++++++++++------ .../sync/operation-log-sync.service.spec.ts | 50 ++ 13 files changed, 1192 insertions(+), 213 deletions(-) create mode 100644 eslint-local-rules/rules/no-adapter-in-tx.js create mode 100644 eslint-local-rules/rules/no-adapter-in-tx.spec.js diff --git a/eslint-local-rules/index.js b/eslint-local-rules/index.js index aaf5e0824a..dc89c4c4de 100644 --- a/eslint-local-rules/index.js +++ b/eslint-local-rules/index.js @@ -14,4 +14,5 @@ module.exports = { 'require-entity-registry': require('./rules/require-entity-registry'), 'no-actions-in-effects': require('./rules/no-actions-in-effects'), 'no-multi-entity-effect': require('./rules/no-multi-entity-effect'), + 'no-adapter-in-tx': require('./rules/no-adapter-in-tx'), }; diff --git a/eslint-local-rules/rules/no-adapter-in-tx.js b/eslint-local-rules/rules/no-adapter-in-tx.js new file mode 100644 index 0000000000..1d9918fd5a --- /dev/null +++ b/eslint-local-rules/rules/no-adapter-in-tx.js @@ -0,0 +1,134 @@ +/** + * ESLint rule: no-adapter-in-tx + * + * Code inside an `adapter.transaction(async (tx) => { ... })` callback MUST use + * only the `tx` handle. The SQLite backend (`SqliteOpLogAdapter`) serializes + * every public entry point through a per-connection FIFO queue; a transaction + * holds one queue slot for its whole BEGIN…COMMIT. Awaiting any adapter method + * inside the callback (`this._adapter.get(...)` instead of `tx.get(...)`) + * enqueues behind the very slot the callback runs in and silently deadlocks all + * op-log persistence. A runtime guard cannot enforce this (a legal concurrent + * call and an illegal re-entrant one are indistinguishable at the queue), so + * enforcement lives here. See `SqliteOpLogAdapter._serialize()` in + * src/app/op-log/persistence/sqlite-op-log-adapter.ts. + * + * Heuristic (deliberately simple, low-false-positive): + * - Inside any function passed as an argument to a `.transaction(...)` call + * (nested functions included), flag member access on the SAME receiver the + * transaction was opened on — both plain identifiers (`dest.put(...)` inside + * `dest.transaction(...)` in op-log-backend-migration.ts) and `this.` + * receivers (`this..get(...)` inside + * `this..transaction(...)`), so the rule survives field renames. + * Access to a DIFFERENT receiver (e.g. `source.get(...)` inside + * `dest.transaction`) stays legal — that is a different connection. + * - Additionally flag `this._adapter` / `this.adapter` (the conventional shared + * adapter fields in this dir) inside ANY tx callback, since a second adapter + * instance over the same connection shares the queue. + * + * Known gaps (accepted — heuristic, not proof): a callback EXTRACTED to a named + * function/method and passed by reference is not analyzed; aliasing + * (`const a = this._adapter`) and adapter access hidden behind a method call + * inside the callback are invisible; identifier matching is name-based, not + * scope-aware. + * + * Scoped to src/app/op-log/** via eslint.config.js. + */ +module.exports = { + meta: { + type: 'problem', + docs: { + description: + 'Inside a .transaction() callback use only the tx handle; adapter methods enqueue behind this transaction’s queue slot and deadlock', + category: 'Possible Errors', + recommended: true, + }, + messages: { + noAdapterInTx: + 'Do not call adapter methods inside a `.transaction()` callback — use the `tx` handle. On the SQLite backend every adapter entry point enqueues behind this transaction’s own FIFO queue slot and deadlocks op-log persistence. See SqliteOpLogAdapter._serialize().', + }, + schema: [], + }, + + create(context) { + // One entry per enclosing `.transaction(...)` callback we are lexically + // inside; holds the receiver identifier name (or null for `this.*`-based + // receivers, which the this-check below covers). + const txCallbackStack = []; + + const isThisAdapterAccess = (node) => + node.type === 'MemberExpression' && + !node.computed && + node.object.type === 'ThisExpression' && + (node.property.name === '_adapter' || node.property.name === 'adapter'); + + const isTransactionCallbackArg = (fnNode) => { + const parent = fnNode.parent; + return ( + parent && + parent.type === 'CallExpression' && + parent.arguments.includes(fnNode) && + parent.callee.type === 'MemberExpression' && + !parent.callee.computed && + parent.callee.property.name === 'transaction' + ); + }; + + // Stable key for a transaction receiver: `id:` for plain identifiers + // (`dest.transaction(...)`), `this:` for `this..transaction(...)` + // — so the rule stays correct if the adapter field is ever renamed. + const receiverKeyOf = (node) => { + if (node.type === 'Identifier') return `id:${node.name}`; + if ( + node.type === 'MemberExpression' && + !node.computed && + node.object.type === 'ThisExpression' + ) { + return `this:${node.property.name}`; + } + return null; + }; + + const enterFunction = (node) => { + if (!isTransactionCallbackArg(node)) return; + txCallbackStack.push({ + fnNode: node, + receiverKey: receiverKeyOf(node.parent.callee.object), + }); + }; + + const exitFunction = (node) => { + const top = txCallbackStack[txCallbackStack.length - 1]; + if (top && top.fnNode === node) { + txCallbackStack.pop(); + } + }; + + return { + FunctionExpression: enterFunction, + 'FunctionExpression:exit': exitFunction, + ArrowFunctionExpression: enterFunction, + 'ArrowFunctionExpression:exit': exitFunction, + + MemberExpression(node) { + if (txCallbackStack.length === 0) return; + // `this._adapter.` / `this.adapter.` — the conventional shared + // adapter fields in this dir; flagged inside ANY tx callback because a + // second adapter instance still shares the per-connection queue. + if (isThisAdapterAccess(node.object)) { + context.report({ node, messageId: 'noAdapterInTx' }); + return; + } + // `.` where is the SAME receiver the enclosing + // `.transaction(...)` was called on — rename-proof: covers `dest.put()` + // inside `dest.transaction(...)` AND `this..get()` inside + // `this..transaction(...)`. Access to a DIFFERENT receiver + // (e.g. `source.get()` inside `dest`'s transaction) stays legal — that + // is a different connection. + const key = receiverKeyOf(node.object); + if (key !== null && txCallbackStack.some((ctx) => ctx.receiverKey === key)) { + context.report({ node, messageId: 'noAdapterInTx' }); + } + }, + }; + }, +}; diff --git a/eslint-local-rules/rules/no-adapter-in-tx.spec.js b/eslint-local-rules/rules/no-adapter-in-tx.spec.js new file mode 100644 index 0000000000..fc282c2b19 --- /dev/null +++ b/eslint-local-rules/rules/no-adapter-in-tx.spec.js @@ -0,0 +1,197 @@ +/** + * Tests for no-adapter-in-tx ESLint rule + */ +const { RuleTester } = require('eslint'); +const rule = require('./no-adapter-in-tx'); + +const ruleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: 'module', + }, +}); + +ruleTester.run('no-adapter-in-tx', rule, { + valid: [ + // The blessed pattern: only the tx handle inside the callback. + { + code: ` + class Store { + async replace() { + await this._adapter.transaction(['ops'], 'readwrite', async (tx) => { + await tx.clear('ops'); + await tx.add('ops', { seq: 1 }); + const all = await tx.getAll('ops'); + return all.length; + }); + } + } + `, + }, + // Adapter methods OUTSIDE any transaction callback are the normal API. + { + code: ` + class Store { + async load() { + const entry = await this._adapter.get('ops', 1); + await this._adapter.put('meta', entry, 'k'); + } + } + `, + }, + // Adapter use after the transaction call completed — not inside the callback. + { + code: ` + class Store { + async run() { + await this._adapter.transaction(['ops'], 'readwrite', async (tx) => { + await tx.add('ops', { seq: 1 }); + }); + await this._adapter.get('ops', 1); + } + } + `, + }, + // A DIFFERENT adapter identifier inside the callback is a different + // connection (op-log-backend-migration.ts: source reads inside dest's tx + // would be legal; only dest re-entry deadlocks). + { + code: ` + const migrate = async (source, dest) => { + await dest.transaction(['ops'], 'readwrite', async (tx) => { + const row = await source.get('ops', 1); + await tx.put('ops', row, 1); + }); + }; + `, + }, + // `.transaction` on the native IDB handle inside the adapter impl itself + // uses the returned tx object, not the adapter. + { + code: ` + class IdbAdapter { + iterate(store) { + const tx = this._database.transaction(store, 'readwrite'); + return tx.done; + } + } + `, + }, + ], + invalid: [ + // The core footgun: awaiting an adapter method inside its own tx callback. + { + code: ` + class Store { + async run() { + await this._adapter.transaction(['ops'], 'readwrite', async (tx) => { + await this._adapter.get('ops', 1); + }); + } + } + `, + errors: [{ messageId: 'noAdapterInTx' }], + }, + // Rename-proof: ANY \`this.\` receiver is matched against its own + // transaction — the rule must not depend on the field being named _adapter. + { + code: ` + class Store { + async run() { + await this._opLogDb.transaction(['ops'], 'readwrite', async (tx) => { + await this._opLogDb.get('ops', 1); + }); + } + } + `, + errors: [{ messageId: 'noAdapterInTx' }], + }, + // Alternate field name `this.adapter`. + { + code: ` + class Store { + async run() { + await this.adapter.transaction(['ops'], 'readwrite', async (tx) => { + await this.adapter.put('ops', {}, 1); + }); + } + } + `, + errors: [{ messageId: 'noAdapterInTx' }], + }, + // Nested arrow function inside the callback still deadlocks. + { + code: ` + class Store { + async run() { + await this._adapter.transaction(['ops'], 'readwrite', async (tx) => { + const helper = async () => this._adapter.count('ops'); + await helper(); + }); + } + } + `, + errors: [{ messageId: 'noAdapterInTx' }], + }, + // Nested (non-arrow) async function expression inside the callback. + { + code: ` + class Store { + async run() { + await this._adapter.transaction(['ops'], 'readwrite', async function (tx) { + const helper = async function () { + return this._adapter.getAll('ops'); + }; + await helper(); + }); + } + } + `, + errors: [{ messageId: 'noAdapterInTx' }], + }, + // Identifier receiver (migration style): re-entering `dest` inside + // `dest.transaction` deadlocks. + { + code: ` + const migrate = async (source, dest) => { + await dest.transaction(['ops'], 'readwrite', async (tx) => { + await dest.put('ops', {}, 1); + }); + }; + `, + errors: [{ messageId: 'noAdapterInTx' }], + }, + // Nested transaction on the same adapter is itself re-entry. + { + code: ` + class Store { + async run() { + await this._adapter.transaction(['ops'], 'readwrite', async (tx) => { + await this._adapter.transaction(['meta'], 'readwrite', async (tx2) => { + await tx2.put('meta', {}, 'k'); + }); + }); + } + } + `, + errors: [{ messageId: 'noAdapterInTx' }], + }, + // Two violations in one callback are each reported. + { + code: ` + class Store { + async run() { + await this._adapter.transaction(['ops'], 'readwrite', async (tx) => { + await this._adapter.delete('ops', 1); + await this._adapter.add('ops', { seq: 2 }); + }); + } + } + `, + errors: [{ messageId: 'noAdapterInTx' }, { messageId: 'noAdapterInTx' }], + }, + ], +}); + +// eslint-disable-next-line no-console +console.log('no-adapter-in-tx: all RuleTester cases passed'); diff --git a/eslint.config.js b/eslint.config.js index 4344757658..1ac7e190ba 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -224,17 +224,26 @@ module.exports = tseslint.config( 'local-rules/no-multi-entity-effect': 'warn', }, }, + // Op-log persistence: inside an adapter.transaction() callback only the tx + // handle may be used — adapter methods enqueue behind the transaction's own + // FIFO queue slot on the SQLite backend and deadlock (see + // SqliteOpLogAdapter._serialize()). + { + files: ['src/app/op-log/**/*.ts'], + plugins: { + 'local-rules': localRules, + }, + rules: { + 'local-rules/no-adapter-in-tx': 'error', + }, + }, // App code must route logging through Log/SyncLog/OpLog/... helpers. // Direct console.* calls bypass the exportable log history users attach // to bug reports. The Log implementation itself, tests, and benchmarks // (which intentionally dump timing numbers to stdout) are exempt. { files: ['src/app/**/*.ts'], - ignores: [ - 'src/app/**/*.spec.ts', - 'src/app/**/*.benchmark.ts', - 'src/app/core/log.ts', - ], + ignores: ['src/app/**/*.spec.ts', 'src/app/**/*.benchmark.ts', 'src/app/core/log.ts'], rules: { 'no-console': 'error', }, diff --git a/packages/sync-providers/src/file-based-sync-data.ts b/packages/sync-providers/src/file-based-sync-data.ts index 4a9251959b..aa19f69886 100644 --- a/packages/sync-providers/src/file-based-sync-data.ts +++ b/packages/sync-providers/src/file-based-sync-data.ts @@ -167,6 +167,10 @@ export const FILE_BASED_SYNC_CONSTANTS = { // sync setting). See FileBasedOpsFile / FileBasedStateFile / FileBasedSplitTombstone. OPS_FILE: 'sync-ops.json', STATE_FILE: 'sync-state.json', + // SPAP-8-style recovery artifacts for the split files. Remote-format surface: + // old and new clients must agree on these names forever. + OPS_BACKUP_FILE: 'sync-ops.json.bak', + STATE_BACKUP_FILE: 'sync-state.json.bak', SPLIT_FILE_VERSION: 3 as const, // Post-compaction RETAINED size for the split ops file (≈ MAX_RECENT_OPS/2). // NOTE: this is the trim target, NOT the trigger — a recompaction fires when the diff --git a/src/app/imex/sync/dialog-sync-conflict/dialog-sync-conflict.component.spec.ts b/src/app/imex/sync/dialog-sync-conflict/dialog-sync-conflict.component.spec.ts index c46056368f..84685e3e2f 100644 --- a/src/app/imex/sync/dialog-sync-conflict/dialog-sync-conflict.component.spec.ts +++ b/src/app/imex/sync/dialog-sync-conflict/dialog-sync-conflict.component.spec.ts @@ -12,8 +12,10 @@ const buildConflictData = (overrides: { localVectorClock?: VectorClock; remoteVectorClock?: VectorClock; lastSyncedVectorClock?: VectorClock | null; + localUnsyncedOpsCount?: number; }): ConflictData => ({ reason: ConflictReason.NoLastSync, + localUnsyncedOpsCount: overrides.localUnsyncedOpsCount, remote: { lastUpdate: 1000, lastUpdateAction: 'Remote data', @@ -91,6 +93,50 @@ describe('DialogSyncConflictComponent', () => { expect(component.remoteChangeCount).toBeNull(); }); + it('shows the exact pending-op count when the clock delta under-counts (compaction fold)', () => { + // Compaction folded the unsynced ops into the last-synced clock, so the + // per-client delta computes 0 even though 3 ops are known to be pending. + // The measured count is what USE_REMOTE would discard — show it. + const component = createComponent( + buildConflictData({ + localVectorClock: { clientA: 5 }, + remoteVectorClock: { clientA: 5, clientB: 2 }, + lastSyncedVectorClock: { clientA: 5 }, + localUnsyncedOpsCount: 3, + }), + ); + + expect(component.localChangeCount).toBe(3); + // The remote count still comes from the clock delta. + expect(component.remoteChangeCount).toBe(2); + }); + + it('prefers the measured pending-op count over the clock delta whenever provided', () => { + const component = createComponent( + buildConflictData({ + localVectorClock: { clientA: 10 }, + remoteVectorClock: { clientA: 3 }, + lastSyncedVectorClock: { clientA: 3 }, + localUnsyncedOpsCount: 3, + }), + ); + + // delta = 7, but 3 is the exact figure for "what USE_REMOTE discards". + expect(component.localChangeCount).toBe(3); + }); + + it('falls back to the clock delta when no pending-op count is provided', () => { + const component = createComponent( + buildConflictData({ + localVectorClock: { clientA: 10 }, + remoteVectorClock: { clientA: 3 }, + lastSyncedVectorClock: { clientA: 3 }, + }), + ); + + expect(component.localChangeCount).toBe(7); + }); + it('returns null when vector clocks are entirely absent', () => { const component = createComponent( buildConflictData({ @@ -150,5 +196,32 @@ describe('DialogSyncConflictComponent', () => { ).getConfirmationMessage('USE_REMOTE'); expect(msg).toContain(T.F.SYNC.D_CONFLICT.OVERWRITE_WARNING_UNKNOWN); }); + + it('confirms USE_REMOTE from the exact pending count when the clock delta reads 0', () => { + // Compaction folded 25 unsynced ops into the baseline clock: the delta + // computed local=0, remote=0 and shouldConfirmOverwrite('USE_REMOTE') + // returned false — no secondary confirmation despite 25 real unsynced + // local changes about to be discarded. The measured count restores the + // designed >= 20-difference confirmation. + const component = createComponent( + buildConflictData({ + localVectorClock: { clientA: 5 }, + remoteVectorClock: { clientA: 5 }, + lastSyncedVectorClock: { clientA: 5 }, + localUnsyncedOpsCount: 25, + }), + ); + + expect(component.localChangeCount).toBe(25); + const typed = component as unknown as { + shouldConfirmOverwrite(r: string): boolean; + getConfirmationMessage(r: string): string; + }; + expect(typed.shouldConfirmOverwrite('USE_REMOTE')).toBe(true); + // The counted (not count-free) warning is used — the figure is exact. + expect(typed.getConfirmationMessage('USE_REMOTE')).toContain( + T.F.SYNC.D_CONFLICT.OVERWRITE_WARNING, + ); + }); }); }); diff --git a/src/app/imex/sync/dialog-sync-conflict/dialog-sync-conflict.component.ts b/src/app/imex/sync/dialog-sync-conflict/dialog-sync-conflict.component.ts index d220411329..5de25c805e 100644 --- a/src/app/imex/sync/dialog-sync-conflict/dialog-sync-conflict.component.ts +++ b/src/app/imex/sync/dialog-sync-conflict/dialog-sync-conflict.component.ts @@ -57,7 +57,7 @@ export class DialogSyncConflictComponent { isHighlightLocal = !this.isHighlightRemote; remoteChangeCount = this.getChangeCount('remote'); - localChangeCount = this.getChangeCount('local'); + localChangeCount = this.getLocalChangeCount(); isHighlightRemoteChanges = (this.remoteChangeCount ?? 0) > (this.localChangeCount ?? 0); isHighlightLocalChanges = !this.isHighlightRemoteChanges; @@ -156,9 +156,22 @@ export class DialogSyncConflictComponent { return changeCount; } + /** + * Local change count. Prefers the EXACT pending-op count measured from the + * op log over the vector-clock delta: compaction can fold still-unsynced ops + * into the last-synced baseline clock, so the delta can under-count real + * pending changes (e.g. report 0 while N unsynced ops exist — which also + * wrongly skipped the secondary overwrite confirmation). The measured count + * is precisely "what USE_REMOTE would discard", so it is the decision-relevant + * figure; the delta remains the fallback for producers that don't supply it. + */ + private getLocalChangeCount(): number | null { + return this.data.localUnsyncedOpsCount ?? this.getChangeCount('local'); + } + private shouldConfirmOverwrite(resolution: DialogConflictResolutionResult): boolean { - const remoteChanges = this.getChangeCount('remote'); - const localChanges = this.getChangeCount('local'); + const remoteChanges = this.remoteChangeCount; + const localChanges = this.localChangeCount; // If we cannot quantify the changes (fresh client, no last-synced // baseline), still show the confirmation — overwriting could discard real @@ -181,8 +194,8 @@ export class DialogSyncConflictComponent { } private getConfirmationMessage(resolution: DialogConflictResolutionResult): string { - const remoteChanges = this.getChangeCount('remote'); - const localChanges = this.getChangeCount('local'); + const remoteChanges = this.remoteChangeCount; + const localChanges = this.localChangeCount; const [sourceName, targetName] = resolution === 'USE_REMOTE' ? ['remote', 'local'] : ['local', 'remote']; diff --git a/src/app/imex/sync/sync-wrapper.service.ts b/src/app/imex/sync/sync-wrapper.service.ts index c6131b73b7..d98d8e213f 100644 --- a/src/app/imex/sync/sync-wrapper.service.ts +++ b/src/app/imex/sync/sync-wrapper.service.ts @@ -1249,6 +1249,7 @@ export class SyncWrapperService { vectorClock: localClock, lastSyncedVectorClock: error.lastSyncedVectorClock ?? null, }, + localUnsyncedOpsCount: error.unsyncedCount, }; SyncLog.log( diff --git a/src/app/op-log/core/types/sync.types.ts b/src/app/op-log/core/types/sync.types.ts index 9370648034..97b70f33a5 100644 --- a/src/app/op-log/core/types/sync.types.ts +++ b/src/app/op-log/core/types/sync.types.ts @@ -153,6 +153,15 @@ export interface ConflictData { reason: ConflictReason; remote: RemoteMeta; local: LocalMeta; + /** + * Exact number of unsynced local ops known at conflict time (from + * LocalDataConflictError.unsyncedCount). The conflict dialog prefers it over + * the vector-clock delta as the local change count: compaction can fold + * still-unsynced ops into the last-synced baseline clock, so the delta can + * under-count real pending local changes, while this is precisely what + * USE_REMOTE would discard. + */ + localUnsyncedOpsCount?: number; additional?: unknown; } diff --git a/src/app/op-log/persistence/op-log-db-adapter.ts b/src/app/op-log/persistence/op-log-db-adapter.ts index e0c2e749a9..01c6b2e012 100644 --- a/src/app/op-log/persistence/op-log-db-adapter.ts +++ b/src/app/op-log/persistence/op-log-db-adapter.ts @@ -137,11 +137,13 @@ export interface OpLogTx { * * Concurrency: callers may issue operations concurrently — the op-log does * (capture append, archive write and compaction overlap). Transactions are - * mutually exclusive per database, and no bare operation may interleave into an - * open transaction. IndexedDB provides this natively (independent transactions); - * a backend over a single connection with no nested transactions (SQLite) MUST - * serialize internally to honor it. Callers therefore never need to lock around - * these methods themselves. + * mutually exclusive per overlapping store scope, and no bare operation may + * interleave into an open transaction. IndexedDB provides this natively + * (transactions with overlapping scopes are serialized; disjoint-scope ones may + * run concurrently); a backend over a single connection with no nested + * transactions (SQLite) MUST serialize internally and provides the stronger + * whole-connection exclusion via its FIFO queue. Callers therefore never need + * to lock around these methods themselves. */ export interface OpLogDbAdapter { /** diff --git a/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts b/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts index d82a975370..afd2831209 100644 --- a/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts +++ b/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts @@ -861,11 +861,22 @@ describe('FileBasedSyncAdapterService', () => { expect(uploadedData.recentOps).toEqual([]); // Fresh start }); - it('should upload snapshot directly without backup (snapshots replace state completely)', async () => { + it('writes the SAME snapshot payload to .bak BEFORE the primary (rotation-safe replace)', async () => { + // A snapshot replaces the remote wholesale, so the pre-snapshot .bak must + // not survive it: after an E2EE password rotation a stale old-key .bak + // would be silently "recovered" by a still-old-key client (suppressing its + // wrong-password prompt) and heal-uploaded back over the new-key primary. mockProvider.downloadFile.and.throwError( new RemoteFileNotFoundAPIError('sync-data.json'), ); - mockProvider.uploadFile.and.returnValue(Promise.resolve({ rev: 'rev-1' })); + const uploads: { path: string; data: string }[] = []; + mockProvider.uploadFile.and.callFake( + async (path: string, data: string, _rev: string | null, force?: boolean) => { + uploads.push({ path, data }); + expect(force).toBe(true); + return { rev: `${path}-rev` }; + }, + ); await adapter.uploadSnapshot( {}, @@ -877,20 +888,17 @@ describe('FileBasedSyncAdapterService', () => { 'test-op-id-2', // opId ); - // Should upload to main sync file with force overwrite (no backup created) - expect(mockProvider.uploadFile).toHaveBeenCalledWith( - FILE_BASED_SYNC_CONSTANTS.SYNC_FILE, - jasmine.any(String), - null, - true, // Force overwrite - snapshots replace state completely - ); - // Should NOT create backup file - expect(mockProvider.uploadFile).not.toHaveBeenCalledWith( - FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE, - jasmine.any(String), - jasmine.anything(), - jasmine.anything(), - ); + const paths = uploads.map((u) => u.path); + const bakIdx = paths.indexOf(FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE); + const mainIdx = paths.indexOf(FILE_BASED_SYNC_CONSTANTS.SYNC_FILE); + expect(bakIdx).toBeGreaterThanOrEqual(0); + expect(mainIdx).toBeGreaterThanOrEqual(0); + // .bak first, then the primary — a crash in between leaves a valid old + // primary + already-refreshed .bak (nothing recoverable, nothing stale). + expect(bakIdx).toBeLessThan(mainIdx); + // Identical payload: the .bak is the snapshot itself, not the pre-snapshot + // remote it deliberately replaces. + expect(uploads[bakIdx].data).toBe(uploads[mainIdx].data); }); describe('uploadSnapshot gap detection', () => { @@ -2331,6 +2339,80 @@ describe('FileBasedSyncAdapterService', () => { expect(mockSnackService.open).toHaveBeenCalled(); }); + it('(b) recovers from .bak when the primary fails to DECRYPT (real DecryptError)', async () => { + // Same shape as the DecompressError case, driven through a REAL decrypt + // failure: the adapter holds a key, the primary claims encryption but its + // body is garbage. Safe against key rotation because uploadSnapshot + // refreshes .bak with the same payload/key as the primary, so a + // primary/.bak key mismatch cannot exist on a healthy remote. + const encAdapter = service.createAdapter(mockProvider, mockCfg, 'test-password'); + const backupData = createMockSyncData({ + syncVersion: 5, + recentOps: [compactOp('recovered-from-decrypt-failure') as never], + }); + const undecryptableMain = + getSyncFilePrefix({ isCompress: false, isEncrypt: true, modelVersion: 2 }) + + 'bm90LXZhbGlkLWNpcGhlcnRleHQ='; + mockProvider.downloadFile.and.callFake((path: string) => { + if (path === FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE) { + return Promise.resolve({ + dataStr: addPrefix(backupData), + rev: 'bak-rev-decrypt', + }); + } + return Promise.resolve({ dataStr: undecryptableMain, rev: 'corrupt-enc-rev' }); + }); + + const result = await encAdapter.downloadOps(0); + + expect(result.ops.length).toBe(1); + expect(result.ops[0].op.id).toBe('recovered-from-decrypt-failure'); + expect(result.latestSeq).toBe(5); + expect(mockSnackService.open).toHaveBeenCalled(); + }); + + it('(b) refuses a PLAINTEXT .bak when encryption is expected (key-suppression vector)', async () => { + // A plaintext .bak decodes via its own prefix flags even under a + // wrong/rotated key. Accepting it would suppress the wrong-password + // dialog and let the heal upload clobber the encrypted primary — the + // same class as the E2EE-rotation revert. Recovery must refuse it and + // surface the ORIGINAL DecryptError instead. + const encAdapter = service.createAdapter( + mockProvider, + { isEncrypt: true, isCompress: false }, + 'test-password', + ); + const backupData = createMockSyncData({ syncVersion: 5 }); + const undecryptableMain = + getSyncFilePrefix({ isCompress: false, isEncrypt: true, modelVersion: 2 }) + + 'bm90LXZhbGlkLWNpcGhlcnRleHQ='; + mockProvider.downloadFile.and.callFake((path: string) => { + if (path === FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE) { + // addPrefix uses mockCfg (plaintext) → a plaintext-prefixed .bak. + return Promise.resolve({ dataStr: addPrefix(backupData), rev: 'plain-bak' }); + } + return Promise.resolve({ dataStr: undecryptableMain, rev: 'corrupt-enc-rev' }); + }); + + await expectAsync(encAdapter.downloadOps(0)).toBeRejected(); + expect(mockSnackService.open).not.toHaveBeenCalled(); + }); + + it('(b) rethrows the ORIGINAL error when both primary and .bak are undecodable', async () => { + const garbage = + getSyncFilePrefix({ isCompress: true, isEncrypt: false, modelVersion: 2 }) + + 'not-valid-gzip'; + mockProvider.downloadFile.and.callFake((path: string) => { + if (path === FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE) { + return Promise.resolve({ dataStr: garbage, rev: 'bak-corrupt-too' }); + } + return Promise.resolve({ dataStr: garbage, rev: 'main-corrupt' }); + }); + + await expectAsync(adapter.downloadOps(0)).toBeRejected(); + expect(mockSnackService.open).not.toHaveBeenCalled(); + }); + it('(b) after recovery, the next upload heals the corrupt primary via ITS rev (no revToMatch pollution)', async () => { // Regression for the self-perpetuating degraded state: recovery must seed the // cache with the CORRUPT PRIMARY rev, not the .bak rev, so the follow-up @@ -2759,6 +2841,47 @@ describe('FileBasedSyncAdapterService', () => { expect(mockProvider.downloadFile).toHaveBeenCalledTimes(1); expect(result.snapshotState).toBeDefined(); }); + + it('(g) .bak recovery never promotes the corrupt primary rev to last-seen (heal cannot wedge)', async () => { + // Regression: recovery serves the CORRUPT primary's rev (for the in-cycle + // heal), and setLastServerSeq promotes staged revs. If the corrupt rev were + // promoted, every later poll's pre-check would read "unchanged" and skip the + // full download while the upload path (no .bak recovery) keeps failing on the + // corrupt primary — sync wedged until another client rewrites the file. + const CORRUPT_MAIN_REV = 'corrupt-main-rev-wedge'; + const backupData = createMockSyncData({ syncVersion: 2, recentOps: [] }); + const undecodableMain = + getSyncFilePrefix({ isCompress: true, isEncrypt: false, modelVersion: 2 }) + + 'not-valid-gzip'; + mockProvider.downloadFile.and.callFake((path: string) => { + if (path === FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE) { + return Promise.resolve({ dataStr: addPrefix(backupData), rev: 'bak-rev' }); + } + return Promise.resolve({ dataStr: undecodableMain, rev: CORRUPT_MAIN_REV }); + }); + + // Recovery download (nothing new to apply) + the caller persisting the + // cursor — the promotion point that previously committed the corrupt rev. + await adapter.downloadOps(2); + await adapter.setLastServerSeq(2); + + crossPollBoundary(); + mockProvider.downloadFile.calls.reset(); + // The remote is still the same corrupt file. + mockProvider.getFileRev.and.callFake(async (path: string) => { + if (path === FILE_BASED_SYNC_CONSTANTS.SYNC_FILE) { + return { rev: CORRUPT_MAIN_REV }; + } + throw new RemoteFileNotFoundAPIError(path); + }); + + const result = await adapter.downloadOps(2); + + // Must NOT short-circuit on the corrupt rev: the full download re-recovers + // and re-seeds the heal cache so a later upload can still heal the primary. + expect(mockProvider.downloadFile).toHaveBeenCalled(); + expect(result.latestSeq).toBe(2); + }); }); // ═══════════════════════════════════════════════════════════════════════════ @@ -2841,10 +2964,11 @@ describe('FileBasedSyncAdapterService', () => { // Never builds a snapshot on the op-only path. expect(mockStateSnapshotService.getStateSnapshot).not.toHaveBeenCalled(); - // Every upload targeted the ops file only. + // Every upload targeted the ops file or its SPAP-8 backup — never the + // snapshot (sync-state.json) or legacy (sync-data.json) files. const paths = uploadedPaths(); - expect(paths.length).toBeGreaterThan(0); - paths.forEach((p) => expect(p).toBe(C.OPS_FILE)); + expect(paths).toContain(C.OPS_FILE); + paths.forEach((p) => expect([C.OPS_FILE, C.OPS_BACKUP_FILE]).toContain(p as never)); // Only the ops file was downloaded. mockProvider.downloadFile.calls.allArgs().forEach((args) => { expect(args[0]).toBe(C.OPS_FILE); @@ -2929,6 +3053,50 @@ describe('FileBasedSyncAdapterService', () => { expect(result.snapshotState).toBeDefined(); }); + // (a4) Same review follow-up as the single-file path: a strictly-dominating + // (GREATER_THAN) remote clock is NOT proof this client received the ops that + // a snapshot reset compacted into sync-state.json, so a syncVersion + // regression must flag a gap. The split fork originally suppressed on + // EQUAL || GREATER_THAN and silently diverged. + it('(a4) split download flags a gap when a dominating-clock reset compacted ops the client had not yet seen', async () => { + // First download establishes expected syncVersion (5) + last-seen clock. + routeDownloads({ + [C.OPS_FILE]: addPrefix( + makeOpsFile({ + syncVersion: 5, + vectorClock: { client1: 5 }, + recentOps: [makeCompactOp({ sv: 5, v: { client1: 5 } })], + oldestOpSyncVersion: 5, + }), + 3, + ), + }); + await adapter.downloadOps(5, 'client2'); + + // A strictly-ahead client snapshot-reset the folder (compacting ops this + // client never saw into the snapshot) and then made one more edit. + const stateFile = makeStateFile({ + syncVersion: 1, + vectorClock: { client1: 6 }, + }); + const opsFile = makeOpsFile({ + syncVersion: 2, + vectorClock: { client1: 7 }, // GREATER_THAN last-seen {client1: 5} + recentOps: [makeCompactOp({ id: 'op-post-reset', sv: 2, v: { client1: 7 } })], + oldestOpSyncVersion: 2, + snapshotRef: { syncVersion: 1, vectorClock: { client1: 6 }, rev: 'state-rev-x' }, + }); + routeDownloads({ + [C.OPS_FILE]: addPrefix(opsFile, 3), + [C.STATE_FILE]: addPrefix(stateFile, 3), + }); + + const result = await adapter.downloadOps(5, 'client2'); + + expect(result.gapDetected).toBe(true); + expect(result.snapshotState).toBeDefined(); + }); + // (b) compaction triggers when the buffer exceeds MAX_RECENT_OPS, writing // state THEN ops. it('(b) compaction past MAX_RECENT_OPS writes sync-state.json BEFORE sync-ops.json', async () => { @@ -2999,7 +3167,7 @@ describe('FileBasedSyncAdapterService', () => { }), 3, ), - [C.STATE_FILE + '.bak']: addPrefix( + [C.STATE_BACKUP_FILE]: addPrefix( makeStateFile({ syncVersion: 1, vectorClock: { client1: 1 }, @@ -3075,11 +3243,16 @@ describe('FileBasedSyncAdapterService', () => { expect(tomb.version).toBe(C.SPLIT_FILE_VERSION); expect(tomb.format).toBe(C.SPLIT_TOMBSTONE_FORMAT); - // .bak neutralized to a v3 tombstone too. + // .bak neutralized to a v3 tombstone too — and BEFORE the tombstone write, + // so a crash in between leaves a valid v2 primary (nothing stale to + // recover) instead of a tombstone + live v2 .bak that an OFF client's + // SPAP-8 recovery would resurrect over the tombstone. + const bakIdx = paths.indexOf(C.BACKUP_FILE); + expect(bakIdx).toBeGreaterThanOrEqual(0); + expect(bakIdx).toBeLessThan(tombIdx); const bakCall = mockProvider.uploadFile.calls .allArgs() .find((a) => a[0] === C.BACKUP_FILE); - expect(bakCall).toBeDefined(); const bak = parseWithPrefix(bakCall![1] as string) as unknown as { version: number; }; @@ -3193,5 +3366,150 @@ describe('FileBasedSyncAdapterService', () => { expect(downloaded).toContain(C.OPS_FILE); expect(downloaded).toContain(C.STATE_FILE); }); + + it('(i) op upload backs up the current sync-ops.json to .bak BEFORE overwriting it', async () => { + // sync-ops.json is the hot file (rewritten on every op-bearing sync) — it + // gets the same SPAP-8 backup-before-overwrite as the single-file format. + const existing = makeOpsFile({ + syncVersion: 3, + recentOps: [makeCompactOp({ id: 'op-existing' })], + }); + routeDownloads({ [C.OPS_FILE]: addPrefix(existing, 3) }); + + await adapter.uploadOps([createMockSyncOp()], 'client1'); + + const paths = uploadedPaths(); + const bakIdx = paths.indexOf(C.OPS_BACKUP_FILE); + const opsIdx = paths.lastIndexOf(C.OPS_FILE); + expect(bakIdx).toBeGreaterThanOrEqual(0); + expect(bakIdx).toBeLessThan(opsIdx); + // The .bak preserves the PRE-overwrite remote (syncVersion 3, not the new 4). + const bakCall = mockProvider.uploadFile.calls + .allArgs() + .find((a) => a[0] === C.OPS_BACKUP_FILE); + const bak = parseWithPrefix(bakCall![1] as string) as unknown as FileBasedOpsFile; + expect(bak.syncVersion).toBe(3); + }); + + it('(i) recovers a corrupt sync-ops.json from .bak and heals it via ITS rev', async () => { + const bakOps = makeOpsFile({ + syncVersion: 4, + recentOps: [makeCompactOp({ id: 'op-from-bak' })], + }); + const garbage = + getSyncFilePrefix({ isCompress: true, isEncrypt: false, modelVersion: 3 }) + + 'not-valid-gzip'; + mockProvider.downloadFile.and.callFake(async (path: string) => { + if (path === C.OPS_FILE) return { dataStr: garbage, rev: 'corrupt-ops-rev' }; + if (path === C.OPS_BACKUP_FILE) { + return { dataStr: addPrefix(bakOps, 3), rev: 'ops-bak-rev' }; + } + throw new RemoteFileNotFoundAPIError(path); + }); + + const result = await adapter.downloadOps(1, 'client2'); + + expect(result.ops.some((o) => o.op.id === 'op-from-bak')).toBe(true); + expect(mockSnackService.open).toHaveBeenCalled(); + + // The next upload heals the corrupt primary via a conditional PUT matching + // ITS rev (not the .bak rev, which would mismatch forever). + const opsRevToMatch: (string | null)[] = []; + mockProvider.uploadFile.and.callFake( + async (path: string, _d: string, revToMatch: string | null) => { + if (path === C.OPS_FILE) opsRevToMatch.push(revToMatch); + return { rev: `${path}-newrev` }; + }, + ); + await adapter.uploadOps([createMockSyncOp()], 'client1'); + expect(opsRevToMatch).toContain('corrupt-ops-rev'); + expect(opsRevToMatch).not.toContain('ops-bak-rev'); + }); + + it('(i) split .bak recovery never promotes the corrupt ops rev to last-seen (heal cannot wedge)', async () => { + mockProvider.id = SyncProviderId.Dropbox; + adapter = service.createAdapter(mockProvider, mockCfg, mockEncryptKey); + const bakOps = makeOpsFile({ syncVersion: 4, recentOps: [] }); + const garbage = + getSyncFilePrefix({ isCompress: true, isEncrypt: false, modelVersion: 3 }) + + 'not-valid-gzip'; + mockProvider.downloadFile.and.callFake(async (path: string) => { + if (path === C.OPS_FILE) return { dataStr: garbage, rev: 'corrupt-ops-rev' }; + if (path === C.OPS_BACKUP_FILE) { + return { dataStr: addPrefix(bakOps, 3), rev: 'ops-bak-rev' }; + } + throw new RemoteFileNotFoundAPIError(path); + }); + + await adapter.downloadOps(1, 'client2'); + await adapter.setLastServerSeq(4); // the promotion point + + const lastSeen = (service as unknown as { _lastSeenRevs: Map }) + ._lastSeenRevs; + expect(lastSeen.get('Dropbox')).toBeUndefined(); + }); + + it('(i) snapshot upload refreshes the OPS .bak (not the ref-validated state .bak) before the primary', async () => { + // Rotation-safe replace: sync-ops.json.bak feeds UNVALIDATED recovery, so + // a pre-snapshot (possibly rotated-key) copy must not survive a snapshot + // upload. sync-state.json.bak is deliberately NOT refreshed — its adoption + // is ref-validated (EQUAL clock vs snapshotRef), so a stale copy is inert, + // and it must keep serving the compaction crash window it was made for. + routeDownloads({}); + + await adapter.uploadSnapshot( + {}, + 'client1', + 'recovery', + { client1: 1 }, + 1, + undefined, + 'op-id-split-snap', + ); + + const paths = uploadedPaths(); + const oBak = paths.indexOf(C.OPS_BACKUP_FILE); + const oMain = paths.indexOf(C.OPS_FILE); + expect(oBak).toBeGreaterThanOrEqual(0); + expect(oMain).toBeGreaterThanOrEqual(0); + expect(oBak).toBeLessThan(oMain); + // Identical payloads: the ops .bak IS the fresh snapshot-reset ops file. + const argAt = (i: number): string => + mockProvider.uploadFile.calls.allArgs()[i][1] as string; + expect(argAt(oBak)).toBe(argAt(oMain)); + // State .bak untouched. + expect(paths).not.toContain(C.STATE_BACKUP_FILE); + expect(paths).toContain(C.STATE_FILE); + }); + + it('(i) deleteAllData removes the split files (BEFORE the tombstone) and their .baks', async () => { + mockProvider.removeFile.and.returnValue(Promise.resolve(undefined as never)); + + await adapter.deleteAllData(); + + const removed = mockProvider.removeFile.calls.allArgs().map((a) => a[0] as string); + expect(removed).toContain(C.OPS_FILE); + expect(removed).toContain(C.STATE_FILE); + expect(removed).toContain(C.OPS_BACKUP_FILE); + expect(removed).toContain(C.STATE_BACKUP_FILE); + // Split source-of-truth files go before sync-data.json: a partial failure + // must not leave a deleted tombstone next to live split data (an OFF + // client would fresh-start a v2 file against it). + expect(removed.indexOf(C.OPS_FILE)).toBeLessThan(removed.indexOf(C.SYNC_FILE)); + expect(removed.indexOf(C.STATE_FILE)).toBeLessThan(removed.indexOf(C.SYNC_FILE)); + }); + + it('(i) deleteAllData FAILS when a split source-of-truth file cannot be deleted', async () => { + // Reporting success while sync-ops.json still holds user data on the + // remote would be a lie — only .bak/lock artifacts are best-effort. + mockProvider.removeFile.and.callFake(async (path: string) => { + if (path === C.OPS_FILE) throw new Error('server error'); + return undefined as never; + }); + + const res = await adapter.deleteAllData(); + + expect(res.success).toBe(false); + }); }); }); diff --git a/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts b/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts index 5fd586e09a..ed6ddbc816 100644 --- a/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts +++ b/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts @@ -11,6 +11,7 @@ import { RestorePointType, } from '../provider.interface'; import { EncryptAndCompressHandlerService } from '../../encryption/encrypt-and-compress-handler.service'; +import { extractSyncFileStateFromPrefix } from '../../util/sync-file-prefix'; import { EncryptAndCompressCfg } from '../../core/types/sync.types'; import { Operation, @@ -699,10 +700,17 @@ export class FileBasedSyncAdapterService { // Step 2.5: Backup-before-overwrite (two-phase write). Copy the CURRENT remote // content to sync-data.json.bak before overwriting the primary file, so an - // interrupted/corrupt write can be recovered on the next download. Non-fatal: - // a provider without copy support must still be able to sync. + // interrupted/corrupt write can be recovered on the next download. We re-encode + // the already-in-hand `currentData` rather than issuing another GET. if (fileExists && currentData) { - await this._backupCurrentRemote(provider, cfg, encryptKey, currentData); + await this._writeBakFile( + provider, + cfg, + encryptKey, + FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE, + currentData, + FILE_BASED_SYNC_CONSTANTS.FILE_VERSION, + ); } // Step 3: Upload conditionally; on rev mismatch re-download and retry @@ -721,7 +729,7 @@ export class FileBasedSyncAdapterService { // SPAP-10: the remote is now exactly what we just uploaded, so its rev is the // fresh last-seen rev. Recording it lets the next poll short-circuit if no // other client writes in between. Persisted via _persistState() below. - this._lastSeenRevs.set(providerKey, finalRev); + this._commitLastSeenRev(providerKey, finalRev); // Use finalSyncVersion (NOT mergedOps.length) to match download behavior. // mergedOps.length is the total ops count, which can be much larger than syncVersion @@ -832,6 +840,7 @@ export class FileBasedSyncAdapterService { let syncData: FileBasedSyncData; let rev: string; + let recoveredFromBackup = false; try { const result = await this._downloadSyncFile(provider, cfg, encryptKey); syncData = result.data; @@ -857,8 +866,15 @@ export class FileBasedSyncAdapterService { if (this._isRecoverableCorruption(e)) { // Primary sync-data.json is corrupt/empty/unparseable (e.g. interrupted // write). Try to recover from the .bak artifact before failing. - const recovered = await this._recoverFromBackup(provider, cfg, encryptKey); + const recovered = await this._readBakFile( + provider, + cfg, + encryptKey, + FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE, + FILE_BASED_SYNC_CONSTANTS.FILE_VERSION, + ); if (recovered) { + recoveredFromBackup = true; OpLog.warn( 'FileBasedSyncAdapter: Primary sync file unreadable; recovered from backup', ); @@ -873,16 +889,7 @@ export class FileBasedSyncAdapterService { (e as { primaryRev?: string } | null)?.primaryRev ?? recovered.rev; rev = primaryRev; this._setCachedSyncData(providerKey, syncData, rev); - // De-dup the user-facing notice: only surface it the first time we see a - // given corrupt primary rev, so a download-only client (which cannot heal - // the file itself) does not re-toast on every sync cycle. - if (this._lastRecoveredCorruptRev.get(providerKey) !== primaryRev) { - this._lastRecoveredCorruptRev.set(providerKey, primaryRev); - // Lazy resolve — absent in some unit harnesses (returns null → no notice). - this._injector - .get(SnackService, null) - ?.open({ msg: T.F.SYNC.S.SYNC_DATA_RECOVERED_FROM_BACKUP }); - } + this._notifyRecoveredCorruptPrimaryOnce(providerKey, primaryRev); } else { throw e; } @@ -1005,13 +1012,7 @@ export class FileBasedSyncAdapterService { // SPAP-9: remember this file's causal clock so the next download can decide // whether a syncVersion regression is cosmetic or a genuine reset. this._lastSeenVectorClocks.set(providerKey, syncData.vectorClock); - // SPAP-10 (review follow-up): stage this file's rev as PENDING rather than - // committing it to `_lastSeenRevs` here. It is promoted to last-seen (and - // persisted) only once the caller confirms the ops were durably applied, via - // setLastServerSeq — the same ordering as the seq cursor. Committing it eagerly - // here would let a throw/crash between download and apply strand a rev ahead of - // un-applied ops, so the next poll's cheap pre-check would skip them for good. - this._pendingRevs.set(providerKey, rev); + this._stageOrDropDownloadedRev(providerKey, rev, recoveredFromBackup); // Filter ops using operation IDs instead of synthetic seq numbers. // Synthetic seq numbers based on array indices shift when the array is trimmed, @@ -1168,7 +1169,6 @@ export class FileBasedSyncAdapterService { recentOps: [], // Fresh start - no recent ops }; - // Upload snapshot (no backup - snapshots replace state completely) const uploadData = await this._encryptAndCompressHandler.compressAndEncryptData( cfg, encryptKey, @@ -1176,11 +1176,11 @@ export class FileBasedSyncAdapterService { FILE_BASED_SYNC_CONSTANTS.FILE_VERSION, ); this._assertUploadDataNotEmpty(uploadData, 'FileBasedSyncAdapter._uploadSnapshot'); - const snapshotUploadRes = await provider.uploadFile( + const snapshotUploadRes = await this._forceUploadWithBakFirst( + provider, FILE_BASED_SYNC_CONSTANTS.SYNC_FILE, + FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE, uploadData, - null, - true, ); // Reset local state @@ -1188,7 +1188,7 @@ export class FileBasedSyncAdapterService { this._localSeqCounters.set(providerKey, newSyncVersion); // SPAP-10: record the rev of the snapshot we just wrote as the last-seen rev // so the next poll can skip a redundant full download. - this._lastSeenRevs.set(providerKey, snapshotUploadRes.rev); + this._commitLastSeenRev(providerKey, snapshotUploadRes.rev); this._persistState(); OpLog.warn( @@ -1213,30 +1213,40 @@ export class FileBasedSyncAdapterService { OpLog.normal('FileBasedSyncAdapter: Deleting all sync data'); try { - // Delete main sync file — re-throw real errors (not "file not found") - try { - await provider.removeFile(FILE_BASED_SYNC_CONSTANTS.SYNC_FILE); - } catch (e) { - if (!(e instanceof RemoteFileNotFoundAPIError)) { - throw e; + // Source-of-truth files: a failed deletion MUST fail the whole operation + // (outer catch → success:false) — reporting success while user data + // remains on the remote would be a lie. The split files go FIRST so a + // partial failure can't leave a deleted tombstone next to live split data + // (an OFF client would then fresh-start a v2 file against it). "Not + // found" is fine — most users are on the single-file format. + for (const dataFile of [ + FILE_BASED_SYNC_CONSTANTS.OPS_FILE, + FILE_BASED_SYNC_CONSTANTS.STATE_FILE, + FILE_BASED_SYNC_CONSTANTS.SYNC_FILE, + ]) { + try { + await provider.removeFile(dataFile); + } catch (e) { + if (!(e instanceof RemoteFileNotFoundAPIError)) { + throw e; + } } } - // Delete backup file - try { - await provider.removeFile(FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE); - } catch (e) { - if (!(e instanceof RemoteFileNotFoundAPIError)) { - OpLog.warn('FileBasedSyncAdapter: Unexpected error deleting backup file', e); - } - } - - // Delete migration lock if exists - try { - await provider.removeFile(FILE_BASED_SYNC_CONSTANTS.MIGRATION_LOCK_FILE); - } catch (e) { - if (!(e instanceof RemoteFileNotFoundAPIError)) { - OpLog.warn('FileBasedSyncAdapter: Unexpected error deleting migration lock', e); + // Disposable artifacts: best-effort — leaving one behind degrades nothing + // (recovery only triggers on a corrupt primary, never on a missing one). + for (const artifact of [ + FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE, + FILE_BASED_SYNC_CONSTANTS.OPS_BACKUP_FILE, + FILE_BASED_SYNC_CONSTANTS.STATE_BACKUP_FILE, + FILE_BASED_SYNC_CONSTANTS.MIGRATION_LOCK_FILE, + ]) { + try { + await provider.removeFile(artifact); + } catch (e) { + if (!(e instanceof RemoteFileNotFoundAPIError)) { + OpLog.warn(`FileBasedSyncAdapter: Unexpected error deleting ${artifact}`, e); + } } } @@ -1246,7 +1256,10 @@ export class FileBasedSyncAdapterService { // SPAP-10: drop the last-seen rev so a stale value can't drive a false // "nothing new" short-circuit after the remote file has been deleted. this._lastSeenRevs.delete(providerKey); + this._pendingRevs.delete(providerKey); + this._lastSeenVectorClocks.delete(providerKey); this._clearCachedSyncData(providerKey); + this._clearCachedOpsData(providerKey); this._persistState(); return { success: true }; @@ -1260,9 +1273,6 @@ export class FileBasedSyncAdapterService { // SPAP-11: SPLIT-FILE ("SURGICAL SYNC") FORMAT (opt-in) // ═══════════════════════════════════════════════════════════════════════════ - /** State-file backup artifact name (SPAP-8-style recovery for snapshotRef mismatch). */ - private readonly _STATE_BACKUP_FILE = FILE_BASED_SYNC_CONSTANTS.STATE_FILE + '.bak'; - /** * Whether the opt-in split-file sync setting is ON. Resolved lazily via the * injector (like SnackService) so the single-file unit harness — which does not @@ -1321,19 +1331,25 @@ export class FileBasedSyncAdapterService { encryptKey: string | undefined, ): Promise<{ data: FileBasedOpsFile; rev: string }> { const response = await provider.downloadFile(FILE_BASED_SYNC_CONSTANTS.OPS_FILE); - const data = - await this._encryptAndCompressHandler.decompressAndDecryptData( - cfg, - encryptKey, - response.dataStr, - ); - if (data.version !== FILE_BASED_SYNC_CONSTANTS.SPLIT_FILE_VERSION) { - throw new SyncDataCorruptedError( - `Unsupported ops-file version: ${data.version} (expected ${FILE_BASED_SYNC_CONSTANTS.SPLIT_FILE_VERSION})`, - FILE_BASED_SYNC_CONSTANTS.OPS_FILE, - ); + try { + const data = + await this._encryptAndCompressHandler.decompressAndDecryptData( + cfg, + encryptKey, + response.dataStr, + ); + if (data.version !== FILE_BASED_SYNC_CONSTANTS.SPLIT_FILE_VERSION) { + throw new SyncDataCorruptedError( + `Unsupported ops-file version: ${data.version} (expected ${FILE_BASED_SYNC_CONSTANTS.SPLIT_FILE_VERSION})`, + FILE_BASED_SYNC_CONSTANTS.OPS_FILE, + ); + } + return { data, rev: response.rev }; + } catch (decodeErr) { + // Annotate the corrupt file's rev so the .bak recovery path can seed the + // heal cache with IT (mirrors _downloadSyncFile). + throw this._annotatePrimaryRev(decodeErr, response.rev); } - return { data, rev: response.rev }; } /** Downloads + decodes `sync-state.json` and validates its version. */ @@ -1391,20 +1407,22 @@ export class FileBasedSyncAdapterService { cfg: EncryptAndCompressCfg, encryptKey: string | undefined, ): Promise { + let current: { data: FileBasedStateFile }; try { - const current = await this._downloadStateFile(provider, cfg, encryptKey); - const backupData = await this._encryptAndCompressHandler.compressAndEncryptData( - cfg, - encryptKey, - current.data, - FILE_BASED_SYNC_CONSTANTS.SPLIT_FILE_VERSION, - ); - if (!backupData || backupData.trim().length === 0) return; - await provider.uploadFile(this._STATE_BACKUP_FILE, backupData, null, true); + current = await this._downloadStateFile(provider, cfg, encryptKey); } catch (e) { // Non-fatal — e.g. first compaction has no existing state file to back up. OpLog.normal('FileBasedSyncAdapter: state-file backup skipped (non-fatal)', e); + return; } + await this._writeBakFile( + provider, + cfg, + encryptKey, + FILE_BASED_SYNC_CONSTANTS.STATE_BACKUP_FILE, + current.data, + FILE_BASED_SYNC_CONSTANTS.SPLIT_FILE_VERSION, + ); } /** Recovers the snapshot from `sync-state.json.bak` (null if unusable). */ @@ -1413,20 +1431,14 @@ export class FileBasedSyncAdapterService { cfg: EncryptAndCompressCfg, encryptKey: string | undefined, ): Promise { - try { - const response = await provider.downloadFile(this._STATE_BACKUP_FILE); - const data = - await this._encryptAndCompressHandler.decompressAndDecryptData( - cfg, - encryptKey, - response.dataStr, - ); - if (data.version !== FILE_BASED_SYNC_CONSTANTS.SPLIT_FILE_VERSION) return null; - return data; - } catch (e) { - OpLog.warn('FileBasedSyncAdapter: state backup recovery failed', e); - return null; - } + const recovered = await this._readBakFile( + provider, + cfg, + encryptKey, + FILE_BASED_SYNC_CONSTANTS.STATE_BACKUP_FILE, + FILE_BASED_SYNC_CONSTANTS.SPLIT_FILE_VERSION, + ); + return recovered?.data ?? null; } /** True when a downloaded snapshot matches the ops file's snapshotRef. */ @@ -1495,9 +1507,10 @@ export class FileBasedSyncAdapterService { } /** - * Overwrites `sync-data.json` with a v3 split tombstone (NEVER deletes it) and - * neutralizes `sync-data.json.bak` so an old client's SPAP-8 `.bak` recovery - * cannot resurrect a v2 file and diverge. + * Neutralizes `sync-data.json.bak` and then overwrites `sync-data.json` with a + * v3 split tombstone (NEVER deletes it), so an old client's SPAP-8 `.bak` + * recovery cannot resurrect a v2 file and diverge. Order matters — see the + * inline comment. */ private async _writeTombstoneAndNeutralizeBak( provider: FileSyncProvider, @@ -1516,23 +1529,28 @@ export class FileBasedSyncAdapterService { tombstone, FILE_BASED_SYNC_CONSTANTS.SPLIT_FILE_VERSION, ); + // Neutralize the legacy .bak FIRST, then the tombstone. In the old order + // (tombstone first, .bak best-effort) a crash/failure between the two left a + // live v2 .bak next to the tombstone: an OFF client reading the tombstone + // gets SyncDataCorruptedError → SPAP-8 recovery adopts the v2 .bak → its + // next upload heals v2 back OVER the tombstone, forking the folder into two + // sync worlds. Neutralize-first is crash-safe (a crash in between leaves a + // valid v2 primary + tombstone .bak — nothing recoverable, nothing stale) + // and the failure is deliberately FATAL: better a missing tombstone (OFF + // clients are still caught by the ops-file probe in _downloadSyncFile) than + // a tombstone with a resurrectable v2 .bak beside it. + // + // Retry caveat: at the _uploadSnapshotSplit call site a throw fails the + // whole snapshot upload, which the user/caller retries. At the MIGRATION + // call site the split files are already committed before this runs, and + // migration is only re-entered while sync-ops.json is missing — so a + // failure here leaves a live v2 sync-data.json until the next snapshot + // upload rewrites the tombstone. Accepted residual (needs a transient PUT + // failure right after two successful PUTs); a periodic tombstone re-assert + // during split compaction would close it if it ever shows up in practice. + await provider.uploadFile(FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE, encoded, null, true); // Overwrite the legacy single file in place (never remove it). await provider.uploadFile(FILE_BASED_SYNC_CONSTANTS.SYNC_FILE, encoded, null, true); - // Neutralize the legacy .bak (non-fatal): a leftover v2 .bak could otherwise - // let an old client recover + re-upload v2. - try { - await provider.uploadFile( - FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE, - encoded, - null, - true, - ); - } catch (e) { - OpLog.warn( - 'FileBasedSyncAdapter: failed to neutralize legacy .bak during migration (non-fatal)', - e, - ); - } } /** @@ -1804,6 +1822,21 @@ export class FileBasedSyncAdapterService { snapshotRef, }; + // Backup-before-overwrite (SPAP-8, same as the single-file path): preserve + // the CURRENT remote ops file in .bak so an interrupted/corrupt write of the + // hot file (rewritten on every op-bearing sync — the highest torn-write + // exposure of any sync file) can be recovered on the next download. + if (opsFile) { + await this._writeBakFile( + provider, + cfg, + encryptKey, + FILE_BASED_SYNC_CONSTANTS.OPS_BACKUP_FILE, + opsFile, + FILE_BASED_SYNC_CONSTANTS.SPLIT_FILE_VERSION, + ); + } + // Commit point. const { finalRev } = await this._uploadOpsFileWithMismatchFallback( provider, @@ -1815,7 +1848,7 @@ export class FileBasedSyncAdapterService { this._clearCachedOpsData(providerKey); this._expectedSyncVersions.set(providerKey, newSyncVersion); - this._lastSeenRevs.set(providerKey, finalRev); + this._commitLastSeenRev(providerKey, finalRev); this._lastSeenVectorClocks.set(providerKey, mergedClock); this._persistState(); @@ -1879,6 +1912,7 @@ export class FileBasedSyncAdapterService { let opsFile: FileBasedOpsFile; let opsRev: string; + let recoveredFromBackup = false; try { const r = await this._downloadOpsFile(provider, cfg, encryptKey); opsFile = r.data; @@ -1890,7 +1924,30 @@ export class FileBasedSyncAdapterService { // (migration WRITES happen on the upload path). return this._tryLegacyReadOnlyDownload(provider, cfg, encryptKey, sinceSeq); } - throw e; + if (this._isRecoverableCorruption(e)) { + // Corrupt/torn sync-ops.json (the hot file, rewritten every op-bearing + // sync): recover from .bak — same SPAP-8 semantics as the single-file + // path, including seeding the heal cache with the CORRUPT primary's rev + // so this cycle's upload conditionally matches and heals it. + const recovered = await this._readBakFile( + provider, + cfg, + encryptKey, + FILE_BASED_SYNC_CONSTANTS.OPS_BACKUP_FILE, + FILE_BASED_SYNC_CONSTANTS.SPLIT_FILE_VERSION, + ); + if (!recovered) throw e; + recoveredFromBackup = true; + OpLog.warn( + 'FileBasedSyncAdapter: Primary ops file unreadable; recovered from backup', + ); + opsFile = recovered.data; + opsRev = (e as { primaryRev?: string } | null)?.primaryRev ?? recovered.rev; + this._setCachedOpsData(providerKey, opsFile, opsRev); + this._notifyRecoveredCorruptPrimaryOnce(providerKey, opsRev); + } else { + throw e; + } } // Gap detection on the ops file (mirrors the single-file logic). @@ -1901,7 +1958,12 @@ export class FileBasedSyncAdapterService { const lastSeenClock = this._lastSeenVectorClocks.get(providerKey); if (syncVersionRegressed && lastSeenClock) { const cmp = compareVectorClocks(opsFile.vectorClock, lastSeenClock); - if (cmp === 'EQUAL' || cmp === 'GREATER_THAN') { + // EQUAL only — same rationale as the single-file path above: GREATER_THAN + // proves the writer did strictly more work, NOT that this client received + // the intervening ops. A dominating client's snapshot reset compacts ops + // this client never saw into sync-state.json; suppressing the reset here + // would skip the snapshot hydration and silently diverge. + if (cmp === 'EQUAL') { versionWasReset = false; } } @@ -1924,12 +1986,7 @@ export class FileBasedSyncAdapterService { this._expectedSyncVersions.set(providerKey, opsFile.syncVersion); this._lastSeenVectorClocks.set(providerKey, opsFile.vectorClock); - // SPAP-10 crash-safety (review follow-up): stage the ops-file rev as PENDING, - // mirroring the single-file download path. It is promoted to _lastSeenRevs only - // in setLastServerSeq, after the caller has durably applied the ops. Committing - // it eagerly here would let a crash before apply strand the rev ahead of - // un-applied ops, so the next poll's precheck would skip them for good. - this._pendingRevs.set(providerKey, opsRev); + this._stageOrDropDownloadedRev(providerKey, opsRev, recoveredFromBackup); this._persistState(); const isForceFromZero = sinceSeq === 0; @@ -2052,6 +2109,11 @@ export class FileBasedSyncAdapterService { clock, schemaVersion, ); + // sync-state.json.bak is deliberately NOT refreshed here: its adoption is + // ref-validated (_loadValidatedSnapshot requires an EQUAL clock against the + // ops file's snapshotRef), so a stale — even rotated-key — state .bak is + // inert, and it must keep serving the COMPACTION crash window (state + // written, ops not yet) it was backed up for. const stateRev = await this._writeStateFile(provider, cfg, encryptKey, stateData); const opsData: FileBasedOpsFile = { @@ -2074,17 +2136,17 @@ export class FileBasedSyncAdapterService { opsEncoded, 'FileBasedSyncAdapter._uploadSnapshotSplit', ); - const opsRes = await provider.uploadFile( + const opsRes = await this._forceUploadWithBakFirst( + provider, FILE_BASED_SYNC_CONSTANTS.OPS_FILE, + FILE_BASED_SYNC_CONSTANTS.OPS_BACKUP_FILE, opsEncoded, - null, - true, ); await this._writeTombstoneAndNeutralizeBak(provider, cfg, encryptKey); this._expectedSyncVersions.set(providerKey, newSyncVersion); this._localSeqCounters.set(providerKey, newSyncVersion); - this._lastSeenRevs.set(providerKey, opsRes.rev); + this._commitLastSeenRev(providerKey, opsRes.rev); this._lastSeenVectorClocks.set(providerKey, clock); this._clearCachedOpsData(providerKey); this._persistState(); @@ -2106,53 +2168,190 @@ export class FileBasedSyncAdapterService { } /** - * Writes the CURRENT remote content to sync-data.json.bak before the primary - * file is overwritten (two-phase write). We re-encode the already-in-hand - * `currentData` from `_getCurrentSyncState()` rather than issuing another GET. + * Writes `data` to a `.bak` recovery artifact before its primary file is + * overwritten (two-phase write). * * MUST be non-fatal: a provider that cannot write the backup (e.g. no copy * support, transient failure) must still be able to sync. The backup is a * recovery artifact — losing it only degrades recoverability, never sync. * * Force-overwrite is used here because .bak is a disposable recovery artifact, - * NOT the source-of-truth sync file — the lost-update guarantee on - * sync-data.json is unaffected. + * NOT a source-of-truth sync file — the lost-update guarantee on the primary + * (conditional PUT) is unaffected. */ - private async _backupCurrentRemote( + private async _writeBakFile( provider: FileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, - currentData: FileBasedSyncData, + bakPath: string, + data: T, + fileVersion: number, ): Promise { try { const backupData = await this._encryptAndCompressHandler.compressAndEncryptData( cfg, encryptKey, - currentData, - FILE_BASED_SYNC_CONSTANTS.FILE_VERSION, + data, + fileVersion, ); if (!backupData || backupData.trim().length === 0) { OpLog.warn( - 'FileBasedSyncAdapter: Skipping backup — encoded backup data was empty', + `FileBasedSyncAdapter: Skipping ${bakPath} — encoded backup data was empty`, ); return; } - await provider.uploadFile( - FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE, - backupData, - null, - true, - ); - OpLog.normal('FileBasedSyncAdapter: Wrote backup before overwrite'); + await provider.uploadFile(bakPath, backupData, null, true); + OpLog.normal(`FileBasedSyncAdapter: Wrote ${bakPath} before overwrite`); } catch (e) { // Non-fatal by design — proceed with the primary upload regardless. OpLog.warn( - 'FileBasedSyncAdapter: Backup-before-overwrite failed (non-fatal), proceeding', + `FileBasedSyncAdapter: ${bakPath} write failed (non-fatal), proceeding`, e, ); } } + /** + * Downloads + decodes a `.bak` recovery artifact. Returns null when the backup + * is missing, undecodable, mode-mismatched, or has an unsupported version — the + * caller then surfaces its ORIGINAL corruption error. + */ + private async _readBakFile( + provider: FileSyncProvider, + cfg: EncryptAndCompressCfg, + encryptKey: string | undefined, + bakPath: string, + expectedVersion: number, + ): Promise<{ data: T; rev: string } | null> { + try { + const response = await provider.downloadFile(bakPath); + // Security: when encryption is expected, refuse a PLAINTEXT .bak. Decoding + // trusts the file's own prefix flags, so a plaintext .bak decodes even + // under a wrong/rotated key — silently suppressing the wrong-password + // dialog and letting the heal upload clobber the encrypted primary (same + // class as the E2EE-rotation revert). + if ( + cfg.isEncrypt && + !extractSyncFileStateFromPrefix(response.dataStr).isEncrypted + ) { + OpLog.warn( + `FileBasedSyncAdapter: ${bakPath} is plaintext but encryption is expected — refusing recovery`, + ); + return null; + } + const data = await this._encryptAndCompressHandler.decompressAndDecryptData( + cfg, + encryptKey, + response.dataStr, + ); + if (data.version !== expectedVersion) { + OpLog.warn( + `FileBasedSyncAdapter: ${bakPath} has unsupported version ${data.version}; cannot recover`, + ); + return null; + } + return { data, rev: response.rev }; + } catch (e) { + OpLog.warn(`FileBasedSyncAdapter: ${bakPath} recovery failed`, e); + return null; + } + } + + /** + * Force-writes the SAME payload to the `.bak` FIRST, then the primary. Used by + * snapshot uploads (force-upload / "Use Local" / E2EE re-encryption): a + * snapshot replaces the remote wholesale, so the pre-snapshot .bak must not + * survive it — a stale .bak encrypted with a ROTATED-AWAY key would otherwise + * be silently "recovered" by a still-old-key client (suppressing its + * wrong-password prompt) and heal-uploaded back over the new-key primary, + * reverting the rotation. Unlike the op-path backup, the .bak write here is + * deliberately FATAL: leaving the stale artifact behind breaks the snapshot's + * replace-everything contract, and aborting BEFORE the primary write leaves the + * remote fully consistent for a retry. (.bak-first also means a crash between + * the writes leaves a valid old primary + new .bak — nothing stale to recover.) + */ + private async _forceUploadWithBakFirst( + provider: FileSyncProvider, + primaryPath: string, + bakPath: string, + encoded: string, + ): Promise<{ rev: string }> { + await provider.uploadFile(bakPath, encoded, null, true); + return provider.uploadFile(primaryPath, encoded, null, true); + } + + /** + * Commits a freshly-written remote rev as last-seen and drops any rev still + * staged by an earlier download whose apply never completed (e.g. a conflict + * dialog aborted it) — promoting a stale staged rev later would clobber the + * fresh one and re-open the SPAP-10 precheck to a wrong "unchanged" answer. + */ + private _commitLastSeenRev(providerKey: string, rev: string): void { + this._lastSeenRevs.set(providerKey, rev); + this._pendingRevs.delete(providerKey); + } + + /** + * SPAP-10 (review follow-up): stage a downloaded file's rev as PENDING rather + * than committing it to `_lastSeenRevs`. It is promoted to last-seen (and + * persisted) only once the caller confirms the ops were durably applied, via + * setLastServerSeq — the same ordering as the seq cursor. Committing it eagerly + * would let a throw/crash between download and apply strand a rev ahead of + * un-applied ops, so the next poll's cheap pre-check would skip them for good. + * + * EXCEPTION — .bak recovery: `rev` is then the CORRUPT primary's rev (kept in + * the sync-cycle cache so this cycle's upload can heal it via a matching + * conditional overwrite). It must NEVER reach `_lastSeenRevs`: promoting it + * would make every later poll's pre-check read "unchanged" and skip the full + * download, while the upload path (which has no .bak recovery) keeps failing + * on the corrupt primary — wedging sync until another client rewrites the + * file. Dropping both entries instead forces each poll to re-download, + * re-recover, and re-seed the heal cache until the primary is actually healed. + */ + private _stageOrDropDownloadedRev( + providerKey: string, + rev: string, + recoveredFromBackup: boolean, + ): void { + if (recoveredFromBackup) { + this._pendingRevs.delete(providerKey); + this._lastSeenRevs.delete(providerKey); + } else { + this._pendingRevs.set(providerKey, rev); + } + } + + /** + * De-dups the user-facing "recovered from backup" notice per corrupt primary + * rev, so a download-only client (which cannot heal the file itself) does not + * re-toast on every sync cycle. + */ + private _notifyRecoveredCorruptPrimaryOnce( + providerKey: string, + corruptRev: string, + ): void { + if (this._lastRecoveredCorruptRev.get(providerKey) !== corruptRev) { + this._lastRecoveredCorruptRev.set(providerKey, corruptRev); + // Lazy resolve — absent in some unit harnesses (returns null → no notice). + this._injector + .get(SnackService, null) + ?.open({ msg: T.F.SYNC.S.SYNC_DATA_RECOVERED_FROM_BACKUP }); + } + } + + /** + * Annotates a decode error with the corrupt primary file's rev so the .bak + * recovery path can seed the heal cache with IT (not the .bak rev) and heal + * the primary via a matching conditional overwrite. Returns the error for + * rethrowing. + */ + private _annotatePrimaryRev(e: unknown, rev: string): unknown { + if (e && typeof e === 'object' && !('primaryRev' in e)) { + (e as { primaryRev?: string }).primaryRev = rev; + } + return e; + } + /** * Whether a download error indicates a corrupt/empty/unparseable primary file * that we should attempt to recover from the .bak artifact. Missing files are @@ -2168,43 +2367,18 @@ export class FileBasedSyncAdapterService { // decrypt/decompress stage rather than JSON parse. Without these, .bak // recovery silently no-ops for exactly the users who enable encryption or // compression — the interrupted-write case this feature targets. Safe to - // include: if the .bak is also undecodable, _recoverFromBackup returns null - // and the original error still surfaces. + // include: if the .bak is also undecodable, _readBakFile returns null + // and the original error still surfaces. DecryptError is only safe here + // because snapshot uploads refresh .bak with the same payload/key as the + // primary (see _forceUploadWithBakFirst) — a primary/.bak KEY mismatch + // cannot exist on a remote whose snapshots were all written by fixed + // clients. Mixed-fleet caveat: a pre-fix client's snapshot still leaves a + // stale-key .bak behind until its first op-bearing sync refreshes it. e instanceof DecryptError || e instanceof DecompressError ); } - /** - * Attempts to recover sync data from sync-data.json.bak. Returns null if the - * backup is missing, unreadable, or has an unsupported version. - */ - private async _recoverFromBackup( - provider: FileSyncProvider, - cfg: EncryptAndCompressCfg, - encryptKey: string | undefined, - ): Promise<{ data: FileBasedSyncData; rev: string } | null> { - try { - const response = await provider.downloadFile(FILE_BASED_SYNC_CONSTANTS.BACKUP_FILE); - const data = - await this._encryptAndCompressHandler.decompressAndDecryptData( - cfg, - encryptKey, - response.dataStr, - ); - if (data.version !== FILE_BASED_SYNC_CONSTANTS.FILE_VERSION) { - OpLog.warn( - `FileBasedSyncAdapter: Backup has unsupported version ${data.version}; cannot recover`, - ); - return null; - } - return { data, rev: response.rev }; - } catch (e) { - OpLog.warn('FileBasedSyncAdapter: Backup recovery failed', e); - return null; - } - } - /** * Downloads and decrypts the sync file. * @@ -2280,15 +2454,9 @@ export class FileBasedSyncAdapterService { if (decodeErr instanceof SplitSyncFormatDetectedError) { throw decodeErr; } - // Annotate the corrupt primary file's current rev onto the error so the - // download-time recovery path can seed the cache with IT (not the .bak rev) - // and heal sync-data.json via a matching conditional overwrite, instead of - // mismatching forever and re-recovering every cycle. We already hold the rev - // here (from the download above), so no extra request is needed. - if (decodeErr && typeof decodeErr === 'object' && !('primaryRev' in decodeErr)) { - (decodeErr as { primaryRev?: string }).primaryRev = response.rev; - } - throw decodeErr; + // We already hold the corrupt primary's rev (from the download above), so + // no extra request is needed to enable the heal-via-conditional-overwrite. + throw this._annotatePrimaryRev(decodeErr, response.rev); } // SPAP-11 (Q4): a valid v2 sync-data.json can still coexist with a diff --git a/src/app/op-log/sync/operation-log-sync.service.spec.ts b/src/app/op-log/sync/operation-log-sync.service.spec.ts index 799a727360..dde580c6bc 100644 --- a/src/app/op-log/sync/operation-log-sync.service.spec.ts +++ b/src/app/op-log/sync/operation-log-sync.service.spec.ts @@ -2924,6 +2924,56 @@ describe('OperationLogSyncService', () => { expect(conflictError.unsyncedCount).toBe(0); // No unsynced ops expect(conflictError.remoteSnapshotState).toEqual(remoteSnapshot); expect(conflictError.remoteVectorClock).toEqual(remoteVectorClock); + // Fresh client has no prior sync — the last-synced baseline must be + // explicitly null (SPAP-7), not undefined or a clock. + expect(conflictError.lastSyncedVectorClock).toBeNull(); + } + }); + + it('passes a null last-synced clock when fresh client with store data receives remote ops (no snapshot)', async () => { + // Covers the second fresh-client throw site: incremental ops without a + // snapshotState. The error must carry an explicit null baseline (SPAP-7). + stateSnapshotServiceSpy.getStateSnapshot.and.returnValue({ + task: { ids: ['task-1'] }, + project: { ids: [INBOX_PROJECT.id] }, + tag: { ids: [TODAY_TAG.id] }, + note: { ids: [] }, + } as any); + + downloadServiceSpy.downloadRemoteOps.and.resolveTo({ + newOps: [ + { + id: 'remote-op-1', + clientId: 'clientB', + actionType: 'test' as ActionType, + opType: OpType.Update, + entityType: 'TASK', + entityId: 'task-1', + payload: {}, + vectorClock: { clientB: 1 }, + timestamp: Date.now(), + schemaVersion: 1, + }, + ], + needsFullStateUpload: false, + success: true, + providerMode: 'superSyncOps', + failedFileCount: 0, + latestServerSeq: 1, + }); + + const mockProvider = { + supportsOperationSync: true, + } as any; + + try { + await service.downloadRemoteOps(mockProvider); + fail('Expected LocalDataConflictError to be thrown'); + } catch (error) { + expect(error).toBeInstanceOf(LocalDataConflictError); + const conflictError = error as LocalDataConflictError; + expect(conflictError.unsyncedCount).toBe(0); + expect(conflictError.lastSyncedVectorClock).toBeNull(); } });