fix: Properly order dependent moves in sync (#2447)

  `compareChanges` returned `KEEP_ORDER` for two chained moves where one
  frees the path the other takes (e.g. `i` → `i*` followed by
  `j` → `i`). The dependent move could run before its destination was
  freed, failing with a "destination already exists" error.

  We now detect this case in `compareChanges` — both ops are `MOVE` with
  a `moveFrom` and one's destination equals the other's source — and
  order the freeing move first.

  Fixes #2445

Author: taratatach

core/sync/index.js

@@ -204,6 +204,17 @@ const compareChanges = (
       // Handle move to dir after dir addition
       if (docA.path.startsWith(docB.path + sep)) return B_FIRST
     }
+    if (
+      opA.type === 'MOVE' &&
+      opB.type === 'MOVE' &&
+      docA.moveFrom != null &&
+      docB.moveFrom != null
+    ) {
+      // Handle chained rename: A frees the path B will take (e.g. i->i*
+      // must happen before j->i so the destination path is free).
+      if (docB.path === docA.moveFrom.path) return A_FIRST
+      if (docA.path === docB.moveFrom.path) return B_FIRST
+    }
   }
 
   return KEEP_ORDER

test/unit/sync/index.js

@@ -1745,5 +1745,88 @@ describe('Sync', function() {
         })
       }
     )
+
+    context(
+      'with two chained moves where one frees the path the other takes',
+      () => {
+        let moveAToC, moveBToA
+        beforeEach(async function() {
+          // i -> i*  (must happen first: frees the path "i")
+          const srcA = await builders
+            .metadir()
+            .path('i')
+            .upToDate()
+            .create()
+          const dstA = await builders
+            .metadir()
+            .moveFrom(srcA)
+            .path('i*')
+            .changedSide('local')
+            .create()
+          // j -> i  (must happen second: takes the freed path "i")
+          const srcB = await builders
+            .metadir()
+            .path('j')
+            .upToDate()
+            .create()
+          const dstB = await builders
+            .metadir()
+            .moveFrom(srcB)
+            .path('i')
+            .changedSide('local')
+            .create()
+
+          moveAToC = makeChange(dstA, 'MOVE', this)
+          moveBToA = makeChange(dstB, 'MOVE', this)
+        })
+
+        it('returns -1 when the freeing move is passed first', () => {
+          should(compareChanges(moveAToC, moveBToA)).eql(-1)
+        })
+
+        it('returns 1 when the freeing move is passed second', () => {
+          should(compareChanges(moveBToA, moveAToC)).eql(1)
+        })
+      }
+    )
+
+    context(
+      'with two unrelated moves where neither frees a path the other takes',
+      () => {
+        let moveX, moveY
+        beforeEach(async function() {
+          const srcX = await builders
+            .metadir()
+            .path('x')
+            .upToDate()
+            .create()
+          const dstX = await builders
+            .metadir()
+            .moveFrom(srcX)
+            .path('x-renamed')
+            .changedSide('local')
+            .create()
+          const srcY = await builders
+            .metadir()
+            .path('y')
+            .upToDate()
+            .create()
+          const dstY = await builders
+            .metadir()
+            .moveFrom(srcY)
+            .path('y-renamed')
+            .changedSide('local')
+            .create()
+
+          moveX = makeChange(dstX, 'MOVE', this)
+          moveY = makeChange(dstY, 'MOVE', this)
+        })
+
+        it('returns 0', () => {
+          should(compareChanges(moveX, moveY)).eql(0)
+          should(compareChanges(moveY, moveX)).eql(0)
+        })
+      }
+    )
   })
 })

test/unit/sync/multiple_errors.js

@@ -244,6 +244,85 @@ describe('Multiple sync errors', function() {
       this.sync.scheduleRetry.restore()
     })
 
+    it('blocks a chained move when the move freeing its path fails', async function() {
+      // Reproduces the remote rename case: i->i* (blocked on linux because
+      // `*` is a reserved char) followed by j->i (must wait for i->i* to
+      // free the destination path). Before the fix, j->i was attempted
+      // anyway and failed with a "destination already exists" error.
+      const srcI = await builders
+        .metadir()
+        .path('i')
+        .upToDate()
+        .create()
+      const dstIStar = await builders
+        .metadir()
+        .moveFrom(srcI)
+        .path('i*')
+        .changedSide('local')
+        .create()
+      const srcJ = await builders
+        .metadir()
+        .path('j')
+        .upToDate()
+        .create()
+      const dstIFromJ = await builders
+        .metadir()
+        .moveFrom(srcJ)
+        .path('i')
+        .changedSide('local')
+        .create()
+
+      const changeI = {
+        changes: [{ rev: dstIStar._rev }],
+        doc: dstIStar,
+        id: dstIStar._id,
+        seq: 40,
+        operation: { type: 'MOVE', side: 'local' }
+      }
+      const changeJ = {
+        changes: [{ rev: dstIFromJ._rev }],
+        doc: dstIFromJ,
+        id: dstIFromJ._id,
+        seq: 41,
+        operation: { type: 'MOVE', side: 'local' }
+      }
+
+      sinon
+        .stub(this.sync, 'getNextChanges')
+        .onFirstCall()
+        .resolves([changeI, changeJ])
+        .onSecondCall()
+        .resolves([])
+
+      const applyStub = sinon.stub(this.sync, 'apply')
+      applyStub.callsFake(async change => {
+        if (change.id === dstIStar._id) throw blockingSyncError(change.doc)
+        if (change.id === dstIFromJ._id) {
+          throw new Error('j->i must not be applied while i->i* is blocked')
+        }
+        throw new Error(`Unexpected apply call for ${change.id}`)
+      })
+
+      sinon.stub(this.sync, 'scheduleRetry').resolves()
+
+      await this.sync.syncBatch()
+
+      // i->i* was attempted (and blocked)...
+      should(applyStub).have.been.calledOnce()
+      should(applyStub.args[0][0].id).equal(dstIStar._id)
+
+      // ...and j->i was skipped as a dependent of the blocked i->i*.
+      const appliedIds = applyStub.args.map(args => args[0].id)
+      should(appliedIds).not.containEql(dstIFromJ._id)
+
+      // localSeq frozen at the failed change's predecessor (no success).
+      should(await this.pouch.getLocalSeq()).equal(0)
+
+      this.sync.getNextChanges.restore()
+      this.sync.apply.restore()
+      this.sync.scheduleRetry.restore()
+    })
+
     it('freezes localSeq at last success before first failure (crash recovery)', async function() {
       const docA = await builders
         .metafile()