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:
This removes the old SignalFX reporter that we weren't using
Adding documentation around logging
Test Plan: manual
Reviewers: juan, halla, spang
Reviewed By: juan, halla, spang
Differential Revision: https://phab.nylas.com/D4341
Summary:
According to bunyan, https://github.com/trentm/node-bunyan, errors are
treated special. They either need to be the first argument of a log, or
they need to be attached to the special `err` (NOT `error`) key of the
logger object.
Test Plan: manual
Reviewers: halla, juan, spang
Reviewed By: halla, juan, spang
Differential Revision: https://phab.nylas.com/D4337
Summary:
Sometimes, when logging out of your NylasID and restarting the app, we
would continue making some requests from the worker window that required a
NylasID. This would make the app enter a restart loop and become
completely unresponsive because when we made a request without a
NylasID, we would force the user to log out and restart the app, and
then we would again make the requests without the id, ad infinitum.
To fix this, we make sure we have a NylasID before making any requests
that require it
Test Plan: manual
Reviewers: halla, evan
Reviewed By: halla, evan
Differential Revision: https://phab.nylas.com/D4344
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:
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:
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: 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:
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:
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:
When querying transactions for the delta stream, if no cursor is provided,
we should default to 0. Otherwise this will generate a query like:
```
WHERE `transaction`.`id` > ‘null’
```
Which is obviously wrong
Test Plan: manual
Reviewers: evan, halla, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D4242
Summary:
We were using a version that was ~9 months old and a lot of development has
happened since.
v3 is not compatible w/v2, but it looks like we aren't using any of the
features that had breaking changes:
http://nodemailer.com/about/migrate/
I chose not to switch to the new built-in OAuth2 token refresh support
because we already have a mechanism for refreshing oauth tokens and
adding a different implementation specifically for SMTP would introduce
more opportunities for bugs.
Since mailcomposer is no longer a dependency of nodemailer, I added this
dependency as well.
Test Plan: manual - sent a message, sent a message w/an attachment
Reviewers: evan, khamidou, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4201
Summary:
Previously we limited it to 3. Gmail supports up to 15, many IMAP
servers support 10 by default. So let's bump it for those folks.
Test Plan: Run locally, verify things still work.
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4175
Summary:
If an error is thrown while opening an imap box, we need to make sure to
set the `_isOpening` back to false
Test Plan: manual
Reviewers: mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D4134
Summary:
In many cases when send fails, the server settings may be incorrect.
Send this data to Sentry with the original error in order to help us
diagnose.
I think the grouping will only contain settings for a single user
getting the same error, but should still be good enough to do further
investigation, e.g. https://sentry.io/nylas/nylas-mail/issues/230529222/
We can always go look up the requests in the n1Cloud logs if more info is
needed.
Test Plan: yolo
Reviewers: mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4131
Arc land messed up and landed a not fully merged branch. (Seriously – I
had merged a copy of my branch with master to see how easy it would be.
Because I didn't want to merge the whole thing, I blindly committed my
changes and switched back to my real branch). To my great surprise, arc
decided to use the wrong branch when landing it.
Original commit message:
Summary:
Finally, here it is! Send later, with support for open tracking but
without support for attachments yet. It took me some time to find the
right way to do things.
**The send later dilemna**
There's two ways we could handle send later:
1. do everything on the client
2. process the message in the cloud
1. is very tempting because it would make the cloud server very
simple. Unfortunately, it has some serious limitations, for example,
setting the "Date" message header. That's why I chose to go with 2. When
a user presses the "Send Later" button, we save the open/link tracking
metadata and fills in all the required fields. I added a custom endpoint
to the K2 API to do this, `/drafts/build`. After that, we save the JSON
contents of the message as metadata.
When we process metadata, we simply create a MIME message from the
JSON and send it.
**Limitations**
Right now, send later doesn't support and attachments. There's also
some minor code duplication which needs to be refactored away.
Test Plan: Tested manually. Checked that regular send still worked, too.
Reviewers: mark, spang, halla, juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4054
Summary:
Finally, here it is! Send later, with support for open tracking but without support for attachments yet. It took me some time to find the right way to do things.
**The send later dilemna**
There's two ways we could handle send later:
1. do everything on the client
2. process the message in the cloud
1. is very tempting because it would make the cloud server very simple. Unfortunately, it has some serious limitations, for example, setting the "Date" message header. That's why I chose to go with 2. When a user presses the "Send Later" button, we save the open/link tracking metadata and fills in all the required fields. I added a custom endpoint to the K2 API to do this, `/drafts/build`. After that, we save the JSON contents of the message as metadata.
When we process metadata, we simply create a MIME message from the JSON and send it.
**Limitations**
Right now, send later doesn't support and attachments. There's also some minor code duplication which needs to be refactored away.
Test Plan: Tested manually. Checked that regular send still worked, too.
Reviewers: mark, spang, halla, juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4054
Summary:
If it's expensive for us to SELECT a folder (which happens a lot for
large gmail accounts), we should try to sync more messages so that we
don't waste a ton of time SELECT-ing during initial sync. This speeds up
initial sync quite a bit.
Test Plan: Run locally, verify batches are properly computed
Reviewers: evan, juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D4050
Summary:
Isomorphic-core should be functional outside of the client environment
Depends on D4056
Test Plan: Run the test suite
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D4062
Summary:
Move the base Jasmine spec runner into isomorphic-core to prevent
code duplication. Jasmine will look for the config file relative to
the directory it's being run in though, so we need to symlink the
config file into each package that will need it.
Test Plan: Run tests once the suites are integrated
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D4056
Summary:
Convert the isomorphic-core specs to Jasmine 1 and symlink them into
client-app/internal_packages so they are run as part of the client
test suite.
Test Plan: Ran the suite with fdescribes in iso-core to ensure they were being called properly
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D4055
Summary:
This commit is an attempt to cleanup duplicated code and crufty code
inside IMAPConnection and IMAPBox
Specifically:
- It replaces `_createConnectionPromise` with a more aptly named (imo) `_withPreparedConnection` helper, which provides the user a node-imap connection that will correctly time out and handle `error` and `end` events. Most of these changes are just changing existing code to use the new interface.
- Adds a subtle change to how we handle `end` and `error` events on the connections. Previously, we manually called `this.end()` on `error`, but not on `end`. From what I could gather from the old comment documenting `_createConnectionPromise`, we should /also/ call `this.end()` on `end` because node-imap doesn't clean up correctly and can leave the connection hanging (taking care not to introduce a recursion loop by `end`ing on `end`). Additionally, it no longer listens to the events via `once` but via `on`, which should be okay given that the listeners get cleared at the end.
This /might/ fix some instances of sync freezing up (T7837).
Depends on D4035
Test Plan: manual -- this really needs some unit tests 😢
Reviewers: spang, halla, mark, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4036
Summary:
Remember how we made it so imap socket timeouts would increment up to 10m if
we constantly got timeout errors? Remember how we told everyone in
https://github.com/nylas/nylas-mail/issues/3232 that we'd fixed their problems, but they weren't actually fixed?
Well, we weren't /actually/ applying the correct timeout value. 😢
Depends on D4033
Test Plan: manual
Reviewers: spang, halla, mark, evan
Reviewed By: mark, evan
Differential Revision: https://phab.nylas.com/D4035
Summary:
The way it was set up was a little smelly.
- `this._settings` were /not/ the actual settings. Now, internally we have access to `this._resolvedSettings`
- `this.resolvedSettings` was being set kind of out of nowhere (gmail-oauth-helpers needs it, hence adding a getter instead)
Test Plan: manual
Reviewers: spang, mark, evan, halla
Reviewed By: mark, evan, halla
Subscribers: khamidou
Differential Revision: https://phab.nylas.com/D4033
Summary:
If for whatever reason we passed an error with a `source` property
we weren't accounting for in `convertIMAPError`, we would return an
`undefined` error. Bad!
Instead, just return the original error
Test Plan: manual
Reviewers: spang, halla, evan, mark
Reviewed By: evan, mark
Differential Revision: https://phab.nylas.com/D4037
Summary:
Since we only persist updates to fetchedmin/fetchedmax at the end of a
batch, and a batch can contain many messages, if the sync loop is
getting interrupted often we will download the same messages over and
over again and not make much progress in downloading the message
backlog. This patch keeps a set of already downloaded messages in memory
for each batch and skips downloading UIDs we've processed in interrupted
sync loops.
Messages may still be redownloaded across app restarts.
Fixes T7798
Test Plan: manual
Reviewers: juan, mark
Reviewed By: juan, mark
Maniphest Tasks: T7798
Differential Revision: https://phab.nylas.com/D4040
This is a temporary commit to prevent connections from being closed
while we release logic to re-establish delta connections after they are closed.
Our infrastructure should be able to support current usage of delta
connections after our fixes to offline notification and infrastructure
This reverts commit 78f67d4a76.
Summary:
We were only detecting and classifying IMAP errors as retryable and non
retryable in a few places, but errors thrown in any of our imap
operations were not being properly passed through our `convertImapErrors` :(
This was broken, oh so broken
Also loosen condition to detect System Errors
Test Plan: manual
Reviewers: spang, mark, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4022
Summary:
Fix errors and inconsistencies in MetricsReporter code like:
- We were modifying the passed in data object in place
- We were accessing NylasEnv without knowing we were in client env to
get the version
- We were creating an incorrect logger instance
Test Plan: manual
Reviewers: spang, halla, evan
Reviewed By: halla, evan
Differential Revision: https://phab.nylas.com/D4010
Summary:
Previously, we would create a nodemailer SMTP transport object when the
sync worker booted up. The transport object would be passed the account
SMTP credentials at the time of object creation. If the Google auth
token later expired, we would continue to try to send mail using the
expired token, resulting in "Invalid login" failures.
This patch makes it so we refresh the transport object if the auth token
changes, and also turns on SMTP connection pooling to limit simultaneous
SMTP connections (& maybe make sending multiple messages faster).
Fixes T7891
Test Plan: manual
Reviewers: juan, halla
Reviewed By: juan, halla
Subscribers: mark
Maniphest Tasks: T7891
Differential Revision: https://phab.nylas.com/D3997
Summary:
This fixes a regression introduced in D3980 that prevented sending from working
on Office365 and generic IMAP accounts.
Fixes T7892
Test Plan: manual - we could use unit tests for this but need to set up tests for iso-core first
Reviewers: juan, halla, evan
Reviewed By: halla, evan
Maniphest Tasks: T7892
Differential Revision: https://phab.nylas.com/D4003
