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
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:
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:
Before this commit, it was impossible to remove inline images via the x
button
Test Plan: manual
Reviewers: mark, halla
Reviewed By: mark, halla
Differential Revision: https://phab.nylas.com/D4257
Summary:
I had a bit of downtime this morning so I decided to look into how to store Electron crash data. Electron, like Chromium, stores crash data in an arcane file format named minidump.
There's a bunch of services you can use to store files formatted in this format, like Mozilla's Socorro or zcbenz's own mini-breakpad-server but they're all pretty hard to install. Luckily, it turns out there's a Ruby gem to process minidump files and send them to Sentry! Seeing that, I whipped up a quick sinatra service and hosted it on Heroku – you can take a look at the source code here if you're curious: https://github.com/khamidou/electron-breakpad-sentry
This diff adds the client-side code we need to start sending crash reports.
Test Plan: Tested manually.
Reviewers: juan, evan
Reviewed By: juan, evan
Maniphest Tasks: T7926
Differential Revision: https://phab.nylas.com/D4259
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:
We'd like to get an idea of how long the client app is having to wait
when requesting a file. Waiting can cause things like inline image
attachments to appear very slow to load.
Test Plan: Run locally
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4255
Summary:
The original name seems like it's initiating the download, when really
it's just returning the data of an already in-progress/completed
download.
Test Plan: Manual, specs
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4251
Summary:
We need to download the files and then treat them as uploads. Rather
than using an actual Upload object, which would require another data
transfer, we create an object with all the necessary Upload-like
properties and point it to the downloaded file.
Addresses part of T7960
Test Plan: Manual, some specs
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4249
Summary:
Found a funny send-later bug I didn't catch when testing on staging: sometimes the data we're saving in the metadata table overflows. That's because MySQL's TEXT column are at most 64k, which is easy to reach when you have a draft + clearbit information and additional stuff.
To work around this, I decided to switch the database type of the metadata table to LONGTEXT. Since it can store 4Gb of text, we should be good. This diff makes those code changes. Obviously, we'll have to run migrations both on staging and prod.
Test Plan: Ran a basic smoke test. Shouldn't break anything.
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4250
Summary: Using `await` instead of `advanceClock()` fixes all the things!
Test Plan: Ran the spec file
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4248
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
