diff --git a/src/app/features/config/form-cfgs/sync-form.const.ts b/src/app/features/config/form-cfgs/sync-form.const.ts index 6b97bce35d..46a34bc2b0 100644 --- a/src/app/features/config/form-cfgs/sync-form.const.ts +++ b/src/app/features/config/form-cfgs/sync-form.const.ts @@ -2,6 +2,7 @@ import { T } from '../../../t.const'; import { ConfigFormSection, SyncConfig } from '../global-config.model'; import { SyncProviderId } from '../../../op-log/sync-providers/provider.const'; +import { notifyFileProviderTargetChanged } from '../../../op-log/sync-providers/provider-manager.service'; import { IS_ANDROID_WEB_VIEW } from '../../../util/is-android-web-view'; import { IS_ELECTRON } from '../../../app.constants'; import { @@ -235,7 +236,16 @@ export const SYNC_FORM: ConfigFormSection = { const localProvider = providers.find( (p) => p.id === SyncProviderId.LocalFile, ); - return (localProvider as LocalFileSyncPicker | undefined)?.pickDirectory(); + const pickedPath = await ( + localProvider as LocalFileSyncPicker | undefined + )?.pickDirectory(); + // The picker persists the folder main-side (post-#8228) without + // going through setProviderConfig, so invalidate file-provider + // target state here (Task 2). Only on an actual pick, not cancel. + if (pickedPath) { + notifyFileProviderTargetChanged(); + } + return pickedPath; }, }, }, @@ -267,7 +277,16 @@ export const SYNC_FORM: ConfigFormSection = { const localProvider = providers.find( (p) => p.id === SyncProviderId.LocalFile, ); - return (localProvider as LocalFileSyncPicker | undefined)?.setupSaf(); + const safUri = await ( + localProvider as LocalFileSyncPicker | undefined + )?.setupSaf(); + // setupSaf writes safFolderUri to privateCfg directly (outside + // setProviderConfig), so invalidate file-provider target state + // here (Task 2). setupSaf throws on cancel, so a value = success. + if (safUri) { + notifyFileProviderTargetChanged(); + } + return safUri; }, }, expressions: { diff --git a/src/app/imex/sync/dialog-sync-cfg/dialog-sync-cfg.component.spec.ts b/src/app/imex/sync/dialog-sync-cfg/dialog-sync-cfg.component.spec.ts index d342bb5216..dd15d35bec 100644 --- a/src/app/imex/sync/dialog-sync-cfg/dialog-sync-cfg.component.spec.ts +++ b/src/app/imex/sync/dialog-sync-cfg/dialog-sync-cfg.component.spec.ts @@ -61,6 +61,7 @@ describe('DialogSyncCfgComponent', () => { mockProviderManager = jasmine.createSpyObj('SyncProviderManager', [ 'getProviderById', + 'notifyProviderTargetChanged', ]); mockGlobalConfigService = jasmine.createSpyObj('GlobalConfigService', [], { @@ -799,4 +800,62 @@ describe('DialogSyncCfgComponent', () => { expect(mockDialogRef.close).toHaveBeenCalled(); }); }); + + describe('save() — OneDrive pre-auth cfg write (Task 2 target isolation)', () => { + const storedCfg = { + clientId: 'cid', + tenantId: 'common', + syncFolderPath: 'Super Productivity', + accessToken: 'at', + refreshToken: 'rt', + tokenExpiresAt: 1, + }; + let setComplete: jasmine.Spy; + + const arrangeOneDrive = (formOneDriveCfg: Record): void => { + setComplete = jasmine.createSpy('setComplete').and.resolveTo(undefined); + mockProviderManager.getProviderById.and.resolveTo({ + id: SyncProviderId.OneDrive, + isReady: jasmine.createSpy('isReady').and.resolveTo(true), + privateCfg: { + load: jasmine.createSpy('load').and.resolveTo({ ...storedCfg }), + setComplete, + }, + } as any); + mockSyncWrapperService.configuredAuthForSyncProviderIfNecessary.and.resolveTo({ + isSuccess: true, + } as any); + (component as any)._tmpUpdatedCfg = { + ...(component as any)._tmpUpdatedCfg, + syncProvider: SyncProviderId.OneDrive, + isEnabled: true, + oneDrive: formOneDriveCfg, + }; + }; + + it('signals a target change when the sync folder moves', async () => { + // This write bypasses setProviderConfig(), so by the time the save's later + // setProviderConfig runs, load() already returns the NEW folder and its + // diff is a no-op. Without the explicit signal the previous folder's seq + // cursor/revs/clocks stay keyed under 'OneDrive' and get reused against the + // new folder — the cross-target data loss Task 2 exists to prevent. + arrangeOneDrive({ ...storedCfg, syncFolderPath: 'Elsewhere' }); + + await component.save(); + + expect(setComplete).toHaveBeenCalled(); + expect(mockProviderManager.notifyProviderTargetChanged).toHaveBeenCalledTimes(1); + }); + + it('does NOT signal a target change when nothing moved', async () => { + // This runs on EVERY OneDrive save. Signalling unconditionally would wipe + // the seq cursor and dead-end the next sync in a spurious conflict dialog. + arrangeOneDrive({ ...storedCfg }); + + await component.save(); + + expect(setComplete).toHaveBeenCalled(); + expect(mockProviderManager.notifyProviderTargetChanged).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/app/imex/sync/dialog-sync-cfg/dialog-sync-cfg.component.ts b/src/app/imex/sync/dialog-sync-cfg/dialog-sync-cfg.component.ts index 2b7601cad8..4027867c40 100644 --- a/src/app/imex/sync/dialog-sync-cfg/dialog-sync-cfg.component.ts +++ b/src/app/imex/sync/dialog-sync-cfg/dialog-sync-cfg.component.ts @@ -38,6 +38,7 @@ import { toSyncProviderId } from '../../../op-log/sync-exports'; import { isFileBasedProviderId } from '../../../op-log/sync/operation-sync.util'; import { SyncLog } from '../../../core/log'; import { SyncProviderManager } from '../../../op-log/sync-providers/provider-manager.service'; +import { isSyncTargetChanged } from '../../../op-log/sync-providers/sync-target-identity.util'; import { GlobalConfigService } from '../../../features/config/global-config.service'; import { isOnline } from '../../../util/is-online'; @@ -611,6 +612,17 @@ export class DialogSyncCfgComponent implements AfterViewInit { tokenExpiresAt: identityChanged ? 0 : existingCfg?.tokenExpiresAt, }; await oneDriveProvider.privateCfg.setComplete(mergedCfg); + + // This write bypasses setProviderConfig() (the OAuth flow below reads + // clientId/tenantId straight from the store), so the save's later + // setProviderConfig sees THIS cfg on both sides of its diff and can't + // infer the move — `syncFolderPath` would change while the old folder's + // cursor stays keyed under 'OneDrive'. Raise the signal here instead. + // Gated because this runs on EVERY save: an unconditional notify would + // wipe the seq cursor on a no-op save. See `providerConfigChanged$`. + if (isSyncTargetChanged(existingCfg, mergedCfg)) { + this._providerManager.notifyProviderTargetChanged(); + } } } diff --git a/src/app/imex/sync/sync-wrapper.service.spec.ts b/src/app/imex/sync/sync-wrapper.service.spec.ts index ef7ae48906..a65f7b9a41 100644 --- a/src/app/imex/sync/sync-wrapper.service.spec.ts +++ b/src/app/imex/sync/sync-wrapper.service.spec.ts @@ -43,6 +43,7 @@ import { ForceUploadPendingOpsError, HttpNotOkAPIError, IncompleteRemoteOperationsError, + FileSyncTargetChangedError, } from '../../op-log/core/errors/sync-errors'; import { DialogEnterEncryptionPasswordComponent } from './dialog-enter-encryption-password/dialog-enter-encryption-password.component'; import { MAX_LWW_REUPLOAD_RETRIES } from '../../op-log/core/operation-log.const'; @@ -1238,6 +1239,25 @@ describe('SyncWrapperService', () => { ); }); + it('should handle FileSyncTargetChangedError as a silent self-healing re-sync', async () => { + // The file target changed mid-upload; the guarded write was abandoned. + // This is benign — the next sync re-reads/re-uploads against the current + // target — so it must report UNKNOWN_OR_CHANGED with no error snackbar. + mockSyncService.uploadPendingOps.and.returnValue( + Promise.reject(new FileSyncTargetChangedError(0, 1)), + ); + + const result = await service.sync(true); + + expect(result).toBe('HANDLED_ERROR'); + expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith( + 'UNKNOWN_OR_CHANGED', + ); + expect(mockSnackService.open).not.toHaveBeenCalledWith( + jasmine.objectContaining({ type: 'ERROR' }), + ); + }); + it('should handle NetworkUnavailableSPError silently for automatic syncs', async () => { // The auto-sync fired on Android resume hits a not-yet-ready network and // throws this transient error; the next cycle retries, so no snack should @@ -1855,6 +1875,32 @@ describe('SyncWrapperService', () => { ); }); + it('self-heals silently when the target changes during USE_LOCAL force upload', async () => { + const conflictError = new LocalDataConflictError( + 2, + { tasks: [] }, + { clientB: 3 }, + ); + mockSyncService.downloadRemoteOps.and.returnValue(Promise.reject(conflictError)); + mockMatDialog.open.and.returnValue({ + afterClosed: () => of('USE_LOCAL'), + } as any); + mockSyncService.forceUploadLocalState = jasmine + .createSpy('forceUploadLocalState') + .and.rejectWith(new FileSyncTargetChangedError(0, 1)); + + const result = await service.sync(); + + expect(result).toBe('HANDLED_ERROR'); + expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith( + 'UNKNOWN_OR_CHANGED', + ); + // Benign target switch → no error snackbar. + expect(mockSnackService.open).not.toHaveBeenCalledWith( + jasmine.objectContaining({ type: 'ERROR' }), + ); + }); + it('should translate a typed force-upload failure during conflict resolution', async () => { const conflictError = new LocalDataConflictError( 2, diff --git a/src/app/imex/sync/sync-wrapper.service.ts b/src/app/imex/sync/sync-wrapper.service.ts index d5294a6ed6..466292a167 100644 --- a/src/app/imex/sync/sync-wrapper.service.ts +++ b/src/app/imex/sync/sync-wrapper.service.ts @@ -26,6 +26,7 @@ import { UploadRevToMatchMismatchAPIError, ForceUploadFailedError, ForceUploadPendingOpsError, + FileSyncTargetChangedError, } from '../../op-log/core/errors/sync-errors'; import { MAX_LWW_REUPLOAD_RETRIES } from '../../op-log/core/operation-log.const'; import { SyncConfig } from '../../features/config/global-config.model'; @@ -954,6 +955,17 @@ export class SyncWrapperService { ); this._providerManager.setSyncStatus('UNKNOWN_OR_CHANGED'); return 'HANDLED_ERROR'; + } else if (error instanceof FileSyncTargetChangedError) { + // The file sync target (provider/account/folder) changed while this + // upload was in flight; the guarded write was abandoned before it could + // land the previous target's data on the new one — self-healing. Do not + // show an error snackbar; UNKNOWN_OR_CHANGED triggers the next sync, + // which re-reads and re-uploads against the current target from zero. + SyncLog.log( + 'SyncWrapperService: Sync target changed mid-operation, will re-sync against the current target', + ); + this._providerManager.setSyncStatus('UNKNOWN_OR_CHANGED'); + return 'HANDLED_ERROR'; } else if (error instanceof OperationIntegrityError) { // A decrypted op's unauthenticated metadata contradicted its authenticated // payload, or a plaintext op arrived while encryption is mandatory @@ -1079,11 +1091,20 @@ export class SyncWrapperService { ); SyncLog.log('SyncWrapperService: Force upload complete'); } catch (error) { - // GHSA-9544-hjjr-fg8h: a keyless-but-encryption-enabled provider makes - // force upload refuse to send plaintext. Route to the enter-password - // recovery dialog like the main sync path, not a dead-end error snack — - // otherwise "force overwrite" (offered as the lost-key recovery) loops. - if (error instanceof EncryptNoPasswordError) { + if (error instanceof FileSyncTargetChangedError) { + // The file target switched during the force upload; the guarded write + // was abandoned before it could hit the new target. Self-healing like + // the main sync path — report UNKNOWN_OR_CHANGED (no error snack) so + // the next sync re-reads/re-uploads against the current target. + SyncLog.log( + 'SyncWrapperService: target changed mid-force-upload, will re-sync against the current target', + ); + this._providerManager.setSyncStatus('UNKNOWN_OR_CHANGED'); + } else if (error instanceof EncryptNoPasswordError) { + // GHSA-9544-hjjr-fg8h: a keyless-but-encryption-enabled provider makes + // force upload refuse to send plaintext. Route to the enter-password + // recovery dialog like the main sync path, not a dead-end error snack — + // otherwise "force overwrite" (offered as the lost-key recovery) loops. this._handleMissingPasswordDialog(); } else { SyncLog.err('SyncWrapperService: Force upload failed:', error); @@ -1404,6 +1425,16 @@ export class SyncWrapperService { return 'HANDLED_ERROR'; } } catch (resolutionError) { + if (resolutionError instanceof FileSyncTargetChangedError) { + // Target switched during USE_LOCAL force upload; the guarded write was + // abandoned. Self-heal silently (UNKNOWN_OR_CHANGED) like the main sync + // path instead of a dead-end error snack. + SyncLog.log( + 'SyncWrapperService: target changed mid-conflict-resolution, will re-sync against the current target', + ); + this._providerManager.setSyncStatus('UNKNOWN_OR_CHANGED'); + return 'HANDLED_ERROR'; + } // GHSA-9544-hjjr-fg8h: USE_LOCAL force-uploads, which refuses to send // plaintext when the key is missing. Route to the enter-password recovery // dialog instead of a dead-end error snack (mirrors the main sync path). diff --git a/src/app/op-log/core/errors/sync-errors.ts b/src/app/op-log/core/errors/sync-errors.ts index faf06e65aa..6c7bfae08e 100644 --- a/src/app/op-log/core/errors/sync-errors.ts +++ b/src/app/op-log/core/errors/sync-errors.ts @@ -122,6 +122,25 @@ export class ForceUploadPendingOpsError extends Error { override name = 'ForceUploadPendingOpsError'; } +/** + * The file-sync target changed (provider switch, account switch behind the same + * provider id, or an identity-affecting config/folder change) while a file + * upload was in flight — detected by a bumped adapter target generation before a + * remote write. The in-flight write carries the previous target's merged data, + * so it is abandoned rather than committed to the new target. The next sync + * re-reads and re-uploads against the current target from zero. Transient by + * design; not a corruption. (Task 2, docs/plans/2026-07-13-sync-simplification-plan.md.) + */ +export class FileSyncTargetChangedError extends Error { + override name = 'FileSyncTargetChangedError'; + + constructor(capturedGeneration: number, currentGeneration: number) { + super( + `File sync target changed mid-operation (generation ${capturedGeneration} → ${currentGeneration}); write abandoned.`, + ); + } +} + /** * A deferred action can never be persisted (invalid entity identifiers or an * invalid operation payload) — a deterministic condition, not a transient 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 bbb277572d..06c78cbb9d 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 @@ -14,6 +14,7 @@ import { } from './file-based-sync.types'; import { EncryptNoPasswordError, + FileSyncTargetChangedError, InvalidDataSPError, RemoteFileNotFoundAPIError, SplitSyncFormatDetectedError, @@ -232,6 +233,104 @@ describe('FileBasedSyncAdapterService', () => { }); }); + describe('in-flight target guard (Task 2)', () => { + it('aborts the upload without writing when the target changes mid-operation', async () => { + // The first download happens inside the upload (to read current state). + // Simulate a concurrent provider/account/folder switch during it, then + // report the file as absent so the upload would otherwise create it. + mockProvider.downloadFile.and.callFake(async () => { + service.invalidateAllTargets(); // bumps the target generation mid-op + throw new RemoteFileNotFoundAPIError('sync-data.json'); + }); + mockProvider.uploadFile.and.returnValue(Promise.resolve({ rev: 'rev-1' })); + + await expectAsync( + adapter.uploadOps([createMockSyncOp()], 'client1'), + ).toBeRejectedWithError(FileSyncTargetChangedError); + + // The critical assertion: no write reached the (now different) target. + expect(mockProvider.uploadFile).not.toHaveBeenCalled(); + }); + + it('lets a normal upload write when the generation is stable', async () => { + mockProvider.downloadFile.and.throwError( + new RemoteFileNotFoundAPIError('sync-data.json'), + ); + mockProvider.uploadFile.and.returnValue(Promise.resolve({ rev: 'rev-1' })); + + await adapter.uploadOps([createMockSyncOp()], 'client1'); + + expect(mockProvider.uploadFile).toHaveBeenCalled(); + }); + + it('aborts a download before committing its baseline when the target changes mid-download', async () => { + // The download reads target A; a switch during that read must not let the + // stale baseline (sync-version/clock/rev) or its seq cursor be committed + // under the shared provider id — otherwise the next sync skips the new + // target's ops from a stale cursor (data loss). + const syncData = createMockSyncData({ syncVersion: 5, vectorClock: { c1: 5 } }); + mockProvider.downloadFile.and.callFake(async () => { + service.invalidateAllTargets(); // target switch mid-download + return { dataStr: addPrefix(syncData), rev: 'rev-A' }; + }); + + await expectAsync(adapter.downloadOps(0)).toBeRejectedWithError( + FileSyncTargetChangedError, + ); + + // Baseline was not committed: the seq cursor stayed at zero. + expect(await adapter.getLastServerSeq()).toBe(0); + }); + + it('guards removeFile as well as uploadFile, and passes reads through', async () => { + const guarded = service['_withTargetGuard']( + mockProvider, + service['_targetGeneration'], + ); + + // Reads always pass through. + await guarded.downloadFile('any'); + expect(mockProvider.downloadFile).toHaveBeenCalled(); + + // A target change after capture makes both writes throw. + service.invalidateAllTargets(); + await expectAsync(guarded.uploadFile('p', 'd', null, true)).toBeRejectedWithError( + FileSyncTargetChangedError, + ); + await expectAsync(guarded.removeFile('p')).toBeRejectedWithError( + FileSyncTargetChangedError, + ); + expect(mockProvider.uploadFile).not.toHaveBeenCalled(); + expect(mockProvider.removeFile).not.toHaveBeenCalled(); + }); + + it('aborts a snapshot upload without writing when the target changes mid-operation', async () => { + // uploadSnapshot is the second write entry point (initial/recovery/ + // migration + REPAIR). Loading archive data happens after the operation + // captures its generation but before the snapshot write, so bump the + // generation there to simulate a concurrent target switch. + mockArchiveDbAdapter.loadArchiveYoung.and.callFake(async () => { + service.invalidateAllTargets(); + return mockArchiveYoung; + }); + mockProvider.uploadFile.and.returnValue(Promise.resolve({ rev: 'rev-1' })); + + await expectAsync( + adapter.uploadSnapshot( + { tasks: [] }, + 'client1', + 'initial', + { client1: 1 }, + 2, + undefined, // isPayloadEncrypted + 'op-1', // opId + ), + ).toBeRejectedWithError(FileSyncTargetChangedError); + + expect(mockProvider.uploadFile).not.toHaveBeenCalled(); + }); + }); + describe('uploadOps', () => { it('should return empty results when no ops to upload and file exists', async () => { // Mock that a sync file already exists @@ -1336,6 +1435,36 @@ describe('FileBasedSyncAdapterService', () => { }); }); + describe('invalidateAllTargets', () => { + it('clears target-scoped state so the next sync full-reads the current target', async () => { + // Establish per-target state the way a normal sync cycle would. + await adapter.setLastServerSeq(50); + expect(await adapter.getLastServerSeq()).toBe(50); + + // A real target move (provider switch, account switch behind the same + // provider id, or a folder/URL change) must invalidate the + // provider-id-keyed state, or it is reused against the new target and can + // read/write one target's data against another. + service.invalidateAllTargets(); + + // NOTE: resetting the cursor to 0 is correct ONLY for a real target move. + // On a save that left the target put, a 0 cursor makes the next download + // return a snapshotState (isForceFromZero), which for a client holding + // unsynced ops dead-ends in a binary conflict dialog — see the + // latestSeq/snapshotState suite below. Hence invalidateAllTargets() rides + // providerTargetChanged$, never providerConfigChanged$; that wiring is + // covered in wrapped-provider.service.spec.ts. + expect(await adapter.getLastServerSeq()).toBe(0); + }); + + it('advances the target generation on each invalidation', () => { + const before = service['_targetGeneration']; + service.invalidateAllTargets(); + service.invalidateAllTargets(); + expect(service['_targetGeneration']).toBe(before + 2); + }); + }); + describe('latestSeq and snapshotState behavior', () => { it('should return latestSeq based on syncVersion (not recentOps.length) to prevent repeated fresh downloads', async () => { // This test ensures that after a snapshot upload (where recentOps is empty), @@ -3743,6 +3872,53 @@ describe('FileBasedSyncAdapterService', () => { expect(finalizedMarker).toBeDefined(); }); + it('(e2c) aborts a pending split migration resume without writing when the target changes mid-download', async () => { + // The split-format DOWNLOAD path resumes a crashed migration by writing + // remote files. Task 2's in-flight guard must cover it too: a target + // switch during the download must abort those writes. + const clock = { client1: 7 }; + const legacy = createMockSyncData({ + syncVersion: 7, + vectorClock: clock, + state: { tasks: ['legacy'] }, + }); + const pendingOps = makeOpsFile({ + syncVersion: 7, + vectorClock: clock, + snapshotRef: { syncVersion: 7, vectorClock: clock }, + migration: { status: 'pending', legacyRev: `${C.SYNC_FILE}-rev` }, + }); + const map: Record = { + [C.SYNC_FILE]: addPrefix(legacy, 2), + [C.OPS_FILE]: addPrefix(pendingOps, 3), + [C.STATE_FILE]: addPrefix( + makeStateFile({ + syncVersion: 7, + vectorClock: clock, + state: { tasks: ['legacy'] }, + }), + 3, + ), + }; + // Simulate a concurrent target switch during the download (before the + // migration-resume writes) by bumping the generation on the first read. + let bumped = false; + mockProvider.downloadFile.and.callFake(async (path: string) => { + if (!bumped) { + bumped = true; + service.invalidateAllTargets(); + } + if (path in map) return { dataStr: map[path], rev: `${path}-rev` }; + throw new RemoteFileNotFoundAPIError(path); + }); + + await expectAsync(adapter.downloadOps(0, 'client2')).toBeRejectedWithError( + FileSyncTargetChangedError, + ); + // No migration-resume write reached the (now switched) target. + expect(mockProvider.uploadFile).not.toHaveBeenCalled(); + }); + it('(e3) retries migration from a newer legacy revision instead of tombstoning over it', async () => { const legacyV7 = createMockSyncData({ syncVersion: 7, 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 18b0033955..c4b1e93890 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 @@ -34,6 +34,7 @@ import { OpLog } from '../../../core/log'; import { DecompressError, DecryptError, + FileSyncTargetChangedError, InvalidDataSPError, JsonParseError, LegacySyncFormatDetectedError, @@ -84,6 +85,22 @@ import { * * @see FileBasedSyncData for the file schema */ +declare const GUARDED_PROVIDER_BRAND: unique symbol; + +/** + * A file provider whose `uploadFile`/`removeFile` are wrapped by + * `_withTargetGuard` — a write aborts if the target changed mid-operation. Every + * write-path helper takes this branded type instead of the raw provider, so the + * compiler rejects passing an unguarded provider to a write path: the in-flight + * guard invariant is enforced at compile time, not by call-graph convention (a + * future entry point or helper that forgets the guard is a build error, not a + * silent cross-target write). Only the raw entry points `createAdapter` and the + * intentionally-unguarded `_deleteAllData` keep `FileSyncProvider`. (Task 2.) + */ +type GuardedFileSyncProvider = FileSyncProvider & { + readonly [GUARDED_PROVIDER_BRAND]: true; +}; + @Injectable({ providedIn: 'root' }) export class FileBasedSyncAdapterService { private _encryptAndCompressHandler = new EncryptAndCompressHandlerService(); @@ -100,6 +117,18 @@ export class FileBasedSyncAdapterService { */ private readonly _MAX_UPLOAD_RETRIES = 2; + /** + * Monotonic in-tab target-transition generation. Bumped by + * `invalidateAllTargets()` on any user-authoritative provider/target/ + * configuration change. Captured at each remote-operation boundary + * (`_uploadOps`/`_uploadSnapshot`/`_downloadOps`) and checked by + * `_withTargetGuard` before every write, so an in-flight operation refuses a + * remote side effect against a target that is no longer current. Not + * persisted (a fresh tab starts clean; there is nothing in-flight to guard). + * (Task 2, docs/plans/2026-07-13-sync-simplification-plan.md.) + */ + private _targetGeneration = 0; + /** Expected sync version for optimistic locking, keyed by provider+user */ private _expectedSyncVersions = new Map(); @@ -322,6 +351,173 @@ export class FileBasedSyncAdapterService { this._syncCycleCache.delete(providerKey); } + /** + * Single source of truth for the target-scoped state maps — everything keyed + * by provider id that belongs to one remote target and must not survive a + * target transition. Both `_resetTargetState` (single key, used by + * `deleteAllData`) and `invalidateAllTargets` (all keys) iterate this list, so + * a new per-target map only has to be added here. The two within-cycle caches + * are included so a subsequent sync starts clean. + * + * Note: clearing these maps is not by itself what prevents a cross-target + * write. A download already in flight when the switch fires can repopulate a + * cache (a read path — `_setCachedSyncData` inside `_downloadOps`), and a + * switch landing entirely between the download and the upload is caught by + * neither per-operation guard. What actually blocks committing one target's + * data to another is the in-flight generation guard (`_withTargetGuard`) plus + * the conditional-write rev check (`revToMatch`): a stale-rev write fails + * against a populated new target and self-heals on the next sync. + */ + private get _targetScopedMaps(): Map[] { + return [ + this._expectedSyncVersions, + this._pendingExpectedSyncVersions, + this._lastSeenVectorClocks, + this._pendingVectorClocks, + this._localSeqCounters, + // Included deliberately: without it a re-used provider key after a target + // switch could suppress a genuine corruption notice for the NEW target + // based on the OLD target's corrupt rev. + this._lastRecoveredCorruptRev, + // SPAP-10: dropping the last-seen rev prevents a stale value driving a + // false "nothing new" short-circuit against a different target. + this._lastSeenRevs, + this._pendingRevs, + this._syncCycleCache, + this._splitOpsCache, + ] as Map[]; + } + + /** + * Clears every target-scoped in-memory entry for one provider key. Shared by + * `deleteAllData` and per-key resets. Callers persist afterwards. + */ + private _resetTargetState(providerKey: string): void { + for (const map of this._targetScopedMaps) { + map.delete(providerKey); + } + } + + /** + * Invalidate all cached target-scoped state after a user-authoritative + * provider/target/configuration change. The adapter is keyed only by provider + * id, so a provider switch, an account switch behind the same provider id, or + * an identity-affecting configuration save would otherwise reuse the previous + * target's sync version, rev, vector clock, and within-cycle caches — reading + * or writing one target's data against another. Clearing every key forces the + * next sync to discover and full-read the current target from zero. Bumps the + * target generation so a later in-flight guard can detect the transition. + * + * CANONICAL WARNING — only call this when the target ACTUALLY moved + * (`providerConfigChanged$`'s `isTargetChanged`, via `isSyncTargetChanged`), + * never on every config save. This drops `_localSeqCounters`, and a cursor back + * at 0 makes the next download return a `snapshotState` (`isForceFromZero`); + * for a client holding unsynced ops that classifies CONCURRENT and, with + * `AUTO_MERGE_CONCURRENT_SNAPSHOT` false, dead-ends in a binary conflict dialog + * whose either answer discards data. (The same hazard is documented from the + * other direction at the `latestSeq` computation in `_downloadOps`.) A real + * target move has no cursor worth keeping, so the reset is correct there and + * only there. + * + * Not wired to machine-only writes for an UNCHANGED account: OAuth token + * refresh goes through the provider credential store directly (never + * `setProviderConfig`), and content-only saves — encryption key rotation, the + * `isEncryptionEnabled` backfill — are filtered out by `isSyncTargetChanged`. + */ + invalidateAllTargets(): void { + this._loadPersistedState(); + this._targetGeneration++; + for (const map of this._targetScopedMaps) { + map.clear(); + } + this._persistState(); + OpLog.normal( + 'FileBasedSyncAdapter: target state invalidated after configuration change', + ); + } + + /** + * Wraps a provider so every remote write (`uploadFile`, `removeFile`) first + * asserts the target generation has not moved since `capturedGeneration`. + * A config/account/folder change mid-upload bumps the generation (via + * `invalidateAllTargets()`) and the same provider object then reads the new + * target's config, so an in-flight write of the previous target's merged data + * would land on the new target. The guard aborts before that write with + * `FileSyncTargetChangedError`; the next sync re-reads the current target. + * + * Reads (`downloadFile`, `getFileRev`) pass through — reading the wrong target + * cannot corrupt it, and the subsequent write is what the guard blocks. Writes + * are checked individually rather than once per operation: this narrows (does + * not eliminate) the check→write race, and a mid-atomic-pair abort strands the + * write on the *previous* target exactly as a crash would, which the read path + * already tolerates. Capture the generation once at the operation boundary and + * thread this wrapper to every write site. (Task 2.) + */ + private _withTargetGuard( + rawProvider: FileSyncProvider, + capturedGeneration: number, + ): GuardedFileSyncProvider { + const assertUnchanged = (): void => { + if (this._targetGeneration !== capturedGeneration) { + throw new FileSyncTargetChangedError(capturedGeneration, this._targetGeneration); + } + }; + // The Proxy is structurally a FileSyncProvider; the brand is a phantom type + // with no runtime property, so cast through unknown. + return new Proxy(rawProvider, { + get: (target, prop, receiver) => { + if (prop === 'uploadFile' || prop === 'removeFile') { + const writeFn = Reflect.get(target, prop, receiver) as ( + ...args: unknown[] + ) => Promise; + // Async wrapper so a guard rejection is a promise rejection, matching + // the real (Promise-returning) methods rather than a synchronous throw. + return async (...args: unknown[]): Promise => { + assertUnchanged(); + return writeFn.apply(target, args); + }; + } + const value = Reflect.get(target, prop, receiver); + return typeof value === 'function' ? value.bind(target) : value; + }, + }) as unknown as GuardedFileSyncProvider; + } + + /** + * Aborts an in-flight DOWNLOAD before it commits a remote baseline if the + * target changed since the download started. Writes are already covered by + * `_withTargetGuard`, but a download READS target A, and if the target then + * switches (config/account/folder), staging A's baseline (sync version, + * vector clock, rev) and letting the caller advance the seq cursor under the + * shared provider id is silent data loss on the NEXT sync. The mechanism is + * suppressed gap detection, not skipped ops: both download paths return every + * op in the file and let the caller's `appliedOpIds` dedup filter, but a + * stale-HIGH `sinceSeq` makes `partialTrimGap` (`oldestOpSyncVersion > + * sinceSeq + 1`) false while a cleared `_expectedSyncVersions` makes + * `versionWasReset` false — so `needsGapDetection` stays false and the new + * target's SNAPSHOT is never loaded. Any history the new target compacted below + * that snapshot is then silently missing. Dropping the target-scoped state and + * aborting forces the next sync to re-read the current target from zero; + * `SyncWrapperService` maps the error to a silent self-heal. + * + * This closes the dominant switch-during-download window. A switch after a + * successful download — during op apply, or between an upload's write and its + * own `setLastServerSeq` cursor commit — is a narrower residual that only a + * per-cycle session-generation captured in the orchestrator could fully close; + * within the current per-operation model the write guard plus this download + * abort cover the realistic exposure. (Task 2.) + */ + private _abortDownloadIfTargetChanged( + capturedGeneration: number, + providerKey: string, + ): void { + if (this._targetGeneration !== capturedGeneration) { + this._resetTargetState(providerKey); + this._persistState(); + throw new FileSyncTargetChangedError(capturedGeneration, this._targetGeneration); + } + } + /** * Creates an OperationSyncCapable adapter for a file-based provider. * @@ -455,7 +651,7 @@ export class FileBasedSyncAdapterService { * Gets the current sync state from cache or by downloading. */ private async _getCurrentSyncState( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, providerKey: string, @@ -592,7 +788,7 @@ export class FileBasedSyncAdapterService { * snapshot whose state never saw those ops. */ private async _uploadWithMismatchFallback( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, newData: FileBasedSyncData, @@ -670,7 +866,7 @@ export class FileBasedSyncAdapterService { } private async _uploadOps( - provider: FileSyncProvider, + rawProvider: FileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, ops: SyncOperation[], @@ -678,6 +874,12 @@ export class FileBasedSyncAdapterService { lastKnownServerSeq?: number, localStateSnapshot?: unknown, ): Promise { + // Capture the target generation at the operation boundary and thread a + // write-guarded provider through the whole upload (single-file, split, + // REPAIR, and backup paths all receive it), so a mid-operation target switch + // aborts before any write instead of committing this target's merged data to + // the next one. (Task 2.) + const provider = this._withTargetGuard(rawProvider, this._targetGeneration); const providerKey = this._getProviderKey(provider); // SPAP-11: split-file ("Surgical sync") path is fully separate and only @@ -790,13 +992,21 @@ export class FileBasedSyncAdapterService { // ═══════════════════════════════════════════════════════════════════════════ private async _downloadOps( - provider: FileSyncProvider, + rawProvider: FileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, sinceSeq: number, excludeClient?: string, limit: number = 500, ): Promise { + // Download is read-only for the caller, but the split-format path resumes a + // crashed split migration by WRITING remote files (state/tombstone/marker) + // via _resumePendingSplitMigration. Guard those writes with the same + // generation captured at the download boundary, so a mid-download target + // switch aborts the resume before it lands the old target's migration on the + // new one. Reads (downloadFile/getFileRev) pass through unaffected. (Task 2.) + const capturedGeneration = this._targetGeneration; + const provider = this._withTargetGuard(rawProvider, capturedGeneration); const providerKey = this._getProviderKey(provider); // SPAP-11: split-file ("Surgical sync") download path (opt-in). When OFF @@ -810,6 +1020,7 @@ export class FileBasedSyncAdapterService { excludeClient, limit, providerKey, + capturedGeneration, ); } @@ -1038,6 +1249,12 @@ export class FileBasedSyncAdapterService { ); } + // Abort before committing a baseline read from a target that switched + // mid-download: staging its sync-version/clock/rev and letting the caller + // advance the seq cursor under the shared provider id could make the next + // sync skip the NEW target's ops from a stale cursor. (Task 2.) + this._abortDownloadIfTargetChanged(capturedGeneration, providerKey); + // Stage the remote baseline until the caller confirms that the downloaded // snapshot/ops were durably applied. A cancelled conflict dialog must leave // the committed baseline intact so a later reset still triggers a gap. @@ -1146,7 +1363,7 @@ export class FileBasedSyncAdapterService { // ═══════════════════════════════════════════════════════════════════════════ private async _uploadSnapshot( - provider: FileSyncProvider, + rawProvider: FileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, state: unknown, @@ -1156,6 +1373,12 @@ export class FileBasedSyncAdapterService { schemaVersion: number, snapshotOpType?: RestorePointType, ): Promise { + // Same in-flight target guard as _uploadOps: this is the second remote-write + // entry point (initial/recovery/migration + REPAIR snapshot writes, incl. + // _conditionalUploadRepairSnapshot and backups). A mid-operation target + // switch aborts before any write instead of committing this target's + // snapshot to the next one. (Task 2.) + const provider = this._withTargetGuard(rawProvider, this._targetGeneration); const providerKey = this._getProviderKey(provider); OpLog.normal(`FileBasedSyncAdapter: Uploading snapshot (reason=${reason})`); @@ -1322,18 +1545,10 @@ export class FileBasedSyncAdapterService { } } - // Reset local state - this._expectedSyncVersions.delete(providerKey); - this._localSeqCounters.delete(providerKey); - // 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._pendingExpectedSyncVersions.delete(providerKey); - this._pendingVectorClocks.delete(providerKey); - this._clearCachedSyncData(providerKey); - this._clearCachedOpsData(providerKey); + // Reset all target-scoped local state (incl. last-seen rev, so a stale + // value can't drive a false "nothing new" short-circuit after the remote + // file has been deleted). + this._resetTargetState(providerKey); this._persistState(); return { success: true }; @@ -1404,7 +1619,7 @@ export class FileBasedSyncAdapterService { /** Downloads + decodes `sync-ops.json` and validates its version. */ private async _downloadOpsFile( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, ): Promise<{ data: FileBasedOpsFile; rev: string }> { @@ -1436,7 +1651,7 @@ export class FileBasedSyncAdapterService { * the immutable snapshot referenced by an ops file's `snapshotRef.file`. */ private async _downloadStateFile( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, path: string = FILE_BASED_SYNC_CONSTANTS.STATE_FILE, @@ -1470,7 +1685,7 @@ export class FileBasedSyncAdapterService { * pre-#9040 clients that don't read `snapshotRef.file`. */ private async _writeStateFile( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, data: FileBasedStateFile, @@ -1521,7 +1736,7 @@ export class FileBasedSyncAdapterService { * capability-gated). */ private async _removeGenStateFile( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, file: string, ): Promise { try { @@ -1533,7 +1748,7 @@ export class FileBasedSyncAdapterService { /** Copies the current `sync-state.json` to its `.bak` (non-fatal). */ private async _backupStateFile( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, ): Promise { @@ -1557,7 +1772,7 @@ export class FileBasedSyncAdapterService { /** Recovers the snapshot from `sync-state.json.bak` (null if unusable). */ private async _recoverStateFromBackup( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, ): Promise { @@ -1596,7 +1811,7 @@ export class FileBasedSyncAdapterService { * Returns null when no snapshot validates the ref (caller signals a gap). */ private async _loadValidatedSnapshot( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, opsFile: FileBasedOpsFile, @@ -1671,7 +1886,7 @@ export class FileBasedSyncAdapterService { * inline comment. */ private async _writeTombstoneAndNeutralizeBak( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, expectedLegacyRev?: string, @@ -1714,7 +1929,7 @@ export class FileBasedSyncAdapterService { } private async _writePendingSplitMigration( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, legacy: FileBasedSyncData, @@ -1775,7 +1990,7 @@ export class FileBasedSyncAdapterService { } private async _finalizeSplitMigrationMarker( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, pending: FileBasedOpsFile, @@ -1809,7 +2024,7 @@ export class FileBasedSyncAdapterService { * used to build it or fails and causes the newer v2 payload to be re-imported. */ private async _resumePendingSplitMigration( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, initialPending: FileBasedOpsFile, @@ -1925,7 +2140,7 @@ export class FileBasedSyncAdapterService { * rev), or null for a truly fresh folder. */ private async _maybeMigrateLegacyToSplit( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, clientId: string, @@ -1978,7 +2193,7 @@ export class FileBasedSyncAdapterService { * to classify transient vs genuine concurrency and never force-overwrites. */ private async _uploadOpsFileWithMismatchFallback( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, newOpsFile: FileBasedOpsFile, @@ -2038,7 +2253,7 @@ export class FileBasedSyncAdapterService { * then the trimmed `sync-ops.json` referencing it via snapshotRef. */ private async _uploadOpsSplit( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, ops: SyncOperation[], @@ -2268,13 +2483,14 @@ export class FileBasedSyncAdapterService { * validates it against the ops file's snapshotRef (mismatch ⇒ gap). */ private async _downloadOpsSplit( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, sinceSeq: number, excludeClient: string | undefined, limit: number, providerKey: string, + capturedGeneration: number, ): Promise { // SPAP-10 rev pre-check, extended to the ops file. Gated on `sinceSeq > 0` // (review follow-up): a forceFromSeq0 download (sinceSeq === 0) re-pulls the @@ -2394,6 +2610,10 @@ export class FileBasedSyncAdapterService { opsFile.oldestOpSyncVersion > sinceSeq + 1; let needsGapDetection = versionWasReset || snapshotReplacement || partialTrimGap; + // See _downloadOps: abort before committing a baseline read from a target + // that switched mid-download. (Task 2.) + this._abortDownloadIfTargetChanged(capturedGeneration, providerKey); + this._pendingExpectedSyncVersions.set(providerKey, opsFile.syncVersion); this._pendingVectorClocks.set(providerKey, opsFile.vectorClock); this._stageOrDropDownloadedRev(providerKey, opsRev, recoveredFromBackup); @@ -2467,7 +2687,7 @@ export class FileBasedSyncAdapterService { * empty when the folder is truly fresh (or only a tombstone remains). */ private async _tryLegacyReadOnlyDownload( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, sinceSeq: number, @@ -2522,7 +2742,7 @@ export class FileBasedSyncAdapterService { * `sync-data.json` so OFF clients don't diverge. */ private async _uploadSnapshotSplit( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, clientId: string, @@ -2648,7 +2868,7 @@ export class FileBasedSyncAdapterService { * (conditional PUT) is unaffected. */ private async _writeBakFile( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, bakPath: string, @@ -2685,7 +2905,7 @@ export class FileBasedSyncAdapterService { * caller then surfaces its ORIGINAL corruption error. */ private async _readBakFile( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, bakPath: string, @@ -2739,7 +2959,7 @@ export class FileBasedSyncAdapterService { * the writes leaves a valid old primary + new .bak — nothing stale to recover.) */ private async _forceUploadWithBakFirst( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, primaryPath: string, bakPath: string, encoded: string, @@ -2760,7 +2980,7 @@ export class FileBasedSyncAdapterService { * absent" — the provider rejects the write if another client created it. */ private async _conditionalUploadRepairSnapshot( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, primaryPath: string, bakPath: string, encoded: string, @@ -2886,7 +3106,7 @@ export class FileBasedSyncAdapterService { * @returns The sync data and its revision (ETag) for conditional upload */ private async _downloadSyncFile( - provider: FileSyncProvider, + provider: GuardedFileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, ): Promise<{ data: FileBasedSyncData; rev: string }> { diff --git a/src/app/op-log/sync-providers/provider-manager.service.spec.ts b/src/app/op-log/sync-providers/provider-manager.service.spec.ts new file mode 100644 index 0000000000..72d18e9f0f --- /dev/null +++ b/src/app/op-log/sync-providers/provider-manager.service.spec.ts @@ -0,0 +1,121 @@ +import { TestBed } from '@angular/core/testing'; +import { Subject } from 'rxjs'; +import { provideMockStore } from '@ngrx/store/testing'; +import { + SyncProviderManager, + notifyFileProviderTargetChanged, +} from './provider-manager.service'; +import { DataInitStateService } from '../../core/data-init/data-init-state.service'; +import { SnackService } from '../../core/snack/snack.service'; +import { SyncProviderId } from './provider.const'; +import { SyncProviderBase } from './provider.interface'; + +/** + * Task 2 (sync-simplification plan). providerConfigChanged$ fires on every save + * and carries `isTargetChanged`: true only when the save moved the TARGET. That + * flag drives invalidateAllTargets(), which wipes the seq cursor — so a false + * positive sends the next sync down the sinceSeq===0 snapshot-bootstrap path. + * The picker / Android setupSaf / OneDrive pre-auth write bypass + * setProviderConfig() entirely and assert a target change directly. + */ +describe('SyncProviderManager target-change notification', () => { + let service: SyncProviderManager; + let configSpy: jasmine.Spy; + + const webdavCfg = { + baseUrl: 'https://a.example/dav', + userName: 'me', + password: 'pw', + encryptKey: 'key-1', + isEncryptionEnabled: true, + }; + + /** Stubs getProviderById so setProviderConfig can run without loading providers. */ + const stubProvider = (loadedCfg: unknown): jasmine.SpyObj> => { + const provider = { + id: SyncProviderId.WebDAV, + setPrivateCfg: jasmine.createSpy('setPrivateCfg').and.resolveTo(undefined), + privateCfg: { load: jasmine.createSpy('load').and.resolveTo(loadedCfg) }, + } as unknown as jasmine.SpyObj>; + spyOn(service, 'getProviderById').and.resolveTo( + provider as unknown as SyncProviderBase, + ); + return provider; + }; + + beforeEach(() => { + TestBed.configureTestingModule({ + providers: [ + SyncProviderManager, + provideMockStore({}), + { + provide: DataInitStateService, + // Never emits, so the constructor's sync-config subscription stays + // inert and we don't need to mock provider loading. + useValue: { isAllDataLoadedInitially$: new Subject() }, + }, + { + provide: SnackService, + useValue: jasmine.createSpyObj('SnackService', ['open']), + }, + ], + }); + service = TestBed.inject(SyncProviderManager); + + configSpy = jasmine.createSpy('providerConfigChanged'); + service.providerConfigChanged$.subscribe(configSpy); + }); + + describe('notifyProviderTargetChanged() (bypass ingresses)', () => { + it('emits isTargetChanged — the caller asserts the move directly', () => { + service.notifyProviderTargetChanged(); + + expect(configSpy).toHaveBeenCalledOnceWith({ isTargetChanged: true }); + }); + + it('routes the module-level notifyFileProviderTargetChanged() to the registered instance', () => { + // Injecting the service self-registered it as the module singleton. + notifyFileProviderTargetChanged(); + + expect(configSpy).toHaveBeenCalledOnceWith({ isTargetChanged: true }); + }); + }); + + describe('setProviderConfig()', () => { + // The per-field identity matrix lives in sync-target-identity.util.spec.ts. + // These two only pin that the flag is wired to the diff, in both directions. + it('reports isTargetChanged:false when nothing moved (e.g. Save with no edits)', async () => { + // The sync-settings dialog saves with isForce=true, bypassing the + // equality dedup, so this rewrite happens on every Save — including one + // that only touched a global setting like the sync interval. + stubProvider(webdavCfg); + + await service.setProviderConfig(SyncProviderId.WebDAV, { ...webdavCfg } as never); + + expect(configSpy).toHaveBeenCalledOnceWith({ isTargetChanged: false }); + }); + + it('reports isTargetChanged:true when the folder moves', async () => { + stubProvider(webdavCfg); + + await service.setProviderConfig(SyncProviderId.WebDAV, { + ...webdavCfg, + syncFolderPath: '/elsewhere', + } as never); + + expect(configSpy).toHaveBeenCalledOnceWith({ isTargetChanged: true }); + }); + + it('reads the previous config BEFORE overwriting it', async () => { + // The diff is only meaningful against the pre-write value. + const provider = stubProvider(webdavCfg); + + await service.setProviderConfig(SyncProviderId.WebDAV, { + ...webdavCfg, + userName: 'someone-else', + } as never); + + expect(provider.privateCfg.load).toHaveBeenCalledBefore(provider.setPrivateCfg); + }); + }); +}); diff --git a/src/app/op-log/sync-providers/provider-manager.service.ts b/src/app/op-log/sync-providers/provider-manager.service.ts index eb54504715..3ec34273bc 100644 --- a/src/app/op-log/sync-providers/provider-manager.service.ts +++ b/src/app/op-log/sync-providers/provider-manager.service.ts @@ -22,6 +22,7 @@ import { CurrentProviderPrivateCfg, } from '../core/types/sync.types'; import { loadSyncProviders } from './sync-providers.factory'; +import { isSyncTargetChanged } from './sync-target-identity.util'; /** * Sync status change payload type @@ -32,6 +33,37 @@ export type SyncStatusChangePayload = | 'IN_SYNC' | 'SYNCING'; +/** Payload of `providerConfigChanged$`. See that observable for the semantics. */ +export interface ProviderConfigChange { + /** True only when the write moved the sync target (see `isSyncTargetChanged`). */ + isTargetChanged: boolean; +} + +// Module-level reference so static sync-form handlers can signal a target +// change without an injector (mirrors the encryption-dialog-opener pattern). +let providerManagerInstance: SyncProviderManager | null = null; + +const setProviderManagerInstance = (instance: SyncProviderManager): void => { + providerManagerInstance = instance; +}; + +/** + * Signal that the active file-provider target changed through an ingress that + * bypasses `setProviderConfig()` — the Electron LocalFile folder picker (which + * persists the folder main-side, post-#8228) and Android `setupSaf()` (which + * writes `safFolderUri` straight to the credential store). Both mutate the + * target without firing `providerTargetChanged$`, so the file adapter would keep + * the previous folder's revs/clocks/caches keyed by the (unchanged) `LocalFile` + * provider id. Both ingresses are unambiguous target moves (the user picked a + * different folder), so this asserts a target change directly rather than + * inferring one from a config diff. It no-ops if the manager was never + * instantiated — nothing is cached to leak. + * (Task 2, docs/plans/2026-07-13-sync-simplification-plan.md.) + */ +export const notifyFileProviderTargetChanged = (): void => { + providerManagerInstance?.notifyProviderTargetChanged(); +}; + /** * Service for managing sync providers. * @@ -74,7 +106,7 @@ export class SyncProviderManager { new BehaviorSubject(null); // Emits whenever provider config is updated via setProviderConfig() - private _providerConfigChanged$ = new Subject(); + private _providerConfigChanged$ = new Subject(); private _hasShownLocalFileReselectSnack = false; /** @@ -115,10 +147,21 @@ export class SyncProviderManager { this._currentProviderPrivateCfg$.pipe(shareReplay(1)); /** - * Emits whenever provider config is updated via setProviderConfig(). - * Used by WrappedProviderService to auto-invalidate its adapter cache. + * Emits on EVERY provider-config write, carrying whether that write moved the + * sync TARGET — an account switch behind the same provider id, or a folder/URL + * change — as opposed to a content-only edit. Both ride ONE emission so a + * caller cannot raise a move without the cache drop, which would leave a stale + * encryption key cached against fresh target state. + * See `isSyncTargetChanged` and `FileBasedSyncAdapterService.invalidateAllTargets`. + * + * `isTargetChanged` is scoped to ONE provider's config and says nothing about + * which provider is ACTIVE — the two axes have separate detectors. A provider + * SWITCH is handled by `SyncWrapperService`'s `getLastSyncedProviderId()` check + * → `forceFromSeq0`, so switching to an already-configured provider correctly + * emits `isTargetChanged: false`: its provider-id-keyed state still describes + * its own unchanged remote. */ - public readonly providerConfigChanged$: Observable = + public readonly providerConfigChanged$: Observable = this._providerConfigChanged$.asObservable(); /** @@ -130,6 +173,10 @@ export class SyncProviderManager { ); constructor() { + // Self-register so the module-level notifyFileProviderTargetChanged() can + // reach this singleton from static form config handlers. + setProviderManagerInstance(this); + // Listen to sync config changes and update active provider this._syncConfig$.subscribe((cfg) => { try { @@ -236,13 +283,22 @@ export class SyncProviderManager { if (!provider) { throw new Error(`Provider not found: ${providerId}`); } + // Read the previous config BEFORE the write so an identity-affecting change + // (account/folder/URL) can be told apart from a content-only one. Callers + // save the whole privateCfg on every settings-dialog Save — including saves + // that only touched a global setting — so "config was written" is far weaker + // than "the target moved". See providerTargetChanged$. + const prevCfg = await provider.privateCfg.load(); + // Use setPrivateCfg() instead of privateCfg.setComplete() to ensure // provider-specific caches (like lastServerSeq key) are invalidated. // This is critical for server migration detection when switching users. await provider.setPrivateCfg(config); // Notify subscribers (e.g., WrappedProviderService) that config changed - this._providerConfigChanged$.next(); + this._providerConfigChanged$.next({ + isTargetChanged: isSyncTargetChanged(prevCfg, config), + }); // If this is the active provider, update the current config observable if (this._activeProvider?.id === providerId) { @@ -257,6 +313,21 @@ export class SyncProviderManager { } } + /** + * Asserts a target change for a write that bypassed `setProviderConfig()`, so + * no config diff is available to infer it from. Three such ingresses exist: the + * Electron LocalFile folder picker, Android `setupSaf()`, and the OneDrive + * pre-auth cfg write in `dialog-sync-cfg.component`. Callers that fire on every + * save (OneDrive) MUST gate this on `isSyncTargetChanged` — an unconditional + * notify reintroduces the cursor wipe this signal exists to avoid. + * + * Kept minimal on purpose: it does not reload provider config (the caller + * already persisted the new target). See `notifyFileProviderTargetChanged()`. + */ + notifyProviderTargetChanged(): void { + this._providerConfigChanged$.next({ isTargetChanged: true }); + } + /** * Clears authentication credentials for a provider while preserving non-auth config. * Updates readiness state and config observable after clearing. diff --git a/src/app/op-log/sync-providers/sync-target-identity.util.spec.ts b/src/app/op-log/sync-providers/sync-target-identity.util.spec.ts new file mode 100644 index 0000000000..a63a006724 --- /dev/null +++ b/src/app/op-log/sync-providers/sync-target-identity.util.spec.ts @@ -0,0 +1,94 @@ +import { isSyncTargetChanged } from './sync-target-identity.util'; + +describe('isSyncTargetChanged', () => { + const webdavCfg = { + baseUrl: 'https://a.example/dav', + userName: 'me', + password: 'pw', + syncFolderPath: '/sp', + encryptKey: 'key-1', + isEncryptionEnabled: true, + }; + + describe('content-only edits (must NOT invalidate)', () => { + it('ignores key order', () => { + // Subsumes the identical-config case; a persist/load round-trip is not + // guaranteed to preserve key order. + expect( + isSyncTargetChanged( + { baseUrl: 'https://a.example/dav', userName: 'me' }, + { userName: 'me', baseUrl: 'https://a.example/dav' }, + ), + ).toBe(false); + }); + + it('treats an absent field and an explicit undefined as equal', () => { + expect( + isSyncTargetChanged( + { baseUrl: 'u' }, + { baseUrl: 'u', syncFolderPath: undefined }, + ), + ).toBe(false); + }); + + it("treats PROVIDER_FIELD_DEFAULTS' '' sentinel as equal to absent", () => { + // The first save after a new default field lands merges `{field: ''}` onto + // a config that lacks it; that is not a target move. + expect(isSyncTargetChanged({ baseUrl: 'u' }, { baseUrl: 'u', password: '' })).toBe( + false, + ); + }); + + it('ignores an encryption key rotation', () => { + expect(isSyncTargetChanged(webdavCfg, { ...webdavCfg, encryptKey: 'key-2' })).toBe( + false, + ); + }); + + it('ignores the GHSA-9544 isEncryptionEnabled backfill', () => { + // WrappedProviderService._backfillEncryptionIntent writes exactly this + // (undefined -> true) mid-sync. Firing a target change for it would wipe + // the seq cursor and abort the in-flight sync it was spawned from. + const preFix = { ...webdavCfg, isEncryptionEnabled: undefined }; + expect(isSyncTargetChanged(preFix, { ...preFix, isEncryptionEnabled: true })).toBe( + false, + ); + }); + }); + + describe('target moves (MUST invalidate)', () => { + it('detects a folder change', () => { + expect( + isSyncTargetChanged(webdavCfg, { ...webdavCfg, syncFolderPath: '/other' }), + ).toBe(true); + }); + + it('still detects clearing a configured folder', () => { + // The '' <-> absent collapse must not mask a real move: '/foo' -> root. + expect( + isSyncTargetChanged({ baseUrl: 'u', syncFolderPath: '/foo' }, { baseUrl: 'u' }), + ).toBe(true); + }); + + it('detects an OAuth account switch (refresh token replaced)', () => { + // Dropbox has no account field at all — the tokens ARE the identity, which + // is why they must stay outside CONTENT_ONLY_CFG_FIELDS. + const dropbox = { accessToken: 'a1', refreshToken: 'r1' }; + expect(isSyncTargetChanged(dropbox, { ...dropbox, refreshToken: 'r2' })).toBe(true); + }); + + it('treats first-time setup (no previous config) as a change', () => { + expect(isSyncTargetChanged(null, webdavCfg)).toBe(true); + }); + }); + + it('fails safe: an unrecognised field counts as identity-affecting', () => { + // Only encryptKey/isEncryptionEnabled are provably content-only. Anything a + // future provider adds must invalidate rather than silently reuse a cursor — + // a false negative is silent cross-target corruption, which is worse than + // the (also bad) spurious full re-read a false positive costs. + expect( + isSyncTargetChanged({ baseUrl: 'u' }, { baseUrl: 'u', someNewTargetField: 'x' }), + ).toBe(true); + }); +}); diff --git a/src/app/op-log/sync-providers/sync-target-identity.util.ts b/src/app/op-log/sync-providers/sync-target-identity.util.ts new file mode 100644 index 0000000000..69395cd3a2 --- /dev/null +++ b/src/app/op-log/sync-providers/sync-target-identity.util.ts @@ -0,0 +1,57 @@ +/** + * Fields describing HOW content is encrypted, not WHICH remote is addressed. + * + * Every other field counts as identity-affecting, so an unrecognised or newly + * added one errs toward invalidating rather than silently reusing one target's + * cursor against another. Only add a field here once it provably cannot change + * which remote is addressed. + */ +const CONTENT_ONLY_CFG_FIELDS: ReadonlySet = new Set([ + 'encryptKey', + 'isEncryptionEnabled', +]); + +/** + * `PROVIDER_FIELD_DEFAULTS` (sync-config.service) is re-merged on every save, so + * a config predating a default gets that key seeded on its next save — which + * without this would read as a target move. Attested: `loginName: ''` was added + * to Nextcloud's defaults after configs existed. Cannot mask a real move — + * clearing a folder still compares `'/foo'` → absent. + * + * Deliberately incomplete: OneDrive's defaults are the only non-`''` ones + * (`tenantId: 'common'`, `syncFolderPath: 'Super Productivity'`, …), so such a + * config still reports a spurious move on its first save — one-time and + * self-correcting. Covering it would drag the imex/sync defaults table into + * op-log for less than it costs. + */ +const isUnset = (value: unknown): boolean => value === undefined || value === ''; + +/** + * Sorted because key order need not survive a persist/load round-trip; flat + * because every provider privateCfg is `string | boolean | number` throughout. + */ +const toTargetIdentity = (cfg: Record): string => + Object.entries(cfg) + .filter(([k, v]) => !CONTENT_ONLY_CFG_FIELDS.has(k) && !isUnset(v)) + .sort(([a], [b]) => (a < b ? -1 : 1)) + .map(([k, v]) => `${k}=${JSON.stringify(v)}`) + .join('&'); + +/** + * Whether two configs address a DIFFERENT remote (account switch behind the same + * provider id, folder/URL change) rather than a content-only edit. Keeps + * per-target state alive across routine saves — see + * `FileBasedSyncAdapterService.invalidateAllTargets` for why firing on a + * content-only save loses data. No previous config (first-time setup) counts as + * a change; there is no state to keep. + * + * Do NOT add a `prevCfg === nextCfg` fast path. It cannot speed up any real + * input, and the one case it changes the answer it gets wrong: + * `SyncCredentialStore.load()` returns the live cached object, so a caller that + * mutates it in place and passes it back would be told "no change" — a silent + * false negative. Callers must pass a new object (all do today). + */ +export const isSyncTargetChanged = (prevCfg: unknown, nextCfg: object): boolean => + !prevCfg || + toTargetIdentity(prevCfg as Record) !== + toTargetIdentity(nextCfg as Record); diff --git a/src/app/op-log/sync-providers/wrapped-provider.service.spec.ts b/src/app/op-log/sync-providers/wrapped-provider.service.spec.ts index ac0d1f18e8..3c62efb95b 100644 --- a/src/app/op-log/sync-providers/wrapped-provider.service.spec.ts +++ b/src/app/op-log/sync-providers/wrapped-provider.service.spec.ts @@ -14,7 +14,7 @@ describe('WrappedProviderService', () => { let service: WrappedProviderService; let mockProviderManager: jasmine.SpyObj; let mockFileBasedAdapter: jasmine.SpyObj; - let providerConfigChanged$: Subject; + let providerConfigChanged$: Subject<{ isTargetChanged: boolean }>; const createMockBaseProvider = ( id: SyncProviderId, @@ -66,7 +66,7 @@ describe('WrappedProviderService', () => { }); beforeEach(() => { - providerConfigChanged$ = new Subject(); + providerConfigChanged$ = new Subject<{ isTargetChanged: boolean }>(); mockProviderManager = jasmine.createSpyObj( 'SyncProviderManager', @@ -81,6 +81,7 @@ describe('WrappedProviderService', () => { mockFileBasedAdapter = jasmine.createSpyObj('FileBasedSyncAdapterService', [ 'createAdapter', + 'invalidateAllTargets', ]); TestBed.configureTestingModule({ @@ -306,12 +307,43 @@ describe('WrappedProviderService', () => { expect(result1).toBe(mockAdapter1); // Simulate config change - providerConfigChanged$.next(); + providerConfigChanged$.next({ isTargetChanged: false }); // Next call should create a new adapter (cache was auto-cleared) const result2 = await service.getOperationSyncCapable(dropboxProvider); expect(result2).toBe(mockAdapter2); expect(mockFileBasedAdapter.createAdapter).toHaveBeenCalledTimes(2); }); + + it('should invalidate file-adapter target state when the target moved', () => { + // A real target move (account switch behind the same provider id, folder + // change); the file adapter is keyed only by provider id, so its per-target + // state must be dropped or it leaks across targets (Task 2). + expect(mockFileBasedAdapter.invalidateAllTargets).not.toHaveBeenCalled(); + + providerConfigChanged$.next({ isTargetChanged: true }); + + expect(mockFileBasedAdapter.invalidateAllTargets).toHaveBeenCalledTimes(1); + }); + + it('should NOT invalidate file-adapter target state on a config-only change', () => { + // The cursor lives in the file adapter's target state. Wiping it on a save + // that did not move the target (sync interval, compression, an encryption + // key rotation, the isEncryptionEnabled backfill, or Save with nothing + // changed) sends the next sync down the sinceSeq===0 snapshot-bootstrap + // path, which dead-ends in a spurious conflict dialog for any client + // holding unsynced ops. + providerConfigChanged$.next({ isTargetChanged: false }); + + expect(mockFileBasedAdapter.invalidateAllTargets).not.toHaveBeenCalled(); + }); + + it('should still drop the adapter cache on a config-only change', () => { + // The cache holds the resolved encryption key/intent, so it must be + // rebuilt even when the target stayed put. + providerConfigChanged$.next({ isTargetChanged: false }); + + expect(service['_cache'].size).toBe(0); + }); }); }); diff --git a/src/app/op-log/sync-providers/wrapped-provider.service.ts b/src/app/op-log/sync-providers/wrapped-provider.service.ts index a1fc10ab9b..348545bf60 100644 --- a/src/app/op-log/sync-providers/wrapped-provider.service.ts +++ b/src/app/op-log/sync-providers/wrapped-provider.service.ts @@ -47,14 +47,27 @@ export class WrappedProviderService { private _cache = new Map(); constructor() { - // Auto-invalidate cache when provider config changes this._providerManager.providerConfigChanged$ .pipe(takeUntilDestroyed(this._destroyRef)) - .subscribe(() => { + .subscribe(({ isTargetChanged }) => { + // Always: the cached adapter closes over the resolved encryption + // key/intent, so any config edit must rebuild it. this._cache.clear(); OpLog.normal( 'WrappedProviderService: Cache auto-invalidated due to config change', ); + + // Only on a real target move: the file adapter is keyed by provider id + // alone, so its sync-version/rev/clock and within-cycle caches would be + // reused against the new target. Guarded rather than unconditional + // because this also wipes the seq cursor — see invalidateAllTargets(). + // (Task 2, docs/plans/2026-07-13-sync-simplification-plan.md.) + if (isTargetChanged) { + this._fileBasedAdapter.invalidateAllTargets(); + OpLog.normal( + 'WrappedProviderService: File-adapter target state invalidated (target changed)', + ); + } }); } diff --git a/src/app/op-log/testing/integration/file-based-sync/target-invalidation.integration.spec.ts b/src/app/op-log/testing/integration/file-based-sync/target-invalidation.integration.spec.ts new file mode 100644 index 0000000000..17214b7795 --- /dev/null +++ b/src/app/op-log/testing/integration/file-based-sync/target-invalidation.integration.spec.ts @@ -0,0 +1,84 @@ +import { + FileBasedSyncTestHarness, + HarnessClient, +} from '../helpers/file-based-sync-test-harness'; +import { FileSnapshotOpDownloadResponse } from '../../../sync-providers/provider.interface'; + +/** + * Task 2 (#9063 + follow-up): what `invalidateAllTargets()` actually COSTS. + * + * The unit specs prove the gate (only a real target move invalidates). These + * prove why the gate matters, against a real `FileBasedSyncAdapterService`: + * invalidate -> cursor 0 -> downloadOps(0) -> isForceFromZero -> a full + * `snapshotState` instead of an incremental op list. + * + * A client holding unsynced ops then classifies that snapshot CONCURRENT and, + * with `AUTO_MERGE_CONCURRENT_SNAPSHOT` false, dead-ends in the binary conflict + * dialog whose either answer discards data (that half lives in + * OperationLogSyncService and has its own specs). So invalidating on a save that + * did not move the target is a data-loss hazard, not an "extra full read". + */ +describe('File-Based Sync Integration - target invalidation cost (Task 2)', () => { + let harness: FileBasedSyncTestHarness; + let clientA: HarnessClient; + let clientB: HarnessClient; + + /** A→B: A uploads one op, B pulls from its own committed cursor. */ + const uploadFromAAndPullToB = async ( + title: string, + ): Promise => { + const op = clientA.createOp('Task', `task-${title}`, 'CRT', 'ADD_TASK', { title }); + await clientA.uploadOps([op]); + + // Mirror production ordering: read the committed cursor, download from it, + // then commit the new cursor only after a successful "apply". + const sinceSeq = await clientB.adapter.getLastServerSeq(); + const res = (await clientB.adapter.downloadOps( + sinceSeq, + clientB.clientId, + )) as FileSnapshotOpDownloadResponse; + await clientB.adapter.setLastServerSeq(res.latestSeq); + return res; + }; + + beforeEach(async () => { + harness = FileBasedSyncTestHarness.create({}); + clientA = harness.createClient('client-a-target-inv'); + clientB = harness.createClient('client-b-target-inv'); + + // Establish a real committed cursor on B by syncing once. + await uploadFromAAndPullToB('first'); + expect(await clientB.adapter.getLastServerSeq()).toBeGreaterThan(0); + }); + + afterEach(() => { + harness.reset(); + }); + + it('keeps the download incremental while the cursor survives', async () => { + // The control: no invalidation -> no snapshot bootstrap. Without this, the + // test below would pass even if EVERY download returned a snapshot. + const res = await uploadFromAAndPullToB('second'); + + expect(res.snapshotState).toBeUndefined(); + expect(res.ops.length).toBeGreaterThan(0); + }); + + it('forces a full snapshot bootstrap once the cursor is invalidated', async () => { + const cursorBefore = await clientB.adapter.getLastServerSeq(); + expect(cursorBefore).toBeGreaterThan(0); + + // Exactly what a target move triggers via providerConfigChanged$. + clientB.adapterService.invalidateAllTargets(); + + // The cursor is gone — and it is PERSISTED gone, so a restart won't recover it. + expect(await clientB.adapter.getLastServerSeq()).toBe(0); + + const res = await uploadFromAAndPullToB('third'); + + // sinceSeq === 0 => isForceFromZero => the whole remote state comes back. + // This is the payload that classifies CONCURRENT against unsynced local ops + // and surfaces the data-losing conflict dialog. + expect(res.snapshotState).toBeDefined(); + }); +}); diff --git a/src/app/op-log/testing/integration/helpers/file-based-sync-test-harness.ts b/src/app/op-log/testing/integration/helpers/file-based-sync-test-harness.ts index 006a950188..6840fce70c 100644 --- a/src/app/op-log/testing/integration/helpers/file-based-sync-test-harness.ts +++ b/src/app/op-log/testing/integration/helpers/file-based-sync-test-harness.ts @@ -28,6 +28,13 @@ export interface HarnessClient { /** The operation sync adapter for this client */ adapter: OperationSyncCapable; + /** + * The adapter SERVICE backing `adapter` — one per client, mirroring production + * (each app instance owns its own). Exposed so tests can drive service-level + * transitions such as `invalidateAllTargets()`. + */ + adapterService: FileBasedSyncAdapterService; + /** Test client for vector clock management */ testClient: TestClient; @@ -294,6 +301,7 @@ export class FileBasedSyncTestHarness { const client: HarnessClient = { clientId, adapter, + adapterService, testClient, uploadOps: async (ops: SyncOperation[]): Promise => { diff --git a/src/app/op-log/testing/integration/target-invalidation-wiring.integration.spec.ts b/src/app/op-log/testing/integration/target-invalidation-wiring.integration.spec.ts new file mode 100644 index 0000000000..bf7c9a477a --- /dev/null +++ b/src/app/op-log/testing/integration/target-invalidation-wiring.integration.spec.ts @@ -0,0 +1,130 @@ +import { TestBed } from '@angular/core/testing'; +import { Subject } from 'rxjs'; +import { provideMockStore } from '@ngrx/store/testing'; +import { SyncProviderManager } from '../../sync-providers/provider-manager.service'; +import { WrappedProviderService } from '../../sync-providers/wrapped-provider.service'; +import { FileBasedSyncAdapterService } from '../../sync-providers/file-based/file-based-sync-adapter.service'; +import { ArchiveDbAdapter } from '../../../core/persistence/archive-db-adapter.service'; +import { StateSnapshotService } from '../../backup/state-snapshot.service'; +import { DataInitStateService } from '../../../core/data-init/data-init-state.service'; +import { SnackService } from '../../../core/snack/snack.service'; +import { SyncProviderId } from '../../sync-providers/provider.const'; +import { SyncProviderBase } from '../../sync-providers/provider.interface'; + +/** + * Task 2 (#9063 + follow-up): the invalidation wiring, through the REAL injector. + * + * The other specs for this fix mock `SyncProviderManager` and assert against a + * hand-made Subject, which proves the handler logic but assumes the wiring. Here + * all three services are real and only `getProviderById` is stubbed, so the whole + * production path runs: setProviderConfig -> isSyncTargetChanged -> + * providerConfigChanged$ -> the real subscription -> invalidateAllTargets -> the + * persisted cursor. Catches what mocked specs structurally cannot: the observable + * never being subscribed, or the flag not reaching the adapter. + */ +describe('Target-invalidation wiring (real DI)', () => { + let providerManager: SyncProviderManager; + let adapterService: FileBasedSyncAdapterService; + + const webdavCfg = { + baseUrl: 'https://a.example/dav', + userName: 'me', + password: 'pw', + syncFolderPath: '/sp', + encryptKey: 'key-1', + isEncryptionEnabled: true, + }; + + /** Reads the cursor the way createAdapter's closure does. */ + const getCursor = (): number => + adapterService['_localSeqCounters'].get(SyncProviderId.WebDAV) ?? 0; + + const stubProvider = (storedCfg: Record): void => { + let stored = { ...storedCfg }; + spyOn(providerManager, 'getProviderById').and.resolveTo({ + id: SyncProviderId.WebDAV, + isReady: async () => true, + setPrivateCfg: async (cfg: Record) => { + stored = { ...cfg }; + }, + privateCfg: { load: async () => stored }, + } as unknown as SyncProviderBase); + }; + + beforeEach(() => { + TestBed.configureTestingModule({ + providers: [ + SyncProviderManager, + WrappedProviderService, + FileBasedSyncAdapterService, + provideMockStore({}), + { + provide: DataInitStateService, + // Never emits, so the manager's sync-config subscription stays inert. + useValue: { isAllDataLoadedInitially$: new Subject() }, + }, + { + provide: SnackService, + useValue: jasmine.createSpyObj('SnackService', ['open']), + }, + { provide: ArchiveDbAdapter, useValue: {} }, + { provide: StateSnapshotService, useValue: {} }, + ], + }); + + providerManager = TestBed.inject(SyncProviderManager); + adapterService = TestBed.inject(FileBasedSyncAdapterService); + // Constructing this is what registers the real subscription — exactly as the + // effects -> SyncWrapperService -> WrappedProviderService chain does at boot. + TestBed.inject(WrappedProviderService); + + adapterService['_localSeqCounters'].set(SyncProviderId.WebDAV, 42); + }); + + afterEach(() => { + adapterService.invalidateAllTargets(); + localStorage.removeItem('FILE_SYNC_VERSION_state'); + }); + + it('keeps the seq cursor when a save does not move the target', async () => { + // The real Defect 1 trigger: the settings dialog saves with isForce=true, so + // this rewrite happens even when only a global setting (sync interval, + // compression) changed — or nothing at all. + stubProvider(webdavCfg); + + await providerManager.setProviderConfig(SyncProviderId.WebDAV, { + ...webdavCfg, + } as never); + + expect(getCursor()).toBe(42); + }); + + it('keeps the seq cursor across an encryption-key rotation and the intent backfill', async () => { + stubProvider({ ...webdavCfg, isEncryptionEnabled: undefined }); + + await providerManager.setProviderConfig(SyncProviderId.WebDAV, { + ...webdavCfg, + encryptKey: 'key-2', + isEncryptionEnabled: true, + } as never); + + expect(getCursor()).toBe(42); + }); + + it('drops the seq cursor when the target actually moves', async () => { + stubProvider(webdavCfg); + + await providerManager.setProviderConfig(SyncProviderId.WebDAV, { + ...webdavCfg, + syncFolderPath: '/elsewhere', + } as never); + + expect(getCursor()).toBe(0); + }); + + it('drops the seq cursor when a bypass ingress asserts a move (picker / SAF / OneDrive)', () => { + providerManager.notifyProviderTargetChanged(); + + expect(getCursor()).toBe(0); + }); +});