mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
fix(sync): isolate file-provider state across target changes (#9063)
* fix(sync): invalidate file-provider target state on config change Task 2 (sync-simplification plan), core increment. The file adapter keys all per-target state (sync version, revs, vector clocks, seq cursor, within-cycle caches) by provider id only, and nothing cleared it on a configuration save. A provider switch, an account switch behind the same provider id, or an identity-affecting setting change would reuse the previous target's state against the new target — reading or writing one target's data against another. - Extract the delete-all reset into a shared _resetTargetState() over a single _targetScopedMaps source of truth; this also closes the one-field gap where deleteAllData never cleared _lastRecoveredCorruptRev. - Add invalidateAllTargets() (+ a target generation counter for the later in-flight guard) and call it from WrappedProviderService's existing providerConfigChanged$ subscription. Machine-only token refreshes go through the credential store (setComplete), not setProviderConfig, so they do not fire providerConfigChanged$ and correctly do not invalidate. Remaining Task 2 scope (follow-ups): Electron LocalFile picker + Android setupSaf ingresses that bypass providerConfigChanged$, and in-flight generation validation before each remote side effect (incl. the #9023 REPAIR rebase loop). * fix(sync): invalidate file target on LocalFile picker/SAF change Task 2 follow-up. The Electron LocalFile folder picker (persists main-side post-#8228) and Android setupSaf() (writes safFolderUri to the credential store) change the sync target without going through setProviderConfig(), so they never fired providerConfigChanged$ — leaving the file adapter's per-target revs/clocks/ caches (keyed only by the unchanged LocalFile provider id) pointed at the old folder. Route both ingresses through the existing providerConfigChanged$ signal via a module-level bridge, so WrappedProviderService clears its cache and calls invalidateAllTargets() exactly as it does for a config save. Remaining Task 2 scope: in-flight generation validation before each remote side effect (incl. the #9023 REPAIR rebase loop). * fix(sync): abort file upload when the target changes mid-operation Task 2 follow-up (in-flight guard). The file adapter keys per-target state by provider id, and the same provider object reads live config, so a target switch (provider/account/folder/identity-affecting setting) DURING an upload would let the in-flight write commit the previous target's merged data to the new target. Capture the target generation at the _uploadOps boundary and thread a write-guarded provider (Proxy over uploadFile/removeFile) through every write path — single-file, split, REPAIR snapshot, and backups. A generation bump (invalidateAllTargets) between capture and a write throws FileSyncTargetChangedError before the write; reads pass through. SyncWrapper maps it to UNKNOWN_OR_CHANGED (silent self-healing re-sync), like a concurrent-upload rev mismatch. Residual (documented): a check->write TOCTOU window remains (narrowed, not closed), and the REPAIR rebaseStaleRepair loop across RejectedOpsHandlerService/ RepairOperationService is not yet generation-threaded (follow-up #2b). Tests: adapter guard (mid-op abort writes nothing; removeFile guarded; reads pass), sync-wrapper mapping (silent UNKNOWN_OR_CHANGED). * fix(sync): extend in-flight target guard to snapshot uploads Completes the Task 2 in-flight guard. #2a guarded _uploadOps, but the file adapter has a SECOND remote-write entry point — uploadSnapshot/_uploadSnapshot (initial/recovery/migration + the #9023 REPAIR snapshot via _conditionalUploadRepairSnapshot and its backups) — which bypassed _uploadOps and was left unguarded. Apply the same generation-capture-at-boundary + write-guarded-provider shadow to _uploadSnapshot. The REPAIR rebase loop itself needs no guard: rebaseStaleRepair (RepairOperation Service) performs NO remote I/O — it rebuilds the repair op from local state (stateSnapshotService + opLogStore.replaceRejectedRepair) and defers the re-upload to the next sync cycle, which flows through the now-guarded write paths. deleteAllData's removeFile stays unguarded by design: it is a deliberate user-initiated wipe of a chosen target, not an in-flight-sync write race. Test: a target switch during the snapshot's archive-load phase aborts before the write with FileSyncTargetChangedError. * fix(sync): guard split-migration writes on the download path Review follow-up (two independent reviewers). The in-flight target guard covered the two upload entry points, but the split-format DOWNLOAD path also writes: when a remote carries a pending split-migration marker, _downloadOpsSplit -> _resumePendingSplitMigration force-writes the state file, tombstone/.bak, and migration marker via the RAW provider. A target switch during that resume could land the previous target's migration on the new one — the same corruption class the guard prevents. Apply the same generation-capture-at-boundary shadow to _downloadOps so the resume writes are guarded; reads (downloadFile/getFileRev) still pass through, so normal downloads are unaffected. Test (e2c): a target switch during a pending-migration download aborts the resume writes with FileSyncTargetChangedError. * docs(sync): correct Task 2 guard comments; make targetGeneration private Multi-review cleanup (no behavior change): - Fix the _targetGeneration doc comment (the in-flight guard it called a 'follow-up' shipped in the same work; it drives _withTargetGuard now). - Correct the _targetScopedMaps comment, which overstated the guarantee: a mid-download switch can repopulate a cache (read path) and a switch between download and upload is caught by neither per-operation guard. What actually prevents a cross-target write is the generation guard + the conditional-write rev check, which self-heals on the next sync — document that honestly. - Make the targetGeneration getter private (test-only surface; the guard reads the private field directly); specs use bracket access. * fix(sync): abort a download whose target changed before committing its baseline Multi-review (Codex) found a data-loss window the write-guard missed: a download READS target A, and if the target then switches mid-download, staging A's baseline (sync-version/clock/rev) and letting the caller advance the seq cursor under the shared provider id makes the NEXT sync skip the new target's ops from a stale cursor. The write-guard only covers writes; reads pass through. Capture the generation at the download boundary and, before committing the baseline (single-file and split paths), abort + reset the target-scoped state if it changed — SyncWrapper maps the error to a silent self-heal. Closes the dominant switch-during-download window; a switch after a successful download (during op-apply / an upload's own cursor commit) is a narrower residual only per-cycle session capture can fully close (documented). * fix(sync): map FileSyncTargetChangedError on the force-upload paths Multi-review (Codex): the error was only mapped in the normal sync() catch. The forceUpload and USE_LOCAL conflict-resolution catches handled only EncryptNoPasswordError, so a target switch during a force upload aborted safely (guard held — no data loss) but surfaced a scary ERROR snack instead of the silent UNKNOWN_OR_CHANGED self-heal. Add the branch to both force paths. * refactor(sync): compile-enforce the in-flight guard via a branded provider type Multi-review (Architecture) hardening: the guard was threaded by an implicit call-graph convention — a future entry point or write-helper caller that forgot the _withTargetGuard shadow would silently re-open cross-target writes, the worst failure class here. Introduce a phantom-branded GuardedFileSyncProvider (FileSyncProvider & { [unique symbol]: true }) returned by _withTargetGuard and required by every write-path helper param. Passing a raw provider to a write path is now a compile error (verified: TS2345). Only createAdapter and the intentionally-unguarded _deleteAllData keep the raw type. No runtime/behavior change (brand is a phantom type); adapter suite 135/135. * fix(sync): invalidate file-adapter state only on real target moves Task 2's invalidation was wired to "any privateCfg was saved", but its semantics are "the sync target moved". setProviderConfig() fires providerConfigChanged$ unconditionally, so two content-only writes reached invalidateAllTargets(): 1. The sync-settings dialog saves with isForce=true, which bypasses the JSON-equality dedup, and _updatePrivateConfig then rewrites privateCfg unconditionally. So changing the sync interval, toggling compression, or pressing Save with nothing changed wiped _localSeqCounters and persisted it. 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 it dead-ends in a binary conflict dialog whose either answer discards data. file-based-sync-adapter.service.ts already documented this exact hazard at the latestSeq computation. 2. WrappedProviderService fires the GHSA-9544 isEncryptionEnabled backfill fire-and-forget on adapter creation. Its setProviderConfig() lands within ms (local IO) while the sync it was spawned from is still on the network, so it wiped the cursor mid-cycle and aborted that sync with FileSyncTargetChangedError even though nothing moved. (The abort does not heal the cursor: invalidateAllTargets clears and persists it before the abort throws, so the next cycle still bootstraps from 0.) providerConfigChanged$ now carries isTargetChanged, set from isSyncTargetChanged(). Every subscriber still drops config-derived caches (the adapter closes over the resolved encryption key/intent); only a real move additionally calls invalidateAllTargets(). Both facts ride one emission so a caller cannot raise a move without the cache drop. isSyncTargetChanged treats only encryptKey/isEncryptionEnabled as content-only and everything else as identity-affecting, so an unknown or newly added field errs toward invalidating rather than reusing one target's cursor against another. Both directions can lose data; the false negative is worse only because it is silent. The OneDrive pre-auth cfg write (dialog-sync-cfg) bypasses setProviderConfig, so by the time the save's setProviderConfig runs the diff is a no-op and a folder move would have silently kept the previous folder's cursor. It now asserts the move directly, gated on a real diff since it runs on every save. Covered by a test verified to fail without the fix. Also correct two comments: invalidateAllTargets' trigger, and the download-abort's mechanism (a stale cursor suppresses gap detection so the new target's snapshot is never loaded — it does not skip ops; both download paths return every op and dedup via appliedOpIds). Known residual: OneDrive's PROVIDER_FIELD_DEFAULTS are the only ones not seeded with '', so a config missing one still reports a spurious move on its first save (one-time, self-correcting). Documented at isUnset.
This commit is contained in:
Johannes Millan
GitHub
parent
71a9a4338a
commit
542da1eb6c
17 changed files with 1248 additions and 56 deletions
|
|
@ -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<SyncConfig> = {
|
|||
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<SyncConfig> = {
|
|||
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: {
|
||||
|
|
|
|||
|
|
@ -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<string, unknown>): 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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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).
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<string, string> = {
|
||||
[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,
|
||||
|
|
|
|||
|
|
@ -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<SyncProviderId> & {
|
||||
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<string, number>();
|
||||
|
||||
|
|
@ -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<string, unknown>[] {
|
||||
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<string, unknown>[];
|
||||
}
|
||||
|
||||
/**
|
||||
* 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<SyncProviderId>,
|
||||
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<unknown>;
|
||||
// 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<unknown> => {
|
||||
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<SyncProviderId>,
|
||||
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<SyncProviderId>,
|
||||
provider: GuardedFileSyncProvider,
|
||||
cfg: EncryptAndCompressCfg,
|
||||
encryptKey: string | undefined,
|
||||
newData: FileBasedSyncData,
|
||||
|
|
@ -670,7 +866,7 @@ export class FileBasedSyncAdapterService {
|
|||
}
|
||||
|
||||
private async _uploadOps(
|
||||
provider: FileSyncProvider<SyncProviderId>,
|
||||
rawProvider: FileSyncProvider<SyncProviderId>,
|
||||
cfg: EncryptAndCompressCfg,
|
||||
encryptKey: string | undefined,
|
||||
ops: SyncOperation[],
|
||||
|
|
@ -678,6 +874,12 @@ export class FileBasedSyncAdapterService {
|
|||
lastKnownServerSeq?: number,
|
||||
localStateSnapshot?: unknown,
|
||||
): Promise<OpUploadResponse> {
|
||||
// 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<SyncProviderId>,
|
||||
rawProvider: FileSyncProvider<SyncProviderId>,
|
||||
cfg: EncryptAndCompressCfg,
|
||||
encryptKey: string | undefined,
|
||||
sinceSeq: number,
|
||||
excludeClient?: string,
|
||||
limit: number = 500,
|
||||
): Promise<FileSnapshotOpDownloadResponse> {
|
||||
// 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<SyncProviderId>,
|
||||
rawProvider: FileSyncProvider<SyncProviderId>,
|
||||
cfg: EncryptAndCompressCfg,
|
||||
encryptKey: string | undefined,
|
||||
state: unknown,
|
||||
|
|
@ -1156,6 +1373,12 @@ export class FileBasedSyncAdapterService {
|
|||
schemaVersion: number,
|
||||
snapshotOpType?: RestorePointType,
|
||||
): Promise<SnapshotUploadResponse> {
|
||||
// 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<SyncProviderId>,
|
||||
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<SyncProviderId>,
|
||||
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<SyncProviderId>,
|
||||
provider: GuardedFileSyncProvider,
|
||||
cfg: EncryptAndCompressCfg,
|
||||
encryptKey: string | undefined,
|
||||
data: FileBasedStateFile,
|
||||
|
|
@ -1521,7 +1736,7 @@ export class FileBasedSyncAdapterService {
|
|||
* capability-gated).
|
||||
*/
|
||||
private async _removeGenStateFile(
|
||||
provider: FileSyncProvider<SyncProviderId>,
|
||||
provider: GuardedFileSyncProvider,
|
||||
file: string,
|
||||
): Promise<void> {
|
||||
try {
|
||||
|
|
@ -1533,7 +1748,7 @@ export class FileBasedSyncAdapterService {
|
|||
|
||||
/** Copies the current `sync-state.json` to its `.bak` (non-fatal). */
|
||||
private async _backupStateFile(
|
||||
provider: FileSyncProvider<SyncProviderId>,
|
||||
provider: GuardedFileSyncProvider,
|
||||
cfg: EncryptAndCompressCfg,
|
||||
encryptKey: string | undefined,
|
||||
): Promise<void> {
|
||||
|
|
@ -1557,7 +1772,7 @@ export class FileBasedSyncAdapterService {
|
|||
|
||||
/** Recovers the snapshot from `sync-state.json.bak` (null if unusable). */
|
||||
private async _recoverStateFromBackup(
|
||||
provider: FileSyncProvider<SyncProviderId>,
|
||||
provider: GuardedFileSyncProvider,
|
||||
cfg: EncryptAndCompressCfg,
|
||||
encryptKey: string | undefined,
|
||||
): Promise<FileBasedStateFile | null> {
|
||||
|
|
@ -1596,7 +1811,7 @@ export class FileBasedSyncAdapterService {
|
|||
* Returns null when no snapshot validates the ref (caller signals a gap).
|
||||
*/
|
||||
private async _loadValidatedSnapshot(
|
||||
provider: FileSyncProvider<SyncProviderId>,
|
||||
provider: GuardedFileSyncProvider,
|
||||
cfg: EncryptAndCompressCfg,
|
||||
encryptKey: string | undefined,
|
||||
opsFile: FileBasedOpsFile,
|
||||
|
|
@ -1671,7 +1886,7 @@ export class FileBasedSyncAdapterService {
|
|||
* inline comment.
|
||||
*/
|
||||
private async _writeTombstoneAndNeutralizeBak(
|
||||
provider: FileSyncProvider<SyncProviderId>,
|
||||
provider: GuardedFileSyncProvider,
|
||||
cfg: EncryptAndCompressCfg,
|
||||
encryptKey: string | undefined,
|
||||
expectedLegacyRev?: string,
|
||||
|
|
@ -1714,7 +1929,7 @@ export class FileBasedSyncAdapterService {
|
|||
}
|
||||
|
||||
private async _writePendingSplitMigration(
|
||||
provider: FileSyncProvider<SyncProviderId>,
|
||||
provider: GuardedFileSyncProvider,
|
||||
cfg: EncryptAndCompressCfg,
|
||||
encryptKey: string | undefined,
|
||||
legacy: FileBasedSyncData,
|
||||
|
|
@ -1775,7 +1990,7 @@ export class FileBasedSyncAdapterService {
|
|||
}
|
||||
|
||||
private async _finalizeSplitMigrationMarker(
|
||||
provider: FileSyncProvider<SyncProviderId>,
|
||||
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<SyncProviderId>,
|
||||
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<SyncProviderId>,
|
||||
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<SyncProviderId>,
|
||||
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<SyncProviderId>,
|
||||
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<SyncProviderId>,
|
||||
provider: GuardedFileSyncProvider,
|
||||
cfg: EncryptAndCompressCfg,
|
||||
encryptKey: string | undefined,
|
||||
sinceSeq: number,
|
||||
excludeClient: string | undefined,
|
||||
limit: number,
|
||||
providerKey: string,
|
||||
capturedGeneration: number,
|
||||
): Promise<FileSnapshotOpDownloadResponse> {
|
||||
// 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<SyncProviderId>,
|
||||
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<SyncProviderId>,
|
||||
provider: GuardedFileSyncProvider,
|
||||
cfg: EncryptAndCompressCfg,
|
||||
encryptKey: string | undefined,
|
||||
clientId: string,
|
||||
|
|
@ -2648,7 +2868,7 @@ export class FileBasedSyncAdapterService {
|
|||
* (conditional PUT) is unaffected.
|
||||
*/
|
||||
private async _writeBakFile<T>(
|
||||
provider: FileSyncProvider<SyncProviderId>,
|
||||
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<T extends { version: number }>(
|
||||
provider: FileSyncProvider<SyncProviderId>,
|
||||
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<SyncProviderId>,
|
||||
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<SyncProviderId>,
|
||||
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<SyncProviderId>,
|
||||
provider: GuardedFileSyncProvider,
|
||||
cfg: EncryptAndCompressCfg,
|
||||
encryptKey: string | undefined,
|
||||
): Promise<{ data: FileBasedSyncData; rev: string }> {
|
||||
|
|
|
|||
121
src/app/op-log/sync-providers/provider-manager.service.spec.ts
Normal file
121
src/app/op-log/sync-providers/provider-manager.service.spec.ts
Normal file
|
|
@ -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<SyncProviderBase<never>> => {
|
||||
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<SyncProviderBase<never>>;
|
||||
spyOn(service, 'getProviderById').and.resolveTo(
|
||||
provider as unknown as SyncProviderBase<SyncProviderId>,
|
||||
);
|
||||
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<boolean>() },
|
||||
},
|
||||
{
|
||||
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);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
@ -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<CurrentProviderPrivateCfg | null>(null);
|
||||
|
||||
// Emits whenever provider config is updated via setProviderConfig()
|
||||
private _providerConfigChanged$ = new Subject<void>();
|
||||
private _providerConfigChanged$ = new Subject<ProviderConfigChange>();
|
||||
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<void> =
|
||||
public readonly providerConfigChanged$: Observable<ProviderConfigChange> =
|
||||
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.
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
57
src/app/op-log/sync-providers/sync-target-identity.util.ts
Normal file
57
src/app/op-log/sync-providers/sync-target-identity.util.ts
Normal file
|
|
@ -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<string> = 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, unknown>): 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<string, unknown>) !==
|
||||
toTargetIdentity(nextCfg as Record<string, unknown>);
|
||||
|
|
@ -14,7 +14,7 @@ describe('WrappedProviderService', () => {
|
|||
let service: WrappedProviderService;
|
||||
let mockProviderManager: jasmine.SpyObj<SyncProviderManager>;
|
||||
let mockFileBasedAdapter: jasmine.SpyObj<FileBasedSyncAdapterService>;
|
||||
let providerConfigChanged$: Subject<void>;
|
||||
let providerConfigChanged$: Subject<{ isTargetChanged: boolean }>;
|
||||
|
||||
const createMockBaseProvider = (
|
||||
id: SyncProviderId,
|
||||
|
|
@ -66,7 +66,7 @@ describe('WrappedProviderService', () => {
|
|||
});
|
||||
|
||||
beforeEach(() => {
|
||||
providerConfigChanged$ = new Subject<void>();
|
||||
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);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -47,14 +47,27 @@ export class WrappedProviderService {
|
|||
private _cache = new Map<string, OperationSyncCapable>();
|
||||
|
||||
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)',
|
||||
);
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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<FileSnapshotOpDownloadResponse> => {
|
||||
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();
|
||||
});
|
||||
});
|
||||
|
|
@ -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<OpUploadResponse> => {
|
||||
|
|
|
|||
|
|
@ -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<string, unknown>): void => {
|
||||
let stored = { ...storedCfg };
|
||||
spyOn(providerManager, 'getProviderById').and.resolveTo({
|
||||
id: SyncProviderId.WebDAV,
|
||||
isReady: async () => true,
|
||||
setPrivateCfg: async (cfg: Record<string, unknown>) => {
|
||||
stored = { ...cfg };
|
||||
},
|
||||
privateCfg: { load: async () => stored },
|
||||
} as unknown as SyncProviderBase<SyncProviderId>);
|
||||
};
|
||||
|
||||
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<boolean>() },
|
||||
},
|
||||
{
|
||||
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);
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue