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:
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:
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:
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:
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
