From f59f2396c1d38cdfa8535bde46043f705e7713b1 Mon Sep 17 00:00:00 2001 From: Henry Heino Date: Tue, 8 Oct 2024 20:45:38 -0700 Subject: [PATCH 1/2] Cli,Desktop,Mobile: Fix errors removing sync locks break future syncs --- packages/lib/Synchronizer.ts | 87 ++++++++++++++++++++++++------------ 1 file changed, 58 insertions(+), 29 deletions(-) diff --git a/packages/lib/Synchronizer.ts b/packages/lib/Synchronizer.ts index 1812f25aae1..ae4c2181854 100644 --- a/packages/lib/Synchronizer.ts +++ b/packages/lib/Synchronizer.ts @@ -54,6 +54,20 @@ function isCannotSyncError(error: any): boolean { return false; } +interface SyncContext { + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Partially refactored old code + delta?: any; +} + +interface SyncOptions { + context: SyncContext; + throwOnError: boolean; + saveContextHandler: ((context: SyncContext)=> void)|null; + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Partially refactored old code + onProgress: (progressReport: any)=> void; + syncSteps: string[]; +} + export default class Synchronizer { public static verboseMode = true; @@ -386,35 +400,26 @@ export default class Synchronizer { // 2. DELETE_REMOTE: Delete on the sync target, the items that have been deleted locally. // 3. DELTA: Find on the sync target the items that have been modified or deleted and apply the changes locally. // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - public async start(options: any = null) { - if (!options) options = {}; - + private async syncInternal_(synchronizationId: string, options: SyncOptions) { + // This should be checked for in the calling code if (this.state() !== 'idle') { - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - const error: any = new Error(sprintf('Synchronisation is already in progress. State: %s', this.state())); - error.code = 'alreadyStarted'; - throw error; + throw new Error('state must be idle when starting a sync'); } this.state_ = 'in_progress'; - this.onProgress_ = options.onProgress ? options.onProgress : function() {}; - this.progressReport_ = { errors: [] }; - - const lastContext = options.context ? options.context : {}; - - const syncSteps = options.syncSteps ? options.syncSteps : ['update_remote', 'delete_remote', 'delta']; + const syncSteps = options.syncSteps; + const lastContext: SyncContext = options.context ? options.context : {}; // The default is to log errors, but when testing it's convenient to be able to catch and verify errors const throwOnError = options.throwOnError === true; + this.onProgress_ = options.onProgress; const syncTargetId = this.api().syncTargetId(); this.syncTargetIsLocked_ = false; this.cancelling_ = false; - const synchronizationId = time.unixMs().toString(); - const outputContext = { ...lastContext }; this.progressReport_.startTime = time.unixMs(); @@ -1160,28 +1165,52 @@ export default class Synchronizer { } } - this.progressReport_.completedTime = time.unixMs(); + if (errorToThrow) throw errorToThrow; - this.logSyncOperation('finished', null, null, `Synchronisation finished [${synchronizationId}]`); + return outputContext; + } - await this.logSyncSummary(this.progressReport_); + public async start(options: Partial = {}) { + if (this.state() !== 'idle') { + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied + const error: any = new Error(sprintf('Synchronisation is already in progress. State: %s', this.state())); + error.code = 'alreadyStarted'; + throw error; + } - eventManager.emit(EventName.SyncComplete, { - withErrors: Synchronizer.reportHasErrors(this.progressReport_), - }); + const synchronizationId = time.unixMs().toString(); + this.progressReport_ = { errors: [] }; + const syncSteps = options.syncSteps ?? ['update_remote', 'delete_remote', 'delta']; - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - await checkDisabledSyncItemsNotification((action: any) => this.dispatch(action)); + try { + return await this.syncInternal_(synchronizationId, { + context: options.context ?? {}, + throwOnError: options.throwOnError ?? false, + saveContextHandler: options.saveContextHandler ?? null, + onProgress: options.onProgress ?? (() => {}), + syncSteps, + }); + } finally { + this.state_ = 'idle'; - this.onProgress_ = function() {}; - this.progressReport_ = {}; + this.progressReport_.completedTime = time.unixMs(); - this.dispatch({ type: 'SYNC_COMPLETED', isFullSync: this.isFullSync(syncSteps) }); + this.logSyncOperation('finished', null, null, `Synchronisation finished [${synchronizationId}]`); - this.state_ = 'idle'; + await this.logSyncSummary(this.progressReport_); - if (errorToThrow) throw errorToThrow; + eventManager.emit(EventName.SyncComplete, { + withErrors: Synchronizer.reportHasErrors(this.progressReport_), + }); - return outputContext; + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied + await checkDisabledSyncItemsNotification((action: any) => this.dispatch(action)); + + this.onProgress_ = () => {}; + this.progressReport_ = {}; + + this.dispatch({ type: 'SYNC_COMPLETED', isFullSync: this.isFullSync(syncSteps) }); + + } } } From 5bc36bb6b950c7fb166a6431b62b392e4ad96c54 Mon Sep 17 00:00:00 2001 From: Henry Heino Date: Wed, 9 Oct 2024 10:31:17 -0700 Subject: [PATCH 2/2] Commenting and typing improvements --- packages/lib/Synchronizer.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/lib/Synchronizer.ts b/packages/lib/Synchronizer.ts index ae4c2181854..d07cb45ab95 100644 --- a/packages/lib/Synchronizer.ts +++ b/packages/lib/Synchronizer.ts @@ -399,17 +399,17 @@ export default class Synchronizer { // 1. UPLOAD: Send to the sync target the items that have changed since the last sync. // 2. DELETE_REMOTE: Delete on the sync target, the items that have been deleted locally. // 3. DELTA: Find on the sync target the items that have been modified or deleted and apply the changes locally. - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied + // + // Note: syncInternal_ does not clean up state post sync. private async syncInternal_(synchronizationId: string, options: SyncOptions) { - // This should be checked for in the calling code if (this.state() !== 'idle') { - throw new Error('state must be idle when starting a sync'); + throw new Error('State must be idle when entering syncInternal_'); } this.state_ = 'in_progress'; const syncSteps = options.syncSteps; - const lastContext: SyncContext = options.context ? options.context : {}; + const lastContext: SyncContext = options.context; // The default is to log errors, but when testing it's convenient to be able to catch and verify errors const throwOnError = options.throwOnError === true; @@ -1182,6 +1182,8 @@ export default class Synchronizer { this.progressReport_ = { errors: [] }; const syncSteps = options.syncSteps ?? ['update_remote', 'delete_remote', 'delta']; + // The main sync logic is wrapped in a try { ... } finally { ... } to ensure that + // the sync cleanup logic runs even if syncInternal_ throws an unhandled Error. try { return await this.syncInternal_(synchronizationId, { context: options.context ?? {}, @@ -1203,14 +1205,12 @@ export default class Synchronizer { withErrors: Synchronizer.reportHasErrors(this.progressReport_), }); - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - await checkDisabledSyncItemsNotification((action: any) => this.dispatch(action)); + await checkDisabledSyncItemsNotification((action) => this.dispatch(action)); this.onProgress_ = () => {}; this.progressReport_ = {}; this.dispatch({ type: 'SYNC_COMPLETED', isFullSync: this.isFullSync(syncSteps) }); - } } }