diff --git a/packages/models/src/Domain/Runtime/Deltas/Conflict.spec.ts b/packages/models/src/Domain/Runtime/Deltas/Conflict.spec.ts index 63c97883f7b..edd117ac5db 100644 --- a/packages/models/src/Domain/Runtime/Deltas/Conflict.spec.ts +++ b/packages/models/src/Domain/Runtime/Deltas/Conflict.spec.ts @@ -99,4 +99,22 @@ describe('conflict delta', () => { expect(delta.getConflictStrategy()).toBe(ConflictStrategy.KeepApply) }) + + it('if keep base strategy, always use the apply payloads updated_at_timestamp', () => { + const basePayload = createDecryptedItemsKey('123', 'secret', 2) + + const baseCollection = createBaseCollection(basePayload) + + const applyPayload = createDecryptedItemsKey('123', 'other secret', 1) + + const delta = new ConflictDelta(baseCollection, basePayload, applyPayload, historyMap) + + expect(delta.getConflictStrategy()).toBe(ConflictStrategy.KeepBaseDuplicateApply) + + const result = delta.result() + + expect(result.emits).toHaveLength(1) + + expect(result.emits[0].updated_at_timestamp).toEqual(applyPayload.updated_at_timestamp) + }) }) diff --git a/packages/models/src/Domain/Runtime/Deltas/Conflict.ts b/packages/models/src/Domain/Runtime/Deltas/Conflict.ts index 07f8fb10149..843fafe3fff 100644 --- a/packages/models/src/Domain/Runtime/Deltas/Conflict.ts +++ b/packages/models/src/Domain/Runtime/Deltas/Conflict.ts @@ -1,4 +1,4 @@ -import { greaterOfTwoDates, uniqCombineObjArrays } from '@standardnotes/utils' +import { uniqCombineObjArrays } from '@standardnotes/utils' import { ImmutablePayloadCollection } from '../Collection/Payload/ImmutablePayloadCollection' import { CreateDecryptedItemFromPayload, CreateItemFromPayload } from '../../Utilities/Item/ItemGenerator' import { HistoryMap, historyMapFunctions } from '../History/HistoryMap' @@ -114,9 +114,9 @@ export class ConflictDelta { } private handleKeepBaseStrategy(): SyncResolvedPayload[] { - const updatedAt = greaterOfTwoDates(this.basePayload.serverUpdatedAt, this.applyPayload.serverUpdatedAt) + const updatedAt = this.applyPayload.serverUpdatedAt - const updatedAtTimestamp = Math.max(this.basePayload.updated_at_timestamp, this.applyPayload.updated_at_timestamp) + const updatedAtTimestamp = this.applyPayload.updated_at_timestamp const leftPayload = this.basePayload.copyAsSyncResolved( { @@ -146,9 +146,9 @@ export class ConflictDelta { } private handleKeepBaseDuplicateApplyStrategy(): SyncResolvedPayload[] { - const updatedAt = greaterOfTwoDates(this.basePayload.serverUpdatedAt, this.applyPayload.serverUpdatedAt) + const updatedAt = this.applyPayload.serverUpdatedAt - const updatedAtTimestamp = Math.max(this.basePayload.updated_at_timestamp, this.applyPayload.updated_at_timestamp) + const updatedAtTimestamp = this.applyPayload.updated_at_timestamp const leftPayload = this.basePayload.copyAsSyncResolved( { @@ -201,9 +201,9 @@ export class ConflictDelta { 'content_type', ]) - const updatedAt = greaterOfTwoDates(this.basePayload.serverUpdatedAt, this.applyPayload.serverUpdatedAt) + const updatedAt = this.applyPayload.serverUpdatedAt - const updatedAtTimestamp = Math.max(this.basePayload.updated_at_timestamp, this.applyPayload.updated_at_timestamp) + const updatedAtTimestamp = this.applyPayload.updated_at_timestamp const payload = this.basePayload.copyAsSyncResolved( { diff --git a/packages/snjs/mocha/sync_tests/conflicting.test.js b/packages/snjs/mocha/sync_tests/conflicting.test.js index 87dd0100ce6..07d3be138a6 100644 --- a/packages/snjs/mocha/sync_tests/conflicting.test.js +++ b/packages/snjs/mocha/sync_tests/conflicting.test.js @@ -885,6 +885,38 @@ describe('online conflict handling', function () { await this.sharedFinalAssertions() }) + it('conflict where server updated_at_timestamp is less than base updated_at should not result in infinite loop', async function () { + /** + * While this shouldn't happen, I've seen this happen locally where a single UserPrefs object has a timestamp of A + * on the server, and A + 10 on the client side. Somehow the client had a newer timestamp than the server. The + * server rejects any change if the timestamp is not exactly equal. When we use the KeepBase strategy during conflict + * resolution, we keep the base item, but give it the timestamp of the server item, so that the server accepts it. + * However, RemoteDataConflict would only take the server's timestamp if it was greater than the base's timestamp. + * Because this was not the case, the client kept sending up its own base timestamp and the server kept rejecting it, + * and it never resolved. The fix made here was to take the server's timestamp no matter what, even if it is less than client's. + */ + const note = await Factory.createSyncedNote(this.application) + this.expectedItemCount++ + + /** First modify the item without saving so that our local contents digress from the server's */ + await this.application.mutator.changeItem(note, (mutator) => { + mutator.title = `${Math.random()}` + }) + const modified = note.payload.copy({ + updated_at_timestamp: note.payload.updated_at_timestamp + 1, + content: { + ...note.content, + title: Math.random(), + }, + dirty: true, + }) + this.expectedItemCount++ + await this.application.itemManager.emitItemFromPayload(modified) + await this.application.sync.sync() + expect(this.application.itemManager.getDisplayableNotes().length).to.equal(2) + await this.sharedFinalAssertions() + }) + it('conflicting should not over resolve', async function () { /** * Before refactoring to use dirtyIndex instead of dirtiedDate, sometimes an item could be dirtied