Improve handling of resumeSession failures
#2010
Conversation
estrattonbailey
force-pushed
the
eric/resume-session-handling
branch
3 times, most recently
from
50f56fb to
37bb367
Compare
estrattonbailey
changed the title
AtpSessionEvent for unknown exceptions
estrattonbailey
force-pushed
the
eric/resume-session-handling
branch
from
bfd9b1b to
1d03455
Compare
estrattonbailey
changed the title
AtpSessionEvent for unknown exceptionsresumeSession failures
packages/api/src/agent.ts
Outdated
| * | ||
| * Everything else is unexpected. | ||
| */ | ||
| if (['InvalidDID'].includes(e.error)) { |
There was a problem hiding this comment.
I'm a bit confused what circumstance this happens in?
There was a problem hiding this comment.
This is just handling the logic that was already here, where a did doesn't match the requested session did. I honestly don't know why this would happen.
There was a problem hiding this comment.
and i guess if that happens, then somethings gone wonky & the appropriate action would be to just log the user out - hence the expired
|
|
||
| if (e instanceof XRPCError && e.error) { | ||
| /* | ||
| * `ExpiredToken` and `InvalidToken` are handled in |
There was a problem hiding this comment.
is this._refreshSession implicitly called by the retry logic from calling getSession?
There was a problem hiding this comment.
It's a little confusing, but we override the static AtpAgent.fetch here so that if we get an ExpiredToken we can try to refresh the session. So calling getSession actually calls this._fetch here, which calls this._refreshSession.
But yeah, basically expired will be emit from that handling + afaict we don't get a ExpiredToken error from getSession.
packages/api/src/agent.ts
Outdated
| } else if (['InternalServerError'].includes(e.error)) { | ||
| this._persistSession?.('network-error', undefined) | ||
| } |
There was a problem hiding this comment.
I was under the impression that network errors (e.g. user does not have connectivity to the service) would have status ResponseType.Unknown, and may not have an error property.
There was a problem hiding this comment.
Alright, I think I see! These 500s are treated akin to a network error. That makes sense since it's another kind of temporary unavailability, and we don't necessarily want to log the user out. Other status codes for temporary unavailability such as 502 and 503 might be handled similarly.
Tiny P.S.: I think that checking the status property of an XRPCError for this kind of thing might be more reliable.
There was a problem hiding this comment.
network-error is just a name Paul suggested for this. This particular branch is tested in agent.test.ts, where we do get an XRPCError called InternalServerError.
Basically we're trying to handle any not-already-handled knowns with expired, and leave unknowns (like a network timeout that doesn't even hit our server) as network-error.
There was a problem hiding this comment.
thx for talking us through this - makes sense to me 👍
I agree with Devin that status code checks are probably more reliable than error message checks. But it seems like any unknown error gets turned into network-error regardless, so it probably works either way
…andling * origin/main: Revert "lexicons: more string limits (#1994)"
Motivation: flaky networks currently result in the app nuking JWTs, effectively logging the user out. We want to log out the user if the session is invalid, but for other unknowns, we can keep it around on-device until we get a legit response from our server that says otherwise.
This PR adds an additional
AtpSessionEventevent,network-error, and emits that event fromresumeSessionif we catch an unexpected exception.