Desktop, Mobile: Automatically retrigger the sync if there are more unsynced outgoing changes when sync completes (#12989)

packages/lib/Synchronizer.ts

@@ -387,7 +387,7 @@ 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) {
+	public async start(options: any = null): Promise<any> {
 		if (!options) options = {};
 
 		if (this.state() !== 'idle') {
@@ -476,6 +476,7 @@ export default class Synchronizer {
 
 		let errorToThrow = null;
 		let syncLock = null;
+		let hasCaughtError = false;
 
 		try {
 			await this.api().initialize();
@@ -611,12 +612,25 @@ export default class Synchronizer {
 
 						// Safety check to avoid infinite loops.
 						// - In fact this error is possible if the item is marked for sync (via sync_time or force_sync) while synchronisation is in
-						//   progress. In that case exit anyway to be sure we aren't in a loop and the item will be re-synced next time.
+						//   progress. When force_sync is not true, this is because the user is typing while the sync is running, so we should continue
+						//   looping, as we don't want the sync to stop when there are still un-synced outgoing changes, otherwise this creates a race condition
+						//   on mobile, where additional changes made during upload are not synced and don't trigger another sync, whereas a change made immediately
+						//   after the sync has finished will trigger another sync. Once the user has stopped typing, it can then break out of the loop and continue
+						//   the rest of the process.
 						// - It can also happen if the item is directly modified in the sync target, and set with an update_time in the future. In that case,
 						//   the local sync_time will be updated to Date.now() but on the next loop it will see that the remote item still has a date ahead
 						//   and will see a conflict. There's currently no automatic fix for this - the remote item on the sync target must be fixed manually
 						//   (by setting an updated_time less than current time).
-						if (donePaths.indexOf(path) >= 0) throw new JoplinError(sprintf('Processing a path that has already been done: %s. sync_time was not updated? Remote item has an updated_time in the future?', path), 'processingPathTwice');
+						if (donePaths.indexOf(path) >= 0) {
+							const syncItem = await BaseItem.syncItem(syncTargetId, local.id, { fields: ['force_sync'] });
+							if (local.updated_time > time.unixMs()) {
+								throw new JoplinError(sprintf('Processing a path that has already been done: %s. Remote item has an updated_time in the future', path), 'processingPathTwice');
+							} else if (syncItem.force_sync) {
+								throw new JoplinError(sprintf('Processing a path that has already been done: %s. Item was marked for sync using force_sync', path), 'processingPathTwice');
+							} else {
+								logger.info(sprintf('Processing a path that has already been done: %s. The user is making changes while the sync is in progress', path));
+							}
+						}
 
 						const remote: RemoteItem = result.neverSyncedItemIds.includes(local.id) ? null : await this.apiCall('stat', path);
 						let action: SyncAction = null;
@@ -1121,6 +1135,8 @@ export default class Synchronizer {
 				}
 			} // DELTA STEP
 		} catch (error) {
+			hasCaughtError = true;
+
 			if (error.code === ErrorCode.MustUpgradeApp) {
 				this.dispatch({
 					type: 'MUST_UPGRADE_APP',
@@ -1163,9 +1179,12 @@ export default class Synchronizer {
 
 		this.syncTargetIsLocked_ = false;
 
+		let cancelledBeforeClearedState = false;
+
 		if (this.cancelling()) {
 			logger.info('Synchronisation was cancelled.');
 			this.cancelling_ = false;
+			cancelledBeforeClearedState = true;
 		}
 
 		this.progressReport_.completedTime = time.unixMs();
@@ -1174,8 +1193,10 @@ export default class Synchronizer {
 
 		await this.logSyncSummary(this.progressReport_);
 
+		const hasErrors = Synchronizer.reportHasErrors(this.progressReport_);
+
 		eventManager.emit(EventName.SyncComplete, {
-			withErrors: Synchronizer.reportHasErrors(this.progressReport_),
+			withErrors: hasErrors,
 		});
 
 		// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
@@ -1190,6 +1211,19 @@ export default class Synchronizer {
 
 		if (errorToThrow) throw errorToThrow;
 
+		// If there are any un-synced outgoing changes made up to the point just before the sync completes, then trigger the sync again to reduce the likelihood
+		// that the user will close or minimise the app when there are un-synced changes, because the sync is reported as completed.
+		// IMPORTANT: This must be the very last step in the sync, to avoid any window to allow an un-synced change to get missed
+		if (!hasErrors && !hasCaughtError && !cancelledBeforeClearedState && !this.cancelling()) {
+			const result = await BaseItem.itemsThatNeedSync(syncTargetId);
+			options.context = outputContext;
+
+			if (result.items.length > 0) {
+				logger.info('There are more outgoing changes to sync, trigger the sync again');
+				return await this.start(options);
+			}
+		}
+
 		return outputContext;
 	}
 }