Summary: While working on separating send out of the sync loop, I realized sync tasks could use some cleanup to be more consistent with how we implemented syncback tasks. I reorganized and renamed things a little bit. This will also help us move in the direction of the scheduler implementation under which everything is a task.
Test Plan: manual
Reviewers: evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3660
Summary:
Only updated within month precision. We can use this to show how
far back a folder has been synced.
Test Plan: tested locally
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3662
Summary:
Fixes https://phab.nylas.com/T7435
The old deepScan (now `scanForAttributeChanges`) and shallowScan (now
`fetchLatestAttributeChanges`) had some fatal flaws.
If you deep scanned it would attempt to load the message attributes of all
messages ever and cause very bad memory leaks.
Also, if you left a mailbox running for a long time, there was a query
that would eventually run `Message.findAll` and, even though it was just
returning the headers, would still run insanely expensive operations
This fixes (and renames) these issues.
Test Plan: manual
Reviewers: spang, halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3657
Summary:
This swaps out our generic IMAP threading mechanism to use the threading
headers on the message instead of the prior way of grouping by subject
and then differentiating based on participants, as that design was
somewhat driven by what we could accomplish easily given legacy data
schema decisions and has serious caveats, such as different threads between
the same people with the same subject being misthreaded together. With K2, we
have free reign to change the data format, so we can do it right.
The algorithm is super simple:
- Define "references" as the union of the Message-Id, In-Reply-To, and
References headers on a message, filtered for valid RFC2822 Message-IDs
- On message sync, if any element of the new message's references
matches any element of an existing message's references, thread them
together
In order to accomplish this, we need to store References in a way that
allows each element to be indexed for fast lookup. That meant either
using the sqlite JSON1 extension + expression-based indices, or creating
a new table. I chose the latter as a time-tested and simple solution,
since we don't need the flexibility of JSON here.
Test Plan: manual - unit tests coming
Reviewers: khamidou, evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D3651
Examined the headers on a message we sent and found this:
> X-Mailer: nodemailer (2.5.0; +http://nodemailer.com/; SMTP/2.6.0[client:2.8.0])
No need to plaster which sendmail library we're using all over every
email our users send. Turn it off!
Summary:
Previously we would unconditionally issue a SELECT when openBox was
called. Now we check if the currently open box is the one we want first and
return immediately if it is, avoiding the unnecessary SELECT (which can be
quite expensive on large folders like INBOX). We were also calling closeBox
after iterating all the messages in a thread to mark them as read/unread.
This was unnecessary and was causing extra SELECTs to be issued. Now we don't!
This diff is a 5x speedup over the old behavior when marking lots of
threads in the same folder as read all at once.
Test Plan: Run locally, measure perf with log statements
Reviewers: evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D3654
Summary:
We were returning the wrong type in the case that we got no messages
back from the Gmail search API.
Test Plan: Run locally
Reviewers: evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3646
Summary:
Add a new dedicated imap connection to listen for any updates or new mail on the inbox.
Previously, we wouldn't be able to receive new mail events on the inbox during the sync loop
because other mailboxes would be open while we sync them. This would cause big
delays in receiving new mail, especially if you have a lot of folders
Test Plan: manual
Reviewers: spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3650
Summary:
We don't want to inflate delete Transactions, but we do still want
to pass the delta itself along.
Test Plan: tested locally
Reviewers: evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3647
There are plenty of valid use cases for sending subject-only emails,
and we also want to still download the headers and create message
objects if e.g. the email consists of only an event invitation or
an attachment.
Summary:
This commit passes down the `socketTimeout` option to node-imap. However, just
passing this option doesn't seem to work reliably, so this commit manually implements
the socketTimeout option for our IMAPConnection.
How it works is that basically every operation is wrapped with a timeout by
augmenting the `createConnectionPromise` construct that already existed.
Test Plan:
Locally, tested by sleeping computer and turning off wifi. The
connection will successfully error and be restarted. It will reconnect when the
network is available again
Reviewers: khamidou, halla, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3642
Summary:
We may find it useful at some point to be able to tell which messages in
a user's mailbox were sent using N1/the REST API vs Nylas Mail.
Test Plan: manual
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3631
Summary:
Sometimes things go very wrong when trying to syncback an action. For example, the worker window could crash, preventing us from marking the current task as failed. What's worse, another sync iteration could try to run the task which crashed, thinking it a new one. To prevent this, this diff adds a fourth syncback task state, `INPROGRESS`.
New syncback tasks are marked as `INPROGRESS` before being executed. When they complete we mark them as `SUCCEEDED/FAILED`. Stray `INPROGRESS` tasks are automatically marked as `FAILED` at the beginning of every sync iteration, to make sure we don't retry them again.
Test Plan: Tested manually.
Reviewers: evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D3635
A bug allowed multiple sync loop processes to start. This could lead to
double sending and the sync loop appearing as thouogh it couldn't be
interrupted
Summary:
Well-behaved MUAs don't do this, but it is totally legal and we should
handle it.
The Python sync engine handles this properly also:
7db949fec9/sync-engine/inbox/util/addr.py (L26-L47)
(Found this out while scratching my head over the output format of
mimelib.parseHeaders---turns out it's an array for a reason:
> let to = mimelib.parseHeaders(`To: Christine Spang <spang@nylas.com>
... To: Foo Bar <foo@example.com>`);
undefined
> to
{ to:
[ 'Christine Spang <spang@nylas.com>',
'Foo Bar <foo@example.com>' ] }
)
Test Plan: unit test included
Reviewers: halla, jackie, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D3625
Summary: Update `_getNewSyncbackTasks` to return any send tasks first, and then others
Test Plan: locally
Reviewers: halla, evan
Reviewed By: halla, evan
Differential Revision: https://phab.nylas.com/D3627
Summary:
See title. Got rid of that syncback-worker class which was kind of useless and
made things harder. My b.
Test Plan: locally
Reviewers: evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3624
Summary:
We have another function called extractContacts, in its own file,
extract-contacts.js, which is used to create Contact objects. This has
confused me a number of times and also leads to grep collisions.
This patch also makes the snippet unit tests pass again after a recent
API change.
Test Plan: included unit tests, manual
Reviewers: halla, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3623
Summary:
Message bodies, drafts aside, are immutable, and we set the snippet on
new messages manually in parseFromImap()---meaning this hook, if
invoked, is likely to replace the snippet with a broken version computed
with this old implementation. If we need a hook in the future (e.g. for
updating drafts), it should use the snippet function from
message-factory.
Test Plan: n/a
Reviewers: juan, halla
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3622
1. It could be expensive to delete many messages at the exact moment
when the folder is deleted
2. The folder delete could actually just be a rename, and if we
deleted all the messages, we would have to re-process them all
3. We already do a clean-up check for orphaned messages at the end
of the sync loop, where we already know if the folder was
actually deleted or just renamed
Summary:
If we inflate delete deltas, the object we're trying to find won't exist
anymore (we just deleted it!). This is likely causing the `While inflating
${sourceName} transactions, we couldn't find models for some ${modelName}
IDs` error.
Fixes T7436
Test Plan: manual
Reviewers: spang, juan, halla
Reviewed By: halla
Maniphest Tasks: T7436
Differential Revision: https://phab.nylas.com/D3621
Summary:
Delete associated children when a parent is deleted to prevent foreign
key constraint errors. Also make sure any child hooks are run.
Test Plan: tested locally
Reviewers: evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3620
Summary: N1 uses this to show the little attachment icon in the thread list.
Test Plan: tested locally
Reviewers: evan, juan, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3617
Summary:
This commit introduces interruptible sync operations. Now, the `SyncWorker`, `FetchFolderList` operation and `FetchMessagesInFolder` operation can be interrupted at several points during their execution. This improves the performance of SyncbackTasks, which now run almost immediately.
To achieve this, this commit adds an Interruptible abstraction, which is an object that can run functions and interrupt them at points marked by the function. For more info on how this works, see the docs on the Interruptible class.
This commit also splits up the SyncWorker a little bit to make it smaller, byadding a SyncbackTaskWorker.
Depends on D3613
Test Plan: Manual
Reviewers: spang, mark, jackie, khamidou, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D3612
Since we weren't giving Node IMAP the proper format for the changedsince
flag, and since node IMAP never warned of improper schemas, we weren't
properly requesting the correct range. This would cause us to request the
metadata attributes of EVERY message in the mailbox and attempt to store
them in a hash. This would eventually lead to a memory leak and take down
the worker window, which caused other subtle issues like sends failing
when the worker window dropped task half way through their perform remotes
and never re-sent the deltas notifying of their success or failure.
This was only triggered when new highestmodseq numbers fired on the remote
server, which would be triggered by the underlying mailbox getting folders
or labels changed on messages
We separately filter out contacts without email addresses before
committing to the contacts table in the database for autocomplete (in
isContactMeaningful()), and if we filter out these already we can end up
excluding legitimate elements of the headers. For example, the Clutter
feature of Office 365 sends emails with a From: header like this:
From: Microsoft Outlook
Fixes: T7413
Summary:
Extract files for inline attachments and store their content id
Fixes T7414
Test Plan: tested locally
Reviewers: evan, spang
Reviewed By: spang
Maniphest Tasks: T7414
Differential Revision: https://phab.nylas.com/D3609
Summary: Cleaning up almost all the linting errors in the logger
Test Plan: ... run the build
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3605
Summary:
Fixes T7398
We were create unnecessary and duplicate indices for the IDs of all of
our objects and increasing db write overhead.
We were not creating the correct reverse index for our join tables.
The search API'd db is already in scope of the accountId, this is an
unnecessary constraint on the query
Test Plan: manual
Reviewers: spang, juan
Reviewed By: juan
Maniphest Tasks: T7398
Differential Revision: https://phab.nylas.com/D3606
Summary:
Headers can be quite big, so we might as well download and store only
the ones that we care about. This patch also makes it so we stop
downloading MIME structures twice per message.
While it's possible that we _may_ want to make more headers accessible
later, we don't currently make the generic pile of headers accessible to
N1 or N1 plugins in any way, so doing that would end up requiring
changes to the sync code regardless. I think it's worth optimizing the
base experience rather than trying to predict what we may want in the
future. Plus, it seems more likely that we'll want to build future
extensibility using thread metadata, rather than message headers.
On inboxapptest1@fastmail.fm, this patch decreases the size of the
generated sqlite file for a fully synced mailbox by 35%.
Test Plan: manual
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3611
Summary:
It's possible to have multiple inline HTML parts in a message, or even
a multipart/alternative part that contains text and HTML, followed by a
plaintext signature. Previously, if there was more than one text part in
an email, we would pick the _last_ text/html or text/plain part that we
found, and treat that as the entire message body. This works most of the
time, but fails to display the full message body in some edge cases.
This patch fixes that by resolving multipart/alternative subparts to a
single part in the mimepart fetch stage, and then treating each desired
mime part separately when parsing the message, concatenating them if
there are multiple.
This makes K2's handling of multipart MIME message text better,
bug-wise, than the Python sync engine's, which has been mangling some
rare messages forever. (Example from my email: every email from the MIT
EECS Jobs List has never displayed the mailing list signature in N1.)
Note that this patch also removes our tentative support for PGP
encrypted messages. I'd rather add that back in later when I've dug up
some real example messages to test on, rather than leaving it in in its
current not-really-tested and probably not-really-working state, since
it makes it harder to make sure that the rest of the logic isn't broken.
Test Plan: manual for now - added examples of this to my growing list of regression tests to add to the message parser unit tests once I fix them
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3600
Summary:
We sync messages in the same order as the Python sync engine (newest to
oldest, generally), so we should be able to just use the same threading
algorithm. While we may still want to take into account References /
In-Reply-To at some point, this is a big step up from the current
thread-matching-only.
Test Plan: manual --- could pretty easily port the unit tests from the python codebase if we wanted
Reviewers: khamidou, juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3604
