diff --git a/packages/lib/Synchronizer.ts b/packages/lib/Synchronizer.ts index 03eac85d156..a322b52840a 100644 --- a/packages/lib/Synchronizer.ts +++ b/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 = 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()) { + } else if (content && 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); } } } diff --git a/packages/lib/file-api.test.ts b/packages/lib/file-api.test.ts index 40db7328768..0e2efb50197 100644 --- a/packages/lib/file-api.test.ts +++ b/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) { diff --git a/packages/lib/models/BaseItem.ts b/packages/lib/models/BaseItem.ts index e97c82052ec..476dfde9883 100644 --- a/packages/lib/models/BaseItem.ts +++ b/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> { 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(); 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); } diff --git a/packages/lib/services/synchronizer/Synchronizer.conflicts.test.ts b/packages/lib/services/synchronizer/Synchronizer.conflicts.test.ts index 17374913a1e..11d1acd39ce 100644 --- a/packages/lib/services/synchronizer/Synchronizer.conflicts.test.ts +++ b/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 });