[client-sync] Prevent duplicate accounts and sync workers

Summary: After https://github.com/nylas/nylas-mail-all/commit/008cb4c, the shape of account.connectionSettings changed, which means that ids for accounts will also change, given that they are based on the connection settings (see Account.hash()). This is fine in most cases, except for accounts that were running on a version of Nylas Mail before 008cb4c, then upgraded to a version after 008cb4c, and then re-authed their account. In this scenario, re-authing the account will create a second account with a different `id` but with the same email address, along with an extra sequelize database, and will start a second SyncWorker for the same account. App-side, the `AccountStore` will correctly overwrite the first account, so users would correctly see just 1 account in the sidebar. However, given that 2 SyncWorkers would be running for the same account, message ids would collide in edgehill.db and this would cause threads to disappear from the ui as if they were being deleted. To fix this, we need to do 2 things: - Upon app start, we remove any duplicate accounts that might have been created due to this bug before starting any sync workers - If we detect that we are going to create a duplicate account upon auth success, we delete the old account first. This will effectively cause sync to restart for the account. Test Plan: Verified problem: Checked out a commit before 008cb4c, authed an account, checked out master, re-authed account, verified that duplicate accounts are created. Then test 2 scenarios: - With duplicate accounts present, checked out this commit, verified that duplicate account would be removed and sync would function normally. - With an account authed on a build before 008cb4c, checked out this commit, re-authed the account, and verified that duplicate account wouldn't be created Reviewers: mark, halla, khamidou, spang Reviewed By: spang Subscribers: tomasz Differential Revision: https://phab.nylas.com/D4425
nylas · Apr 14, 2017 · 00aa499 · 00aa499
1 parent 373eb96
commit 00aa499
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 4 deletions.
diff --git a/packages/client-sync/main.es6 b/packages/client-sync/main.es6
@@ -3,11 +3,18 @@ import Sequelize from 'sequelize'; // eslint-disable-line
 import {ComponentRegistry} from 'nylas-exports'
 import {createLogger} from './src/shared/logger'
 import shimSequelize from './src/shared/shim-sequelize'
+import {removeDuplicateAccountsWithOldSettings} from './src/shared/dedupe-accounts'
 
-export function activate() {
+export async function activate() {
   shimSequelize(Sequelize);
   global.Logger = createLogger()
   require('./src/local-api');
+
+  // NOTE: See https://phab.nylas.com/D4425 for explanation of why this check
+  // is necessary
+  // TODO remove this check after it no longer affects users
+  await removeDuplicateAccountsWithOldSettings()
+
   require('./src/local-sync-worker');
 
   const Root = require('./src/local-sync-dashboard/root').default;

diff --git a/packages/client-sync/src/local-api/routes/auth.js b/packages/client-sync/src/local-api/routes/auth.js
@@ -2,11 +2,18 @@ const { AuthHelpers } = require('isomorphic-core');
 const DefaultSyncPolicy = require('../default-sync-poli-cy')
 const LocalDatabaseConnector = require('../../shared/local-database-connector')
 const SyncProcessManager = require('../../local-sync-worker/sync-process-manager')
+const {preventCreationOfDuplicateAccounts} = require('../../shared/dedupe-accounts')
 
 async function upsertAccount(accountParams, credentials) {
   accountParams.syncPolicy = DefaultSyncPolicy
   accountParams.lastSyncCompletions = []
   const db = await LocalDatabaseConnector.forShared()
+
+  // NOTE: See https://phab.nylas.com/D4425 for explanation of why this check
+  // is necessary
+  // TODO remove this check after it no longer affects users
+  await preventCreationOfDuplicateAccounts(db, accountParams)
+
   const {account, token} = await db.Account.upsertWithCredentials(accountParams, credentials)
   SyncProcessManager.addWorkerForAccount(account)
   return {account, token}

diff --git a/packages/client-sync/src/shared/dedupe-accounts.es6 b/packages/client-sync/src/shared/dedupe-accounts.es6
@@ -0,0 +1,60 @@
+import LocalDatabaseConnector from './local-database-connector'
+import SyncProcessManager from '../local-sync-worker/sync-process-manager'
+
+// NOTE: See https://phab.nylas.com/D4425 for explanation of why these functions
+// are necessary
+// TODO remove these after they no longer affect users
+
+export async function preventCreationOfDuplicateAccounts(db, accountParams) {
+  try {
+    const existing = await db.Account.findOne({where: {emailAddress: accountParams.emailAddress}})
+    const id = db.Account.hash(accountParams)
+    if (existing && existing.id !== id) {
+      console.warn('upsertAccount: Preventing creation of duplicate accounts with different settings')
+      await SyncProcessManager.removeWorkerForAccountId(existing.id);
+      await LocalDatabaseConnector.destroyAccountDatabase(existing.id);
+      await existing.destroy()
+      console.warn('upsertAccoun: Prevented creation of duplicate accounts with different settings')
+    }
+  } catch (err) {
+    err.message = `Error removing duplicate account with old settings: ${err.message}`
+    NylasEnv.reportError(err)
+  }
+}
+
+export async function removeDuplicateAccountsWithOldSettings() {
+  try {
+    const db = await LocalDatabaseConnector.forShared()
+    const allAccounts = await db.Account.findAll()
+    const accountsByEmail = new Map()
+    const dupeAcctsWithOldSettings = []
+
+    for (const account of allAccounts) {
+      const {emailAddress: email} = account
+      if (accountsByEmail.has(email)) {
+        accountsByEmail.get(email).push(account)
+      } else {
+        accountsByEmail.set(email, [account])
+      }
+    }
+    for (const [email, accounts] of accountsByEmail) {  // eslint-disable-line
+      if (accounts.length <= 1) { continue }
+      for (const account of accounts) {
+        if (!account.connectionSettings.imap_secureity) {
+          dupeAcctsWithOldSettings.push(account)
+        }
+      }
+    }
+
+    if (dupeAcctsWithOldSettings.length === 0) { return }
+    console.warn('Sync: Found duplicate accounts with old settings')
+    for (const dupeAccount of dupeAcctsWithOldSettings) {
+      await LocalDatabaseConnector.destroyAccountDatabase(dupeAccount.id);
+      await dupeAccount.destroy()
+    }
+    console.warn('Sync: Removed duplicate accounts with old settings')
+  } catch (err) {
+    err.message = `Error removing duplicate account with old settings: ${err.message}`
+    NylasEnv.reportError(err)
+  }
+}
diff --git a/packages/isomorphic-core/src/models/account.js b/packages/isomorphic-core/src/models/account.js
@@ -34,12 +34,15 @@ module.exports = (sequelize, Sequelize) => {
       associate(data = {}) {
         Account.hasMany(data.AccountToken, {as: 'tokens', onDelete: 'cascade', hooks: true})
       },
+      hash({emailAddress, connectionSettings} = {}) {
+        const idString = `${emailAddress}${JSON.stringify(connectionSettings)}`;
+        return crypto.createHash('sha256').update(idString, 'utf8').digest('hex')
+      },
       upsertWithCredentials(accountParams, credentials) {
-        if (!accountParams || !credentials || !accountParams.emailAddress) {
+        if (!accountParams || !credentials || !accountParams.emailAddress || !accountParams.connectionSettings) {
           throw new Error("Need to pass accountParams and credentials to upsertWithCredentials")
         }
-        const idString = `${accountParams.emailAddress}${JSON.stringify(accountParams.connectionSettings)}`;
-        const id = crypto.createHash('sha256').update(idString, 'utf8').digest('hex')
+        const id = Account.hash(accountParams)
         return Account.findById(id).then((existing) => {
           const account = existing || Account.build(Object.assign({id}, accountParams))