fix(snjs): keep apply payload timestamp when using keep base conflict strategy (#2031)

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)
+  })
 })

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(
       {

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