feat: Update local metadata when merging trashing

  By keeping track of the remote metadata `trashed` attribute in PouchDB
  we're able to detect the restoration of the remote file from the Cozy
  trash before we had a chance to trash the local file and thus avoid
  it.

  However, we don't have any metadata in local trashing events (the file
  does not exist anymore so we can't get any from the local filesystem)
  so we were simply looking for the matching document in PouchDB and
  marking it with a `trashed` attribute, not making any change to the
  local metadata.
  This means that if we would receive a subsequent event with the exact
  same metadata as the "old" local one, we would find it perfectly equal
  to the local metadata in PouchDB thus not save it and thus not cancel
  the trashing.

  It's unclear to this point how this situation can happen but we've
  seen production logs with the trashing of the file during the local
  initial scan because the `scan` event would be handled after we
  generated the `deleted` event.
  Adding the `trashed` attribute in the local metadata would at least
  avoid the consequences.

core/merge.js

@@ -871,22 +871,22 @@ class Merge {
         delete was.overwrite
 
         if (overwrite) {
-          overwrite.trashed = true
           metadata.markSide(side, overwrite, overwrite)
+          metadata.markAsTrashed(overwrite, side)
           delete overwrite._rev
 
           await this.save(overwrite)
         }
       }
 
       metadata.markSide(side, was, was)
-      // Save the updated side metadata. We only save the remote metadata for
-      // now as we don't have updated local metadata when a document is trashed
-      // on the local filesystem.
       if (side === 'remote') {
         was.remote = doc.remote
+      } else {
+        was.local = doc.local
       }
-      was.trashed = true
+      metadata.markAsTrashed(was, side)
+
       try {
         return await this.save(was)
       } catch (err) {
@@ -904,7 +904,7 @@ class Merge {
       { path: was.path, side, was, doc, sentry: true },
       'marking document for deletion while not linked to the trashed one'
     )
-    was.trashed = true
+    metadata.markAsTrashed(was, side)
     return this.save(was)
   }
 

core/metadata.js

@@ -514,6 +514,23 @@ function dissociateLocal(doc /*: Metadata */) {
   if (doc.local) delete doc.local
 }
 
+function markAsTrashed(doc /*: Metadata */, sideName /*: SideName */) {
+  if (sideName === 'remote') {
+    if (doc.remote && doc.remote.type === REMOTE_DIR_TYPE) {
+      // FIXME: Remote directories have no `trashed` attribute so we know
+      // they're trashed when their path is within the remote trashbin. We
+      // should find a way to reconstruct that path or stop relying on this
+      // function altogether.
+    } else {
+      doc.remote.trashed = true
+    }
+  } else if (doc.local) {
+    doc.local.trashed = true
+  }
+
+  doc.trashed = true
+}
+
 function markAsUnsyncable(doc /*: SavedMetadata */) {
   removeActionHints(doc)
   // Cannot be done in removeActionHints as markAsUnmerged uses it as well and
@@ -972,6 +989,7 @@ module.exports = {
   removeNoteMetadata,
   dissociateRemote,
   dissociateLocal,
+  markAsTrashed,
   markAsUnmerged,
   markAsUnsyncable,
   markAsUpToDate,

core/prep.js

@@ -4,6 +4,7 @@
  */
 
 const autoBind = require('auto-bind')
+const _ = require('lodash')
 
 const metadata = require('./metadata')
 const logger = require('./utils/logger')
@@ -185,7 +186,13 @@ class Prep {
       // XXX: Local deletions don't generate new records as we don't have any
       // information about deleted files and folders (while we have new metadata
       // for remote documents that were trashed).
-      return this.merge.trashFileAsync(side, was, was)
+      //
+      // To make sure we always have a complete record update, we generate a
+      // fake modified record based on the existing one.
+      const doc = _.cloneDeep(was)
+      metadata.markAsTrashed(doc, side)
+
+      return this.merge.trashFileAsync(side, was, doc)
     } else {
       metadata.ensureValidPath(doc)
       doc.docType = metadata.FILE
@@ -207,7 +214,13 @@ class Prep {
       // XXX: Local deletions don't generate new records as we don't have any
       // information about deleted files and folders (while we have new metadata
       // for remote documents that were trashed).
-      return this.merge.trashFolderAsync(side, was, was)
+      //
+      // To make sure we always have a complete record update, we generate a
+      // fake modified record based on the existing one.
+      const doc = _.cloneDeep(was)
+      metadata.markAsTrashed(doc, side)
+
+      return this.merge.trashFolderAsync(side, was, doc)
     } else {
       metadata.ensureValidPath(doc)
       doc.docType = metadata.FOLDER

test/integration/trash.js

@@ -64,10 +64,10 @@ describe('Trash', () => {
         should(helpers.putDocs('path', 'trashed')).deepEqual([
           { path: path.normalize('parent/file'), trashed: true }
         ])
-        await should(pouch.db.get(file._id)).be.rejectedWith({ status: 404 })
 
         await helpers.syncAll()
 
+        await should(pouch.db.get(doc._id)).be.rejectedWith({ status: 404 })
         should(await helpers.remote.tree()).deepEqual([
           '.cozy_trash/',
           '.cozy_trash/file',
@@ -169,6 +169,47 @@ describe('Trash', () => {
           })
         })
       })
+
+      // XXX: This situation should not exist but it actually does so, until we
+      // find its root cause, we'll try to deal with the consequences:
+      // 1. Initial scan starts
+      // 2. `initial-scan-done` event emitted
+      // 3. `scan` event emitted for the file
+      // 4. Initial diff ends before the file event is processed and emits a
+      //    `deleted` event for the file
+      //
+      // → we end up merging first a `deleted` event and then a `scan` event
+      //   for the file with the same stats
+      //
+      context('because it was found too late during the initial scan', () => {
+        it('does not trash the file on the remote Cozy', async () => {
+          const doc = await helpers.pouch.bySyncedPath(
+            path.normalize('parent/file')
+          )
+
+          // XXX: Fake `deleted` event emitted by the initial diff
+          await prep.trashFileAsync('local', doc)
+          should(helpers.putDocs('path', 'trashed')).deepEqual([
+            { path: path.normalize('parent/file'), trashed: true }
+          ])
+          helpers.resetPouchSpy()
+
+          // XXX: actually run the scan so we emit a `scan` event for the file
+          await helpers.local.scan()
+          should(helpers.putDocs('path', 'trashed')).deepEqual([
+            { path: path.normalize('parent/file') }
+          ])
+          helpers.resetPouchSpy()
+
+          await helpers.syncAll()
+          await should(pouch.db.get(doc._id)).be.fulfilled()
+          should(await helpers.remote.tree()).deepEqual([
+            '.cozy_trash/',
+            'parent/',
+            'parent/file'
+          ])
+        })
+      })
     })
 
     context('on the remote Cozy', () => {