diff --git a/packages/lib/Synchronizer.ts b/packages/lib/Synchronizer.ts index 65aedf24c5..de67a613a8 100644 --- a/packages/lib/Synchronizer.ts +++ b/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 { 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; } }