[local-sync] syncback(Part 2): Reinstate send tasks back into the sync loop

Summary:
We had previously ripped send tasks outside the sync loop to make them run faster,
but they run fast enough inside the loop.

This commit will also fix the scenario where if you closed the app in the
middle of a send task, the task would just hang forever and never succeed or
fail (T7818); given that it was excluded from the loop, we also had to exclude it
from the cleanup step to mark any INPROGRESS tasks as failed at the beginning
of each loop, which caused send tasks in progress to never get cleaned.

Putting them back inside the loop allows us to  fix this without adding more messy
logic, and it cleans up ugly duplicated code. Additionally, it will prevent more
duplicated code for upcoming diffs that will improve syncback task reliability
when the app is closed or window is restarted in the middle of a task.

Depends on D3893

Test Plan:
manually test sending, it still works, it's still fast. Restarted
window in the middle of send task, task fails.

Reviewers: mark, spang, halla, evan

Reviewed By: spang, halla, evan

Differential Revision: https://phab.nylas.com/D3894

packages/local-sync/src/local-api/routes/send.js

@@ -1,7 +1,5 @@
 const Joi = require('joi');
 const Utils = require('../../shared/utils');
-const SyncbackTaskFactory = require('../../local-sync-worker/syncback-task-factory');
-const SyncbackTaskRunner = require('../../local-sync-worker/syncback-task-runner').default;
 const {createAndReplyWithSyncbackRequest} = require('../route-helpers');
 
 
@@ -26,27 +24,11 @@ module.exports = (server) => {
     config: {
     },
     async handler(request, reply) {
-      const account = request.auth.credentials;
-      const syncbackRequest = await createAndReplyWithSyncbackRequest(request, reply, {
+      createAndReplyWithSyncbackRequest(request, reply, {
         type: "SendMessage",
         props: {
           messagePayload: request.payload,
         },
-        // TODO this is a hack to run send outside the sync loop. This should be
-        // refactored when we implement the sync scheduler
-        wakeSync: false,
-      })
-
-      const sendTask = SyncbackTaskFactory.create(account, syncbackRequest)
-      const db = await request.getAccountDatabase()
-      const runner = new SyncbackTaskRunner({
-        db,
-        account,
-        logger: request.logger.child(),
-      })
-      await runner.runSyncbackTask({
-        task: sendTask,
-        runTask: (t) => t.run(db),
       })
     },
   });
@@ -58,29 +40,13 @@ module.exports = (server) => {
     config: {
     },
     async handler(request, reply) {
-      const account = request.auth.credentials;
-      const syncbackRequest = await createAndReplyWithSyncbackRequest(request, reply, {
+      createAndReplyWithSyncbackRequest(request, reply, {
         type: "SendMessagePerRecipient",
         props: {
           messagePayload: request.payload.message,
           usesOpenTracking: request.payload.uses_open_tracking,
           usesLinkTracking: request.payload.uses_link_tracking,
         },
-        // TODO this is a hack to run send outside the sync loop. This should be
-        // refactored when we implement the sync scheduler
-        wakeSync: false,
-      })
-
-      const sendTask = SyncbackTaskFactory.create(account, syncbackRequest)
-      const db = await request.getAccountDatabase()
-      const runner = new SyncbackTaskRunner({
-        db,
-        account,
-        logger: request.logger.child(),
-      })
-      await runner.runSyncbackTask({
-        task: sendTask,
-        runTask: (t) => t.run(db),
       })
     },
   });

packages/local-sync/src/local-sync-worker/syncback-task-runner.es6

@@ -4,9 +4,11 @@ const SyncbackTaskFactory = require('./syncback-task-factory');
 
 const MAX_TASK_RETRIES = 2
 
-// TODO NOTE! These are the tasks we exclude from the sync loop. This should be
-// refactored later.
-export const SendTaskTypes = ['SendMessage', 'SendMessagePerRecipient']
+const SendTaskTypes = [
+  'SendMessage',
+  'SendMessagePerRecipient',
+  'EnsureMessageInSentFolder',
+]
 
 class SyncbackTaskRunner {
 
@@ -38,33 +40,30 @@ class SyncbackTaskRunner {
    * change /first/, otherwise the message would be moved and it would receive a
    * new IMAP uid, and then attempting to change labels with an old uid would
    * fail.
-   *
-   * TODO NOTE: This function excludes Send tasks because these are run outside fo the
-   * sync loop for performance reasons.
    */
   async getNewSyncbackTasks() {
     const {SyncbackRequest, Message} = this._db;
 
-    const ensureSentFolderTasks = await SyncbackRequest.findAll({
+    const sendTasks = await SyncbackRequest.findAll({
       limit: 100,
-      where: {type: ['EnsureMessageInSentFolder'], status: 'NEW'},
+      where: {type: SendTaskTypes, status: 'NEW'},
       order: [['createdAt', 'ASC']],
     })
     .map((req) => SyncbackTaskFactory.create(this._account, req))
-    const tasks = await SyncbackRequest.findAll({
+    const otherTasks = await SyncbackRequest.findAll({
       limit: 100,
-      where: {type: {$notIn: [...SendTaskTypes, 'EnsureMessageInSentFolder']}, status: 'NEW'},
+      where: {type: {$notIn: SendTaskTypes}, status: 'NEW'},
       order: [['createdAt', 'ASC']],
     })
     .map((req) => SyncbackTaskFactory.create(this._account, req))
 
-    if (ensureSentFolderTasks.length === 0 && tasks.length === 0) { return [] }
+    if (sendTasks.length === 0 && otherTasks.length === 0) { return [] }
 
     const tasksToProcess = [
-      ...ensureSentFolderTasks,
-      ...tasks.filter(t => !t.affectsImapMessageUIDs()),
+      ...sendTasks,
+      ...otherTasks.filter(t => !t.affectsImapMessageUIDs()),
     ]
-    const tasksAffectingUIDs = tasks.filter(t => t.affectsImapMessageUIDs())
+    const tasksAffectingUIDs = otherTasks.filter(t => t.affectsImapMessageUIDs())
 
     const changeFolderTasks = tasksAffectingUIDs.filter(t =>
       t.description() === 'RenameFolder' || t.description() === 'DeleteFolder'
@@ -116,9 +115,7 @@ class SyncbackTaskRunner {
     // at the next sync iteration. We use this function for that.
     const {SyncbackRequest} = this._db;
     const inProgressTasks = await SyncbackRequest.findAll({
-      // TODO this is a hack
-      // NOTE: We exclude SendTaskTypes because they are run outside of the sync loop
-      where: {type: {$notIn: SendTaskTypes}, status: 'INPROGRESS'},
+      where: {status: 'INPROGRESS'},
     });
 
     for (const inProgress of inProgressTasks) {