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:
When loading accounts, if we detect any removed accounts, we should get
rid of the tokens for that account as well.
Test Plan: manually removed accounts, everything seems a-ok
Reviewers: mark, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3907
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:
The current error condition check is to make sure that accounts are exclusively
removed via `Actions.removeAccount`. However, when removing accounts,
the in-memory `AccoutStore`s in each window are kept in sync via listening
to the config, as opposed to dispatching Actions globally between windows
(Actions.removeAccount is scoped to a single window, see actions.es6).
So, when we remove an account in one window, config.json will change, and the
other windows will always encounter `removedIds.length > 0` to be true
when their `_loadAccounts` runs. This will only generate unecessary noise in Sentry.
Test Plan: manual
Reviewers: mark, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3902
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:
- Create ghost models if we receive metadata for an object we
haven't synced yet
- Fix logic for finding equivalent threads
- Send `messageIds` when syncing thread metadata
See D3876 for unit tests
K2 pairing: D3880
Test Plan: unit tests, local testing soon
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3875
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 believe this might fix T7559, or at least provide a better error message.
Depends on D3895
Test Plan: manual
Reviewers: spang, mark, halla, evan
Reviewed By: mark, halla, evan
Maniphest Tasks: T7559
Differential Revision: https://phab.nylas.com/D3897
Summary:
If the worker window were to restart or the app were to close in the
middle of a send task, sending to a single recipient, we would not
properly await the task and always mark as success.
This diff, along with (D3897) should fix T7559.
Test Plan: manual
Reviewers: spang, mark, evan, halla
Reviewed By: mark, evan, halla
Maniphest Tasks: T7559
Differential Revision: https://phab.nylas.com/D3895
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:
A prerequesite to integrating with `arc unit` or CI for each patchset is
being able to generate JUnit XML output for spec runs. This commit adds
this feature using the JUnitXMLReporter from jasmine-reporters. Invoke it like
this:
npm run test-junit
(We output to the terminal as well when this is run, so in the case that
you're doing `arc diff` you have some idea of what is going on.)
Test Plan: run it
Reviewers: halla, juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D3891
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
Before this commit, running the running the tests would clear all of
your config inside `.nylas-dev` and thus your accounts, forcing you to re-add
them everytime after you ran the tests
This was happening because we weren't correctly setting the
`configDirPath` to `.nylas-spec` when running with the --test flag. When
we run with --test, both options, `specMode` and `devMode` are true, so
the logic to set the path would fall into both conditions and ultimately
set the path to `.nylas-dev`. Now it's fixed!
Summary:
The global `before/afterEach` functions were made async, but the
`masterBefore/AfterEach` functions were applied to the references
in `jasmineExports` instead of the global references. Fix that,
and await for `destroyTestDatabase()`. Also fix a random test
failure about not receiving any json.
This diff is necessary for D3878 to work properly.
Test Plan: ran the tests
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3890
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, verify that we backoff on failure
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3887
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:
Fixes T7832
The _savePromise mechanism of the IdentityStore was really cumbersome and
was throwing errors on the console for people. In hindsight, this is an
unnecessary complication to a very sensitive system and a source of very
hairy async bugs.
Test Plan:
I also updated the specs to ensure that when you call saveIdentity the
promise resolves after the local cache has been updated
Reviewers: spang, halla, juan
Reviewed By: juan
Maniphest Tasks: T7832
Differential Revision: https://phab.nylas.com/D3882
Summary:
The app regressed to launching a blank white page for a while before
showing the UI. This was because I forgot to await for the now async
package loading
Test Plan: Manually boot app with compile cache disabled
Reviewers: mark, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3883
Summary:
Previously, these tests were mostly testing the library itself, instead
of our code. The library performed expensive operations and caused the
test to time out more often than not
This commit makes it so we test our code, mock out any calls to side
effects, and removes a line that was overriding our jasmine timeout
Test Plan: unit
Reviewers: spang, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D3885
Summary:
This notification was randomly appearing and not going away on its own.
This was due to some weird logic in the react component. This diff
refactors things to make them a little more consistent.
Test Plan:
Run locally, disconnecting and reconnecting to make sure we properly
show and hide the notification.
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3881
