fix: catch lock timeout errors instead of leaking to app#117
Conversation
After laptop sleep, AbortSignal.timeout on navigator.locks.request fires before the browser event loop resumes, causing a raw LockError to escape to the application. This converts the error to a typed RefreshTimeoutError (extends RefreshError) and applies an expiry-aware fallback: return the existing token if still valid, throw RefreshTimeoutError if expired or forceRefresh requested. - Add RefreshTimeoutError class extending RefreshError - Convert LockError to RefreshTimeoutError in #doRefresh - Expiry-aware fallback in getAccessToken (return unexpired token) - Handle RefreshTimeoutError in switchToOrganization (warn, don't redirect) - Catch TimeoutError DOMException alongside AbortError in withNativeLock - Export RefreshTimeoutError for consumers who need to catch it
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| : undefined; | ||
| if (token) return token; | ||
| throw err; | ||
| } else if (err instanceof RefreshError) { |
There was a problem hiding this comment.
seems like throwing a RefreshError is still going to result in the user being erroneously logged out? (i don't remember this codebase well maybe i'm wrong) The token in the example cited here is definitely expired (laptop has been suspended). What do you think about retrying N time(s) instead?
There was a problem hiding this comment.
Yeah I thought about just silently catching. Retrying a few times and then throwing this more specific error might be best. Will update!
- Remove unused LockError and MockWebLocks imports from tests - Use native Error cause via super() instead of manual assignment - Replace @ts-ignore with cast for navigator.locks cleanup - Hoist installLockTimeout helper to shared scope, remove duplicate
When lock acquisition times out and the token is expired (or forceRefresh requested), retry the refresh once before giving up. This handles the common post-sleep case where the first attempt times out but the lock clears on retry.
|
Addressed @cmatheson's feedback — added a bounded retry (1 attempt) in 803683c. When the lock times out and the token is expired, we now retry the refresh once before throwing New test covers the retry-success path (first attempt times out, retry succeeds and returns a fresh token). |
2 participants
Summary
AbortSignal.timeout(10s)onnavigator.locks.requestfires before the browser event loop resumes. The resultingLockErrorwas escaping raw to the application becausegetAccessToken()only caughtRefreshError.RefreshTimeoutError extends RefreshError— flows through existing catch blocks, distinguishable viainstanceoffor apps that need it.forceRefresh: true, throwsRefreshTimeoutError(notLoginRequiredError, since the session is valid).switchToOrganization()— catches the timeout and warns instead of crashing.AbortErrorandTimeoutErrorDOMException names inwithNativeLock.Fixes the issue reported by VectorShift (same family as the Coast investigation from Oct 2024).
Test plan
getAccessToken()returns existing token when lock times out and token is unexpiredgetAccessToken()throwsRefreshTimeoutErrorwhen lock times out and token is expiredgetAccessToken({ forceRefresh: true })throwsRefreshTimeoutErroreven with unexpired tokenswitchToOrganization()does not throw rawLockError— warns and resolvestsc --noEmit)tsup)