Summary:
This prevents the app from entering a restart loop when there's no
identity.
Specifically, when a user logs out of their identity, or when she resets the
email cache, or any other scenario that leaves the app without an
identity but with accounts added, the sync loop (and deltas) will start
without an Identity.
This will cause NylasAPIRequest to throw an error that
forces the user to close the app. When the app restarts, sync will start
again without an identity, and the user will be forced to close the app
again, and so on and so forth for the rest of eternity
Relevant error:
https://github.com/nylas/nylas-mail-all/blob/master/packages/client-app/src/flux/nylas-api-request.es6#L165-L174
Additionaly, this makes sure that after resetting the email cache, the sync
process starts when the identity becomes available
This solves T7989
Test Plan: manual
Reviewers: evan, spang, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4212
Summary:
Previously, while resetting the email cache, we would try to stop sync
and just wait for an arbitraty amount of time for it to stop and the
proceed to blow away the database.
This commit makes it so we correctly wait for sync to stop, and then we
blow away the db. It adds a timeout anyway in case sync is stuck and we
can't stop it
Depends on D4207
Test Plan: manual
Reviewers: evan, spang, halla
Reviewed By: spang, halla
Differential Revision: https://phab.nylas.com/D4209
Summary:
We no longer keep `this._accounts` state, which was being accessed
inside `_resetEmailCache`
Test Plan: manual
Reviewers: evan, spang, halla
Reviewed By: spang, halla
Differential Revision: https://phab.nylas.com/D4207
Summary:
We were always incrementing the queue length and resolving the promise
in the constructor :-/
Test Plan: Run locally, make sure queue doesn't start rejecting messages
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4198
Summary:
Errors in the MessageProcessor were causing sync to get stuck
occasionally. This diff refactors queueMessageForProcessing and friends
so that they're more robust to errors during promise construction.
Test Plan: Run locally
Reviewers: juan, spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4190
Summary:
Sequelize can sometimes return promises that will never resolve or
reject. We can wrap the promises we get back from sequelize in a
bluebird promise which gives us the ability to timeout these abandoned
promises. This way, we can track down issues in sequelize as well as
unblocking stuck sync loops.
Test Plan: Run locally, verify that timeouts occur
Reviewers: juan, evan, spang
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4192
Summary:
This commit adds error handling to the sync-loop's `onSyncError` and
`scheduleNextSync`.
These functions generally don't fail, as they are in the `catch` and
`finally` blocks respectively of the sync loop. But as we've seen in
D4152, the datbase can sometime error if it's in a bad state. If it
errors inside these functions, we will never schedule the next run of
the sync loop.
Depends on D4152
Test Plan: manual
Reviewers: evan, spang, mark, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4153
Summary:
This commit consolidates the `DeltaConnectionStatusStore` and the
`DeltaConnectionStore` which kept track of very similar state and made
sense to be the same store (as per feedback in D4118#77647)
Given that this state needs to be available app-wide for plugins to
query the status of delta connections, `internal_packages/deltas` was
removed (given that it only activated that store), in favor of having the
unified store inside `src/flux/stores` and available via `nylas-exports`
The `deltas` package also contained some contacts-ranking code, which is
no longer in use until we restore that fetaure, so I created a
`internal_packages/contact-rankings` which contains this unused code for
now.
Test Plan:
manually open, close, end delta connections, verify that I'm getting
correct results. unit tests to come
Reviewers: halla, spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4145
Summary:
This error is can flood Sentry pretty badly: https://sentry.io/nylas/nylas-mail/issues/230629801/
My initial thought was to rate limit it, but rate limiting wouldn't
do any good because when we get that error we destroy the databases and
restart the app, and we will loose the in-memory rate limiting data.
The real fix, and reason why Sentry is being flooded with this error by
a single user, is that once you encounter this error the app will enter
a restart loop that constantly throws this error. The reason being that
we weren't properly awaiting for the K2 account databases to be dropped
before closing the app, so on restart, the database would still be
malformed.
The fix is to properly `await` for the database drops
Test Plan: manual
Reviewers: spang, mark, halla, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4127
Summary:
This is in preparation for removing the ability of the IMAP pool to
automatically handle timeout errors.
Test Plan: Run locally, verify file download still works
Reviewers: juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4167
Summary: In preparation for removing timeout handling from IMAPConnectionPool.
Test Plan: Run locally
Reviewers: juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4168
Summary:
We had this weird nesting where we would keep the IMAPConnection open
beyond the scope of the connection pool and return the created
Observable. This diff inverts that relationship and flattens out some of
the indentation, making the code easier to read. This is in preparation
for refactoring the IMAPConnectionPool to no longer automatically handle
timeouts.
Test Plan: Run locally, make sure search still works
Reviewers: juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4165
Summary: Misspelled INPROGRESS-NOTRETRYABLE :-(
Test Plan: Run locally, make sure things get cancelled
Reviewers: juan
Differential Revision: https://phab.nylas.com/D4163
Summary:
Previously if we got back a huge number of unknown UIDs from IMAP search
we would try to sync all of them at once. This could lead to hanging the sync
loop trying to download tons of messages. This diff limits the UIDs we're
willing to sync per task to 500 and splits each task up into chunks of 25
messages so that we don't try to download all of them at once. If we need
to sync more than 500 uids then at the end of the syncback task it will
queue another task to run the next time the sync loop rolls back around.
Test Plan:
Run locally, verify that we gracefully handle various situations
including cancelling during the syncback task, cancelling between syncback
tasks, huge numbers of results, etc
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4142
Summary:
We don't want to constantly retry to sync a bunch of random UIDs if
we're not longer in the search perspective so make these syncback tasks
not retryable.
Test Plan: Run locally, verify that we don't retry
Reviewers: spang, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4149
Summary:
We want to enable code to control whether fetching is throttled or not.
We basically only want to throttle syncing the historical message
archive. New messages, initial inbox sync, and unknown search results
should not be throttled. Also some drive-by code refactoring.
Test Plan: Run locally, verify that things still work
Reviewers: spang, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4126
Summary:
This commit splits apart the `AccountDeltaConnection` class, which was
in charge of listening to both cloud /and/ local deltas by way of an
artificial interface, `DeltaStreamingInMemoryConnection`.
Splitting this into 2 modules with separate responsibilities will hopefully
make this code easier to reason about and reduce some cruft and unnecessary
indirection.
Specifically, this commit makes it so:
- `DeltaConnectionManager` is only in charge of starting and ending `DeltaStreamingConnection`s, which are solely in charge of listening to deltas from the cloud api
- `LocalSyncDeltaEmitter` no longer unnecessarily emits events for the `deltas` package to listen to but rather directly processes and saves those deltas from the K2 db to edgehill.db
- `LocalSyncDeltaEmitter` is also in charge of keeping track of the latest received cursor, under its own JSONBlob key in edgehill.db. This migrates localSync cursors saved under the old key.
- `LocalSyncDeltaEmitter` is now instantiated and managed from within the `SyncProcessManager` as opposed to the `SyncWorker`. Apart from removing extra state from the `SyncWorker`, this removes dependencies on the client-app environment from the sync-worker.
- `DeltaStreamingInMemoryConnection` and `AccountDeltaConnection` are now gone
(Sorry for the big diff! This one was a little hard to split up without landing something broken)
Depends on D4121
Test Plan: manual + unit tests planned in upcoming diff
Reviewers: halla, mark, evan, spang
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4122
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:
This diff adds a timeout parameter to queueMessageForProcessing that can
be used to override the CPU-throttled-by-default timeout used by the
message processing queue. The FetchMessagesInFolder sync task now
overrides with a timeout of 0 when it is queuing specific UIDs that it
has downloaded. The thinking here is that if we've requested a specific
set of UIDs, it's very important to sync those quickly as this indicates
a higher priority (e.g. initial inbox sync, search result syncing, etc).
Test Plan: Run locally, verify that initial sync and search syncing run faster
Reviewers: spang, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4114
Summary:
When searching using IMAP/Gmail commands we sometimes get back UIDs for
messages that we have yet to sync. Previously we would just ignore these
results, which would decrease the quality search results for quite some
time during initial sync. This diff enables us to eagerly sync the unknown
messages we get back from the provider by creating a syncback task which
interrupts the sync loop and runs a sync task for the unknown UIDs.
Test Plan: Run locally, verify that we sync unknown messages
Reviewers: spang, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4101
Summary:
This commit adds a new option to the `reportError` interface to allow
to rate limit specific kinds of errors that are known to be reported a
lot if they fall in a loop for example.
For now, it only allows to impose a fixed reporting rate per hour for a specific
error, and will not report any errors past that rate before an hour has
passed.
Depends on D4099
Test Plan:
manually check that it reports and does not report errors based on the rate
limit data passed
Reviewers: khamidou, mark, evan, halla, spang
Reviewed By: evan, halla, spang
Differential Revision: https://phab.nylas.com/D4100
Summary:
Add better messaging and logging to the console.
Depends on D4098
Test Plan: manual
Reviewers: evan, khamidou, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D4099
Summary:
Given that NylasAPIRequest reports all APIErrors for requests that
return 5xx, we were double reporting access token errors, which caused 2
groups in sentry and makes it difficult to track.
This commit removes the extra reporting for that error in the sync loop
Test Plan: manual
Reviewers: evan, khamidou, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D4098
Summary:
Given that we backoff exponentially in the sync loop when we encounter
RetryableErrors (e.g. network errors), if the app goes offline and
reaches the maximum retry backoff of 5 minutes, and if then the app comes
back online, in the worst case, the sync loop will idle for 5 minutes before trying to
sync, which is undesireable.
To mitigate this, everytime we come online we can wake all of the sync
workers. This shouldn't have any adverse effects
Test Plan: manual
Reviewers: evan, mark, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D4097
Summary:
This is mostly to clean up the logic here a little bit.
Specifically:
- The backoff when encountering retryable errors is now truly exponential
- When encountering a permanent error, we back off for a minute
- If we don't encounter consecutive RetryableErrors, we clear the exponential backoff. Previously, we would just clear it when sync completed without errors.
Test Plan: manual
Reviewers: khamidou, spang, evan, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D4086
Summary:
Previously we used the Gmail REST API to get search results. This API
returns the X-GM-MSGID which is different from the folder UID. Gmail also
offers the X-GM-RAW extension for IMAP's UID SEARCH command which allows
us to pass through the raw search query. This enables us to vastly
simplify the GmailSearchClient by having it subclass ImapSearchClient
and override a few select methods. Yay!
Test Plan: Run locally, verify that Gmail search queries still work
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D4094
Summary:
Previously for IMAP we just grabbed the search text and fed it into a TEXT
query. Now we have a proper backend that generates the appropriate
search criteria according to the IMAP spec. Important to note that we
don't support 'in:' yet, which is complicated due to the way that IMAP
search is scoped to the currently selected folder.
Test Plan: Run tests, run locally and verify IMAP search still works.
Reviewers: evan, juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D4071
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:
Disabling verbose logging in production builds just makes it harder to help our
users help us fix their problems.
Test Plan: run app without --dev
Reviewers: mark, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4070
Summary: See title
Test Plan: Run locally, make sure sync still works
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D3998
Summary:
I did a grep for `Message.find.*` to see if there were any other places
we could limit the attributes returned
There were.
This used to be INSANELY wasteful to return all bodies when we only needed
the ids.
Test Plan: Manually bootup app and ensure search still works
Reviewers: mark, juan, spang
Reviewed By: mark, juan, spang
Differential Revision: https://phab.nylas.com/D4042
Summary:
Fixes T7783
This unbounded query, plus a yet-to-be-diagnosed bug causing large numbers
of messages with no folderIds, led to frequent crashes of the worker
window at the end of every sync loop.
This took a fair amount to diagnose. Notes on that are here:
https://paper.dropbox.com/doc/Diagnosing-Crash-X329VTxevrqtIMESBtHNV
Huge thanks to @mark.
Test Plan: Reboot app with patch, no more crashes!
Reviewers: spang, halla, mark, juan
Reviewed By: mark, juan
Subscribers: mark
Maniphest Tasks: T7783
Differential Revision: https://phab.nylas.com/D4039
This goes against the spec, but we've seen emails which use comma-separated
References instead of space-separated references. This could cause threading
to break if the message with comma-separated references was a reply and
we hadn't yet synced the message directly in the In-Reply-To header.
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
Summary:
Previously we would always search all mail. Now, if the user has focused
a particular folder we will limit our search to that folder. The inbox
is an exception--it will always search all mail unless the user
explicitly uses an "in:" clause.
Test Plan: Run locally, verify that searching folders returns the correct results.
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4032
Given that this packages lives inside the NylasMail app, it has access
to Bluebird Promises, which already define a .each method.
Using PromiseUtils here is unecessary
Summary: See title
Test Plan: Run locally, verify IMAP search still works
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4006
Summary: See title
Test Plan: Run locally, verify we can download raw messages
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4005
