Summary:
This commit makes a few changes to how we log database queries:
- We log queries in 2 different places now:
- When `DatabaseStore._executeLocally` takes longer than 100ms. This might be due to the actual db operation taking long, or it might be due to our retry logic, or potentially other factors.
- When the actual raw database operation takes more than 100ms.
- When raw database queries take more than 100ms, we log out the query plan if it is a `SELECT` query and we are in dev mode.
- If `DEBUG_TO_LOG` is true, we always log all queries (including background queries)
Test Plan: manual
Reviewers: mark, halla, evan
Reviewed By: halla, evan
Differential Revision: https://phab.nylas.com/D4288
Summary:
- Rename `databaseAgent` usage to `background`, in the code and in the logs, which I
believe is less confusing. Often times found myself asking what was this agent thing in
the logs.
- Make messaging more explicit
Test Plan: manual
Reviewers: evan, mark, halla
Reviewed By: mark, halla
Differential Revision: https://phab.nylas.com/D4285
Summary:
This commit makes it so that when we retry database queries after
encountering a db locked error, we back off for at most 500ms, instead
of the current max of 3s. This meant that a single query could
potentially take 3+ seconds and block the transaction queue for that long.
Test Plan: manual
Reviewers: evan, mark, halla
Reviewed By: mark, halla
Differential Revision: https://phab.nylas.com/D4283
Summary:
After introducing retry logic, we would always defer db queries to the
next tick even if there was no retry delay present.
This would cause almost all db queries take more than 100ms even if the
actual db operation was really fast. This affected app performance
negatively
Test Plan: manual
Reviewers: evan, halla, mark
Reviewed By: halla, mark
Differential Revision: https://phab.nylas.com/D4282
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
This commit is the same as 7d409f5.
This commit was accidentally reverted in 95ec679, because I (juan) was being
dumb. So I'm re-committing here.
Summary:
We weren't doing that, so we would have accurate search index info for
threads when they were, e.g., moved to another folder.
Test Plan: Run locally
Reviewers: spang, evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D4260
Summary:
By default, the 'local'/'development' environment assumes that all services
are running locally. This makes sense.
However, if you're not making changes to auxiliary services such as
billing.nylas.com and hence do not have them running locally, firing up
Nylas Mail with the 'local' env will not work. It's a bit of a pain to
get a local billing service running locally, especially if you're not
making any changes to that service. To ease development, change the
default billing URL during development to staging, and allow manually
overriding the billing server URL to point to localhost in the case
that you're making changes to the billing service also.
To override the billing URL for local development, run like this:
npm run cloud -- --env billing_local
BILLING_URL=http://billing.lvh.me:5555 npm start
This is both a problem that I've run into during development as well as
(my hunch) why Karim accidentally landed a global override of the
billing server URL.
Test Plan: manual
Reviewers: evan, khamidou, halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4202
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:
This commit makes it so postinstall.es6 only bootstraps the packages we
are interested in based on a new env variable, `INSTALL_TARGET`.
`INSTALL_TARGET` can be set to `cloud`, `client` or `all`, or not set at all,
and our postinstall will bootstrap the packages accordingly. This will make
build and deploy times faster.
Test Plan: manual
Reviewers: spang, halla, evan, khamidou
Reviewed By: evan, khamidou
Differential Revision: https://phab.nylas.com/D4271
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:
This diff will give us insight into how people use Nylas Mail on their
computers with respect to how mobile they are (i.e. whether they use NM
on battery or plugged in).
Test Plan: Run locally
Reviewers: evan, spang, juan
Reviewed By: juan
Subscribers: gleb
Differential Revision: https://phab.nylas.com/D4267
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 ping client-sync's /health endpoint and store the latest
sync activity. If we get an ECONNREFUSED error, the worker window is
unavailable. Report the last known activity and restart the worker
window. Part of T7681.
Test Plan: manual, specs
Reviewers: evan, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4263
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:
We still don't know exactly what scenarios cause us to get 'database is
locked' errors, but generically this means that we have multiple
processes trying to access the database at the same time. In an attempt
to handle this gracefully, this diff makes it so that we retry the
queries in these cases. Theoretically, the database should free up once
the other process is done using it, and the erroring process just needs
to wait its turn. We still throw an error after 5 retries, so if there's
a larger issue, we'll still be able to tell in Sentry.
Addresses T7992
Test Plan:
I opened a transaction in the worker window and then tried to
do the same in the main window. If I didn't release the transaction in
the worker window, the main window eventually errored. If I did release
the transaction, the main window continued creating its own transaction.
Reviewers: mark, juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D4254
Summary: Should help a fair bit with our redis connection pileup.
Test Plan: Tested manually.
Reviewers: mark, spang, juan, evan
Reviewed By: mark, spang, evan
Differential Revision: https://phab.nylas.com/D3916
Summary:
These only really matter for n1cloud, and are being added manually in
production, so no migration file necessary. I am adding them to the
model definitions to prevent them getting out of sync with production
for development.
See T7999 for full SQL review
Test Plan: ship it
Reviewers: evan, khamidou, juan
Reviewed By: juan
Subscribers: vlad, jerm
Differential Revision: https://phab.nylas.com/D4230
