fix(auth): prevent session deletion on proactive refresh failure when token still valid by 7ttp · Pull Request #2146 · supabase/supabase-js

7ttp · 2026-03-01T08:48:48Z

What this PR introduces

Removes the _removeSession() call from _callRefreshToken. This prevents sessions from being deleted from storage on proactive refresh failure when the access token is still valid (e.g., within the 90s EXPIRY_MARGIN_MS window).

No additional logic, just a fix to ensure the session remains recoverable.

Why is this change needed

Previously, if a proactive token refresh failed with a non-retryable error (such as invalid_grant), the session was permanently deleted from storage even though the access token was still technically valid. This caused users to be silently logged out until they re-authenticated, and made recovery impossible until a full app reload.

_callRefreshToken is a low-level helper — it shouldn't own session lifecycle decisions. Callers that need to clean up already do so independently (_recoverAndRefresh has its own _removeSession() call for non-retryable errors). Removing the call here lets __loadSession (the getSession() path) degrade gracefully — returning the error without nuking a still-valid session.

Changes

Removed the _removeSession() call from _callRefreshToken.
Added a test verifying sessions are not deleted from storage on non-retryable refresh errors.
Existing cleanup in other callers (_recoverAndRefresh) remains unchanged.

Summary by CodeRabbit

Bug Fixes
- Enhanced authentication error handling during token refresh. Sessions now persist when encountering non-retryable errors instead of being automatically invalidated, preventing unexpected logouts and improving user experience.

Walkthrough

The PR removes the code path that unconditionally deleted the stored session when a non-retryable error occurred during _callRefreshToken. On non-retryable refresh failures the client now surfaces the error but does not call this._removeSession(), leaving session data in storage intact. Tests were added to verify that a non-retryable AuthError from _refreshAccessToken does not remove the stored session.

Sequence Diagram(s)

(omitted)

Assessment against linked issues

Objective	Addressed	Explanation
Prevent permanent deletion of session on non-retryable refresh failures when access token is still valid [`#2145`]	✅
Only remove session if expires_at*1000 < Date.now(); preserve non-expired sessions [`#2145`]	❌	No new expiry check was added; the removal call was removed entirely rather than conditionally deleting based on expires_at.
Ensure getSession()/__loadSession() returns existing valid session instead of null [`#2145`]	❌	Tests show stored session remains but getSession response may still surface null session with an error; change does not demonstrate getSession returning the preserved session in all error paths.
Preserve retry semantics for retryable errors [`#2145`]	✅

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

pkg-pr-new · 2026-03-01T08:50:11Z

Open in StackBlitz

@supabase/auth-js

npm i https://pkg.pr.new/@supabase/auth-js@2146

@supabase/functions-js

npm i https://pkg.pr.new/@supabase/functions-js@2146

@supabase/postgrest-js

npm i https://pkg.pr.new/@supabase/postgrest-js@2146

@supabase/realtime-js

npm i https://pkg.pr.new/@supabase/realtime-js@2146

@supabase/storage-js

npm i https://pkg.pr.new/@supabase/storage-js@2146

@supabase/supabase-js

npm i https://pkg.pr.new/@supabase/supabase-js@2146

commit: 65e9fb9

…shToken

mandarini · 2026-03-02T17:32:23Z

Thank you @7ttp ! I removed the additional logic and added a test!!

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/core/auth-js/test/GoTrueClient.test.ts`:
- Around line 494-499: The test currently asserts that client.getSession()
returns data.session === null on proactive-refresh failure; update the
assertions to verify that the existing seeded session is returned instead and
that storage remains intact. Specifically, in the GoTrueClient.test where
getSession() is called, replace the expect(data.session).toBeNull() with an
assertion that data.session matches the seeded session object (or at least has
the expected access_token/uid/expires_at values) and add an assertion that the
stored refresh token (or session data in whatever storage mock you used) is
still present, ensuring client.getSession() returns the valid session even when
proactive refresh errors.

ℹ️ Review info

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Cache: Disabled due to Reviews > Disable Cache setting

Disabled knowledge base sources:

Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between f3e0ff3 and 51b4fb1.

📒 Files selected for processing (2)

packages/core/auth-js/src/GoTrueClient.ts
packages/core/auth-js/test/GoTrueClient.test.ts

💤 Files with no reviewable changes (1)

packages/core/auth-js/src/GoTrueClient.ts

mandarini · 2026-03-04T14:34:36Z

@7ttp we're waiting for @hf 's input here. I think we are not going to approve.

mandarini · 2026-03-10T15:46:08Z

Still waiting. Please give us some time :(

haveaguess · 2026-04-17T12:42:46Z

Hi, it's great to see a workaround has been developed for my issue, what's the status?

mandarini · 2026-05-18T12:33:00Z

Hi @7ttp! After more discussion internally and looking at how this interacts with the other refresh-related reports (#2126, #2344, and the recent analysis on #2145 itself), we have decided not to merge this PR. The short reasons:

Removing the _removeSession() call unconditionally leaves the session in storage even when the refresh token is genuinely revoked. Callers still see { session: null, error }, so the user is effectively logged out but the dead session sticks around in storage to be retried again.
This change makes the proactive-refresh storm pattern worse rather than better. With the cleanup removed, every caller hitting the 90s expiry margin keeps re-firing _callRefreshToken against the same broken token until the actual expiry passes or rate limiting kicks in. There is a good writeup of this on Bug: _callRefreshToken permanently deletes session on non-retryable refresh failure, even when access token is still valid #2145 from @thomaslarsson.
We are about to ship a lockless redesign of the auth client (targeting next month). That work changes how refresh deduplication and session lifecycle are coordinated, and it is the right place to fix this class of bug end to end.

Closing this PR for now so it does not sit in limbo, but please do not read that as the underlying issue being dismissed. #2145 stays open, and the redesign is explicitly scoped to address it.

Genuinely grateful for the time you spent on this and for the thorough writeup. I will tag you when the redesign lands so you can sanity check whether your scenario is covered.

fix(auth): prevent session deletion on proactive refresh failure when…

f3e0ff3

… token still valid

7ttp requested review from a team as code owners March 1, 2026 08:48

fix(auth): simplify fix to only remove _removeSession from _callRefre…

51b4fb1

…shToken

mandarini force-pushed the fix/session-del branch from b0edb29 to 51b4fb1 Compare March 2, 2026 17:28

mandarini self-assigned this Mar 2, 2026

coderabbitai Bot reviewed Mar 2, 2026

View reviewed changes

Comment thread packages/core/auth-js/test/GoTrueClient.test.ts

thomaslarsson mentioned this pull request May 16, 2026

Bug: _callRefreshToken permanently deletes session on non-retryable refresh failure, even when access token is still valid #2145

Closed

2 tasks

mandarini closed this May 18, 2026

mandarini mentioned this pull request Jun 10, 2026

fix(auth): preserve valid session on refresh failure and cooldown repeat failures #2436

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(auth): prevent session deletion on proactive refresh failure when token still valid#2146

fix(auth): prevent session deletion on proactive refresh failure when token still valid#2146
7ttp wants to merge 2 commits into
supabase:masterfrom
7ttp:fix/session-del

7ttp commented Mar 1, 2026 •

edited by mandarini

Loading

Uh oh!

coderabbitai Bot commented Mar 1, 2026 •

edited

Loading

Summary by CodeRabbit

Walkthrough

Sequence Diagram(s)

Assessment against linked issues

Uh oh!

pkg-pr-new Bot commented Mar 1, 2026 •

edited

Loading

Uh oh!

mandarini commented Mar 2, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

mandarini commented Mar 4, 2026 •

edited

Loading

Uh oh!

mandarini commented Mar 10, 2026

Uh oh!

haveaguess commented Apr 17, 2026

Uh oh!

mandarini commented May 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

7ttp commented Mar 1, 2026 • edited by mandarini Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR introduces

Why is this change needed

Changes

Related

Uh oh!

coderabbitai Bot commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Sequence Diagram(s)

Assessment against linked issues

Uh oh!

pkg-pr-new Bot commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mandarini commented Mar 2, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mandarini commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mandarini commented Mar 10, 2026

Uh oh!

haveaguess commented Apr 17, 2026

Uh oh!

mandarini commented May 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

7ttp commented Mar 1, 2026 •

edited by mandarini

Loading

coderabbitai Bot commented Mar 1, 2026 •

edited

Loading

pkg-pr-new Bot commented Mar 1, 2026 •

edited

Loading

mandarini commented Mar 4, 2026 •

edited

Loading