Skip to content

feat: Distinguish transient from terminal failures in CookieSession.refresh()#1663

Merged
m0tzy merged 5 commits into
mainfrom
devin/1784819169-sdk-retryable-refresh
Jul 23, 2026
Merged

feat: Distinguish transient from terminal failures in CookieSession.refresh()#1663
m0tzy merged 5 commits into
mainfrom
devin/1784819169-sdk-retryable-refresh

Conversation

@m0tzy

@m0tzy m0tzy commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

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 retryable discriminator to the failed refresh result so callers can keep the existing session and retry later instead of logging users out:

const r = await session.refresh();

if (r.authenticated) {
  // set new cookie
} else if (r.retryable) {
  // keep the existing cookie, serve the request, retry later
} else {
  // terminal — redirect to sign in
}

What changed

  • RefreshSessionFailedResponse gains:
    • retryable: booleanfalse for terminal failures, true for transient ones.
    • retryAfter?: number — seconds parsed from the Retry-After header (e.g. on a 429).
    • error?: unknown — the underlying error, exposed for logging on retryable failures.
  • New RefreshSessionFailureReason values 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 an OauthException with status 408:
    • 429rate_limit_exceeded (+ retryAfter when present)
    • 408timeout
    • >= 500server_error
    • network TypeError (raw, or wrapped as a generic Error with a TypeError cause) → network_error
    • terminal OauthException (invalid_grant / mfa_enrollment / sso_required) → retryable: false
    • anything else is still re-thrown (unchanged behavior)

Existing 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 to false).

Depends on

This assumes server PR https://github.com/workos/workos/pull/66577 has landed, which returns 429 + Retry-After for a transient refresh-token lock timeout (previously 400 invalid_grant). Do not merge until that PR lands — otherwise the transient lock-timeout case still arrives as a terminal invalid_grant.

Test plan

npx jest src/user-management/session.spec.ts — added cases under refresh > when the refresh fails covering terminal invalid_grant, 429 (with retryAfter), 408 timeout, 5xx, and network TypeError, plus the updated pre-flight invalid-cookie result. Full suite (npx jest), npm run lint, npm run typecheck, and npm run build all pass.

Documentation

Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.

[ ] Yes

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

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@m0tzy
m0tzy requested review from a team as code owners July 23, 2026 16:49
@m0tzy
m0tzy requested a review from jonatascastro12 July 23, 2026 16:49
@m0tzy m0tzy self-assigned this Jul 23, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor
Original prompt from madison.packer

SYSTEM:
=== BEGIN THREAD HISTORY (in #team-authkit) ===
<most_recent_message>
Madison Packer (U0ACAL99VSL): @Devin can you investigate the refresh token feedback here and plan for how we might improve the DX <https://work-os.slack.com/archives/C0APCBRV47Q/p1784811878194419|https://work-os.slack.com/archives/C0APCBRV47Q/p1784811878194419>
</most_recent_message>
=== END THREAD HISTORY ===

Thread URL: https://work-os.slack.com/archives/C0173N0DDSQ/p1784817744204199?thread_ts=1784817744.204199&amp;cid=C0173N0DDSQ

The latest message is the one right above that tagged you. The <most_recent_message> is the message that you should use to guide your goals + task for this session, and you should use the rest of the slack thread as context.

@devin-ai-integration

Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration devin-ai-integration Bot changed the title Distinguish transient from terminal failures in CookieSession.refresh() feat: Distinguish transient from terminal failures in CookieSession.refresh() Jul 23, 2026
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Open in Devin Review

Comment on lines 196 to 201
return {
authenticated: false,
reason: error.error,
retryable: false,
};
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 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)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-apps

greptile-apps Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR distinguishes transient from terminal failures in CookieSession.refresh(), replacing raw re-throws for operational errors (429, 408, 5xx, network failures) with a typed retryable: true response, so callers can keep the existing session and retry rather than signing users out during outages.

  • RefreshSessionFailedResponse is refactored into a discriminated union (RetryableRefreshSessionFailureReason / TerminalRefreshSessionFailureReason) so TypeScript narrows retryAfter and error only in the retryable branch.
  • Two classifier helpers — classifyTerminalRefreshError and classifyRetryableRefreshError — cleanly separate error routing; classification precedence correctly prioritises terminal OAuth failures before transient status-code checks, and RateLimitExceededException is matched first among retryable cases to preserve the Retry-After value.
  • Tests cover all new failure paths: invalid_grant (terminal), sso_required, mfa_enrollment, 429 with retryAfter, 408 timeout, 5xx, and network TypeError.

Confidence Score: 5/5

Safe 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

Filename Overview
src/user-management/interfaces/refresh-and-seal-session-data.interface.ts Adds discriminated union types for terminal vs. retryable refresh failures, with well-structured type narrowing via retryable: false / retryable: true literals
src/user-management/session.ts Adds two classifier helpers (terminal and retryable) that cleanly separate error routing logic from refresh flow; classification ordering and instanceof checks are correct
src/user-management/session.spec.ts Good test coverage for all new failure paths: invalid_grant (terminal), sso_required, mfa_enrollment, 429 with retryAfter, 408 timeout, 5xx server error, and network TypeError

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[refresh called] --> B{session.refreshToken\nor session.user missing?}
    B -- yes --> C[Return INVALID_SESSION_COOKIE\nretryable: false]
    B -- no --> D[authenticateWithRefreshToken]
    D -- success --> E[Return authenticated: true]
    D -- throws --> F{classifyTerminalRefreshError}
    F -- OauthException\nerror === invalid_grant --> G[Return INVALID_GRANT\nretryable: false]
    F -- AuthenticationException\ncode === mfa_enrollment --> H[Return MFA_ENROLLMENT\nretryable: false]
    F -- AuthenticationException\ncode === sso_required --> I[Return SSO_REQUIRED\nretryable: false]
    F -- null --> J{classifyRetryableRefreshError}
    J -- RateLimitExceededException --> K[Return RATE_LIMIT_EXCEEDED\nretryable: true\nretryAfter from header]
    J -- status 429 --> L[Return RATE_LIMIT_EXCEEDED\nretryable: true\nno retryAfter]
    J -- status 408 --> M[Return TIMEOUT\nretryable: true]
    J -- status >= 500 --> N[Return SERVER_ERROR\nretryable: true]
    J -- TypeError or\nError with TypeError cause --> O[Return NETWORK_ERROR\nretryable: true]
    J -- null --> P[re-throw error]
Loading

Reviews (3): Last reviewed commit: "Narrow failure reason by retryable discr..." | Re-trigger Greptile

Comment thread src/user-management/session.ts
Comment thread src/user-management/interfaces/refresh-and-seal-session-data.interface.ts Outdated
m0tzy and others added 2 commits July 23, 2026 17:00
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>
@m0tzy
m0tzy merged commit 2c570a5 into main Jul 23, 2026
7 checks passed
@m0tzy
m0tzy deleted the devin/1784819169-sdk-retryable-refresh branch July 23, 2026 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants