feat(auth): add maxAutoRefreshFailures option - #2573
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
Sequence Diagram(s)sequenceDiagram
participant AutoRefreshTicker
participant GoTrueClient
participant RefreshToken
participant AuthStateListener
AutoRefreshTicker->>GoTrueClient: Run auto-refresh tick
GoTrueClient->>RefreshToken: Attempt token refresh
RefreshToken-->>GoTrueClient: Return success or retryable failure
GoTrueClient->>GoTrueClient: Update consecutive failure count
GoTrueClient->>AuthStateListener: Emit TOKEN_REFRESH_FAILED at threshold
GoTrueClient->>AutoRefreshTicker: Stop auto-refresh ticker
Assessment against linked issues
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/auth-js/src/GoTrueClient.ts`:
- Around line 5582-5592: Count refresh errors returned by _useSession() in both
auto-refresh tick paths: update GoTrueClient.ts lines 5582-5592 and 5510-5520 to
apply the existing auto-refresh failure handling, including incrementing
autoRefreshFailureCount and invoking _handleAutoRefreshFailure(), when the
session result contains an error. Extend
GoTrueClient.autoRefreshFailures.test.ts lines 54-58 to exercise the real
_useSession()/__loadSession() refresh path and verify these failures are
counted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c64895b-6ddf-49c0-ab95-e3a1520eb507
📒 Files selected for processing (3)
packages/core/auth-js/src/GoTrueClient.tspackages/core/auth-js/src/lib/types.tspackages/core/auth-js/test/GoTrueClient.autoRefreshFailures.test.ts
b1eeaae to
f64ad7d
Compare
|
@mandarini pls review |
mandarini
left a comment
There was a problem hiding this comment.
Hi @7vignesh, thank you so much for contributing to Supabase! This is a well-shaped implementation of exactly the opt-in behavior we discussed in #1680! There are a few things that need to change before we can merge it.
-
The option never reaches auth-js when set through
createClient.SupabaseClientOptions['auth']is an explicit field list (packages/core/supabase-js/src/lib/types.ts) and_initSupabaseAuthClientdestructures an allow-list (packages/core/supabase-js/src/SupabaseClient.ts), so the usage example in the PR description is a type error for TypeScript users and a silent no-op for JavaScript users. Please addmaxAutoRefreshFailuresto both, plus a small supabase-js test that reads the option off the client to verify flow-through (we do the same forlockAcquireTimeout). -
The counter goes stale because it only resets on a successful refresh. If a user hits the limit and later signs back in, the count is still at max, so one transient blip will stop auto-refresh and emit
TOKEN_REFRESH_FAILEDafter a single failure instead of N. Related:_onVisibilityChangedrestarts the ticker on every tab focus, so during an outage the event re-fires on each refocus. Resetting the counter in_startAutoRefresh()fixes both. It would also be good to reflect the "resumes on tab focus" reality in the JSDoc, since the ticker stop is not permanent in browsers. -
The error path can re-enter itself.
_notifyAllSubscribersre-throws subscriber errors, so if an app callback throws while handlingTOKEN_REFRESH_FAILED, the error lands in the tick's catch block, which increments the counter and calls_handleAutoRefreshFailureagain: the event fires twice, and a second subscriber throw becomes an unhandled rejection out of thesetIntervaltick. Please make sure the failure handler cannot run twice for the same tick. -
Every error currently counts toward the limit, including non-retryable 4xx responses (revoked token, reuse detection, rate limits) that already have their own handling in
_callRefreshToken(sign-out or session preserve). Since the feature targets an unreachable server, please gate the counting onisAuthRetryableFetchError(error)so terminal auth states and rate limits do not shut down auto-refresh. Also worth deciding: the event fires with anullsession even when a still-valid session was preserved, and apps commonly treat a null session as signed out, so consider passing the current session or calling this out in the JSDoc.
On tests: the suite mocks both _useSession and _callRefreshToken, so it mostly exercises the plumbing between mocks. Could you add at least one test that goes through the real session load path (a fetch-level mock is fine)? None of the tests pass a custom lock either, so the legacy path is not actually covered yet despite the PR notes.
One thing you do not need to solve: adding TOKEN_REFRESH_FAILED to AuthChangeEvent has cross-SDK naming implications that we will coordinate on our side.
Really appreciate the care that went into this, the write-up alone made it a pleasure to review. Thank you for helping make Supabase better, contributions like this are what keep these libraries moving.
@supabase/auth-js
@supabase/functions-js
@supabase/postgrest-js
@supabase/realtime-js
@supabase/storage-js
@supabase/supabase-js
commit: |
f64ad7d to
420df5f
Compare
|
Hi @mandarini, thank you for the thorough review! All 4 points addressed in the latest push: 1. Option flow-through via createClient 2. Counter goes stale 3. Re-entry guard 4. Only count retryable errors Tests: 11 tests now covering default behavior, max limit, counter reset, non-retryable errors, thrown errors, retryable session-load errors, no-session without error, startAutoRefresh reset, legacy lock path, and subscriber re-entry. Plus 2 supabase-js flow-through tests. All pass. |
mandarini
left a comment
There was a problem hiding this comment.
Hi @7vignesh! Sorry for the back and forth: my point 2 last round asked you to reset the counter in _startAutoRefresh(), and that was wrong advice. _onVisibilityChanged calls it on every tab refocus, so the budget refills constantly and the event would almost never fire in a browser.
Changes needed:
-
Move the reset out of
_startAutoRefresh()into_saveSession()and_removeSession()(next to the existingthis.lastRefreshFailure = null). The reset in_callRefreshTokenthen becomes redundant. -
Make it recoverable off-browser. In React Native and Node nothing restarts the ticker after the limit is hit, not even signing back in. In
_saveSession, after the reset:if (this.autoRefreshToken && this.autoRefreshTicker === null && !isBrowser()) { void this._startAutoRefresh() }
-
Pass
falseas the third arg to_notifyAllSubscribers. It currently broadcasts, so other tabs emitTOKEN_REFRESH_FAILEDwhile their own ticker is still running. -
Pass the session instead of
null._callRefreshTokenpreserves sessions whose access token is still valid, and(event, session) => setUser(session?.user ?? null)would log those users out. -
Move the caveats onto the public JSDoc for
maxAutoRefreshFailuresin bothauth-js/src/lib/types.tsandsupabase-js/src/lib/types.ts: counts ticks not network attempts (N x 30s), retryable errors only, and the tab refocus / off-browser behavior. -
Add one test that goes through the real path (seed storage, make the injected
fetchreject) instead of mocking_useSession, and makenonRetryableError()a real error:new AuthApiError('Token revoked', 401, 'refresh_token_not_found'). A plain object has no__isAuthErrorbrand, so that test currently passes for the wrong reason. -
pnpm nx format, line 41 of the new test file is over our 100 char width.
Your call on one thing: the two catch (e) blocks count without the retryable gate, so a storage adapter throw counts too. Gate them or document the counter as "failed ticks".
Thank you again for sticking with this, and sorry for the wrong turn on the reset location.
Add an opt-in maxAutoRefreshFailures option that stops the auto-refresh ticker after N consecutive failed ticks and emits a TOKEN_REFRESH_FAILED event via onAuthStateChange. This lets applications detect when the auth server is permanently unreachable and react accordingly (redirect to login, show a banner) instead of retrying silently forever. Defaults to 0 (no limit) so existing behavior is unchanged. Closes supabase#1680
420df5f to
823388f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Hi @mandarini, all 7 points addressed. Sorry for the churn on the earlier rounds, this push should be solid. 1. Reset location: Moved from 2. Recoverable off-browser: Added restart logic in 3. No broadcast: Changed to 4. Pass session: 5. JSDoc caveats: Updated both 6. Real-path test + real errors: Added a test that seeds storage directly and goes through the real 7. Format: Ran On the catch blocks: Left them counting all thrown errors (including storage adapter throws) since they represent genuinely failed ticks from the app's perspective. Documented the counter as "failed ticks" in the JSDoc. 13 tests passing, build green. Thanks for the patience and the detailed guidance! |
Description
What changed?
Added an opt-in
maxAutoRefreshFailuresoption to the auth client that stops the auto-refresh ticker after N consecutive failed ticks and emits a newTOKEN_REFRESH_FAILEDevent viaonAuthStateChange.Why was this change needed?
When the auth server is permanently unreachable (wrong URL, server down, DNS failure), the auto-refresh ticker retries every 30 seconds forever with no way for the app to know it's failing. Users reported pages hanging for 14-21 seconds on load due to retry backoff, with no signal to react to.
This was raised in #1680 and acknowledged by @mandarini as a valid feature request for a configurable "stop after N failed ticks and emit an error event" option that is opt-in.
How it works
maxAutoRefreshFailures(defaults to0which means retry forever, preserving existing behavior)TOKEN_REFRESH_FAILEDthroughonAuthStateChangeUsage
Closes #1680
Breaking changes
The option defaults to 0 (no limit), so existing behavior is completely unchanged. The new
TOKEN_REFRESH_FAILEDevent is additive and only fires when the option is explicitly configured.Checklist
pnpm nx formatto ensure consistent code formattingTesting
Additional notes
_callRefreshTokenon success, so any refresh path (manual or automatic) resets itmaxAutoRefreshFailures === 0)