feat: Distinguish transient from terminal failures in CookieSession.refresh()#1663
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Original prompt from madison.packer
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| return { | ||
| authenticated: false, | ||
| reason: error.error, | ||
| retryable: false, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔍 Terminal mfa_enrollment/sso_required may arrive as AuthenticationException, not OauthException
The terminal-failure branch only matches error instanceof OauthException for invalid_grant/mfa_enrollment/sso_required. However, handleHttpError (src/workos.ts:447-448) routes any error whose code or error field is in AUTHENTICATION_ERROR_CODES (which includes mfa_enrollment and sso_required, see src/common/exceptions/authentication.exception.ts:34-43) to AuthenticationException, which extends GenericServerException and is NOT an OauthException. So if the refresh endpoint returns { error: 'sso_required', ... } or { code: 'mfa_enrollment', ... }, the terminal branch here would not match. It would then fall to classifyRetryableRefreshError, where getErrorStatus returns the exception's status (via the GenericServerException branch). For a typical 4xx status these fall through to a rethrow (matching pre-existing behavior), but if such a terminal error ever carried a >= 500 status it would be misclassified as server_error/retryable: true, keeping a dead session alive. Only invalid_grant (which becomes an OauthException) is covered by tests. This terminal condition is pre-existing and unchanged by the PR, but it interacts with the new classification logic and only invalid_grant is verified — worth confirming the actual wire format the refresh endpoint uses for these terminal cases.
(Refers to lines 190-201)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — you're right. handleHttpError checks isAuthenticationErrorData before the OAuth branch, so mfa_enrollment/sso_required become AuthenticationException (a GenericServerException subclass), not OauthException. The old error instanceof OauthException terminal check only ever matched invalid_grant, and the two auth-code cases would have fallen through to the retryable classifier — harmless at 4xx, but misclassified as server_error/retryable if one ever carried a 5xx status.
Fixed in 2a1fe6b: terminal detection now lives in classifyTerminalRefreshError, which matches invalid_grant on OauthException and mfa_enrollment/sso_required on AuthenticationException (via error.code). Added terminal tests for both sso_required (403 error) and mfa_enrollment (403 code).
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Greptile SummaryThis PR distinguishes transient from terminal failures in
Confidence Score: 5/5Safe to merge once the server-side PR (workos/workos#66577) that returns 429 for transient lock timeouts has landed. The error classification logic is correct: terminal checks run before retryable checks, the RateLimitExceededException instanceof guard fires before the generic status-429 fallback (preserving retryAfter), and the AuthenticationException code lookup correctly handles both code and error fields for sso_required. All new failure paths are tested. The only finding is a type-precision nit on the classifier helper's return type. No files require special attention beyond ensuring the dependent server PR has merged before this one. Important Files Changed
|
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Description
CookieSession.refresh()today only returns a typed unauthenticated result for the terminal OAuth errors (invalid_grant,mfa_enrollment,sso_required) and re-throws everything else raw — timeouts,5xx,429, and network failures. Callers can't tell "the session is dead, sign the user out" from "the refresh token is still valid, this was a transient blip" without catching and inspecting raw exceptions (which is exactly why some customers reported bypassing the SDK to get typed errors). During an outage this leads to users being signed out even though their session was fine.This adds a
retryablediscriminator to the failed refresh result so callers can keep the existing session and retry later instead of logging users out:What changed
RefreshSessionFailedResponsegains:retryable: boolean—falsefor terminal failures,truefor transient ones.retryAfter?: number— seconds parsed from theRetry-Afterheader (e.g. on a429).error?: unknown— the underlying error, exposed for logging on retryable failures.RefreshSessionFailureReasonvalues for transient failures:rate_limit_exceeded,timeout,server_error,network_error.refresh()now classifies thrown errors after the HTTP client has exhausted its own retries. Classification is by HTTP status, not exception subclass, since e.g. a request timeout surfaces as anOauthExceptionwith status408:429→rate_limit_exceeded(+retryAfterwhen present)408→timeout>= 500→server_errorTypeError(raw, or wrapped as a genericErrorwith aTypeErrorcause) →network_errorOauthException(invalid_grant/mfa_enrollment/sso_required) →retryable: falseExisting authenticated refresh, sealed-session update, and token rotation behavior are unchanged. The only compatibility change is that unauthenticated results now carry
retryable(and terminal results set it tofalse).Depends on
This assumes server PR https://github.com/workos/workos/pull/66577 has landed, which returns
429 + Retry-Afterfor a transient refresh-token lock timeout (previously400 invalid_grant). Do not merge until that PR lands — otherwise the transient lock-timeout case still arrives as a terminalinvalid_grant.Test plan
npx jest src/user-management/session.spec.ts— added cases underrefresh > when the refresh failscovering terminalinvalid_grant,429(withretryAfter),408timeout,5xx, and networkTypeError, plus the updated pre-flight invalid-cookie result. Full suite (npx jest),npm run lint,npm run typecheck, andnpm run buildall pass.Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
The AuthKit session resilience guide (workos/workos#66596) documents this typed retryable behavior.
Link to Devin session: https://app.devin.ai/sessions/fc39103abf694f90b1c92dd714a81461
Requested by: @m0tzy