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:
We listen for unhandled rejections on both the `process` and the `window`. From
my understanding, both are necessary because some errors might escape
one or the other. However, most errors get caught by both handlers, so
we don't want to double report them to sentry (or the console).
To prevent this, this commit debounces the unhandled rejection handler
for a very short time so that the same error only gets reported once.
Additionally, it rate limits reports to sentry based on the stack trace
Test Plan: manual
Reviewers: mark, halla
Reviewed By: mark, halla
Differential Revision: https://phab.nylas.com/D4413
Summary:
Previously, we were listening for unhanlded rejections on both the
`window` and the `process`. However, the window handler didn't receive
an error as a parameter, but rather a window event. When trying to
report this event and sending it over IPC we would get an ipc parse
error.
This commit makes it so we correctly report the error instead of the
window event.
Test Plan: manual
Reviewers: mark, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4412
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:
We were reporting 1m+ of these events, heavily contributing towards our
quota
Test Plan: manual
Reviewers: halla, mark
Reviewed By: halla, mark
Differential Revision: https://phab.nylas.com/D4406
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:
Add a sequentialId check in isDependentOnTask() so that the first
SyncbackMetadataTask won't get blocked on other SyncbackMetadataTasks
in the queue for the same pluginId.
Addresses T8112
Test Plan: Manual
Reviewers: spang, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4404
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:
Put the IMAP option back on the provider selection page within
onboarding. Also remove the 'hidden' property on account types since I
don't think we'll be using it for any other providers, and remove the
'Add Custom IMAP Account' from the application menu.
Addresses T8097
Test Plan: Manual
Reviewers: spang, mark, juan
Reviewed By: mark, juan
Differential Revision: https://phab.nylas.com/D4402
Summary:
Previously, every time our NylasLongConnection changed its status to
`connected`, we would reset our backoff scheduler, assuming that
every time our status changed to `connected` it meant that we would not
receive any errors.
However, if our backend is down or overloaded (or returning 401s like in
our recent outage), we would do the following:
1. Establish connection, change status to `connected`
2. Reset backoff scheduler
3. Receive error
4. Close connection
5. Schedule next retry using backoff scheduler. Delay will be 1s or less, because scheduler was reset in 2.
6. Retry after delay,
7. Repeat from 1., ad infinitum
This caused us to consistantly hammer our servers and render our
exponential backoff useless.
This commit makes it so we conly reset the backoff scheduler when we
actually receive deltas, and increments the max backoff to 10 minutes
Test Plan:
run against local n1Cloud, manually return 401s from the api, verify that we
didn't backoff at all before this diff, verify that we backoff correctly after
this diff
Reviewers: mark, spang, halla
Reviewed By: mark, spang, halla
Differential Revision: https://phab.nylas.com/D4401
Summary:
Something weird happens with the mime building when the participant name
has an @ symbol in it (e.g. a name and email of hello@gmail.com turns
into 'hello@ <gmail.com hello@gmail.com>'), so replace it with
whitespace.
Test Plan: manual
Reviewers: evan, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4363
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:
Previously, SyncbackMetadataTasks for the same plugin did not depend on
each other, so they could try to modify the same piece of metadata
concurrently, which would likely produce version conflict errors.
This was happening with send reminders because we were queuing 2 of
these tasks back to back upn send success. Adding this dependency fixes
the version conflict errors when setting the metadata
Test Plan: manual
Reviewers: evan, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4393
Summary:
Fixes T8076. So, the Gmail OAuth API returns us an expiration time in milliseconds, but we convert it to seconds everywhere, except in the cloud-api. This caused us to never refresh tokens because we'd be comparing seconds to milliseconds.
Fix this by converting the `expiry_date` from our credentials to seconds.
Test Plan: Tested manually. Will do more testing tomorrow, after my credentials expire.
Reviewers: evan, juan
Reviewed By: evan, juan
Maniphest Tasks: T8076
Differential Revision: https://phab.nylas.com/D4390
Summary:
MySQL would insert bad JSON due to emoji errors. This would cause JSON
parse errors and prevent us from ever reading objects from the DB. We now
catch and return null instead with an error which will at least let the
query complete to let us isolate and deal with the malformed obeject (like
destroy it)
Test Plan: manual
Reviewers: spang, halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4389
Summary:
We need this in order to be able to store 4-byte emoji and other
extended unicode characters in the database.
Test Plan: deploy to staging - need the change that sets the connection charset on staging :(
Reviewers: khamidou, juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D4387
Summary:
This was annoying because we wouldn't fetch the identity when running `local`
env which produced all sorts of errors
Test Plan: manual
Reviewers: evan, spang, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4380
Summary:
We were not generating the XOAuth2 token on the first go when connecting
accounts. This would cause the first send later to fail. Since this
happens in ISO core, we needed to move the function out of cloud-core and
import from other places that use it
Test Plan: Manual
Reviewers: khamidou, juan, halla, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D4375
Summary:
Previously, we would initialize an `AccountPool` with an account
instance and reference the account as an instance variable. Then,
every time we created a connection, we used the account we had a
reference to obtain the credentials.
However, if the credentials on the account changed, e.g. when we refresh
the access token for gmail accounts, the connection pool would never
realize that the account had new credentials and always try to create
the connection with the old credentials, causing the sync worker to
enter an error loop of `Invalid credentials` errors
To fix this, we could reload the account every time we try to create a
connection, but it seems that the fresh credentials should just be
passed every time we create connections without the pool having to be
aware of potential changes under the rug.
This commit makes it so the pool no longer holds a reference to the
account, but rather it is passed in every time we check out a connection
Test Plan: manual. how did this even work before? :(
Reviewers: evan, halla, spang, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D4370
Summary:
Previously, the generic IMAP auth screen presented one security option to
users: "Require SSL". This was ambiguous and difficult to translate into
the correct security options behind the scenes, causing confusion and problems
connecting some accounts.
This patch does the following:
* Separates security settings for IMAP and SMTP, as these different protocols
may also require different SSL/TLS settings
* Reworks the generic IMAP auth page to allow specifying security settings
with higher fidelity. We looked at various different email apps and decided
that the best solution to this problem was to allow more detailed
specification of security settings and to ease the burden of more options
by having sane defaults that work correctly in the majority of cases.
This new screen allows users to pick from "SSL / TLS", "STARTTLS", or "none"
for the security settings for a protocol, and also to instruct us that
they're OK with us using known insecure SSL settings to connect to their
server by checking a checkbox.
We default to port 993 / SSL/TLS for IMAP and port 587 / STARTTLS for SMTP.
These are the most common settings for providers these days and will work
for most folks.
* Significantly tightens our default security. Now that we can allow folks to
opt-in to bad security, by default we should protect folks as best we can.
* Removes some now-unnecessary jank like specifying the SSLv3 "cipher"
in some custom SMTP configs. I don't think this was actually necessary
as SSLv3 is a protocol and not a valid cipher, but these custom
configs may have been necessary because of how the ssl_required flag was
linked between IMAP and SMTP before (and thus to specify different
settings for SMTP you'd have to override the SMTP config).
* Removes hard-coding of Gmail & Office365 settings in several
locations. (This was a major headache while working on the patch.)
This depends on version 2.0.1 of imap-provider-settings, which has major
breaking changes from version 1.0. See commit for more info:
9851054f91
Among other things, I did a serious audit of the settings in this file and
"upgraded" a few servers which weren't using the SSL-enabled ports for their
provider to the secure ones. Hurray for nmap and openssl.
Test Plan: manual
Reviewers: evan, mark, juan, halla
Reviewed By: juan, halla
Differential Revision: https://phab.nylas.com/D4316
Summary: See title
Test Plan: Run on the coffee machine
Reviewers: juan, evan, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D4368
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:
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
