Summary:
This was showing up in Sentry. Check to see if the recipient has an email
before calling toLowerCase.
Test Plan: yolo
Reviewers: halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4423
Summary:
When we had to refresh our credentials for the mail listener connection we
weren't disposing of the old connection which caused us to slowly leak
connections from the IMAP connection pool, eventually requiring a
restart. Now we always dispose of the connection prior to requesting a
new one from the pool.
Test Plan: Run locally always refreshing credentials, verify we don't get stuck
Reviewers: spang, evan, halla, juan
Reviewed By: halla, juan
Subscribers: halla
Differential Revision: https://phab.nylas.com/D4414
Summary:
This reverts commit c808438ee9.
Adding an overarching transaction to `processMessage` is causing
database locked errors when processing deltas, specifically when trying
to write to the Transaction table, given that that write happens
asynchronously inside the execution of `processMessage`, but doesn't
have access to its transaction. See T8117
Given that the performance gains from wrapping messgae processing in a
transaction were not significant, let's revert this to prevent db
locking issues
Test Plan: manual
Reviewers: halla, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D4411
Summary:
When checking for flag updates for gmail accounts, when syncing for the
first time the all mail folder won't be available because we haven't
fetched the folder list, so it will throw a runtime error
Test Plan: manual
Reviewers: mark, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4405
Summary:
Since buildMime isn't always used explictly for the send operation (we
mostly use it for stuffing the sent folder, which is a separate
operation), move it to MessageUtils. Additionally, add an includeBcc
option. We don't want the BCC header to be visible on outbound messages,
but it needs to be present on the version that is saved to the sent
folder.
When we weren't including the BCC header in the sent folder version, we
were getting an id mismatch between our optimistic message and the
message that gets processed by the sync loop (because we include the bcc
participants when computing the id hash). When the sent message was a
reply, the optimistic message included the threadId, and this had the
side effect of leaving both versions of the sent message in the thread.
Since the optimistic message never receieved it's imap UID, any action
performed on the thread would fail with the missing UID error as seen in
T7941. Adding this BCC header will allow us to compute the proper id
hash and reconcile the synced message with the optimistic message.
Test Plan: manual
Reviewers: evan, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4365
Summary: Just to mixpanel. Addresses T8095
Test Plan: Meh
Reviewers: mark, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4403
Summary:
Normally, the imap connection emits an 'update' event when there are any
flag changes. Gmail, however, only emits the 'update' event when there
are new or removed messages. If there's a fairly large batch size being
synced in the sync loop, we don't want to wait until the end of that to
check if the highestmodseq has changed. To get around this, this diff
updates the new mail listener connection to periodically poll for the
highestmodseq and trigger the same update handler when changes are
detected.
Test Plan: manual
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4397
Summary:
There are some settings that apply to dev and prod modes that we don't want
to use while benchmarking. E.g. the folder where we store messages,
whether we generate long stack traces in our bluebird promises, etc.
This diff adds a benchmark mode so that we can change these settings to
something that works better for benchmarking.
Test Plan: Run locally, verify it works
Reviewers: evan, juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D4374
Summary:
We do lots of writes while processing a single message. To reduce the
write churn from lots of mini-transactions, this diff threads one
overarching transaction to everything in processMessage.
Test Plan: Run locally
Reviewers: spang, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4394
Summary:
The defaults are a little low. edgehill.db already does this. There's no
obvious effect on smaller dbs, but we should at least make the two
consistent.
Test Plan: Run locally
Reviewers: spang, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4395
Summary: It needs to be wrapped in an object.
Test Plan: manual
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4367
Summary:
Make sure SyncActivity.getLastSyncActivityForAccount returns a sensible
default to prevent runtime errors when the SyncProcessManager uses it
Test Plan: manual
Reviewers: evan, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4366
Summary:
Previously, calling the IMAPConnectionPool `onDone` callback when checking out a connection would just return the connection to the pool without re-establishing it the next time we checked it out of the pool.
When the `onDone` callback was called in an error scenario, this had the unintended consequence of returning a bad connection to the pool, and then re-using this bad connection. The only scenario when the connection was re-established was if `onConnected` threw an error, but if the connection was kept open outside the scope of onConnected, this logic would never run.
To make things simpler and more consistent, `onDone` will always destroy all connections and connections will always be re-established from scratch every time they are checked out of the pool. The pool acts more as a limit to the number of connections per account, than an actual shared pool of connections.
Test Plan: manual
Reviewers: evan, spang, halla, mark
Reviewed By: halla, mark
Differential Revision: https://phab.nylas.com/D4360
Summary:
When the sync worker got stuck inside `performSync`, we would never
release the connection back to the connection pool. We would then try to
start a new sync worker, and it would claim a new connection. If this
happened enough times, we would eventually exhaust the whole pool and
keep the sync worker blocked forever.
Test Plan: manual
Reviewers: evan, spang, halla, mark
Reviewed By: halla, mark
Differential Revision: https://phab.nylas.com/D4359
Summary:
Before this commit, we would just establish a single mail listener imap
connection and never check if we needed to re-connect it due to an
expired access token. Even though we correctly refreshed the access
token at the beggining of each sync loop (hidden under
`ensureSMTPConnection`), we would never re-establish the mail listener
connection with the new access token.
This would cause the app to enter an `Invalid Credentials` error loop in
the sync loop, preventing from syncing any mail at all (T8064)
Test Plan: manual
Reviewers: spang, halla, mark, evan
Reviewed By: mark, evan
Differential Revision: https://phab.nylas.com/D4338
Summary:
Given that we keep the imap mail listener conenction open outside of the
imap connection pool, it might close itself after a while or after
putting your computer to sleep.
Previously, if we had the connection object we just assumed it was
connected and proceeded with sync, and if it wasn't, we would fall into
an error loop in the sync worker preventing it from syncing at all (T8065)
To fix this, we just call `connect` every time to ensure the connection is open.
(This was the case before we introduced the connection pool)
Test Plan: manual
Reviewers: mark, spang, halla, evan
Reviewed By: halla, evan
Differential Revision: https://phab.nylas.com/D4334
Summary:
Check that there aren't any other categories with a role before we try
to assign it to the current category.
Addresses T7835
Test Plan: manual
Reviewers: juan, evan, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D4298
Summary:
Bodies that already exist on K2 shouldn't be overridden.
See the comment for how this caused open/link tracking to fail in certain
cases when a lot of messages were being sent
Test Plan: manual
Reviewers: halla, mark, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4319
Summary:
We were using a join for the contact ranking query and for whatever
reason on large databases this was extremely slow in SQLite. This diff
splits the query into first finding the sent Folder/Label and then
searching for non-draft Messages in that category.
Test Plan: Run locally, verify the query is faster
Reviewers: juan, evan, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D4315
Summary:
We were using a couple of left outer joins to avoid checking whether we
should be joining on Labels or Folders. We can greatly simplify the query
by just checking which we should use and issuing the correct inner join.
Test Plan: Run locally, verify contact ranking still works.
Reviewers: spang, juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D4310
Summary:
Most of the times when removing a sync worker (i.e. when removing an
account), we would see database errors in the console. This happened
because more often than not we would interrupt in the middle of message
processing, but `processMessage` is not interruptible, which means that
the worker would be interrupted right after processing its current
message. However, if `processMessage` would take more than 500ms (the
current timeout for stopping the worker), we would destroy the database
before processing was done and it would throw a bunch of errors.
To fix this, we just don't set a timeout when removing the worker as a consequence
of removing an account. However, when we are removing the worker when we
detect that it is stuck, we set a time out of 5 seconds.
Test Plan: manually test removing accounts, verify that it doesn't error
Reviewers: mark, halla, evan
Reviewed By: halla, evan
Differential Revision: https://phab.nylas.com/D4308
Summary:
Previously, it was possible for the sync worker to continue running after being
interrupted, e.g. it would break out of `performSync` and then try to run
`onSyncCompleted`. This is fine if we were just interrupting to restart the loop,
but when we stop it we don't want it to continue running anything at all.
This also refactors the syncworker to have a single `destroy` method, which sets
a `destroyed` flag and uses that one exclusively instead of the `stopped` flag.
Test Plan: manually check it works
Reviewers: spang, halla, mark, evan
Reviewed By: mark, evan
Differential Revision: https://phab.nylas.com/D4307
Summary:
- Change files to .es6 so they recognize the syntax
- Pass arguments down to _run()
- Make sure the responses get returned
Test Plan: manual, even though that somehow failed before. :(
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4309
Summary:
Use the wonderful `debug` library instead of all our random flags.
You can now do things like: `DEBUG="app:*,sync:*" npm start` to print out
everything.
Test Plan: manual
Reviewers: mark, halla, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4297
Summary:
Report how long they take and if they're stopped (due to timeout).
Part of T7978
Test Plan: manual
Reviewers: evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D4272
Summary:
Make `run()` functions generators and change most awaits to yields
Part of T7978
Test Plan: specs from D4269, but really needs some heavy QA
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4270
Summary:
Interrupt retryable syncback tasks that are taking too long so that we can
return control to the sync loop. The sync loop will retry the task later.
This diff adds a `forceReject` param to `interrupt()` so that we can return
control immediately instead of waiting for the current operation to finish
(for instance, the syncback task could be stuck in an imap operation, and a
normal interrupt would still have to wait for that to finish before returning
control to the callee)
Part of T7978
Test Plan: specs
Reviewers: evan, spang, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4269
Summary:
This adds better logging to the DB
You can use `ENABLE_SEQUELIZE_DEBUG_LOGGING=true` and
`ENABLE_RXDB_DEBUG_LOGGING=true` to spit out the raw queries of both DBs.
Test Plan: manual
Reviewers: mark, halla, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4294
Summary:
The problem actually was that we weren't applying the transform for Gmail
messages sent to 1 recipient
Test Plan: manual
Reviewers: halla, juan
Reviewed By: halla, juan
Differential Revision: https://phab.nylas.com/D4291
Summary:
We weren't stripping link and open items from messages we sent properly.
While we stripped it from the first outgoing message, when the sync loop
came back around it would have them back in. This adds it in the message
processor to extra ensure that ANYTHING that comes from us (or one of our
aliases) gets the open/link tracking stripped
Test Plan: New tests
Reviewers: juan, halla
Reviewed By: juan, halla
Differential Revision: https://phab.nylas.com/D4287
Summary:
Prior to this diff, the "in:" search query syntax didn't work for IMAP.
This diff implements "in:" by changing the IMAP search backend to take
folder context into account and emit the appropriate queries for each
folder. Queries that include "in:foo" will replace the corresponding AST
nodes with 'ALL' or 'NOT ALL' depending on whether or not the current folder
is "foo". We also now filter which folders we search based on which
folders are referenced in the query.
Test Plan: Run locally, verify that in: works quickly
Reviewers: evan, juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D4284
Summary:
This prevented the /health endpoint from being available in the
local-api
Test Plan: manual
Reviewers: mark, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4280
Summary:
If `gmailInitialUIDsRemaining` was defined, we need to set `initialUids`
to that value. We were previously setting it to a key in
`folder.syncState` that didn't exist, which caused a runtime error.
This was introduced in d6a2b6935c
Test Plan: manual
Reviewers: spang, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D4279
Summary:
This patch will prevent users from being able to connect accounts which sync
mail but fail to send.
This commit includes a couple pieces:
* Adds a call to nodemailer's `verify()` function in the /auth endpoint
* Adds Error object conversion for SMTP errors. Since we don't implement our
own connection object or connection pool for SMTP, we simply wrap the couple
places we call functions from nodemailer that connect to SMTP, namely
SendmailClient's _send() and the new verify() call in /auth.
* Moves RetryableError to the 'errors' module since it's now a base class for
retryable IMAP //and// SMTP errors.
* Moves the main `smtpConfig()` logic which used to live on the Account model
into AuthHelpers so it can be shared between the Account model and the verify
code.
* Converts a few different places to use `import` syntax instead of
`require` syntax for module imports. Apologies for not splitting this out
into a separate diff—would have been a fair amount of work and looks not too
difficult to skim over in the context of the rest of the patch.
* Fixing a bug in a previous commit where erroring sends would crash because of
using `this._transporter.options` instead of `this._transporter.transporter.options`
Test Plan: manual
Reviewers: evan, halla, juan
Reviewed By: halla, juan
Differential Revision: https://phab.nylas.com/D4200
Summary:
When using the IMAPConnectionPool in the sync-worker, we requested 2
connections from the pool: 1 to listen for new mail, and 1 to actually
operate on the mailbox. These 2 connections would be closed and returned
to the pool at the end of each sync loop iteration.
However, we never want to close the connection to listen for new mail.
Closing this connection caused us to not listen for new mail in between
sync loops, which was especially noticeable when initial sync was done
because the delay between sync loops is 5 minutes.
This commit makes it so we request the 2 connections separately from the
connection pool, and keep the listener connection always open until we
dispose of the sync-worker.
Test Plan:
manually make sure that I got new mail event listeners in between
sync loops :(
Reviewers: evan, spang, halla, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D4276
Summary:
Sent mail is important for initial sync because it's used for contact
ranking. Prior to this diff we would delay syncing sent mail because we
would prioritize the inbox label over all other labels (including sent).
This diff includes the sent folder in the initial set of messages to sync.
Test Plan: Run locally, verify we get sent mail quickly
Reviewers: evan, spang, juan
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D4268
Summary:
We want to get to a usable inbox as quickly as possible, and throttling
prevents this. We should basically only be throttling for historical
mail syncing.
Test Plan: Run initial sync benchmark, it's 117% faster on battery
Reviewers: evan, juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D4266
Summary:
Prior to Nylas Mail, the Nylas Cloud API provided an endpoint that
returned rankings for contacts which it computed based on how frequently
and how recently a user sent mail to a recipient. This diff reimplements
that functionality in Nylas Mail. This should improve contact
auto-complete when composing emails to frequently contacted recipients.
Test Plan: Run locally, verify that frequent contacts are suggested earlier
Reviewers: spang, evan, juan
Reviewed By: evan, juan
Maniphest Tasks: T7948
Differential Revision: https://phab.nylas.com/D4253
