Desktop, Mobile, Cli: Prevent race which allows changes to be lost, during a long delta step (#16208)

packages/lib/Synchronizer.ts

@@ -5,7 +5,7 @@ import shim from './shim';
 import MigrationHandler from './services/synchronizer/MigrationHandler';
 import eventManager, { EventName } from './eventManager';
 import { _ } from './locale';
-import BaseItem from './models/BaseItem';
+import BaseItem, { RemoteItemMetadata } from './models/BaseItem';
 import Folder from './models/Folder';
 import Note from './models/Note';
 import Resource from './models/Resource';
@@ -903,6 +903,7 @@ export default class Synchronizer {
 				while (true) {
 					if (this.cancelling() || hasCancelled) break;
 
+					let localItemMetadata: Map<string, RemoteItemMetadata> = null;
 					const listResult: PaginatedList = await this.apiCall('delta', '', {
 						context: context,
 
@@ -917,7 +918,8 @@ export default class Synchronizer {
 
 						// This is only used by the basic delta
 						allItemMetadataHandler: async () => {
-							return BaseItem.remoteItemMetadata(syncTargetId);
+							localItemMetadata = await BaseItem.remoteItemMetadata(syncTargetId);
+							return localItemMetadata;
 						},
 
 						wipeOutFailSafe: Setting.value('sync.wipeOutFailSafe'),
@@ -1017,14 +1019,18 @@ export default class Synchronizer {
 										// Nothing to do, and no need to fetch the content
 									} else {
 										content = await loadContent();
-										if (content && content.updated_time > local.updated_time) {
+										// Load the latest updated_time, otherwise a change made during a long delta step could overwrite the local version without making a conflict
+										const latestLocalState = await ItemClass.load(remoteId, { fields: ['updated_time'] });
+										const localUpdatedTime = latestLocalState ? latestLocalState.updated_time : local.updated_time;
+										if (content && content.updated_time > localUpdatedTime) {
 											action = SyncAction.UpdateLocal;
 											reason = 'remote is more recent than local';
 										} else if (enableEnhancedBasicDeltaAlgorithm()) {
 											// When the enhanced basic delta algorithm is first used, all items are rescanned and we need to persist the remoteItemUpdatedTime
 											// to set up the initial synced state. This also catches the case if content.updated_time < local.updated_time due to manual manipulation
 											// of the md files, to prevent these items being continually fetched on every sync
-											await ItemClass.saveSyncTime(syncTargetId, local, local.updated_time, remote.updated_time);
+											const syncTime = localItemMetadata.get(local.id)?.sync_time ?? 0;
+											await ItemClass.saveSyncTime(syncTargetId, local, syncTime, remote.updated_time);
 										}
 									}
 								}

packages/lib/file-api.test.ts

@@ -61,6 +61,7 @@ const syncOptions = (noteId: string, localUpdatedTime: number, contextTimestamp:
 	const metadata = {
 		item_id: noteId,
 		updated_time: localUpdatedTime,
+		sync_time: localUpdatedTime,
 	};
 
 	if (noteId) {

packages/lib/models/BaseItem.ts

@@ -45,6 +45,7 @@ export interface ItemsThatNeedSyncResult {
 export interface RemoteItemMetadata {
 	item_id: string;
 	updated_time: number;
+	sync_time: number;
 }
 
 export interface EncryptedItemsStats {
@@ -209,12 +210,13 @@ export default class BaseItem extends BaseModel {
 
 	public static async remoteItemMetadata(syncTarget: number): Promise<Map<string, RemoteItemMetadata>> {
 		if (!syncTarget) throw new Error('No syncTarget specified');
-		const temp = await this.db().selectAll('SELECT item_id, remote_item_updated_time FROM sync_items WHERE sync_time > 0 AND sync_target = ?', [syncTarget]);
+		const temp = await this.db().selectAll('SELECT item_id, remote_item_updated_time, sync_time FROM sync_items WHERE sync_time > 0 AND sync_target = ?', [syncTarget]);
 		const output = new Map<string, RemoteItemMetadata>();
 		for (let i = 0; i < temp.length; i++) {
 			const metadata: RemoteItemMetadata = {
 				item_id: temp[i].item_id,
 				updated_time: temp[i].remote_item_updated_time,
+				sync_time: temp[i].sync_time,
 			};
 			output.set(temp[i].item_id, metadata);
 		}

packages/lib/services/synchronizer/Synchronizer.conflicts.test.ts

@@ -78,6 +78,43 @@ describe('Synchronizer.conflicts', () => {
 		expect(syncItem.sync_time).toBeLessThan(note1.updated_time);
 	}));
 
+	it('should leave changes made after the upload phase for the next sync', (async () => {
+		const folder = await Folder.save({ title: 'folder' });
+		const note = await Note.save({ title: 'original', parent_id: folder.id });
+		await synchronizerStart();
+
+		await switchClient(2);
+		await synchronizerStart();
+		await sleep(0.1);
+		await Note.save({ id: note.id, title: 'remote change' });
+		await synchronizerStart();
+
+		await switchClient(1);
+		let localChange: NoteEntity = null;
+		const loadItemsByIds = BaseItem.loadItemsByIds.bind(BaseItem);
+		const loadItemsByIdsMock = jest.spyOn(BaseItem, 'loadItemsByIds').mockImplementation(async ids => {
+			const items = await loadItemsByIds(ids);
+			if (ids.includes(note.id)) {
+				await sleep(0.1);
+				localChange = await Note.save({ id: note.id, title: 'local change' });
+			}
+			return items;
+		});
+
+		await synchronizerStart(null, { syncSteps: ['delta'] });
+		loadItemsByIdsMock.mockRestore();
+
+		expect((await Note.load(note.id)).title).toBe('local change');
+		const syncItem = await BaseItem.syncItem(syncTargetId(), note.id, { fields: ['sync_time'] });
+		expect(syncItem.sync_time).toBeLessThan(localChange.updated_time);
+
+		// The following upload phase sees both changes and uses the existing
+		// conflict handling path.
+		await synchronizerStart();
+		expect((await Note.load(note.id)).title).toBe('remote change');
+		expect((await Note.conflictedNotes()).map(note => note.title)).toContain('local change');
+	}));
+
 	it('should resolve folders conflicts', (async () => {
 		const folder1 = await Folder.save({ title: 'folder1' });
 		await Note.save({ title: 'un', parent_id: folder1.id });