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
Summary: See title
Test Plan: Run locally, verify downloading files works
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4004
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 was broken, oh so broken
Test Plan: manual
Reviewers: halla, spang, evan, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D4001
Summary:
This will help us aggregate metrics by user. This also makes it so we
don't report events in dev mode
Test Plan: manual
Reviewers: spang, evan
Reviewed By: spang, evan
Differential Revision: https://phab.nylas.com/D3981
Summary:
Renamed it from SyncMetricsReporter to MetricsReporter and moved it to
iso-core.
The new metrics reporter can now be called from any environment and will
correctly report the metrics.
Test Plan: manual
Reviewers: mark, spang, evan
Reviewed By: spang, evan
Differential Revision: https://phab.nylas.com/D3967
Summary:
Adding READMEs for easy and helpful browsing on GitHub.
Also add missing script and `--interpreter` flag
Test Plan: Run new launch commands
Reviewers: mark, spang, juan, halla
Reviewed By: spang, juan, halla
Differential Revision: https://phab.nylas.com/D3971
Summary:
Client-sync has the full imap folder names, but used to only pass the display
name to the application. The application needs the full imap names so that it
can pass them via metadata to cloud-workers that need to open imap boxes.
Test Plan: manual
Reviewers: evan, juan
Reviewed By: evan, juan
Subscribers: juan
Differential Revision: https://phab.nylas.com/D3951
Summary:
If an exception has the same stack trace, by default Sentry will always group
it together in the same event. We don't want to do that for sync loop
errors---e.g. 'Invalid credentials' errors should not be grouped together with
stuff like 'Too many simultaneous connections'. Creating more unique groups
will allow us to better evaluate the effect of sync & other bugfixes.
Test Plan: writing unit test right now
Reviewers: juan, mark
Subscribers:
Differential Revision: https://phab.nylas.com/D3915
[*] update babel
[client-app] remove flow-typed
[client-app] Move build/package.json to main package.json
[client-app] remove spec_integration
[client-app] fix babel support
Add client-private-plugins package.json
[client-app] add node_modules to global path for private-plugins
Move client-sync dependencies to client-app root
fix electron rebuild
[*] moved to monorepo
Summary: App now runs in monorepo
Test Plan: npm test
Reviewers: juan, mark, khamidou, halla, spang
Differential Revision: https://phab.nylas.com/D3947
