From 0644d2663def91cc6a3db8dc77581426a8c10e69 Mon Sep 17 00:00:00 2001
From: Christine Spang <spang@nylas.com>
Date: Fri, 23 Dec 2016 08:25:28 -0800
Subject: [PATCH] [local-sync] Fix UniqueConstraintError inserting contacts
 from messages

Summary:
Because of JavaScript's asynchronous nature, it is possible that we will
be processing several downloaded messages concurrently. This can lead to
calling extractContacts() in an interleaved fashion, which it was not
designed to handle. It looks up which contacts are already in the
database and then performs inserts or updates accordingly, assuming
nothing has changed in the contacts table in between---which is not
true! If several messages have similar contacts, an insert race can
cause one of the inserts to throw an unhandled exception.

We fix this by simply catching the unique constraint error, and falling
back to an update instead. (There's not really a better way to deal
with write races other than to enforce that we process contacts from
messages serially, as transactions are of no help here.)

This commit also removes extractContacts()'s return value, which is not
currently used and I found confusing.

Test Plan: manual

Reviewers: juan, evan, mark, jackie

Reviewed By: jackie

Differential Revision: https://phab.nylas.com/D3555
---
 .../new-message-processor/extract-contacts.js | 26 +++++++++++++------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/packages/local-sync/src/new-message-processor/extract-contacts.js b/packages/local-sync/src/new-message-processor/extract-contacts.js
index 8a634932f..63b359013 100644
--- a/packages/local-sync/src/new-message-processor/extract-contacts.js
+++ b/packages/local-sync/src/new-message-processor/extract-contacts.js
@@ -1,3 +1,4 @@
+const Sequelize = require('sequelize');
 
 function isContactMeaningful(contact) {
   // some suggestions: http://stackoverflow.com/questions/6317714/apache-camel-mail-to-identify-auto-generated-messages
@@ -31,6 +32,7 @@ async function extractContacts({db, message}) {
     }
     contactsDataById.set(id, cdata)
   })
+
   const existingContacts = await Contact.findAll({
     where: {
       id: Array.from(contactsDataById.keys()),
@@ -38,18 +40,26 @@ async function extractContacts({db, message}) {
   })
 
   for (const c of contactsDataById.values()) {
-    const existing = existingContacts.find(({id}) => id === c.id)
+    const existing = existingContacts.find(({id}) => id === c.id);
+
     if (!existing) {
-      await Contact.create(c)
+      Contact.create(c).catch(Sequelize.ValidationError, (err) => {
+        if (err.name !== "SequelizeUniqueConstraintError") {
+          console.log('Unknown error inserting contact', err);
+          throw err;
+        } else {
+          // Another message with the same contact was processing concurrently,
+          // and beat us to inserting. Since contacts are never deleted within
+          // an account, we can safely assume that we can perform an update
+          // instead.
+          Contact.find({where: {id: c.id}}).then(
+            (row) => { row.update(c) });
+        }
+      });
     } else {
-      const updateRequired = (c.name !== existing.name);
-      if (updateRequired) {
-        await existing.update(c)
-      }
+      existing.update(c);
     }
   }
-
-  return message;
 }
 
 module.exports = extractContacts