Summary:
This ensures that the Libhoney instance is a singleton in cloud
processes.
Test Plan: manual
Reviewers: mark, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3969
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:
In client-app/node_modules Lerna symlinks isomorphic-core to '../../'.
When we copy everything over to our tmp directory when building, we copy
over the relative symlink! Damn you lerna.
We get around this by resolving the symlinks BEFORE copying and caching
them in the installer task. Then the file copy always works.
Also, for some reason the glob {absolute} param doesn't seem to work in
the latest version. I'm manually creating an absolute path for the compile
target since it's a bit more transparent what's happening anyway.
Test Plan: `npm run build-client`
Reviewers: spang, jerm, juan, halla
Reviewed By: juan, halla
Differential Revision: https://phab.nylas.com/D3988
Summary:
Grunt is hardcoded to use paths relative to wherever the Gruntfile is
located. Unfortunately it also expects the grunt packages to be siblings
of that gruntfile. We can get around this by changing the relative base
path, but then the cwd is different for each tasks. This is okay as long
as we use absolute paths for various files in each of our tasks. This
updates our grunt tasks to use absolute paths
Test Plan: `npm run build-client`
Reviewers: spang, halla, jerm, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3987
Summary: This error should really be retryable, and will prevent more red boxes
Test Plan: manual
Reviewers: evan, spang
Reviewed By: evan, spang
Differential Revision: https://phab.nylas.com/D3982
Summary:
We previously weren't saving the smtp settings for cloud gmail accounts,
and even though we fixed that, we still need to be able to handle the accounts
that were authed before that fix went out. This diff changes `smtpConfig()` to
always call `credentialsForProvider` instead of depending on what was saved
in the database.
Test Plan: manual
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3980
Summary:
Process reminders metadata and send the reminder email if there
haven't been any replies. Checking for replies involves checking
each of the folders in `folderImapNames` for messages that are in
reply to the messages in `messageIdHeaders`.
Test Plan: manual
Reviewers: khamidou, juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3958
Summary:
The SendmailClient is in isomorphic-core, but wasn't working properly when
called from cloud-workers. This diff includes a variety of fixes to get it
working:
- Add smtp config to connection settings in cloud-core
- Don't use `PromiseUtils.promisify` for a function that needs `this` to
be bound properly
- Don't reference `NylasEnv` (That error gets caught and reported
elsewhere anyway, we don't need to report it here)
- Default `message.uploads` to `[]` to keep iterator happy
- Add Gmail environment variables to cloud-worker app
Test Plan: manual
Reviewers: juan, khamidou, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3954
Summary:
The new send-reminders cloud-worker needs different information to check for
replies now that the sync engine is local. Make sure we save all of the
required information to the metadata. The current values are:
- expiration: the time the reminder should be sent, if no new replies
- folderImapNames: the folders to check for replies
- messageIdHeaders: the Message-Id headers of messages in the thread
(we search for In-Reply-To headers that match these values, and also
use them to decide whether a message is a new reply or an old reply)
- replyTo: the message to reply to when sending the reminder email
- subject: the subject to use when sending the reminder email
Test Plan: manual
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3952
Summary:
This removes client-sync deltas from the developerbar delta list.
We ONLY show cloud deltas now.
The connection between client-sync is no longer a network delta stream,
it's a direct function call. It makes no sense to show its status.
This now shows a single dot representing the state of the cloud delta
stream.
Test Plan: Manually connect and disconnect local cloud API and see icon change
Reviewers: halla, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3977
Summary:
SFDC's task to upload an email to salesforce needs the stripped DOM of a
Message object to call `innerText`. The API was changed to return a string
instead of the DOM. This adds a flag to request the DOM instead of a
string.
Test Plan:
Manually assert `EnsureMessageOnSalesforceTask` properly can add the plain
text to the Salesforce Task object
Reviewers: halla, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3976
Summary:
Node's native `Error` object does NOT implement toJSON. We attempt to call
toJSON when reporting errors. This wasn't noticed until now because
bunyan's pretty logger (which only run in dev mode) started JSONifying
errors
Test Plan:
Try and API auth with a bad username with local setup. See that it throws
toJSON error. After patch, error properly serializes
Reviewers: spang, halla, jerm, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3975
Summary:
Some API Errors, like ECONNREFUSED, have no \.message.
Catch this in the error reporter
Test Plan: Manually create a non-message error and see better error message
Reviewers: spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3973
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: atob() is a global in browser environments, but needs to be imported otherwise.
Test Plan: manual
Reviewers: juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3950
Summary: Moved a file and forgot to update this import :-/
Test Plan: Run locally
Reviewers: juan, evan, spang
Differential Revision: https://phab.nylas.com/D3970
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
Summary: See title
Test Plan: Run locally, verify that double clicking inline images opens them
Reviewers: evan, juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D3963
Summary: This stuff doesn't seem to be used for anything anymore and it's cluttering up the client-app dir.
Test Plan: ran the app, also did `npm start` with no ~/.nylas-dev
Reviewers: evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D3961
Summary:
This is a set of functions which will allow isomorphic-core to detect
which environment it is running on.
This will be useful for moving the metrics reporter to iso-core
Test Plan: manual
Reviewers: mark, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3966
Summary:
We were doing some incorrect processing of args passed to the main
function which was causing us to think we were launching NM by passing a
file (which creates a new draft and tries to attach that file). Since
were trying to attach 'packages/client-app', this was causing an error
dialogue to appear indicating that it wasn't possible to attach a
directory.
Test Plan: Run locally, verify no dialogue
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3962
[*] 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
Summary:
When saving a thread, we weren't properly setting the `participants`
object. Since Sequelize has object properties under getters and setters,
doing `this.participants.push()` did nothing.
Since threads had no `participants`, the related threads widget, which
queries through there, did nothing.
Test Plan: Manually inspect DB and use the related threads widget
Reviewers: halla, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3938
Summary:
This is the result of auto package.json fixing by lerna. Would be nice to
commit this so you can run script/bootstrap without it making local
changes.
I didn't manually bump any versions.
Test Plan: manual
Reviewers: mark, halla, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3934
Summary:
We don't need to check folders that often while on battery. Check every
5 minutes rather than every 10 seconds.
Test Plan: Run locally, verify the timeout is longer while on battery
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D3940
Summary: See title
Test Plan: manual
Reviewers: evan, mark, spang
Reviewed By: spang
Subscribers: mark
Differential Revision: https://phab.nylas.com/D3932
Summary: They obscure the location of our logs in the dev tools.
Test Plan: Run locally, verify that logs link to proper place
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D3939
Summary:
Previously, we would only refresh Google OAuth2 access tokens at the
beginning of the sync loop, and _only_ if the access token had already
expired. This meant that if an access token expired in the middle of a
sync loop iteration, the user would get prompted with the reauth red box
for their account and would have to either go through the oauth flow
again or restart the app for sync to continue.
This diff makes two changes:
1. Adds 5min of padding to the refresh window, so if a token will expire
in <5min, we'll go ahead and refresh the token. This will reduce the
possibility that an access token can expire during a sync loop
iteration.
2. Catches Invalid Credentials IMAPAuthenticationErrors for Gmail
accounts and forces a token refresh on the next sync loop.
These should prevent a user from _ever_ having to reauth their Gmail
account unless the refresh token is revoked, or we encounter some other
permanent error trying to refresh the token.
Fixes T7775 (at least some cases)
Test Plan: manual
Reviewers: khamidou, evan, juan
Reviewed By: juan
Maniphest Tasks: T7775, T7755
Differential Revision: https://phab.nylas.com/D3908
Summary:
Instead of re-implementing exponential backoff, throw the retryable
error so the sync loop handles it and backs-off
Test Plan: manual
Reviewers: spang, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D3914
Summary:
Update Salesforce to use the new search indexer
Depends on D3911
Test Plan: Manually bootup SFDC and ensure it launches and indexes models properly
Reviewers: mark, halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3913
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:
This commit makes it so we always continue retrying syncback tasks as long as they
error with a retryable error. There's really no reason to not continue retrying syncback
tasks after an arbitrary number of retries (especially such a low one) if we
encounter a retryable error. Before this commit, if for example we got 2 random
network errors in a row, we would just mark the task as failed even
though it would eventually succeed in subsequent attempts.
Previously, when N1 synced against the cloud api, we would indefinitely
retry a Task if we continued getting retryable errors. This ensures that
the app can work correctly offline and prevents displaying unecessary errors to
the user, and having actions bouncing back or sending messages without putting them
in the sent folder.
Additionally, this commit ensures that when cleaning up messages without a
folderImapUID we don't delete messages that are currently being added to the sent
folder. This is relevant to this commit because given that we could retry the
EnsureMessageInSentFolder task indefinitely, we might end up deleting that
message because it wont have a uid until the task succeeds.
Depends on D3898
Test Plan: manual
Reviewers: mark, spang, evan, halla
Reviewed By: spang, evan, halla
Differential Revision: https://phab.nylas.com/D3900
Summary:
Since all Nylas Mail actions are thread-based except for sending,
we never use any of these. Make `git grep` less confusing by getting rid
of them.
Test Plan: use the app
Reviewers: evan, halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3910
Summary:
Previously, if you were to close the app while you had any tasks queued,
these would be marked as failed the next time you open the app, showing
an annoying error message and reverting any optimisitic actions.
However, we don't need to be so defensive about retrying tasks because
the only tasks we can't retry are the Sending tasks. All of the other
tasks like moving or changing labels are fine to retry (trying move the same
set of uids twice wont cause an error)
This commit adds an extra status to syncback requests, "NOTRETRYABLE".
Only NOTRETRYABLE requests will be marked as failed at the beginning of
the sync loop, and any INPROGRESS tasks will be marked as NEW so they
can be retried
Depends on D3896
Test Plan: manual
Reviewers: mark, evan, spang, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D3898
Summary:
Now that we don't run Send tasks outside the sync loop, we don't need
that awful hack wich required passing a `runTask` callback to
`runSyncbackTask` in order to customize how to run the task.
Instead, runSyncbackTask now knows 2 ways to run a task, either via imap, or
via smtp, depending on the resource declared by task to run. So now
SyncbackTasks declare a resource type they need to run, and that will be
passed as their second argument when running.
Depends D3894
Test Plan: manual
Reviewers: mark, halla, spang, evan
Reviewed By: halla, spang, evan
Differential Revision: https://phab.nylas.com/D3896
Summary:
We had previously ripped send tasks outside the sync loop to make them run faster,
but they run fast enough inside the loop.
This commit will also fix the scenario where if you closed the app in the
middle of a send task, the task would just hang forever and never succeed or
fail (T7818); given that it was excluded from the loop, we also had to exclude it
from the cleanup step to mark any INPROGRESS tasks as failed at the beginning
of each loop, which caused send tasks in progress to never get cleaned.
Putting them back inside the loop allows us to fix this without adding more messy
logic, and it cleans up ugly duplicated code. Additionally, it will prevent more
duplicated code for upcoming diffs that will improve syncback task reliability
when the app is closed or window is restarted in the middle of a task.
Depends on D3893
Test Plan:
manually test sending, it still works, it's still fast. Restarted
window in the middle of send task, task fails.
Reviewers: mark, spang, halla, evan
Reviewed By: spang, halla, evan
Differential Revision: https://phab.nylas.com/D3894
Summary:
[cloud-api]
Based on the passed in `messageIds`, it finds any existing thread
metadata that might be under a different thread id. If it realizes
there are actually multiple threads that should be the same thread,
(due to getting a missing message link), it reconciles all of them.
[local-sync]
Return `message_ids` in `Thread.toJSON()`
See D3879 for tests
N1 Pairing: D3875
Test Plan: unit tests, local testing soon
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3880
Summary:
Instead of exposing helper functions, make this a class to hold the
shared state of the db, account, and logger required to run any syncback
inside an account sync loop.
Test Plan: manual
Reviewers: mark, spang, halla, evan
Reviewed By: spang, halla, evan
Differential Revision: https://phab.nylas.com/D3893
Summary:
See title.
I really wish we could clean up this error handling a bit better, but I don't
think its super important right now.
Test Plan: manual
Reviewers: spang, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D3903
Summary:
I've encountered random imap errors that we mark as permanent, but that
contain try again in the error message. We should check for that
Test Plan: manual
Reviewers: spang, evan, mark
Differential Revision: https://phab.nylas.com/D3899
Summary: There's no file to unlink, we just need to drop the tables.
Test Plan: manual
Reviewers: evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D3878
Summary:
If you have no messages in your Gmail Inbox (Yay Inbox Zero!) and you
connect your account and do first sync, then we get an error where we try
and fetch a range from null to -1.
This was due to a logical error in the first sync fetch code.
This diff fixes this bug and renames some variables to make it clearer
what's going on
Fixes T7842
{F11176}
Test Plan:
1. Bring Gmail to Inbox Zero
2. Connect account
3. Verify first sync works
Reviewers: spang, halla, juan
Reviewed By: juan
Maniphest Tasks: T7842
Differential Revision: https://phab.nylas.com/D3889
Summary: See diff title
Test Plan: Run locally, make sure we backoff
Reviewers: juan, spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3886
Summary:
Previously, when processing messages during folder sync, if the message already existed, and it belonged to a thread, we would update the message, but forget to update its thread with any changes that new
message would produce on the thread (e.g. updating the threads folders or labels).
One obvious manifestation of this was when replying to a thread: the EnsureMessageInSentFolderTask would create the new message, and then attempt to sync the sent folder to fetch the newly created message. When processing this message during sync, we would update the message but not update its thread, so the thread would not be associated to the sent folder, and it wouldn't show up in your sent items list in the UI.
Test Plan: manually verify that it works
Reviewers: evan, mark, spang
Reviewed By: mark, spang
Differential Revision: https://phab.nylas.com/D3872
Summary:
Our sentry reporter tries to fetch the nylas identity from the database,
and access properties on it. However, if you are in a state where there
is no identity available (like having logged out, or just starting the
app), and encoutnered an error that would be reported to sentry, we
would throw an error while reporting and that would crash the app
Also, fix lint errors and some really janky code
This fixes T7810
Test Plan: manual
Reviewers: halla, spang, evan
Reviewed By: spang, evan
Maniphest Tasks: T7810
Differential Revision: https://phab.nylas.com/D3867
If the first argument to our local-sync logger is an object
(this is bunyan's api, and it's how we log from isomorphic-core and cloud-* packages
in order to have structured json logs for logstash), make sure we log
the object last and the string that comes as the second argument first.
Summary: Fixes T7649
Test Plan: FML writing unit tests now
Reviewers: evan, mark, juan
Reviewed By: mark, juan
Maniphest Tasks: T7649
Differential Revision: https://phab.nylas.com/D3863
Summary:
This error ocurred, to the best of our knowledge, on iCloud accounts
that had been linked to other clients like Airmail.
On such accounts, node-imap would incorrectly parse the mailbox list
from imap, and return an `Airmail` folder which did not exist, causing
us to try to sync that nonexistent folder and error in the sync loop.
This error is amongst the most frequent we've seen in Sentry and
Support: https://sentry.io/nylas/nylas-mail/issues/213158962/events/4897450600/
The fix es detailed in the PR to node-imap: https://github.com/mscdex/node-imap/pull/594/files
This commit only points the node-imap dependency to our fork for now
Test Plan: manual and unit tests in node-imap
Reviewers: mark, khamidou, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3860
Summary:
This diff (and the K2 counterpart diff) allow us to run dev-mode Nylas
Mail side-by-side with prod Nylas Mail.
There were 4 things that needed to change:
1. Use different config dir
2. Use different keychain name
3. Use different localhost port
4. Prevent Electron's `app.makeSingleInstance` from killing our app
All of these are activated through `NylasEnv.inDevMode()`.
Test Plan: Manual
Reviewers: halla, mark, spang, khamidou, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3861
Summary:
In electron, the --enable-logging flag makes it so the main browser
process logs to stdout all of the logs generated from within the renderer
processes.
Unfortunately, the main process will only log out the first argument passed to
`console.log` from within a renderer process (see https://github.com/electron/electron/issues/7061)
This commit makes it so that the local sync logger logs most of the log line in the first
argument passed to `console.log`
Test Plan: manual
Reviewers: evan, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D3852
Summary:
When multiple accounts are syncing, it's very hard to scan the local
sync logs because it is unclear to which account the logs belong to,
and it makes debugging hard.
This commit makes it so that all logs from local-sync include the
account info, with the account email prefixed at the beginning of each
log line (this allows filtering), and color coded by account.
Test Plan: manual
Reviewers: mark, spang, khamidou, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D3851
Summary:
On each sync loop, we increment the socketTimeout based on how many times we've
seen socket timeouts in a row. The max socket timeout is 10m
Test Plan: manual
Reviewers: evan, spang, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D3843
Summary:
In most cases (and especially so on Gmail and in the inbox on generic
IMAP), messages with higher UIDs are newer---and even if they aren't the
newest possible messages in other generic IMAP folders, they are the
most recent messages that have been moved to that folder.
Our previous batching strategy unfortunately resulted in us downloading
the lowest UID in each batch first, which was especially confusing when
connecting a new account and having the first message pop up on the
screen be a message from hours or days ago.
This patch changes the batching strategy in three ways:
1. Within a batch, we process downloaded messages from highest UID to
lowest UID.
2. We download batches in order of the ones containing the highest UIDs
first.
3. We group together more UIDs within a single batch by ignoring charset
and transfer-encoding on parts and grouping only by MIME part IDs (which
is the only thing you have to pass to the IMAP FETCH command---no idea
why we included this extraneous part data before, probably just
convenience.)
Example old grouping:
batch key: '[{"id":"2","transferEncoding":"QUOTED-PRINTABLE","charset":"UTF-8","mimeType":"text/html"}]'
batch UIDs: [356416,356418,356420,356423,356432,356433,356435,356436,356437,356442,356444]
batch key: '[{"id":"2","transferEncoding":"QUOTED-PRINTABLE","charset":"Windows-1252","mimeType":"text/html"}]'
batch UIDs: [353777]
In the new strategy, all of these messages will be downloaded with the
same FETCH command, reducing IMAP round trips before message processing
begins.
Fixes T7770
Test Plan: manual - connect a new account and see that most recent message downloads first
Reviewers: mark, evan, juan
Reviewed By: juan
Maniphest Tasks: T7770
Differential Revision: https://phab.nylas.com/D3838
Summary:
`isSending` was an artifact from the cloud sync engine that was used to
double check that the sending process for a multi-send draft had been
initiated. I don't believe the intermediate steps are API calls anymore,
and we've had the relevant code commented out for awhile. Time to kill it!
I've revived the double-sending tests in N1 to ensure we still have sufficient
checks against sending the draft again while the first call is still sending.
See D3834
Test Plan: N1 unit tests
Reviewers: juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D3835
Summary:
This code is dead and has confused me grepping around the codebase before.
If for some unexpected reason we need these routes back in the future, we
can always extract them from version control. For now the routes we aren't
using are a distraction.
Test Plan: been using Nylas Mail with this local patch all week
Reviewers: evan, halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3833
Summary:
We can't try to set or remove the sent label on gmail accounts because
the operation will silently fail and cause the threads to later bounce
back.
This occurred when trying to delete or archive a thread that contained a
sent message, and we incorrectly tried to overwrite or remove all of the
labels on messages, without regard for sent.
This was causing https://github.com/nylas/nylas-mail/issues/2706 and
sending and archiving to immediately bounce back.
Addresses T7757
Test Plan: unit tests
Reviewers: halla, evan, spang
Reviewed By: halla, evan, spang
Differential Revision: https://phab.nylas.com/D3829
Summary:
Previously, we attempted to immeditely retry syncback tasks when a
RetryableError was encountered. However, if the error was an
IMAPConnection error, we would keep retrying with a broken connection,
which would keep failing.
The correct way to retry is to wait for the next sync loop, since at the
beginning of each loop we ensure that we are correctly connected to
imap.
To achieve this this commit simply marks a failed task as NEW if it
encoutners a RetryableError and we haven't retried too many times. To
keep track of the number of retries, we save a new field in the `props`
field of the request.
Test Plan: manual
Reviewers: evan, halla, mark, spang
Reviewed By: spang
Subscribers: khamidou
Differential Revision: https://phab.nylas.com/D3831
Summary:
I've decided to break my snooze patches in multiple parts to make it easier to review. This diff contains all the code related to running workers in the cloud. Cloud workers all inherit from the `Worker` class which defines a bunch of useful things like error handling.
What's left to do:
- spawn workers based on the plugin type
- add monitoring (I'm going to add a simple HTTP endpoint for that)
- writing a migration for the local sync db and the prod metadata db.
Test Plan: Tested manually.
Reviewers: halla, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3814
Summary: Make them map the same directory structure of `src`
Test Plan: unit
Reviewers: evan, halla, spang
Reviewed By: halla, spang
Differential Revision: https://phab.nylas.com/D3826
Summary:
syncState on folders may contain arbitrarily long arrays of UIDs
(particularly, failedUIDs). If we serialize this JSON column to
edgehill.db, we can end up serializing very large objects when
persisting the local task queue. When the queue contains many tasks,
this can balloon the JSON blob to megabytes, causing the main window and
the worker window to become unresponsive.
The UI doesn't need to know about IMAP bookkeeping internals, so
serialize the sync progress instead of the sync state. This has the
advantages that (1) we don't need to worry about future keys added
to the syncState being large and (2) when we add Exchange support
we already have an abstraction for sync progress.
Test Plan: manual
Reviewers: juan, mark, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D3817
Summary:
This will prevent us from showing error messages to the user when we
can automatically recover from the error
Test Plan: manual-- throw error from syncback task, check expected results
Reviewers: evan, mark, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3812
Summary:
There are 2 types of IMAP errors that need to be treated as retryable. See code
comments as to why.
Test Plan: manual
Reviewers: khamidou, evan, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3811
Summary: This was broken because babel could not compile the `super` keyword
Test Plan: manual
Reviewers: evan, mark, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D3804
Summary:
EnsureMessageInSentFolder also needs to sync the sent folder to fetch
the uid of the newly moved message (like `MoveThreadToFolder` does)
Test Plan: manual
Reviewers: halla, mark, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3803
Summary:
In `FetchNewMessagesInFolder`, sometimes we haven't synced anything in the folder
we are trying to fetch new messages in. Previously this would just throw
an error, now we properly check if we have a fetchedmax, and if not just
run a normal fetch.
Also, when the target folder box was already open, we were not fetching the /latest/ box status to check the latest uidnext value, so we would skip fetching new messages when in fact there were new messages to fetch
(This can happen for example when moving a sent message to the Sent
folder before we've started syncing the Sent folder)
Test Plan: manual
Reviewers: halla, mark, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3802
Summary: see title
Test Plan: manual, deploy to staging, check that it works
Reviewers: evan, spang, tomasz, khamidou
Reviewed By: tomasz
Differential Revision: https://phab.nylas.com/D3800
Summary:
Syncback tasks that move messages to a different folder used to leave
those messages without a uid because it was unknown at the moment, and
it would only be discovered later on in the sync loop.
This had the unwanted effect of not allowing you to perform more than 1
syncback action on the same thread back to back (e.g. undoing an archive, or
sending and archiving immediately)
This commit makes it so that `SetThreadFolderAndLabels` and
`MoveThreadToFolder` runs a new sync task to fetch the new uids for the
moved messages.
We do this via a new sync task, `FetchNewMessagesInFolder` which /only/
fetches new messages in a folder (no fetching old messages or attribute
updates). This is a first step to cleaning up the gigantic
`FetchMessagesInFolder` task into smaller parts-- but that will come in
a separate diff. For now we want to fix the immediate problem.
See D3788 and D3789 for more details
Test Plan:
manually move threads around, undo moving threads, reply on a thread
and immediately archive
Reviewers: khamidou, mark, spang, halla, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3798
Summary:
updateMessagesFromThread requires `messages` to be an array only when
recompute is falsy
Test Plan: manual
Reviewers: halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D3797
Summary:
If there were any uidvalidity changes after we've completely synced a folder, we
would completely ignore them and not attempt to sync the folder.
Also, we weren't saving the latest uidvalidity from the box to the folder
syncState as soon as we recovered, we only saved it until after fetching
messages. This meant that if the operation was interrupted before updating
syncState.uidvalidity, we would always think that we were in a state of uid invalidity
Recovering from uidvalidity was also broken because we weren't resetting the
fetched ranges, and unnecessarily setting the folderId to null, which meant that
we would never restore the uids of messages we had already fetched.
Test Plan: manual
Reviewers: evan, khamidou, mark, halla
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D3796
Summary:
If we attempt to operate on a box that is no longer open, we should make
the error retryable so that we re-open the correct box and continue
syncing instead of showing the scary red box to users
Addresses T7680
Test Plan: manual
Reviewers: evan, spang, halla, mark
Reviewed By: halla, mark
Subscribers: mark
Differential Revision: https://phab.nylas.com/D3792
Summary:
Given that we perform syncback actions optimistically, i.e. we save the changes to the database before they are synced, some messages might not have an IMAP UID if they haven't been synced in the loop.
When we encounter messages without uid, we could just skip them, but this would mean that we end up moving only a subset of the messages we intended to move, or not move any at all, while thinking that we /did/ move everything, which might cause the thread to bounceback later on when we sync the messages that were missing.
The permanent fix is for syncback actions to not succeed until their changes have been synced, which is coming up in a separate diff
Test Plan: manual
Reviewers: evan, spang, khamidou, mark
Reviewed By: mark
Subscribers: mark
Differential Revision: https://phab.nylas.com/D3789
Summary:
In JavaScript, null <= null is truthy. So if you set fetchedmax but not
fetchedmin and minUID, and then highestmodseq doesn't increment, we will
incorrectly skip updates on All Mail after the first batch of Gmail
inbox UID downloads, only making further progress on sync if the user
receives a new mail.
This patch tightens isSyncComplete() to require that all variables used
in the math comparisons are set.
Test Plan: manual 😢
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3787
Summary:
Previously we just assumed that we were in the "charging" state and wouldn't
update that until the state changed. This would cause us to throttle even if
the app was opened while plugged in. Now we don't do that.
Test Plan: Run locally, verify that we no longer throttle
Reviewers: spang, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3782
Summary:
Previously, when updating message attributes during folder sync, we
would fetch local db messages only by folderImapUID. This was incorrect,
because messages in different folders could have the same uid, so our
query would return messages from other folders, causing us to
incorrectly update their attributes.
This commit ensures we fetch local messages in the current folder
Test Plan: manual
Reviewers: evan, halla, mark, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3777
We now correctly `await` for the `setFolders` and `setLabels`
operations, and make sure that the thread is always saved.
Apparently the `thread.id` check wasn't working as expected and we were skipping a
thread save, which only became apparent when we awaited for `setFolders`
and `setLabels`. Now, we always save the thread (like we did before this
method got updated)
Summary:
When I tested if we could use labels in place of the flag, I had the
Drafts folder selected, not All Mail, and the label did not show up.
Turns out that if you have All Mail selected, all draft messages will be
correctly tagged with the Draft label.
With this patch, we should correctly sync isDraft on all supported
providers and suppress these messages from the app until if or when we
decide to implement draft sync.
Thanks Brandon Long from the Gmail team for the help in getting this right.
Test Plan: manual
Reviewers: juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D3775
Summary:
Otherwise accounts with `Sent` and an `Outbox` will both get the `sent`
role, causing inconsistencies in sync
Addresses T7682
Test Plan: manual
Reviewers: khamidou, spang
Reviewed By: khamidou, spang
Differential Revision: https://phab.nylas.com/D3773
Summary:
Now that message processing is serialized, this is unnecessary. I
was debugging something else and watched the code step through the
exact same db query twice in a row.
Test Plan: manual
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3764
Summary:
Now that we don't do strict validation of certificates for non-major IMAP
providers this shouldn't come up as much, but when it does we're gonna
want a better error message to help support out.
I am not 100% sure there aren't other socket errors that should be fatal,
but this was the one I could figure out by test authing against a server
with a self-signed cert and grepping around the node socket source code.
Test Plan: manual
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3774
Summary:
It turns out we don't ever use the /account API endpoint.
The GET /accounts endpoint was designed to query all accounts connected to
a system for old K2, but we don't use that anymore. The environment
variable protecting the endpoint isn't set anywhere.
We used to `GET /account` from the N1 AccountStore to attempt to refresh
the health of the accounts. The accompianing diff to this one makes that
obsolete. We never need to query for the account health since the sync
loop pushes it to us through `Actions.updateAccount`.
We also never `DELETE /account` because our local-sync runs a function
called `ensureK2Consistency` that compares its DB against the Nylas Mail
`AccountStore`.
This file has always been a huge source of confusion to me and a massive
red herring for anyone trying to understand how the account system work.
The accompyaning diff has more comments explaining the existing system
Depends on https://phab.nylas.com/D3770
Test Plan:
Manually boot N1. Ensure existing account works. Add a new account. Remove
an account. Open the developer tools and check that all the tabs still
work.
Lots of grepping through the code base.
Reviewers: halla, mark, juan, khamidou
Reviewed By: juan, khamidou
Differential Revision: https://phab.nylas.com/D3769
Summary:
This will solve T7579 when saving messages to the sent folder. I
attempted to clean up the references code but decided it was better left for a
new diff, so added a bunch of TODO's in this diff
Test Plan: manual
Reviewers: halla, spang, evan
Reviewed By: spang, evan
Differential Revision: https://phab.nylas.com/D3766
Summary:
Unfortunately, many IMAP hosts outside the major ones do not have
certificates issued by a certificate authority, and it is very confusing
to folks to have their account auth not work. This patch relaxes our
certificate requirements for IMAP hosts outside the major providers.
It's cool that node 6 has secure TLS settings by default!
Fixes: T7673
Test Plan: manual
Reviewers: mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3771
Summary:
We've been syncing drafts messages but not the drafts flag in K2, making
them appear in Edgehill as regular old messages.
This commit makes K2 sync the drafts flag, and also correctly label
folders called "Drafts" with the 'drafts' role.
Because 2-way syncing of drafts is very complex and error-prone since
you need to add new drafts and delete the old ones on every update, and
we reaally don't want to do things like create multiplying draft copies
or accidentally lose a draft someone started composing elsewhere, we
simply exclude messages marked as drafts from being serialized to
Edgehill through the delta stream for now. This removes the confusing
behaviour and also sets a better stage for completing drafts sync later.
Eventually we will also want to add functionality to allow users to
select their drafts folder, but for now this code does the right thing
in many more cases.
While investigating this behaviour, I also discovered a bug we've never
seen before where Gmail isn't applying the \Draft flag to draft
messages, no matter which folder we fetch them from. :-/ This is very
unfortunate and there's no way for us to work around it other than to
fetch messages in the Drafts folder and manually apply the flag locally,
since "drafts" is not a label in Gmail, only another IMAP folder. Brandon
Long from the Gmail team says that this is because they've had
problems with clients which sync drafts, so the Gmail web client and
mobile apps do not set the \Draft flag on drafts. (I don't get how this
solves their problem, but okay.) Let's solve the issue on Gmail if it
comes up by user demand—should be relatively straightforward to
implement, but it adds sync work & complexity.
Fixes T7593
Test Plan: manual
Reviewers: halla, juan
Reviewed By: juan
Maniphest Tasks: T7593
Differential Revision: https://phab.nylas.com/D3749
Summary:
We don't want to run message processing full tilt when a user isn't plugged in.
This diff adds some detection logic that causes message processing to be
throttled/unthrottled when a user unplugs/plugs in their computer.
Test Plan: Run locally unplugged and plugged in, verify that CPU use goes up/down
Reviewers: evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3759
Summary:
See title
Depends on D3744
Test Plan: tested locally
Reviewers: spang, evan, juan
Reviewed By: spang, evan, juan
Differential Revision: https://phab.nylas.com/D3745
Summary:
I thought it was gonna be OK that we kept all HTML parts in a multipart/alternative
MIME structure because the world is a sane place and nobody would ever put more
than one HTML part in a multipart/alternative structure.
I was wrong.
We have found extraterrestrial life^W^WI mean emails which contain duplicate,
exactly the same MIME parts within a multipart/alternative MIME structure: two
text/plain parts and two text/html parts. This is likely due to a broken MIME
implementation, or perhaps a bug in someone's email script. So, we should
only keep one text/html MIME part if there are multiple.
Test Plan:
manual for now---added this to my mail parsing regression test list
for implementation once we unify the DBs and have a roughly stable code
structure
Reviewers: halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3750
Summary:
Rather than having a strict model where we don't decode the message if we
don't specifically recognize the CTE, treat any CTEs we don't recognize as
having no encoding. There are several CTE strings that could mean this (e.g.
7bit, 7BITS, 8-bit, binary, NONE, utf8), and we don't want to check for them
all. Additionally, if there is a CTE we don't support, the user will likely
see rendering issues and contact support. This will allow us to obtain more
concrete information about these messages.
Test Plan: manual
Reviewers: spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3748
Summary:
Given that we were marking the account as errored if we've encountered
enough RetryableErrors, we would show the red box to the user when in
fact the problem was the user was offline, causing confusion
If the user is offline, we will constantly get RetryableErrors in the
sync loop, and we can't mark the account as errored in that case.
Test Plan: manual
Reviewers: evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3752
Summary:
This commit ensures that we handle transient errors correctly when refreshing
tokens
Test Plan: manual
Reviewers: khamidou, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3740
Summary:
In the sync worker:
- Move the backoff logic inside `scheduleNextSync`, where all logic to schedule the next sync loop now lives
- If we've retried a RetryableError a bunch of times, show the error to the user, otherwise the user might think the app is not working for no reason
- Clean up logging
In the message processor:
- Report message processing errors to sentry!
Sync Process Manager:
- Listen to new `Actions.debugSync` to show the Activity Window and open dev tools
Test Plan: manual
Reviewers: khamidou, evan
Reviewed By: khamidou, evan
Differential Revision: https://phab.nylas.com/D3736
Summary:
This commit makes it so our syncback tasks send as few imap commands as possible by passing a set of UIDs whenever possible. Previously, we would send 1 command per message, with a single UID, which was very wasteful given that we can pass a set of UIDs. This is especially helpful for operating on threads with a large number of messages.
Syncback actions will now group all messages in a thread by the folder they belong to, and issue a single operation on the folder box. When removing all labels from a thread (setting labels to []), we need to issue a command of the form `box.delLabels(uids, labels)`, so we also group messages by set their set of labels to issue as few commands as possible.
This commit only batches imap commands, but we can still batch syncback actions themselves, which can be implemented in a separate patch.
Test Plan: manual
Reviewers: evan, mark, spang
Reviewed By: spang
Subscribers: halla, mg
Differential Revision: https://phab.nylas.com/D3719
Summary: Double-firing protection since the DatabaseStore can now fire this
Test Plan: manual
Reviewers: juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3730
Summary:
We are getting to many imap timeout connection errors because the
authTimeout was just 5 secs
Test Plan: manual
Reviewers: khamidou, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3733
Summary:
Specifically, these imap connections have been known to hang when you close your laptop or go offline/online, even though we are passing a `socketTimeout` to node-imap. When they hang, everything freezes because the promise waiting for the result never resolves.
`_createConnectionPromise` wraps the operations with a timeout we implemented ourselves, and correctly rejects on timeout.
This commit wraps other imap operations that were missing. (I notices because I encountered the hanging on one of these operations)
Test Plan: manual
Reviewers: evan, mark, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3720
Summary:
We weren't, which meant that us sending with multi-send or generic IMAP
broke threading. :(
Test Plan: manual
Reviewers: juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D3718
Summary:
Before this commit, if folder sync was complete, and the account didn't support CONDSTORE (e.g. Office365, Yahoo), we would only check for attribute updates every 10 minutes.
This commit makes it so we always check for attribute updates if the server doesn't support CONDSTORE
So for example, when marking a thread as read, we would perform the optimistic update in N1, queue the syncback task which would succeed, but the thread in k2s db would never get updated and become stale, with an unreadCount > 0. If we emitted a delta for that thread during the window of time where we ignored attribute updates, it would be set as unread again in N1, even though all of its messages were read.
This still doesn't guarantee that it wont happen (we could still get a delta for the thread before we actually fetch the attribute updates from IMAP), but before this commit it was sure to happen. This should be properly fixed with the sync scheduler refactor
Test Plan: manual
Reviewers: evan, mark, spang
Reviewed By: mark, spang
Differential Revision: https://phab.nylas.com/D3714
Summary:
Previously, we were not pripritizing archive sync when getting folders to sync, causing it to be synced almost last. I believe this was causing the issues regarding archived items coming back, because we would optimistically archive in N1, but the changes wouldn't be reflected in K2's database until we synced the archive, causing the data to become out of sync. If for whatever reason we got a delta for any of those messages before the archive was synced, they would pop back in the inbox because in k2, they were still in the inbox. This was exacerbated by the fact that all syncback tasks would interrupt the loop, so we would reach the archive until very late, making this scenario way more likely.
This still wont guarantee that it wont happen, because we dont do /any/ optimistic updates in K2, so we could still get deltas before we actually sync the folder, but makes the scenario way less likely. This should be properly fixed with the sync scheduler refactor
Test Plan: manual
Reviewers: spang, evan, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D3716
Summary:
All we do is use the SEARCH X-GM-RAW IMAP extension to find the UIDs
to prioritize at the beginning of initial sync, and download these UIDs
until there are none left. Then we continue downloading All Mail as
usual.
Because of the way we batch via ranges, the most expedient way to
implement this means that all prioritized emails will end up being
downloaded twice (the second time we'll detect that the message exists
and do nothing).
This seems like a worthwhile tradeoff for quick appearance of the
messages in a user's inbox.
Test Plan: manual
Reviewers: evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D3706
Summary: Was missing from in envelope when sending custom message
Test Plan: manual, tested sending from Gmail, Office365, Yahoo, all work
Reviewers: evan, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D3700
Summary:
Don't show the attachment icon on threads that only have inline
images. We do this by assuming that inline images have a contentID,
and regular attachments do not. Also updates the way we send
attachments in order to adhere to this standard.
Test Plan: tested manually
Reviewers: spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3696
Summary:
When syncing folders, we check if the folder needs syncing by checking if it has any new messages via the STATUS command (STATUS returns uidnext, highestmodseq among others, and is cheaper than SELECT)
However, we can't issue a STATUS on a box that is already selected. Previously, if the box was already selected, we would just return it, but this was incorrect because we wouldn't get the latest box values (e.g. uidnext), causing us to think that there were no updates available, and skip syncing folders that actually needed to be synced.
Now, if the box is already selected when getting the status, we have to re select it to refresh the latest values
Test Plan: manual
Reviewers: evan, khamidou, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3697
Summary:
This commit also lowers the batch size of messages to fetch on folder sync down to 30. This is in order to prevent sync from getting stuck if we queue too many syncback tasks-- given that we only update the range of fetched uids after we've actually fetched and processed messages, if the batch size is too big and we interrupt too often, we might end up never advancing the range and re fetching the same messages over and over.
This also makes the sync loop run faster through all folders in general.
Depends on D3689 to make sure that the batch size actually reflects a message count, i.e. to ensure that we are making /visible/ progress.
Test Plan: manual
Reviewers: spang, khamidou, evan
Reviewed By: evan
Maniphest Tasks: T7477
Differential Revision: https://phab.nylas.com/D3692
Summary:
Consolidating provider checks to use the same source of truth.
Fixes send issues with some provider types.
Test Plan: tested locally
Reviewers: tomasz
Reviewed By: tomasz
Differential Revision: https://phab.nylas.com/D3694
Summary:
Because we optimistically fetch UIDs by expanding a range without looking
at the actual UIDs in the inbox and the actual space of UIDs with messages
attached may be sparse due to message moves, we need to track how many
messages we actually download during a range expansion and continue
expanding the range if we haven't downloaded enough messages.
If we reach a large gap where we download no messages at all during a batch, we
pause and check the actual UID list for the folder for the next UID to
download, as otherwise we may spin indefinitely fetching UIDs that don't exist.
(Example: my "Deleted Items" folder had about 300k worth of empty UIDs between
a very small UID and a very large UID. With the new system, this registers as a
completed sync within a single iteration as soon as sync hits the gap.)
Test Plan: manual
Reviewers: juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D3689
Summary:
This patch changes the sync worker to back off exponentially when there is an issue syncing an account. This has two goals:
- first, it's a bit dangerous to retry immediately. We don't want hundreds of thousands of machines trying to refresh tokens unsuccessfully because our service is struggling.
- second, it's nicer on the CPU to wait a bit between retries.
Currently, we sleep for at most 2 minutes, with some random jitter added.
Test Plan: Tested manually, stared at the code a long time.
Reviewers: evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D3684
Summary:
Various errors are thrown when the sync worker tries accessing
a database that we've already deleted, so make sure the sync
worker has been stopped before we remove the database. This diff
involves modifying `Interruptible` so that `interrupt()` returns
a promise that resolves once the interrupt has been completed.
Addresses T7472
Test Plan: manual
Reviewers: evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D3679
Summary: Treat any that aren't gmail or office365 as standard imap
Test Plan: manual
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3686
Summary:
On MG's machine this function is EXTREMELY non performant and causes
things like archive to lock up when the console is running here for some
reason. Not entirely sure exactly what's causing it, but there were some
simple DB cleanups that will make it faster for large queries.
There's likely other things involved since the sequelize DB being locked
up shouldn't affect the peformLocal of the edgehill db for things like
archive. Still looking into that
Test Plan: manual
Reviewers: juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3683
Summary:
Before trying to sync a folder, check if we actually need to do so. This will prevent us from doing unnecessary work that slows down the sync loop (like performing SELECT commands)
We will perform a folder sync if any of the following are true
- The folder hasn't been completely synced
- There are new messages (using imap STATUS command)
- There are attribute changes indicated via highestmodseq (using imap STATUS command)
- If server doesn't support highestmodseq, it has passed enough time since we last ran an attribute scan on the folder.
Addresses T7513
Test Plan: manual
Reviewers: evan, halla, spang
Reviewed By: halla, spang
Differential Revision: https://phab.nylas.com/D3675
Summary:
Currently, our mail sync strategy of expanding UID ranges from UIDNEXT
backwards until a UID of 1 implicitly assumes that every UID corresponds to an
actual message. This assumption is incorrect, and results in several
significant bugs regarding sync status.
This patch fixes issue 1:
Since UIDs are persistent and, so long as the UIDVALIDITY is valid, ascend
monotonically upward, every time you move a message to a new folder you "lose"
UIDs lower down in the range. In my work Inbox, where I get a lot of mail,
archive all the time, and generally have only a small number of threads in the
mailbox, the smallest UID is over 100k. This means that, after all my inbox
messages are synced, the sync loop will continue attempting to download
nonexistent old messages in this mailbox for hundreds of sync iterations, and
will not mark the mailbox as fully synced until fetchmin reaches 1, regardless
of the fact that there are no actually messages being pulled down.
This patch needs a small associated patch to N1 to update how sync status is
calculated (coming soon).
The next patch in this series will deal with gaps in the UIDspace that slow
down syncing of a folder.
Test Plan: manual
Reviewers: halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3677
Summary:
We want to do this in order to prevent send tasks from blocking the sync loop given that they can take a very long time to run. This is especially true when sending emails with large attachments to multiple recipients.
There is no real way to make sending in these cases faster, but we can prevent it from blocking the sync loop at least, especially because sending is mostly I/O bound.
This is a bit messy actually, but should be fixed when we properly implement a sync scheduler
Also added a limit to the total size of attachments you can upload to try to prevent weird EPIPE errors when sending.
See: D3670.
Also moved and renamed stuff a little
Test Plan: manual
Reviewers: halla, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3669
Summary: Allows us to reset accounts in local-sync too
Test Plan: manual
Reviewers: mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3672
Summary:
I happened to be testing between Jan 2017 and Dec 2016, so I
missed this logic flaw. Boo.
Test Plan: tested locally
Reviewers: evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3667
Summary: We did this for gmail, but not for other providers.
Test Plan: tested locally
Reviewers: juan, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3665
Summary: While working on separating send out of the sync loop, I realized sync tasks could use some cleanup to be more consistent with how we implemented syncback tasks. I reorganized and renamed things a little bit. This will also help us move in the direction of the scheduler implementation under which everything is a task.
Test Plan: manual
Reviewers: evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3660
Summary:
Only updated within month precision. We can use this to show how
far back a folder has been synced.
Test Plan: tested locally
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3662
