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
In some places we were not accessing `.default` when using `require`,
and when using `import` we couldn't really destructure unless we
accessed `.default` too, or if the functions were regular exports
instead of properties on a default exported object.
This was causing our sync loop to error.
To remain consistent, we always just `require` or `import` the
SyncActivity singleton and access that
Summary:
These locations will provide enough activity for monitoring and are also
more helpful in helping us debug what the sync loop was doing when it
got stuck.
Test Plan: manual
Reviewers: juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4274
Summary: See title. Part of T7681.
Test Plan: ran the specs
Reviewers: evan, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4264
Summary:
Periodically check the latest sync activity times to determine if a sync
worker has been inactive for too long. If it has been too long, discard
the worker and create a new one for that account. Part of T7681.
Test Plan: manual, specs
Reviewers: evan, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4262
Summary:
Add infrastructure to report and retrieve the latest sync activity, and
start reporting when we download a message or detect that a folder
has no new messages. This will be used to detect if the sync loop is
stuck. Part of T7681.
Test Plan: manual, specs
Reviewers: evan, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4261
Summary:
The IdentityStore can trigger any number of times, but we only want to
start sync if we previously didn't have an identity available
Test Plan: manual
Reviewers: spang, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D4246
Summary:
Different clients can have different policies for retrying after
timeouts.
Test Plan: Run locally, run tests
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4247
Summary: In preparation for removing timeout handling from the IMAPConnectionPool.
Test Plan: Run locally
Reviewers: spang, evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D4245
Summary:
Currently, when we auth an account for the first time in Nylas Mail (or
we blow away the database), the app is going to request transactions
since cursor `null` from the /delta/streaming endpoint and from the local-sync
delta observable, instead of requesting transactions since cursor `0`
This is due to a subtle bug with the use of default values when
destructuring an object. Our coded did the following:
```
const {cursor = 0} = this._state
```
Which at a glance seems correct. However, this will only work as
expected if `this._state` has the following shape:
```
{cursor: undefined}
```
And unfortunately, our `this._state` looked like this when authing an
account for the first time:
```
{cursor: null}
```
Which would make `cursor === null` instead of `0`.
This is because when using default values, null is considered an
intentional argument/value, as opposed to not passing any argument/value
(which will mean that the argument is undefined).
This was a regression introduced in d60a23c and 8bc2ec5
Test Plan: manual, will add regression test in upcoming diff
Reviewers: evan, spang, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4243
Summary: see title. also convert to es6
Test Plan: manual
Reviewers: evan, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4225
Summary:
We don't want to bump threads to the top of the inbox when a user sends a
reply. We originally used `!isSent` to prevent this, but that was removed in
a diff that made sure messages showed up in the inbox when users send emails
to themselves. In order to implement both of these cases properly, this diff
introduces `isReceived` and uses that to determine whether lastReceivedDate
should be updated. Addresses T7991.
Also changes the order of some `or` statements, so that we actually check that
the variable exists before comparing against it.
Test Plan: manual
Reviewers: evan, juan, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D4226
Summary:
Use electron's `powerMonitor` module to detect when the computer resumes
from sleep, and restart the sync loop when that happens in order to
sync the inbox immediately, in case we received any new mail events
while the computer was asleep
Test Plan: manual
Reviewers: evan, spang, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4216