From 7b639129550e63066ccc3b080b0248e133366454 Mon Sep 17 00:00:00 2001
From: Juan Tejada <juans.tejada@gmail.com>
Date: Thu, 2 Feb 2017 17:17:51 -0800
Subject: [PATCH] [local-sync] Correctly retry syncback tasks

Summary:
Previously, we attempted to immeditely retry syncback tasks when a
RetryableError was encountered. However, if the error was an
IMAPConnection error, we would keep retrying with a broken connection,
which would keep failing.

The correct way to retry is to wait for the next sync loop, since at the
beginning of each loop we ensure that we are correctly connected to
imap.

To achieve this this commit simply marks a failed task as NEW if it
encoutners a RetryableError and we haven't retried too many times. To
keep track of the number of retries, we save a new field in the `props`
field of the request.

Test Plan: manual

Reviewers: evan, halla, mark, spang

Reviewed By: spang

Subscribers: khamidou

Differential Revision: https://phab.nylas.com/D3831
---
 .../src/local-sync-worker/sync-worker.es6     |  5 +-
 .../syncback-task-helpers.es6                 | 47 +++++++++++--------
 2 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/packages/local-sync/src/local-sync-worker/sync-worker.es6 b/packages/local-sync/src/local-sync-worker/sync-worker.es6
index d976cf8ae..3107b1d93 100644
--- a/packages/local-sync/src/local-sync-worker/sync-worker.es6
+++ b/packages/local-sync/src/local-sync-worker/sync-worker.es6
@@ -409,7 +409,10 @@ class SyncWorker {
     const tasks = yield getNewSyncbackTasks({db: this._db, account: this._account})
     this._shouldIgnoreInboxFlagUpdates = true
     for (const task of tasks) {
-      await runSyncbackTask({task, runTask: (t) => this._conn.runOperation(t)})
+      const {shouldRetry} = await runSyncbackTask({task, runTask: (t) => this._conn.runOperation(t)})
+      if (shouldRetry) {
+        this.syncNow({reason: 'Retrying syncback task', interrupt: true});
+      }
       yield  // Yield to allow interruption
     }
     this._shouldIgnoreInboxFlagUpdates = false
diff --git a/packages/local-sync/src/local-sync-worker/syncback-task-helpers.es6 b/packages/local-sync/src/local-sync-worker/syncback-task-helpers.es6
index 6c9b778ef..e08bcdc88 100644
--- a/packages/local-sync/src/local-sync-worker/syncback-task-helpers.es6
+++ b/packages/local-sync/src/local-sync-worker/syncback-task-helpers.es6
@@ -1,3 +1,4 @@
+const {Actions} = require('nylas-exports')
 const {IMAPErrors} = require('isomorphic-core')
 const SyncbackTaskFactory = require('./syncback-task-factory');
 
@@ -114,6 +115,8 @@ export async function markInProgressTasksAsFailed({db} = {}) {
 export async function runSyncbackTask({task, runTask} = {}) {
   const before = new Date();
   const syncbackRequest = task.syncbackRequestObject();
+  let shouldRetry = false
+
   console.log(`🔃 📤 ${task.description()}`, syncbackRequest.props)
   try {
     // Before anything, mark the task as in progress. This allows
@@ -121,32 +124,38 @@ export async function runSyncbackTask({task, runTask} = {}) {
     syncbackRequest.status = "INPROGRESS";
     await syncbackRequest.save();
 
-    let responseJSON;
-    for (let numTries = 0; numTries <= MAX_TASK_RETRIES; numTries++) {
-      try {
-        // TODO `runTask` is a hack to allow tasks to be executed outside the
-        // context of an imap connection, specifically to allow running send tasks
-        // outside of the sync loop. This should be solved in a better way or
-        // probably refactored when we implement the sync scheduler
-        responseJSON = await runTask(task)
-        break;
-      } catch (err) {
-        if (!(err instanceof IMAPErrors.RetryableError) || numTries >= MAX_TASK_RETRIES) {
-          throw err
-        }
-      }
-    }
-
+    // TODO `runTask` is a hack to allow tasks to be executed outside the
+    // context of an imap connection, specifically to allow running send tasks
+    // outside of the sync loop. This should be solved in a better way or
+    // probably refactored when we implement the sync scheduler
+    const responseJSON = await runTask(task)
     syncbackRequest.status = "SUCCEEDED";
     syncbackRequest.responseJSON = responseJSON || {};
+
     const after = new Date();
     console.log(`🔃 📤 ${task.description()} Succeeded (${after.getTime() - before.getTime()}ms)`)
   } catch (error) {
-    syncbackRequest.error = error;
-    syncbackRequest.status = "FAILED";
     const after = new Date();
-    console.error(`🔃 📤 ${task.description()} Failed (${after.getTime() - before.getTime()}ms)`, {syncbackRequest: syncbackRequest.toJSON(), error})
+    const {numRetries = 0} = syncbackRequest.props
+
+    if (error instanceof IMAPErrors.RetryableError && numRetries < MAX_TASK_RETRIES) {
+      Actions.recordUserEvent('Retrying syncback task', {numRetries})
+      shouldRetry = true
+      // We save this in `props` to avoid a db migration
+      syncbackRequest.props = Object.assign({}, syncbackRequest.props, {
+        numRetries: numRetries + 1,
+      })
+      syncbackRequest.status = "NEW";
+      console.warn(`🔃 📤 ${task.description()} Failed with retryable error, retrying in next loop (${after.getTime() - before.getTime()}ms)`, {syncbackRequest: syncbackRequest.toJSON(), error})
+    } else {
+      error.message = `Syncback Task Failed: ${error.message}`
+      syncbackRequest.error = error;
+      syncbackRequest.status = "FAILED";
+      NylasEnv.reportError(error);
+      console.error(`🔃 📤 ${task.description()} Failed (${after.getTime() - before.getTime()}ms)`, {syncbackRequest: syncbackRequest.toJSON(), error})
+    }
   } finally {
     await syncbackRequest.save();
   }
+  return {shouldRetry}
 }