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

[personalizedrefrigerator]

Summary

This pull request refactors Synchronizer.start to ensure that its cleanup logic runs, even if there is an uncaught Error. This fixes an issue originally reported on the forum — an error while removing sync locks could prevent sync cleanup logic from running.

Testing plan

Locally, I have verified that the following automated test passes:

Show test

diff --git a/packages/lib/services/synchronizer/Synchronizer.basics.test.ts b/packages/lib/services/synchronizer/Synchronizer.basics.test.ts
index 044649dfb..33d3544c3 100644
--- a/packages/lib/services/synchronizer/Synchronizer.basics.test.ts
+++ b/packages/lib/services/synchronizer/Synchronizer.basics.test.ts
@@ -8,6 +8,7 @@ import WelcomeUtils from '../../WelcomeUtils';
 import { NoteEntity } from '../database/types';
 import { fetchSyncInfo, setAppMinVersion, uploadSyncInfo } from './syncInfoUtils';
 import { ErrorCode } from '../../errors';
+import { LockType } from './LockHandler';
 
 describe('Synchronizer.basics', () => {
 
@@ -461,6 +462,35 @@ describe('Synchronizer.basics', () => {
 		expect(remotes.find(r => r.path === `${note.id}.md`)).toBeTruthy();
 	}));
 
+	it('should recover from failures to release sync locks', async () => {
+		await synchronizer().start();
+
+		const lockHandler = synchronizer().lockHandler();
+
+		let allowLockRelease = false;
+
+		// Use .spyOn to simulate an error that might, for example, be caused by network
+		// failure.
+		const spy = jest.spyOn(lockHandler, 'releaseLock').mockImplementation(async (type) => {
+			if (type === LockType.Sync && !allowLockRelease) {
+				throw new Error('Testing lock release failure');
+			}
+		});
+
+		let syncError: Error|null = null;
+		try {
+			await synchronizer().start();
+		} catch (error) {
+			syncError = error;
+		}
+		expect(syncError).not.toBeNull();
+		spy.mockClear();
+
+		// Should sync successfully, after failure
+		allowLockRelease = true;
+		await synchronizerStart();
+	});
+
 	it('should throw an error if the app version is not compatible with the sync target info', (async () => {
 		await synchronizerStart();

This test is not included in this pull request because it relies on .spyOn to mock a method of LockHandler. This seems fragile and likely to break in the future, particularly if sync locks are changed significantly or removed.

Additionally, I have verified that a Dropbox sync completes successfully and reports the number of fetched and created items.

[laurent22]

Does this pull request need to be merged, now that we're going to remove the sync locks?

[personalizedrefrigerator]

Does this pull request need to be merged, now that we're going to remove the sync locks?

I'm not sure...

• Reasons to merge this pull request:
  • Ensures that sync cleanup runs even if there is an uncaught error in the main sync logic.
    • This may make it easier to update the sync logic in the future.
  • Improves type safety.
• Reason not to merge this pull request:
  • Breaks sync up into multiple functions, which may make sync logic more difficult to understand.

[laurent22]

Ensures that sync cleanup runs even if there is an uncaught error in the main sync logic.

Most of the sync logic was already in a big try/catch block. Or was there something outside of the block that was throwing an exception? And in which case, can that be moved within the block?

[personalizedrefrigerator]

Most of the sync logic was already in a big try/catch block. Or was there something outside of the block that was throwing an exception? And in which case, can that be moved within the block?

There are at least two places where an error can result in state_ not being reset to idle:

• If an error occurs that is not ErrorCode.IsReadOnly while updating share IDs:

  • joplin/packages/lib/Synchronizer.ts

    Line 460 in 8fb85ab

    throw error;

• If an error occurs while releasing sync locks:

  • joplin/packages/lib/Synchronizer.ts

    Line 1143 in 8fb85ab

    await this.lockHandler().releaseLock(LockType.Sync, this.lockClientType(), this.clientId_);

It should be possible to move both of these into the try/catch block. I'm closing this pull request and plan to create a new one with this change.