Skip to content

fix: Preserve session on transient refresh failures #79

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
m0tzy merged 2 commits into from
Jul 24, 2026
Merged

fix: Preserve session on transient refresh failures #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c3dcec
fix: Preserve session on transient refresh failures
m0tzy Jul 23, 2026
316bb92
Treat known non-retryable status as terminal in refresh classifier
m0tzy Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,13 @@ export interface RefreshErrorOptions {
error: unknown;
request: Request;
sessionData: SessionData;
/**
* Whether the refresh failed for a transient reason (network error, timeout,
* 429, or 5xx) rather than a terminal one (the refresh token is dead). When
* `true`, the sealed session is preserved (not destroyed) so a later request
* can refresh successfully once the condition clears.
*/
isTransient: boolean;
}

export interface RefreshSuccessOptions {
Expand Down
101 changes: 101 additions & 0 deletions src/session.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,6 +835,83 @@ describe('session', () => {
}
});

it.each([
['a rate limit (429)', Object.assign(new Error('Too many requests'), { status: 429 })],
['a server error (503)', Object.assign(new Error('Service unavailable'), { status: 503 })],
['a request timeout (408)', Object.assign(new Error('Request timeout'), { status: 408 })],
['a network error', new TypeError('fetch failed')],
// The SDK re-wraps a raw network TypeError in a plain Error with the
// TypeError as its cause; the classifier must follow the cause chain.
[
'an SDK-wrapped network error',
new Error('Unexpected error: TypeError: fetch failed', { cause: new TypeError('fetch failed') }),
],
])('should preserve the session cookie when refresh fails transiently: %s', async (_label, transientError) => {
authenticateWithRefreshToken.mockRejectedValue(transientError);
getAuthorizationUrlMock.mockResolvedValue({
url: 'https://auth.workos.com/oauth/authorize?state=abc123',
headers: { 'Set-Cookie': 'wos-auth-verifier-abc=sealed; Path=/; HttpOnly; SameSite=Lax; Max-Age=600' },
});

try {
await authkitLoader(createLoaderArgs(createMockRequest()));
fail('Expected redirect response to be thrown');
} catch (response: unknown) {
assertIsResponse(response);
expect(response.status).toBe(302);
// The sealed session must not be destroyed on a transient failure.
expect(destroySession).not.toHaveBeenCalled();
const setCookies = response.headers.getSetCookie();
expect(setCookies).not.toContain('destroyed-session-cookie');
expect(setCookies).toContain('wos-auth-verifier-abc=sealed; Path=/; HttpOnly; SameSite=Lax; Max-Age=600');
}
});

it('should destroy the session for a terminal status even if its cause chain looks network-like', async () => {
// A terminal HTTP status must win over the network-cause fallback: a
// 400 that happens to wrap a "fetch failed" TypeError is still terminal.
authenticateWithRefreshToken.mockRejectedValue(
Object.assign(new Error('invalid_grant', { cause: new TypeError('fetch failed') }), {
status: 400,
error: 'invalid_grant',
}),
);
getAuthorizationUrlMock.mockResolvedValue({
url: 'https://auth.workos.com/oauth/authorize?state=abc123',
headers: { 'Set-Cookie': 'wos-auth-verifier-abc=sealed; Path=/; HttpOnly; SameSite=Lax; Max-Age=600' },
});

try {
await authkitLoader(createLoaderArgs(createMockRequest()));
fail('Expected redirect response to be thrown');
} catch (response: unknown) {
assertIsResponse(response);
expect(response.status).toBe(302);
expect(destroySession).toHaveBeenCalled();
expect(response.headers.getSetCookie()).toContain('destroyed-session-cookie');
}
});

it('should destroy the session for a terminal refresh failure (invalid_grant)', async () => {
authenticateWithRefreshToken.mockRejectedValue(
Object.assign(new Error('invalid_grant'), { status: 400, error: 'invalid_grant' }),
);
getAuthorizationUrlMock.mockResolvedValue({
url: 'https://auth.workos.com/oauth/authorize?state=abc123',
headers: { 'Set-Cookie': 'wos-auth-verifier-abc=sealed; Path=/; HttpOnly; SameSite=Lax; Max-Age=600' },
});

try {
await authkitLoader(createLoaderArgs(createMockRequest()));
fail('Expected redirect response to be thrown');
} catch (response: unknown) {
assertIsResponse(response);
expect(response.status).toBe(302);
expect(destroySession).toHaveBeenCalled();
expect(response.headers.getSetCookie()).toContain('destroyed-session-cookie');
}
});

it('calls onSessionRefreshSuccess when provided', async () => {
const onSessionRefreshSuccess = jest.fn();
await authkitLoader(createLoaderArgs(createMockRequest()), {
Expand All @@ -855,6 +932,30 @@ describe('session', () => {
expect(onSessionRefreshError).toHaveBeenCalled();
});

it('passes isTransient: true to onSessionRefreshError for a transient failure', async () => {
authenticateWithRefreshToken.mockRejectedValue(
Object.assign(new Error('Service unavailable'), { status: 503 }),
);
const onSessionRefreshError = jest.fn().mockReturnValue(redirect('/error'));

await authkitLoader(createLoaderArgs(createMockRequest()), {
onSessionRefreshError,
});

expect(onSessionRefreshError).toHaveBeenCalledWith(expect.objectContaining({ isTransient: true }));
});

it('passes isTransient: false to onSessionRefreshError for a terminal failure', async () => {
authenticateWithRefreshToken.mockRejectedValue(Object.assign(new Error('invalid_grant'), { status: 400 }));
const onSessionRefreshError = jest.fn().mockReturnValue(redirect('/error'));

await authkitLoader(createLoaderArgs(createMockRequest()), {
onSessionRefreshError,
});

expect(onSessionRefreshError).toHaveBeenCalledWith(expect.objectContaining({ isTransient: false }));
});

it('allows redirect from onSessionRefreshError callback', async () => {
authenticateWithRefreshToken.mockRejectedValue(new Error('Refresh token invalid'));

Expand Down
69 changes: 69 additions & 0 deletions src/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,70 @@ export type TypedResponse<T> = Response & {
};

export class SessionRefreshError extends Error {
/**
* Whether the refresh failed for a transient reason (network error, timeout,
* 429, or 5xx) rather than a terminal one (the refresh token is dead). When
* `true`, the existing session is still valid and should be preserved and
* retried rather than destroyed.
*/
readonly isTransient: boolean;

constructor(cause: unknown) {
super('Session refresh error', { cause });
this.name = 'SessionRefreshError';
this.isTransient = isTransientRefreshError(cause);
}
}

// The WorkOS SDK's HTTP client already retries these with backoff + jitter
// internally. If one of these still surfaces, the failure is transient rather
// than a dead refresh token: request timeouts (normalized to 408), rate limits
// (429), and 5xx.
const RETRYABLE_REFRESH_STATUS_CODES = new Set([408, 429, 500, 502, 503, 504]);

// A network-level fetch failure surfaces as a TypeError ("fetch failed" /
// "Failed to fetch"). Match its message so an unrelated programming TypeError
// (e.g. from a helper after a successful exchange) is not misclassified.
const NETWORK_ERROR_MESSAGE = /fetch failed|failed to fetch|network|load failed|terminated/i;

// A raw network TypeError is not an HttpClientError, so the WorkOS SDK re-wraps
// it in a plain Error whose `cause` is the original TypeError. Follow the cause
// chain to recognize it.
function isNetworkError(error: unknown): boolean {
if (error instanceof TypeError) {
return NETWORK_ERROR_MESSAGE.test(error.message);
}

if (error instanceof Error && error.cause != null && error.cause !== error) {
return isNetworkError(error.cause);
}

return false;
}

/**
* Classifies a refresh failure as transient (retryable) rather than terminal.
* Transient failures carry a retryable numeric `status` (408/429/5xx, mirroring
* the SDK's own retry set) or are network failures (a "fetch failed" `TypeError`,
* possibly wrapped by the SDK with the original `TypeError` as its `cause`).
* Anything else (a terminal `invalid_grant` at 400, a 401, or an unrecognized
* error) is treated as terminal.
*/
export function isTransientRefreshError(error: unknown): boolean {
// A known HTTP status is authoritative: a retryable code is transient, and
// any other status (e.g. a terminal 400 `invalid_grant`) is terminal. Return
// eagerly so a terminal response is never reclassified as transient by the
// network-cause fallback below.
if (typeof error === 'object' && error !== null && 'status' in error) {
const { status } = error;
if (typeof status === 'number') {
return RETRYABLE_REFRESH_STATUS_CODES.has(status);
}
}

return isNetworkError(error);
}
Comment thread
greptile-apps[bot] marked this conversation as resolved.

/**
* This function is used to refresh the session by using the refresh token.
* It will authenticate the user with the refresh token and return a new session object.
Expand Down Expand Up @@ -448,6 +506,7 @@ export async function authkitLoader<Data = unknown>(
error: error.cause,
request,
sessionData: cookieSession,
isTransient: error.isTransient,
});

if (result instanceof Response) {
Expand All @@ -463,6 +522,16 @@ export async function authkitLoader<Data = unknown>(

const returnPathname = getReturnPathname(request.url);
const { url, headers: authHeaders } = await getAuthorizationUrl({ returnPathname, request });

// Only destroy the session for a terminal failure. A transient failure
// (network error, timeout, 429, or 5xx that survived the SDK's internal
// retries) leaves the refresh token valid, so keep the sealed cookie and
// let a later request refresh successfully rather than forcing the user
// to re-authenticate.
if (error.isTransient) {
throw redirect(url, { headers: [['Set-Cookie', authHeaders['Set-Cookie']]] });
}
Comment on lines 523 to +533

@devin-ai-integration devin-ai-integration Bot Jul 23, 2026

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.

🔍 Transient failures still redirect to the sign-in URL

On a transient refresh failure the loader still throws a redirect to the WorkOS authorization URL (src/session.ts:527-528); it only avoids destroying the sealed cookie. Since the access token is expired (that is why refresh was attempted), the current request cannot be served with valid auth, so the user is still bounced to re-authenticate. The stated benefit ('a later request refreshes successfully once the condition clears') therefore only applies when the redirect is not followed (e.g. background/data requests) — for a normal document navigation the user will complete the WorkOS flow and get a fresh session anyway. This matches the PR description's code snippet and the new tests assert a 302 with the cookie preserved, so it appears intentional, but reviewers should confirm this is the desired UX parity with the authkit-remix/authkit-nextjs ports.

Open in Devin Review

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

@devin-ai-integration devin-ai-integration Bot Jul 23, 2026

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.

Intentional, and it matches the authkit-remix/authkit-nextjs ports. You're right that for a normal document navigation the user still completes the WorkOS redirect (the current request's access token is expired, so it can't be served regardless). The value of preserving the cookie is two-fold: (1) concurrent/background data requests (loaders, fetchers, revalidations) that fire during the same transient blip keep a valid sealed session and can succeed instead of all tearing it down, and (2) we never throw away a still-valid refresh token on a transient 5xx/429/network error — which is the BaseTen outage-lockout scenario where destroying the session while login is also degraded locks users out. Terminal invalid_grant still destroys as before.

@m0tzy m0tzy Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Don't users with an active cookie get bounced off the sign in URL?

@devin-ai-integration devin-ai-integration Bot Jul 23, 2026

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 — yes, and it exposes an inconsistency that predates this PR. This catch block redirects to the sign-in URL unconditionally, but the no-session branch above (if (!session), line 422) only redirects when ensureSignedIn: true and otherwise returns UnauthorizedData so the route just renders logged-out.

So today, on an ensureSignedIn: false (optional-auth) route:

  • a user with no session → renders logged-out, no bounce;
  • a user with a valid, recoverable session that hits a transient refresh blip → gets bounced to WorkOS.

That's backwards, and it's exactly your point. (The access token is expired by the time we refresh — updateSession only refreshes a token that already failed verifyAccessToken — so we genuinely can't serve authenticated content for this request; but for a non-ensureSignedIn loader the right answer is to render logged-out for this one request, not to redirect.)

Proposed fix — mirror the no-session semantics in the catch:

if (error instanceof SessionRefreshError) {
  const cookieSession = await getSession(request.headers.get('Cookie'));
  // ...onSessionRefreshError callback unchanged...

  // Terminal failure destroys the cookie; transient preserves it.
  const setCookie = error.isTransient
    ? undefined
    : await destroySession(cookieSession);

  if (ensureSignedIn) {
    const { url, headers: authHeaders } = await getAuthorizationUrl({ returnPathname, request });
    const headers: [string, string][] = [];
    if (setCookie) headers.push(['Set-Cookie', setCookie]);
    headers.push(['Set-Cookie', authHeaders['Set-Cookie']]);
    throw redirect(url, { headers });
  }

  // Not ensureSignedIn: render logged-out instead of bouncing. On a transient
  // failure the sealed cookie is preserved so a later request refreshes.
  const auth: UnauthorizedData = { user: null, impersonator: null, /* ...nulls... */ };
  return await handleAuthLoader(loader, loaderArgs, auth); // + Set-Cookie header when terminal
}

Net effect: ensureSignedIn: true still redirects (that's the contract), but optional-auth routes stop bouncing users with a recoverable session on a transient blip — the sealed cookie is kept and the next navigation/data request refreshes. This also brings react-router closer to the authkit-nextjs port, which returns { user: null } + authorizationUrl rather than force-redirecting.

Want me to push this? It's a small change plus tests (transient + ensureSignedIn: false → no redirect, cookie preserved; terminal + ensureSignedIn: false → no redirect, cookie destroyed). I held off since it's a UX call on your repo.


throw redirect(url, {
headers: [
['Set-Cookie', await destroySession(cookieSession)],
Expand Down