[local-sync] Fix attribute sync for non Gmail accounts

Summary:
Before this commit, if folder sync was complete, and the account didn't support CONDSTORE (e.g. Office365, Yahoo), we would only check for attribute updates every 10 minutes.

This commit makes it so we always check for attribute updates if the server doesn't support CONDSTORE

So for example, when marking a thread as read, we would perform the optimistic update in N1, queue the syncback task which would succeed, but the thread in k2s db would never get updated and become stale, with an unreadCount > 0. If we emitted a delta for that thread during the window of time where we ignored attribute updates, it would be set as unread again in N1, even though all of its messages were read.

This still doesn't guarantee that it wont happen (we could still get a delta for the thread before we actually fetch the attribute updates from IMAP), but before this commit it was sure to happen. This should be properly fixed with the sync scheduler refactor

Test Plan: manual

Reviewers: evan, mark, spang

Reviewed By: mark, spang

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

packages/local-sync/src/local-sync-worker/sync-tasks/fetch-messages-in-folder.imap.es6

@@ -8,7 +8,6 @@ const MessageFlagAttributes = ['id', 'threadId', 'folderImapUID', 'unread', 'sta
 const FETCH_ATTRIBUTES_BATCH_SIZE = 1000;
 const FETCH_MESSAGES_COUNT = 30;
 const GMAIL_INBOX_PRIORITIZE_COUNT = 1000;
-const ATTRIBUTE_SCAN_INTERVAL_MS = 10 * 60 * 1000;
 
 
 class FetchMessagesInFolderIMAP extends SyncTask {
@@ -405,6 +404,45 @@ class FetchMessagesInFolderIMAP extends SyncTask {
     }
   }
 
+  async * _fetchNextMessageBatch() {
+    // Since we expand the UID FETCH range without comparing to the UID list
+    // because UID SEARCH ALL can be slow (and big!), we may download fewer
+    // messages than the batch size (up to zero) --- keep going until full
+    // batch synced
+    const fetchedEnough = () => this._fetchedMsgCount >= FETCH_MESSAGES_COUNT
+    const moreToFetchAvailable = () => !this._folder.isSyncComplete() || this._box.uidnext > this._folder.syncState.fetchedmax
+    while (!fetchedEnough() && moreToFetchAvailable()) {
+      const prevMsgCount = this._fetchedMsgCount;
+      yield this._fetchUnsyncedMessages();
+
+      // If we didn't find any messages at all
+      if (this._fetchedMsgCount === prevMsgCount) {
+        // Find where the gap in the UID space ends --- SEARCH can be slow on
+        // large mailboxes, but otherwise we could spin here arbitrarily long
+        // FETCHing empty space
+        let nextUid;
+        // IMAP range searches include both ends of the range
+        const minSearchUid = this._folder.syncState.fetchedmin - 1;
+        if (minSearchUid) {
+          const uids = await this._box.search([['UID',
+            `${this._folder.syncState.minUID}:${minSearchUid}`]]);
+          // Using old-school max because uids may be an array of a million
+          // items. Math.max can't take that many arguments
+          nextUid = uids[0] || 1;
+          for (const uid of uids) {
+            if (uid > nextUid) {
+              nextUid = uid;
+            }
+          }
+        } else {
+          nextUid = 1;
+        }
+        console.log(`🔃📂 ${this._folder.name} Found gap in UIDs; next fetchedmin is ${nextUid}`);
+        await this._folder.updateSyncState({ fetchedmin: nextUid });
+      }
+    }
+  }
+
   /**
    * We need to periodically check if any attributes have changed on
    * messages. These are things like "starred" or "unread", etc. There are
@@ -529,36 +567,33 @@ class FetchMessagesInFolderIMAP extends SyncTask {
     console.log(`🔃 🚩 ${this._folder.name} via scan through ${recentRange} and ${backScanRange} - took ${Date.now() - start}ms to update ${numChangedLabels + numChangedFlags} messages & threads`)
   }
 
-  * _shouldSyncFolder() {
+  _shouldFetchMessages(boxStatus) {
+    if (boxStatus.name !== this._folder.name) {
+      throw new Error(`FetchMessagesInFolder::_shouldFetchMessages - boxStatus doesn't correspond to folder`)
+    }
     if (!this._folder.isSyncComplete()) {
       return true
     }
-
-    const boxStatus = yield this._imap.getLatestBoxStatus(this._folder.name)
     const {syncState} = this._folder
-    const hasNewMessages = boxStatus.uidnext > syncState.fetchedmax
+    return boxStatus.uidnext > syncState.fetchedmax
+  }
 
-    if (hasNewMessages) {
+  _shouldFetchAttributes(boxStatus) {
+    if (boxStatus.name !== this._folder.name) {
+      throw new Error(`FetchMessagesInFolder::_shouldFetchMessages - boxStatus doesn't correspond to folder`)
+    }
+    if (!this._folder.isSyncComplete()) {
       return true
     }
-
+    const {syncState} = this._folder
     if (this._supportsChangesSince()) {
-      const hasAttributeUpdates = syncState.highestmodseq !== boxStatus.highestmodseq
-      if (hasAttributeUpdates) {
-        return true
-      }
-    } else {
-      const {lastAttributeScanTime} = syncState
-      const shouldScanForAttributeChanges = (
-        !lastAttributeScanTime ||
-        Date.now() - lastAttributeScanTime >= ATTRIBUTE_SCAN_INTERVAL_MS
-      )
-      if (shouldScanForAttributeChanges) {
-        return true
-      }
+      return syncState.highestmodseq !== boxStatus.highestmodseq
     }
+    return true
+  }
 
-    return false
+  _shouldSyncFolder(boxStatus) {
+    return this._shouldFetchMessages(boxStatus) || this._shouldFetchAttributes(boxStatus)
   }
 
   /**
@@ -571,19 +606,14 @@ class FetchMessagesInFolderIMAP extends SyncTask {
     this._db = db;
     this._imap = imap;
 
-    const shouldSyncFolder = yield this._shouldSyncFolder()
-    if (!shouldSyncFolder) {
-      console.log(`🔚 📂 ${this._folder.name} has no updates - skipping sync`)
-      return;
-    }
+    const latestBoxStatus = yield this._imap.getLatestBoxStatus(this._folder.name)
 
-    this._box = yield this._openMailboxAndEnsureValidity();
     // If we haven't set any syncState at all, let's set it for the first time
     // to generate a delta for N1
     if (_.isEmpty(this._folder.syncState)) {
       yield this._folder.updateSyncState({
-        uidnext: this._box.uidnext,
-        uidvalidity: this._box.uidvalidity,
+        uidnext: latestBoxStatus.uidnext,
+        uidvalidity: latestBoxStatus.uidvalidity,
         fetchedmin: null,
         fetchedmax: null,
         minUID: null,
@@ -591,44 +621,28 @@ class FetchMessagesInFolderIMAP extends SyncTask {
       })
     }
 
-    // Since we expand the UID FETCH range without comparing to the UID list
-    // because UID SEARCH ALL can be slow (and big!), we may download fewer
-    // messages than the batch size (up to zero) --- keep going until full
-    // batch synced
-    const fetchedEnough = () => this._fetchedMsgCount >= FETCH_MESSAGES_COUNT
-    const moreToFetchAvailable = () => !this._folder.isSyncComplete() || this._box.uidnext > this._folder.syncState.fetchedmax
-    while (!fetchedEnough() && moreToFetchAvailable()) {
-      const prevMsgCount = this._fetchedMsgCount;
-      yield this._fetchUnsyncedMessages();
-
-      // If we didn't find any messages at all
-      if (this._fetchedMsgCount === prevMsgCount) {
-        // Find where the gap in the UID space ends --- SEARCH can be slow on
-        // large mailboxes, but otherwise we could spin here arbitrarily long
-        // FETCHing empty space
-        let nextUid;
-        // IMAP range searches include both ends of the range
-        const minSearchUid = this._folder.syncState.fetchedmin - 1;
-        if (minSearchUid) {
-          const uids = await this._box.search([['UID',
-            `${this._folder.syncState.minUID}:${minSearchUid}`]]);
-          // Using old-school max because uids may be an array of a million
-          // items. Math.max can't take that many arguments
-          nextUid = uids[0] || 1;
-          for (const uid of uids) {
-            if (uid > nextUid) {
-              nextUid = uid;
-            }
-          }
-        } else {
-          nextUid = 1;
-        }
-        console.log(`🔃📂 ${this._folder.name} Found gap in UIDs; next fetchedmin is ${nextUid}`);
-        await this._folder.updateSyncState({ fetchedmin: nextUid });
-      }
+    if (!this._shouldSyncFolder(latestBoxStatus)) {
+      // Don't even attempt to issue an IMAP SELECT if there are absolutely no
+      // updates
+      console.log(`🔚 📂 ${this._folder.name} has no updates at all - skipping sync`)
+      return;
     }
 
-    yield this._fetchMessageAttributeChanges();
+    this._box = yield this._openMailboxAndEnsureValidity();
+    const shouldFetchMessages = this._shouldFetchMessages(this._box)
+    const shouldFetchAttributes = this._shouldFetchAttributes(this._box)
+
+    // Do as little work as possible
+    if (shouldFetchMessages) {
+      yield this._fetchNextMessageBatch()
+    } else {
+      console.log(`🔚 📂 ${this._folder.name} has no new messages - skipping fetch messages`)
+    }
+    if (shouldFetchAttributes) {
+      yield this._fetchMessageAttributeChanges();
+    } else {
+      console.log(`🔚 📂 ${this._folder.name} has no attribute changes - skipping fetch attributes`)
+    }
     console.log(`🔚 📂 ${this._folder.name} done`)
   }
 }

packages/local-sync/src/local-sync-worker/sync-worker.es6

@@ -431,7 +431,7 @@ class SyncWorker {
   }
 
   async interrupt({reason = 'No reason'} = {}) {
-    console.log(`🔃  Interrupting sync! Reason: ${reason}`)
+    console.log(`🔃 ❌ Interrupting sync! Reason: ${reason}`)
     const interruptPromises = [this._interruptible.interrupt()]
     if (this._currentTask) {
       interruptPromises.push(this._currentTask.interrupt())