Cli,Desktop,Mobile: Fix sync reported as in progress if an error occurs while removing sync locks

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;
@@ -385,36 +399,27 @@ 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
-	public async start(options: any = null) {
-		if (!options) options = {};
-
+	//
+	// Note: syncInternal_ does not clean up state post sync.
+	private async syncInternal_(synchronizationId: string, options: SyncOptions) {
 		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 entering syncInternal_');
 		}
 
 		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;
 
 		// 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<SyncOptions> = {}) {
+		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));
+		// 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 ?? {},
+				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;
+			await checkDisabledSyncItemsNotification((action) => this.dispatch(action));
+
+			this.onProgress_ = () => {};
+			this.progressReport_ = {};
+
+			this.dispatch({ type: 'SYNC_COMPLETED', isFullSync: this.isFullSync(syncSteps) });
+		}
 	}
 }