core/sync: Restart remote watcher when back online (#2027)

When the remote watcher fails to fetch remote changes because the Cozy
is unreachable (i.e. we received a FetchError with a status for which
we don't have a special treatment or that treatment is notifying that
the Cozy is unreachable) we block the synchronization and stop the
remote watcher to prevent further errors.

While we were regularly pinging the remote Cozy to see if it was
reachable again and notifying the GUI with an `online` event in this
case, we were not restarting the remote watcher.
Therefore, we were not fetching remote changes anymore after the Cozy
was made unreachable once.

We now make sure the remote watcher is restarted whenever we unblock
the synchronization.

core/sync/errors.js

@@ -69,10 +69,9 @@ class SyncError extends Error {
 
 const retryDelay = (err /*: RemoteError|SyncError */) /*: number */ => {
   if (err instanceof remoteErrors.RemoteError) {
+    // The error originates from the Remote Watcher and is not a change
+    // application error.
     switch (err.code) {
-      case remoteErrors.NEEDS_REMOTE_MERGE_CODE:
-        return REMOTE_HEARTBEAT
-
       case remoteErrors.UNREACHABLE_COZY_CODE:
         return 10000
 
@@ -83,6 +82,7 @@ const retryDelay = (err /*: RemoteError|SyncError */) /*: number */ => {
         return REMOTE_HEARTBEAT
     }
   } else if (err instanceof SyncError) {
+    // The error originates from Sync and means we failed to apply a change.
     switch (err.code) {
       case MISSING_PERMISSIONS_CODE:
         return 10000
@@ -94,6 +94,7 @@ const retryDelay = (err /*: RemoteError|SyncError */) /*: number */ => {
         return 10000
 
       case remoteErrors.NEEDS_REMOTE_MERGE_CODE:
+        // We want to make sure the remote watcher has run before retrying
         return REMOTE_HEARTBEAT
 
       case remoteErrors.UNREACHABLE_COZY_CODE:

core/sync/index.js

@@ -590,45 +590,46 @@ class Sync {
     cause
     /*: {| err: RemoteError |} | {| err: SyncError, change: MetadataChange |} */
   ) {
+    log.debug(cause, 'blocking sync for error')
+
     const { err } = cause
 
     this.lifecycle.blockFor(err.code)
 
-    if (err instanceof remoteErrors.RemoteError) {
-      this.remote.watcher.stop()
-    }
-
     const retryDelay = syncErrors.retryDelay(err)
 
     const retry = async () => {
       this.events.off('user-action-done', retry)
       this.events.off('user-action-inprogress', waitBeforeRetry)
       this.events.off('user-action-skipped', skip)
 
-      log.debug(cause, 'retrying failed change synchronization')
-
-      // Resest the timer for manual calls
-      // $FlowFixMe intervals have a refresh() method starting with Node v10
-      if (this.retryInterval) this.retryInterval.refresh()
+      log.debug(cause, 'retrying after blocking error')
 
       if (err.code === remoteErrors.UNREACHABLE_COZY_CODE) {
+        // We could simply fetch the remote changes but it could take time
+        // before we're done fetching them and we want to notify the GUI we're
+        // back online as soon as possible.
         if (await this.remote.ping()) {
-          clearInterval(this.retryInterval)
           this.events.emit('online')
-          this.lifecycle.unblockFor(err.code)
         } else {
           this.events.emit('offline')
+          // Resest the timer for manual calls
+          // $FlowFixMe intervals have a refresh() method starting with Node v10
+          if (this.retryInterval) this.retryInterval.refresh()
+          // We're still offline so no need to try fetching changes or
+          // synchronizing.
+          return
         }
-      } else {
-        clearInterval(this.retryInterval)
-        // Await to make sure we've fetched potential remote changes
-        if (this.remote.watcher.running) {
-          await this.remote.watcher.resetTimeout({ manualRun: true })
-        } else {
-          await this.remote.watcher.start()
-        }
-        this.lifecycle.unblockFor(err.code)
       }
+
+      clearInterval(this.retryInterval)
+
+      // Await to make sure we've fetched potential remote changes
+      if (!this.remote.watcher.running) {
+        await this.remote.watcher.start()
+      }
+
+      this.lifecycle.unblockFor(err.code)
     }
 
     const waitBeforeRetry = () => {
@@ -646,6 +647,8 @@ class Sync {
 
       log.debug(cause, 'user skipped required action')
 
+      clearInterval(this.retryInterval)
+
       // We need to check for the presence of `change` because Flow is not able
       // to understand it will automatically be present if `err` is a
       // `SyncError`…
@@ -655,17 +658,10 @@ class Sync {
         await this.updateErrors(change, err)
       }
 
-      if (err instanceof remoteErrors.RemoteError) {
-        this.remote.watcher.start()
+      if (!this.remote.watcher.running) {
+        await this.remote.watcher.start()
       }
 
-      //clearTimeout(retryTimeout)
-      clearInterval(this.retryInterval)
-
-      // We suppose the error triggered a user-action-required event since skip
-      // is only called when the user skips a user action.
-      // this.events.emit('user-action-skipped', err)
-
       this.lifecycle.unblockFor(err.code)
     }
 
@@ -677,6 +673,14 @@ class Sync {
     this.events.once('user-action-inprogress', waitBeforeRetry)
     this.events.once('user-action-skipped', skip)
 
+    // In case the error comes from the RemoteWatcher and not a change
+    // application, we stop the watcher to avoid more errors.
+    // It will be started again with the next retry or if the user action is
+    // skipped.
+    if (err instanceof remoteErrors.RemoteError) {
+      this.remote.watcher.stop()
+    }
+
     switch (err.code) {
       case remoteErrors.UNREACHABLE_COZY_CODE:
         this.events.emit('offline')

test/unit/sync.js

@@ -5,6 +5,7 @@ const _ = require('lodash')
 const sinon = require('sinon')
 const should = require('should')
 const EventEmitter = require('events')
+const { Promise } = require('bluebird')
 const { FetchError } = require('cozy-stack-client')
 
 const { Ignore } = require('../../core/ignore')
@@ -826,4 +827,84 @@ describe('Sync', function() {
       should(this.sync.selectSide(doc)).deepEqual([])
     })
   })
+
+  describe('blockSyncFor', () => {
+    beforeEach(function() {
+      sinon.spy(this.events, 'emit')
+      this.remote.watcher = {
+        start: sinon.stub().returns(),
+        stop: sinon.stub().returns()
+      }
+    })
+    afterEach(function() {
+      delete this.remote.watcher
+      this.events.emit.restore()
+    })
+
+    context('when Cozy is unreachable', () => {
+      beforeEach(function() {
+        this.sync.blockSyncFor({
+          err: remoteErrors.wrapError(
+            new FetchError({ status: 404 }, 'UnreachableCozy test error')
+          )
+        })
+      })
+
+      it('emits offline event', function() {
+        should(this.events.emit).have.been.calledWith('offline')
+      })
+
+      it('stops the remote watcher', function() {
+        should(this.remote.watcher.stop).have.been.called()
+      })
+
+      describe('retry', () => {
+        context('after Cozy is reachable again', () => {
+          beforeEach(async function() {
+            // Reset calls history
+            this.events.emit.reset()
+
+            // Cozy is reachable
+            this.remote.ping = sinon.stub().resolves(true)
+
+            // Force call to `retry`
+            this.events.emit('user-action-done')
+            // Wait for `retry` to run
+            await Promise.delay(1000)
+          })
+
+          it('emits online event', async function() {
+            should(this.events.emit).have.been.calledWith('online')
+          })
+
+          it('restarts the remote watcher', function() {
+            should(this.remote.watcher.start).have.been.called()
+          })
+        })
+
+        context('while Cozy is still unreachable', () => {
+          beforeEach(async function() {
+            // Reset calls history
+            this.events.emit.reset()
+
+            // Cozy is unreachable
+            this.remote.ping = sinon.stub().resolves(false)
+
+            // Force call to `retry`
+            this.events.emit('user-action-done')
+            // Wait for `retry` to run
+            await Promise.delay(1000)
+          })
+
+          it('emits offline event', async function() {
+            should(this.events.emit).have.been.calledWith('offline')
+          })
+
+          it('does not restart the remote watcher', function() {
+            should(this.remote.watcher.start).not.have.been.called()
+          })
+        })
+      })
+    })
+  })
 })