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
This reverts commit 0b3e3d2f39.
Interrupting sync by closing connection causes errors downstream when
`syncNow` is called elsewhere. Instead of interrupting by closing the
connection, we will post a patch to interrupt the sync loop properly
Summary:
Sync operations are mostly bound by I/O and the imap connection.
What we believe that is mostly affecting cpu and battery life is that node’s event
loop is being hosed with cpu intensive message processing operations.
To alleviate this, we do a few things:
- Restore a global message processing queue to process messages serially and meter cpu usage (message processing continues to be a fire and forget call from within sync operations)
- Move actual cpu intensive work to the message processing queue, i.e. `MessageFactory.parseFromImap`
- Keep track of message processing queue length, and skip sync operations if queue is too big to prevent massive memory consumption
This commit also renames the package from new-message-processor to
message-processor, given that now it processes both new and existing
messages, and we like to minimize confusion.
Test Plan: manual
Reviewers: spang, khamidou, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3602
Summary:
- Ensure delete deltas make it through to N1
- Don't fail if we can't find a category that needs to be deleted
Test Plan: local
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3593
Summary:
- Adds `File` objects onto the `Message`s so N1 sees attachments
- Ensures `File` is eagerly loaded for all messages
- Base 64 streams attachments through the local /download endpoint
- ExtractFile only uses disposition type attachment when extracting
attachments
- Makes sure we save existing messages when processing them
Test Plan: manual :(
Reviewers: juan, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3595
Summary:
We were previously not taking into account the 'Content-Disposition'
MIME header, which differentiates between parts intended for display
('inline') and parts that are instead transferred files ('attachment').
See the RFC for more details:
https://www.ietf.org/rfc/rfc2183.txt
Fixes: T7367
Test Plan: unit test coming soon---have the test data and going to fix all message parsing test cases at once
Reviewers: juan, jackie, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D3585
Summary:
Labels don't get added via passing in a labels attribute to
create(). We need to call addLabels() instead.
Test Plan: Tested locally
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3581
Summary:
refactor multi-send
This diff started off by fixing sending with attachments.
The issue is that our `FileUploadStore` listened for
`Actions.sendDraftSuccess` as its signal to remove the files from the
.nylas temp directory. Unfortunately, the old MultiSend tasks, after
delivery of the message, would try and put the base message in the sent
folder. Since we already deleted the file from our local temp dir,
creating the base message for the sent folder would fail.
This exposed a much bigger issue which is that we don't consistently
distinguish between "delivery" of a message and any post-processing we do
(like filling the sent folder). This was leading to a variety of other
subtle issues.
For example, N1 assumes that if the SendMessage task fails, then we pop
the draft back up and ask the user to try again. Unfortunately, since we
were combining "delivery" and "post processing" it was possible for the
message to actually deliver, but fail when stuffing the sent folder, or
fail due to some other random bug. This would cause the user to send the
message twice.
To help us ensure we never "deliver" twice and handle errors more
intuitively, I separated out the two concepts.
Now there are "send" set of tasks and endpoints, and a
"EnsureMessageInSentFolder" set of tasks and endpoint (the latter used to
be ambiguously known as ReconcileMultiSend, whatever that meant)
The logic for send hasn't changed. This is mostly a renaming and moving
files around.
Test Plan: manual :(
Reviewers: jackie, juan, halla
Reviewed By: juan, halla
Differential Revision: https://phab.nylas.com/D3577
Contrary to what you might think, a message can have both an empty From: header
and multiple From: headers / multiple addresses in a From header. In that case,
we must save all of them and let the client decide how to display.
Fixes: T7370
Summary: I've found a pretty annoying bug --- N1 would stop syncing all accounts after the Internet connection dropped. It seems that deep inside node-imap or NodeJS itself, connections aren't timing out the right way. To work around this, this diff unilaterally restarts the sync every `nextSyncIn` milliseconds.
Test Plan: Tested manually by cutting internet access and checking that K2 recovered.
Reviewers: evan, juan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3573
The 'encoding' library transparently upgrades to using iconv instead of
iconv-lite, if available. This allows us to support more encodings in
emails, such as ISO-2022-JP.
Fixes: T7358
Summary:
If you sent an email to yourself it would not show up in your inbox. This
is because sent messages would never get a lastMessageReceived timestamp.
Since we order the inbox by lastMessageReceived, setting that to null to
on sent mail would mean it never shows up in the thread list.
Also fixed an assertion bug in SFDC that requires transactions to return a
promise.
Finally added extra debug interfaces that will show more info if the delta
stream detects an inconsistency
Test Plan: manual
Reviewers: juan, halla, jackie
Reviewed By: jackie
Differential Revision: https://phab.nylas.com/D3552
Summary: We were creating duplicate `Message` objects because the formatting for the date was different between `buildForSend` and `parseFromImap`. Now, we create the initial hash using the same format that `buildmail` uses to ensure that we generate the same IDs.
Test Plan: Tested locally.
Reviewers: evan, juan, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3559
Summary:
Because of the way we were attempting to parse contacts from
From/To/Cc/Bcc headers by converting them to JSON with a regex, we were
erroneously breaking contacts that contained commas in quoted names into
multiple contacts. This could result in things like parsing multiple
addresses for the From: header, incorrectly!
To resolve the problem, replace our homegrown logic with mimelib's
seemingly excellent parseAddresses(), which handles this and a myriad of
other cases correctly.
Fixes: T7370
Test Plan: unit tests included
Reviewers: mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D3565
This doesn't do anything with sqlite, and just generates the following
warning in the logs:
>> WARNING: SQLite does not support TEXT with options. Plain `TEXT` will be used instead.
>> Check: https://www.sqlite.org/datatype3.html
