Summary:
This commit makes it so `resetEmailCache` works as expected, i.e. it
removes all databases, without forcing the user to re sign-in to their
accounts or NylasID
Previously, this method removed the database without removing the
accounts, left users in an un-authed state that was hard to recover
from. This was fixed in D4212 which makes sure that when we get a new
identity, sync and deltas are restarted
However, resetEmailCache would still force you to log in to yoru NylasID
because it was deleted from the database. However, if we reuse the
command `application:relaunch-to-initial-windows` instead of manually
deleting the database, we can relaunc the app while preserving the users
NylasID session, so they don't have to sign back in manually.
Test Plan: manual
Reviewers: evan, spang, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4215
Summary:
This prevents the app from entering a restart loop when there's no
identity.
Specifically, when a user logs out of their identity, or when she resets the
email cache, or any other scenario that leaves the app without an
identity but with accounts added, the sync loop (and deltas) will start
without an Identity.
This will cause NylasAPIRequest to throw an error that
forces the user to close the app. When the app restarts, sync will start
again without an identity, and the user will be forced to close the app
again, and so on and so forth for the rest of eternity
Relevant error:
https://github.com/nylas/nylas-mail-all/blob/master/packages/client-app/src/flux/nylas-api-request.es6#L165-L174
Additionaly, this makes sure that after resetting the email cache, the sync
process starts when the identity becomes available
This solves T7989
Test Plan: manual
Reviewers: evan, spang, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4212
Summary:
Previously, while resetting the email cache, we would try to stop sync
and just wait for an arbitraty amount of time for it to stop and the
proceed to blow away the database.
This commit makes it so we correctly wait for sync to stop, and then we
blow away the db. It adds a timeout anyway in case sync is stuck and we
can't stop it
Depends on D4207
Test Plan: manual
Reviewers: evan, spang, halla
Reviewed By: spang, halla
Differential Revision: https://phab.nylas.com/D4209
Summary:
We no longer keep `this._accounts` state, which was being accessed
inside `_resetEmailCache`
Test Plan: manual
Reviewers: evan, spang, halla
Reviewed By: spang, halla
Differential Revision: https://phab.nylas.com/D4207
Summary:
build-client, aka packaging, aka bundling is now separate from uploading.
This is both to compartmentalize our tasks a bit more and so we can add a
non-grunt windows task inbetween packaging and uploading.
No more heavily-overloaded PUBLISH_BUILD flag.
Added SIGN_BUILD flag instead.
No more TRAVIS and TRAVIS_PULL_REQUEST flag.
Test Plan: Manual
Reviewers: halla, juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D4208
Summary:
This removes specs from the build production version.
One of the major issues with the windows build was that one of the package
specs included a .node compiled file that the codesigner attempte to sign,
but couldn't find for some reason.
In testing, removing the specs from the build prod version fixed this.
While we could just remove the one offending test, I think we should just
remove the whole suite from the build version. That'd save in the number
of files we have to ship, save download time, save build time, and have
less chance for some symlinked issue failling a build in the future.
Test Plan: Manual
Reviewers: khamidou, halla, juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D4206
Summary:
It always drives me crazy when folks use "setup" (no space) as a verb,
because the verb version has a space in it.
Also remove the word "Setup" from the displayName for custom IMAP accounts
because it doesn't make sense for how we use this field, e.g.
a8f36b88d9/packages/client-app/internal_packages/onboarding/lib/page-account-onboarding-success.jsx (L27)
will display "Sucessfully connected to IMAP / SMTP Setup!" with the old
wording when you successfully auth a custom IMAP account.
Test Plan: manual
Reviewers: evan, juan, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4199
Summary:
Dunno if I'm the only one having this problem, but I spent at least an
hour over the last two days wondering why hapi just sits there and hangs
in the Joi validation step when I copy-paste calls to /auth from the
developer bar to my terminal. It turns out that if you fail to send the
correct Content-Type when sending a JSON payload, Joi just hangs
forever. (WTF!)
Test Plan: manual
Reviewers: halla, evan, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4204
Summary:
We were always incrementing the queue length and resolving the promise
in the constructor :-/
Test Plan: Run locally, make sure queue doesn't start rejecting messages
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4198
Summary:
Previously, n1 cloud auth errors were stuffed into an account's
syncState. The problem with that is that the sync loop manages that
state, and as long as the sync loop is running it will set that state to
running. However, it might be the case that the sync loop is running but
we can't connect to n1Cloud, so even though we would set the `syncState`
to `n1_cloud_auth_failed`, the sync loop would just set it back to
'running', and the user wouldn't see the error notification indicating
that it can't connect to n1Cloud
This commit makes it so we keep tracj of the auth failure state for
n1Cloud in a separate field, and makes sure that the error notification
component shows that error.
Test Plan: manual
Reviewers: mark, spang, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D4172
Summary:
On rare occasions, the api error would not have a body, and we would try
to access the message prop on the body, which would in turn throw an
error.
This commit prevents this error, and adds better messaging to the sentry
report
Test Plan: manual
Reviewers: mark, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4194
Summary:
This commit adds a new perf metric for inline composer times.
Additionally, it consolidates the timer logic for all other types of draft
creation (mailto links, dropping a file in the app dock icon, etc).
Test Plan: manual
Reviewers: halla, evan, mark
Reviewed By: evan, mark
Differential Revision: https://phab.nylas.com/D4186
Summary:
Previously we limited it to 3. Gmail supports up to 15, many IMAP
servers support 10 by default. So let's bump it for those folks.
Test Plan: Run locally, verify things still work.
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4175
Summary:
Errors in the MessageProcessor were causing sync to get stuck
occasionally. This diff refactors queueMessageForProcessing and friends
so that they're more robust to errors during promise construction.
Test Plan: Run locally
Reviewers: juan, spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4190
Summary:
Sequelize can sometimes return promises that will never resolve or
reject. We can wrap the promises we get back from sequelize in a
bluebird promise which gives us the ability to timeout these abandoned
promises. This way, we can track down issues in sequelize as well as
unblocking stuck sync loops.
Test Plan: Run locally, verify that timeouts occur
Reviewers: juan, evan, spang
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4192
Summary:
This commit adds error handling to the sync-loop's `onSyncError` and
`scheduleNextSync`.
These functions generally don't fail, as they are in the `catch` and
`finally` blocks respectively of the sync loop. But as we've seen in
D4152, the datbase can sometime error if it's in a bad state. If it
errors inside these functions, we will never schedule the next run of
the sync loop.
Depends on D4152
Test Plan: manual
Reviewers: evan, spang, mark, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4153
Summary:
Before this commit, we would call .close() onError and when the
connection closed. We also closed the connection onError, even though
NylasLongConnection had also closed it, so we ended up calling close a
bunch of times. This would cause us to set a bunch of timeouts to retry
unecessarilly.
This commit makes it so we ensure there's only one retry timeout and
consolidates the logic that calls .close() so we don't call it so many
times unnecessarily
Test Plan: manual
Reviewers: spang, mark, evan
Reviewed By: mark, evan
Differential Revision: https://phab.nylas.com/D4166
Summary:
This commit consolidates the `DeltaConnectionStatusStore` and the
`DeltaConnectionStore` which kept track of very similar state and made
sense to be the same store (as per feedback in D4118#77647)
Given that this state needs to be available app-wide for plugins to
query the status of delta connections, `internal_packages/deltas` was
removed (given that it only activated that store), in favor of having the
unified store inside `src/flux/stores` and available via `nylas-exports`
The `deltas` package also contained some contacts-ranking code, which is
no longer in use until we restore that fetaure, so I created a
`internal_packages/contact-rankings` which contains this unused code for
now.
Test Plan:
manually open, close, end delta connections, verify that I'm getting
correct results. unit tests to come
Reviewers: halla, spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4145
Summary:
This DRY's up code that use these status codes
Depends on D4130
Test Plan: manual
Reviewers: spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4140
Summary:
When reporting error inside `IdentityStore._fetchIdentity`, we would
pass null as the `extra` parameter for `reportError`. This would cause
us to try to assign something to null and throw an error while reporting
the error.
(Even though `extra` has a default value of `{}` (see
declaration of `NylasEnv.reportError`), passing null will be interpreted
as a valid arg and it wont get assigned the default value)
Thankfully, the second error /is/ reported to sentry: https://sentry.io/nylas/nylas-mail/issues/230589717/
Test Plan: manual
Reviewers: mark, halla, spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4150
Summary:
This error is can flood Sentry pretty badly: https://sentry.io/nylas/nylas-mail/issues/230629801/
My initial thought was to rate limit it, but rate limiting wouldn't
do any good because when we get that error we destroy the databases and
restart the app, and we will loose the in-memory rate limiting data.
The real fix, and reason why Sentry is being flooded with this error by
a single user, is that once you encounter this error the app will enter
a restart loop that constantly throws this error. The reason being that
we weren't properly awaiting for the K2 account databases to be dropped
before closing the app, so on restart, the database would still be
malformed.
The fix is to properly `await` for the database drops
Test Plan: manual
Reviewers: spang, mark, halla, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4127
Summary:
This is in preparation for removing the ability of the IMAP pool to
automatically handle timeout errors.
Test Plan: Run locally, verify file download still works
Reviewers: juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4167
Summary: In preparation for removing timeout handling from IMAPConnectionPool.
Test Plan: Run locally
Reviewers: juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4168
Summary:
We had this weird nesting where we would keep the IMAPConnection open
beyond the scope of the connection pool and return the created
Observable. This diff inverts that relationship and flattens out some of
the indentation, making the code easier to read. This is in preparation
for refactoring the IMAPConnectionPool to no longer automatically handle
timeouts.
Test Plan: Run locally, make sure search still works
Reviewers: juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4165
Summary:
This is not a bug. Maaaybe we want to report this to MixPanel for individual
user debugging (how do we do that?), but it doesn't make sense to report this
error to Sentry.
Example:
https://sentry.io/nylas/nylas-mail/issues/230530818/
Test Plan: yolo
Reviewers: mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4130
Summary: Misspelled INPROGRESS-NOTRETRYABLE :-(
Test Plan: Run locally, make sure things get cancelled
Reviewers: juan
Differential Revision: https://phab.nylas.com/D4163
Summary:
If you have the console open and are looking at the Source panel or
something, this will constantly bring you back to the Console panel. Won't
do that anymore with this chec
Test Plan: manual
Reviewers: spang, mark, halla, juan
Reviewed By: mark, halla, juan
Differential Revision: https://phab.nylas.com/D4162
Summary:
Errors weren't getting reported because of a circular reference causing
JSON.stringify to fail.
We can't add the whole API `response` object onto the error object,
because sometimes (like when the delta connection errors with a 401), the
`response` object is circular. We only use it to the extract the status
code anyway and it's a huge object. This diff excludes them from the
APIError objects.
We also have code to recover just in case JSON.stringify errors for some
other weird reason. It'll still attempt to report the error prefixed with
`Recovered Error`
Finally, we used to spit only the `error.stack` to the console. This meant
what we saw in the console didn't properly include the error messages
being incredibly not useful.
It's better to console.error the whole error object since that'll more
nicely display on the inspector console
Test Plan: manual
Reviewers: juan, spang, mark, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4154
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:
This commit bumps the version of sequelize to point to the latest
version in our fork. This version bump also includes latest updates from
upstream sequelize since the last time we bumped the version.
Notably, it includes a critical fix that will prevent the sync
loop from getting stuck on rare occasions.
Specifically, sequelize's `Query.prototype.run` could in some cases return a
Promise that would /never/ resolve or reject, effectively halting the
execution of any code that was waiting for that promise. This was due to
a lack of error handling inside a the query's `afterExecute` function; if an
error was thrown there, the enclosing Promise would never reject, and
the error would just remain uncaught. An example of such an error that
could cause this scenario is: T7742 - https://sentry.io/nylas/nylas-mail/issues/230016155/
Sync getting halted in this way can produce a variety of user facing
bugs like not being able to send or it taking an absurd amount of time
(hours), tasks never finishing or taking an absurd amount of time to
complete, new mail not arriving, among others
The commit that fixes this is: 0deeda9e1a
The full set of changes introduced by this version bump are: https://github.com/nylas/sequelize/compare/nylas-3.30.0...nylas-3.40.0
**Note:**
It is important to note that sequelize might still halt the sync loop if
there are other places in the code that can return Promises that never
resolve or reject under certain circumstances. I can consistently reproduce this
scenario when an error is thrown inside `afterExecute`, and I've seen it
happen in the wild, but I've also ran into this type of sync loop halting (db
promises never resolve) without any sequelize errors being thrown, which
suggests that there are other places in the sequelize code that might end
up returning a Promise that will halt sync.
Unfortunately, we don't have a good way to detect and report when this
happens yet, but we are adding one in upcoming diffs in order have data
on how many people are running into this, and/or if this patch completely
fixes the issue. Otherwise, we'd need to audit sequelize's code.
Should resolve T7837 and T7767
Test Plan: manual
Reviewers: spang, mark, halla, khamidou, evan
Reviewed By: evan
Subscribers: mg, tomasz
Maniphest Tasks: T7767, T7837
Differential Revision: https://phab.nylas.com/D4152
