core/metadata: Consider single side as up-to-date (#2098)

It appears we can end up in situations where the only remaining side
is smaller than the target.
This can lead to conflicts when merging a file modification on the
same side when we don't expect it.

Although we don't know how we get into this situation (it could be
related to multiple sync errors on the other side which is later
dissociated), we want to mitigate its consequences by making sure the
`Metadata.isUpToDate()` and `Metadata.isAtLeastUpToDate()` methods
consider a single side as up-to-date.

core/metadata.js

@@ -476,13 +476,26 @@ function extractRevNumber(doc /*: { _rev: string } */) {
   }
 }
 
-// Return true if the remote file is up-to-date for this document
+// See isAtLeastUpToDate for why we have different checks when we have both
+// sides and when we don't.
 function isUpToDate(sideName /*: SideName */, doc /*: Metadata */) {
-  return side(doc, sideName) === target(doc)
+  return hasBothSides(doc)
+    ? side(doc, sideName) === target(doc)
+    : side(doc, sideName) > 0
 }
 
+// It appears we can end up in situations where the only side left is smaller
+// than the target.
+// Since this function is meant to detect when it is safe to merge a change on
+// one side because no changes were merged on the other one, we'll assume the
+// remaining side is up-to-date (or at least up-to-date) if it's present.
+//
+// FIXME: find out how we end up in this situation, fix it and remove this
+// mitigation.
 function isAtLeastUpToDate(sideName /*: SideName */, doc /*: Metadata */) {
-  return side(doc, sideName) >= target(doc)
+  return hasBothSides(doc)
+    ? side(doc, sideName) >= target(doc)
+    : side(doc, sideName) > 0
 }
 
 function removeActionHints(doc /*: Metadata */) {

core/sync/index.js

@@ -615,6 +615,10 @@ class Sync {
       log.debug({ path: doc.path }, `Applying else for ${doc.docType} change`)
       let old
       try {
+        // FIXME: This is extremely fragile as we don't enforce updating the
+        // sides and target when saving a new record version.
+        // This means the previous version fetched here could be different from
+        // the one we're expecting.
         old = (await this.pouch.getPreviousRev(
           doc._id,
           doc.sides.target - currentRev
@@ -630,12 +634,10 @@ class Sync {
           } else {
             await side.updateFolderAsync(doc)
           }
+        } else if (metadata.sameFile(old, doc)) {
+          log.debug({ path: doc.path }, 'Ignoring timestamp-only change')
         } else if (metadata.sameBinary(old, doc)) {
-          if (metadata.sameFile(old, doc)) {
-            log.debug({ path: doc.path }, 'Ignoring timestamp-only change')
-          } else {
-            await side.updateFileMetadataAsync(doc)
-          }
+          await side.updateFileMetadataAsync(doc)
         } else {
           await side.overwriteFileAsync(doc, old)
           this.events.emit('transfer-started', _.clone(doc))
@@ -827,15 +829,15 @@ class Sync {
     change /*: MetadataChange */,
     err /*: SyncError */
   ) /*: Promise<void> */ {
-    let { doc } = change
-    if (!doc.errors) doc.errors = 0
-    doc.errors++
-
-    // Make sure isUpToDate(sourceSideName, doc) is still true
-    const sourceSideName = otherSide(err.sideName)
-    metadata.markSide(sourceSideName, doc, doc)
+    const { doc } = change
 
     try {
+      doc.errors = (doc.errors || 0) + 1
+
+      // Make sure isUpToDate(sourceSideName, doc) is still true
+      const sourceSideName = otherSide(err.sideName)
+      metadata.markSide(sourceSideName, doc, doc)
+
       await this.pouch.db.put(doc)
     } catch (err) {
       // If the doc can't be saved, it's because of a new revision.

test/unit/metadata.js

@@ -347,7 +347,7 @@ describe('metadata', function() {
       should(metadata.isUpToDate('local', doc)).be.false()
     })
 
-    it('is true when the given side equals the short rev in doc', () => {
+    it('is true when the given side equals the target in doc', () => {
       const doc = builders
         .metafile()
         .rev('2-0123456')
@@ -356,14 +356,93 @@ describe('metadata', function() {
       should(metadata.isUpToDate('local', doc)).be.true()
     })
 
-    it('is false when the given side is lower than the short rev in doc', () => {
+    it('is false when the given side is lower than the target in doc', () => {
       const doc = builders
         .metafile()
         .rev('3-0123456')
         .sides({ remote: 3, local: 2 })
         .build()
       should(metadata.isUpToDate('local', doc)).be.false()
     })
+
+    it('is true when the given side is the only one', () => {
+      const doc = builders
+        .metafile()
+        .rev('3-0123456')
+        .sides({ local: 2 })
+        .build()
+      should(metadata.isUpToDate('local', doc)).be.true()
+    })
+
+    // XXX: We implemented the same workaround as in `isAtLeastUpToDate()`
+    // although we haven't encountered the same issue yet but it is possible.
+    it('is true when the given side is the only one and lower than the target', () => {
+      const doc = builders
+        .metafile()
+        .rev('3-0123456')
+        .build()
+      doc.sides = { local: 2, target: 35 }
+      should(metadata.isUpToDate('local', doc)).be.true()
+    })
+  })
+
+  describe('isAtLeastUpToDate', () => {
+    it('is false when the given side is undefined in doc', function() {
+      const doc = builders
+        .metafile()
+        .rev('1-0123456')
+        .sides({ remote: 1 })
+        .build()
+      should(metadata.isAtLeastUpToDate('local', doc)).be.false()
+    })
+
+    it('is true when the given side equals the target in doc', () => {
+      const doc = builders
+        .metafile()
+        .rev('2-0123456')
+        .sides({ remote: 2, local: 2 })
+        .build()
+      should(metadata.isAtLeastUpToDate('local', doc)).be.true()
+    })
+
+    it('is true when the given side is greater than the target in doc', () => {
+      const doc = builders
+        .metafile()
+        .rev('3-0123456')
+        .sides({ remote: 3, local: 4 })
+        .build()
+      should(metadata.isAtLeastUpToDate('local', doc)).be.true()
+    })
+
+    it('is false when the given side is lower than the target in doc', () => {
+      const doc = builders
+        .metafile()
+        .rev('3-0123456')
+        .sides({ remote: 3, local: 2 })
+        .build()
+      should(metadata.isAtLeastUpToDate('local', doc)).be.false()
+    })
+
+    it('is true when the given side is the only one', () => {
+      const doc = builders
+        .metafile()
+        .rev('3-0123456')
+        .sides({ local: 2 })
+        .build()
+      should(metadata.isAtLeastUpToDate('local', doc)).be.true()
+    })
+
+    // XXX: It is yet unknown how we end up in this situation but it seems like
+    // it can happen when we have sync errors and maybe some side dissociation.
+    // Until we figure out the root cause, we try to prevent its consequences.
+    it('is true when the given side is the only one and lower than the target', () => {
+      const doc = builders
+        .metafile()
+        .rev('3-0123456')
+        .build()
+      doc.sides = { local: 2, target: 35 }
+      should(metadata.isAtLeastUpToDate('local', doc)).be.true()
+    })
   })
 
   describe('assignMaxDate', () => {

test/unit/sync.js

@@ -607,8 +607,6 @@ describe('Sync', function() {
       const actual = await this.pouch.bySyncedPath(doc.path)
       should(actual.errors).equal(1)
       should(actual._rev).not.equal(doc._rev)
-      should(actual.sides).deepEqual({ target: 2, local: 2 })
-      should(metadata.isUpToDate('local', actual)).be.true()
     })
 
     it('retries on second remote -> local sync error', async function() {
@@ -627,8 +625,6 @@ describe('Sync', function() {
       const actual = await this.pouch.bySyncedPath(doc.path)
       should(actual.errors).equal(2)
       should(actual._rev).not.equal(doc._rev)
-      should(actual.sides).deepEqual({ target: 5, local: 2, remote: 5 })
-      should(metadata.isUpToDate('remote', actual)).be.true()
     })
   })