diff --git a/packages/core/auth-js/migrations/lockless-coordination.md b/packages/core/auth-js/migrations/lockless-coordination.md new file mode 100644 index 000000000..6bfe4e261 --- /dev/null +++ b/packages/core/auth-js/migrations/lockless-coordination.md @@ -0,0 +1,89 @@ +# Lockless auth coordination + +**Since:** v2.X.Y (update at release time) +**Action required by:** v3.0.0 + +`@supabase/auth-js` now coordinates session refreshes without a shared mutex by default. The legacy `lock` option is still honored when supplied, but it is deprecated and will be removed in v3. + +## What changed + +- **Default coordination is lockless.** A client constructed without a `lock` option no longer acquires `navigator.locks` or any in-process lock. In-tab concurrent refreshes are deduplicated via the pre-existing single-flight (`refreshingDeferred`); cross-tab refresh races are resolved by GoTrue's server-side parent-of-active mechanism on the v1 refresh-token path. +- **Commit guard inside `_callRefreshToken`.** The client snapshots storage before the rotated-token fetch and re-reads after. If a non-null pre-fetch snapshot was cleared between the two reads (typical case: a concurrent `signOut` ran `_removeSession`), the rotated tokens are discarded instead of being written back over the cleared storage. The discarded result resolves with `{ data: null, error: new AuthRefreshDiscardedError() }`. +- **New `AuthRefreshDiscardedError`** (with `isAuthRefreshDiscardedError` type guard). Surfaces through `refreshSession()` and `getSession()` results when the commit guard fires. Distinct from `AuthRetryableFetchError` (transient network) and `AuthApiError` (server rejection). +- **New `client.auth.dispose()`.** Tears down the auto-refresh interval, the `visibilitychange` listener, the `BroadcastChannel`, and registered `onAuthStateChange` subscribers. Idempotent. Designed for React Strict Mode and HMR cleanup hooks. In-flight `fetch` calls are not aborted — they run to completion. +- **`lock` and `lockAcquireTimeout` options.** Accepted and honored when supplied (legacy opt-in path); both are `@deprecated` and will be removed in v3. + +## Who is affected + +**Most callers: nothing to do.** If you do not pass a `lock` option to `createClient` / `GoTrueClient`, you are already on the lockless default path and will get the bug fixes and new APIs without any code change. + +**Callers passing a custom `lock`** (typically React Native `processLock`, Node multi-process setups with shared AsyncStorage, or a custom lock implementation): + +- v2.x (this release): your custom `lock` is still invoked exactly as before. The legacy `_acquireLock` machinery is preserved on an opt-in path gated by `settings.lock != null`. No code change required. +- v3.0.0 (planned): the `lock` and `lockAcquireTimeout` options will be removed entirely. Drop them from your client options before upgrading to v3. + +## New APIs worth knowing + +### `client.auth.dispose()` + +Tears down the client's background work in one call. Safe to call repeatedly. + +```ts +useEffect(() => { + const supabase = createClient(URL, KEY) + return () => { + supabase.auth.dispose() + } +}, []) +``` + +### `AuthRefreshDiscardedError` + +Returned from `refreshSession()` / `getSession()` when the commit guard discards a successfully-rotated session. + +```ts +import { isAuthRefreshDiscardedError } from '@supabase/auth-js' + +const { data, error } = await supabase.auth.refreshSession() +if (isAuthRefreshDiscardedError(error)) { + // A concurrent signOut cleared storage between fetch start and now. + // The rotated tokens were discarded; the SIGNED_OUT event already fired. + // Treat as a successful no-op — no need to retry. +} +``` + +## Behavior changes worth flagging + +- **`_autoRefreshTokenTick` may run concurrently with `signOut` / `setSession` / `getUser`** on the lockless default path. Previously the tick used `_acquireLock(0, ...)` which skipped whenever any auth op held the lock. The lockless equivalent only skips when `refreshingDeferred` is set. The commit guard keeps storage consistent under the new concurrency. The legacy lock opt-in path retains the old skip-on-any-lock behavior. +- **`onAuthStateChange` async callbacks** that call `getUser`, `setSession`, or read the session from inside the callback are now safe on the default path (previously deadlocked through the lock). One residual hazard remains: calling `refreshSession` (or anything routing through `_callRefreshToken`) from inside a `TOKEN_REFRESHED` handler still deadlocks via `refreshingDeferred`. The `@deprecated` marker on the async overload is kept with its reason updated to point at this specific case. +- **Subscriber timing on the default path:** subscribers stay awaited; same as before. What changes is that `signOut` no longer waits for an in-flight refresh's HTTP and continuation to finish before its own fetch goes out. Both fetches now run concurrently, and the commit guard keeps storage consistent. + +## Migration steps + +### If you do not pass a custom `lock` (most users) + +No action required for v2. No action required for v3. + +### If you pass a custom `lock` (e.g., React Native `processLock`) + +No action required for v2 — your lock continues to work. + +For v3 readiness: + +1. Remove `lock` and `lockAcquireTimeout` from your `createClient` / `GoTrueClient` options before upgrading to v3. +2. If you depended on cross-process serialization (e.g., Node multi-process with shared AsyncStorage), validate that the lockless coordination (in-tab single-flight + server parent-of-active) is sufficient for your runtime. The default is safe for the cases the lock was originally added to handle (cross-tab refresh races), since the server resolves them. + +```ts +// Before (v2.x, still works): +const supabase = createClient(URL, KEY, { + auth: { lock: processLock, lockAcquireTimeout: 5000 }, +}) + +// After (v3-ready): +const supabase = createClient(URL, KEY) +``` + +## Reference + +- Server-side parent-of-active mechanism: `internal/tokens/service.go:376-385` in the [supabase/auth](https://github.com/supabase/auth) repo (v1 branch, the `*models.RefreshToken` type assertion). When a request arrives with a revoked refresh token whose child is the currently-active token, the server returns the active token instead of rejecting — both tabs receive the same rotated token under DB row locking. +- `lib/locks.ts` exports (`navigatorLock`, `processLock`, `LockAcquireTimeoutError`, `NavigatorLockAcquireTimeoutError`, `ProcessLockAcquireTimeoutError`, `internals`) remain available for direct imports, marked `@deprecated`. Direct callers who use these exports outside the `GoTrueClient` constructor option are unaffected. diff --git a/packages/core/auth-js/src/GoTrueClient.ts b/packages/core/auth-js/src/GoTrueClient.ts index 7e9c9f816..e37231ee5 100644 --- a/packages/core/auth-js/src/GoTrueClient.ts +++ b/packages/core/auth-js/src/GoTrueClient.ts @@ -16,11 +16,13 @@ import { AuthInvalidTokenResponseError, AuthPKCECodeVerifierMissingError, AuthPKCEGrantCodeExchangeError, + AuthRefreshDiscardedError, AuthSessionMissingError, AuthUnknownError, isAuthApiError, isAuthError, isAuthImplicitGrantRedirectError, + isAuthRefreshDiscardedError, isAuthRetryableFetchError, isAuthSessionMissingError, } from './lib/errors' @@ -195,11 +197,17 @@ const DEFAULT_OPTIONS: Omit< debug: false, hasCustomAuthorizationHeader: false, throwOnError: false, - lockAcquireTimeout: 5000, // 5 seconds + lockAcquireTimeout: 5000, // 5 seconds. Only used when a custom `lock` is supplied. TODO(v3): remove. skipAutoInitialize: false, experimental: {}, } +/** + * No-op lock used internally as a placeholder. Kept so older test setups that + * inject this exact reference do not break; new code never sees it because + * `this.lock` stays `null` when no custom lock is supplied (lockless path). + * TODO(v3): remove with the legacy lock path. + */ async function lockNoOp(name: string, acquireTimeout: number, fn: () => Promise): Promise { return await fn() } @@ -280,6 +288,14 @@ export default class GoTrueClient { protected autoRefreshTickTimeout: ReturnType | null = null protected visibilityChangedCallback: (() => Promise) | null = null protected refreshingDeferred: Deferred | null = null + /** + * Monotonic counter incremented at the top of `_removeSession`, before any + * `await`. The commit guard inside `_callRefreshToken` captures this value + * before `_saveSession` and re-checks it after, so a `signOut` that + * interleaves inside `_saveSession`'s storage-write awaits is still caught + * (the post-fetch storage snapshot alone misses that window). + */ + protected _sessionRemovalEpoch = 0 /** * Keeps track of the async client initialization. * When null or not yet resolved the auth state is `unknown` @@ -297,10 +313,19 @@ export default class GoTrueClient { protected hasCustomAuthorizationHeader = false protected suppressGetSessionWarning = false protected fetch: Fetch - protected lock: LockFunc + /** + * Custom lock function passed via `settings.lock`. When non-null, every auth + * operation runs inside `_acquireLock`. When null (the default), the client + * uses its lockless coordination (refresh single-flight + commit guard). + * TODO(v3): remove along with the legacy lock path. + */ + protected lock: LockFunc | null = null protected lockAcquired = false protected pendingInLock: Promise[] = [] protected throwOnError: boolean + /** + * Only consulted when a custom `lock` is supplied. TODO(v3): remove. + */ protected lockAcquireTimeout: number /** * Opt-in flags for experimental features. Defaults to an empty object. @@ -371,19 +396,23 @@ export default class GoTrueClient { this.url = settings.url this.headers = settings.headers this.fetch = resolveFetch(settings.fetch) - this.lock = settings.lock || lockNoOp this.detectSessionInUrl = settings.detectSessionInUrl this.flowType = settings.flowType this.hasCustomAuthorizationHeader = settings.hasCustomAuthorizationHeader this.throwOnError = settings.throwOnError + + // Always wire `lockAcquireTimeout` even on the lockless path: consumers + // (including supabase-js tests) read it off the client to verify option + // flow-through. this.lockAcquireTimeout = settings.lockAcquireTimeout - if (settings.lock) { + // TODO(v3): remove. Legacy opt-in path preserved for backwards + // compatibility with callers passing a custom `lock` (typically React + // Native `processLock` or Node multi-process setups). When `settings.lock` + // is null the client uses its lockless coordination — no `navigator.locks` + // by default, no implicit `processLock`. + if (settings.lock != null) { this.lock = settings.lock - } else if (this.persistSession && isBrowser() && globalThis?.navigator?.locks) { - this.lock = navigatorLock - } else { - this.lock = lockNoOp } if (!this.jwks) { @@ -518,9 +547,13 @@ export default class GoTrueClient { } this.initializePromise = (async () => { - return await this._acquireLock(this.lockAcquireTimeout, async () => { - return await this._initialize() - }) + if (this.lock != null) { + // TODO(v3): remove legacy lock path + return await this._acquireLock(this.lockAcquireTimeout, async () => { + return await this._initialize() + }) + } + return await this._initialize() })() return await this.initializePromise @@ -1405,9 +1438,14 @@ export default class GoTrueClient { async exchangeCodeForSession(authCode: string): Promise { await this.initializePromise - return this._acquireLock(this.lockAcquireTimeout, async () => { - return this._exchangeCodeForSession(authCode) - }) + if (this.lock != null) { + // TODO(v3): remove legacy lock path + return this._acquireLock(this.lockAcquireTimeout, async () => { + return this._exchangeCodeForSession(authCode) + }) + } + + return this._exchangeCodeForSession(authCode) } /** @@ -2444,9 +2482,14 @@ export default class GoTrueClient { async reauthenticate(): Promise { await this.initializePromise - return await this._acquireLock(this.lockAcquireTimeout, async () => { - return await this._reauthenticate() - }) + if (this.lock != null) { + // TODO(v3): remove legacy lock path + return await this._acquireLock(this.lockAcquireTimeout, async () => { + return await this._reauthenticate() + }) + } + + return await this._reauthenticate() } private async _reauthenticate(): Promise { @@ -2593,7 +2636,7 @@ export default class GoTrueClient { * - If the session's access token is expired or is about to expire, this method will use the refresh token to refresh the session. * - When using in a browser, or you've called `startAutoRefresh()` in your environment (React Native, etc.) this function always returns a valid access token without refreshing the session itself, as this is done in the background. This function returns very fast. * - **IMPORTANT SECURITY NOTICE:** If using an insecure storage medium, such as cookies or request headers, the user object returned by this function **must not be trusted**. Always verify the JWT using `getClaims()` or your own JWT verification library to securely establish the user's identity and access. You can also use `getUser()` to fetch the user object directly from the Auth server for this purpose. - * - When using in a browser, this function is synchronized across all tabs using the [LockManager](https://developer.mozilla.org/en-US/docs/Web/API/LockManager) API. In other environments make sure you've defined a proper `lock` property, if necessary, to make sure there are no race conditions while the session is being refreshed. + * - Cross-tab refresh races are handled by the GoTrue server (the rotated token from the first tab is returned to subsequent tabs via the parent-of-active mechanism), so no client-side serialization is needed. * * @example Get the session data * ```js @@ -2661,17 +2704,26 @@ export default class GoTrueClient { async getSession() { await this.initializePromise - const result = await this._acquireLock(this.lockAcquireTimeout, async () => { - return this._useSession(async (result) => { - return result + if (this.lock != null) { + // TODO(v3): remove legacy lock path + return await this._acquireLock(this.lockAcquireTimeout, async () => { + return this._useSession(async (result) => { + return result + }) }) - }) + } - return result + return await this._useSession(async (result) => { + return result + }) } /** * Acquires a global lock based on the storage key. + * + * TODO(v3): remove along with the legacy lock path. Only called when + * `this.lock` is non-null (custom lock supplied via constructor). The + * default lockless path bypasses this entirely. */ private async _acquireLock(acquireTimeout: number, fn: () => Promise): Promise { this._debug('#_acquireLock', 'begin', acquireTimeout) @@ -2700,7 +2752,7 @@ export default class GoTrueClient { return result } - return await this.lock(`lock:${this.storageKey}`, acquireTimeout, async () => { + return await this.lock!(`lock:${this.storageKey}`, acquireTimeout, async () => { this._debug('#_acquireLock', 'lock acquired for storage key', this.storageKey) try { @@ -2742,10 +2794,9 @@ export default class GoTrueClient { } /** - * Use instead of {@link #getSession} inside the library. It is - * semantically usually what you want, as getting a session involves some - * processing afterwards that requires only one client operating on the - * session at once across multiple tabs or processes. + * Use instead of {@link #getSession} inside the library. Loads the session + * via `__loadSession` (which may trigger a refresh if the access token is + * within the expiry margin) and runs `fn` with the result. */ private async _useSession( fn: ( @@ -2773,7 +2824,10 @@ export default class GoTrueClient { this._debug('#_useSession', 'begin') try { - // the use of __loadSession here is the only correct use of the function! + // Concurrent callers may both reach __loadSession; storage reads are + // idempotent, and the only write path inside it (refresh) is + // single-flighted downstream by `refreshingDeferred` in + // `_callRefreshToken`. No serialization is needed at this layer. const result = await this.__loadSession() return await fn(result) @@ -2809,7 +2863,8 @@ export default class GoTrueClient { > { this._debug('#__loadSession()', 'begin') - if (!this.lockAcquired) { + if (this.lock != null && !this.lockAcquired) { + // TODO(v3): remove. Only meaningful on the legacy lock path. this._debug('#__loadSession()', 'used outside of an acquired lock!', new Error().stack) } @@ -2976,9 +3031,15 @@ export default class GoTrueClient { await this.initializePromise - const result = await this._acquireLock(this.lockAcquireTimeout, async () => { - return await this._getUser() - }) + let result: UserResponse + if (this.lock != null) { + // TODO(v3): remove legacy lock path + result = await this._acquireLock(this.lockAcquireTimeout, async () => { + return await this._getUser() + }) + } else { + result = await this._getUser() + } if (result.data.user) { this.suppressGetSessionWarning = true @@ -3153,9 +3214,14 @@ export default class GoTrueClient { ): Promise { await this.initializePromise - return await this._acquireLock(this.lockAcquireTimeout, async () => { - return await this._updateUser(attributes, options) - }) + if (this.lock != null) { + // TODO(v3): remove legacy lock path + return await this._acquireLock(this.lockAcquireTimeout, async () => { + return await this._updateUser(attributes, options) + }) + } + + return await this._updateUser(attributes, options) } protected async _updateUser( @@ -3342,9 +3408,14 @@ export default class GoTrueClient { }): Promise { await this.initializePromise - return await this._acquireLock(this.lockAcquireTimeout, async () => { - return await this._setSession(currentSession) - }) + if (this.lock != null) { + // TODO(v3): remove legacy lock path + return await this._acquireLock(this.lockAcquireTimeout, async () => { + return await this._setSession(currentSession) + }) + } + + return await this._setSession(currentSession) } protected async _setSession(currentSession: { @@ -3533,9 +3604,14 @@ export default class GoTrueClient { async refreshSession(currentSession?: { refresh_token: string }): Promise { await this.initializePromise - return await this._acquireLock(this.lockAcquireTimeout, async () => { - return await this._refreshSession(currentSession) - }) + if (this.lock != null) { + // TODO(v3): remove legacy lock path + return await this._acquireLock(this.lockAcquireTimeout, async () => { + return await this._refreshSession(currentSession) + }) + } + + return await this._refreshSession(currentSession) } protected async _refreshSession(currentSession?: { @@ -3783,9 +3859,14 @@ export default class GoTrueClient { async signOut(options: SignOut = { scope: 'global' }): Promise<{ error: AuthError | null }> { await this.initializePromise - return await this._acquireLock(this.lockAcquireTimeout, async () => { - return await this._signOut(options) - }) + if (this.lock != null) { + // TODO(v3): remove legacy lock path + return await this._acquireLock(this.lockAcquireTimeout, async () => { + return await this._signOut(options) + }) + } + + return await this._signOut(options) } protected async _signOut( @@ -3832,16 +3913,19 @@ export default class GoTrueClient { } /** - * Avoid using an async function inside `onAuthStateChange` as you might end - * up with a deadlock. The callback function runs inside an exclusive lock, - * so calling other Supabase Client APIs that also try to acquire the - * exclusive lock, might cause a deadlock. This behavior is observable across - * tabs. In the next major library version, this behavior will not be supported. - * - * Receive a notification every time an auth event happens. + * Receive a notification every time an auth event happens. Common reentry + * patterns (`getUser`, `setSession`, reading the session from inside a + * handler) complete normally. One hazard remains: calling `refreshSession` + * (or anything that routes through `_callRefreshToken`) from inside a + * `TOKEN_REFRESHED` handler. `refreshingDeferred` resolves only after + * `_notifyAllSubscribers` returns, so the inner refresh dedupes onto the + * outer's unresolved promise and the two wait on each other. * * @param callback A callback function to be invoked when an auth event happens. - * @deprecated Due to the possibility of deadlocks with async functions as callbacks, use the version without an async function. + * + * @deprecated Async callbacks can deadlock when they trigger a nested + * refresh from a `TOKEN_REFRESHED` event. Prefer the sync overload, or move + * refresh-triggering work outside the callback. */ onAuthStateChange(callback: (event: AuthChangeEvent, session: Session | null) => Promise): { data: { subscription: Subscription } @@ -3854,18 +3938,8 @@ export default class GoTrueClient { * - Subscribes to important events occurring on the user's session. * - Use on the frontend/client. It is less useful on the server. * - Events are emitted across tabs to keep your application's UI up-to-date. Some events can fire very frequently, based on the number of tabs open. Use a quick and efficient callback function, and defer or debounce as many operations as you can to be performed outside of the callback. - * - **Important:** A callback can be an `async` function and it runs synchronously during the processing of the changes causing the event. You can easily create a dead-lock by using `await` on a call to another method of the Supabase library. - * - Avoid using `async` functions as callbacks. - * - Limit the number of `await` calls in `async` callbacks. - * - Do not use other Supabase functions in the callback function. If you must, dispatch the functions once the callback has finished executing. Use this as a quick way to achieve this: - * ```js - * supabase.auth.onAuthStateChange((event, session) => { - * setTimeout(async () => { - * // await on other Supabase function here - * // this runs right after the callback has finished - * }, 0) - * }) - * ``` + * - Callbacks can be `async` and can safely call other Supabase auth methods (`getUser`, `setSession`, etc.) from inside the callback. + * - Keep callbacks quick. Events are awaited in order, so a slow callback delays subsequent events to subscribers in this tab. * - Emitted events: * - `INITIAL_SESSION` * - Emitted right after the Supabase client is constructed and the initial session from storage is loaded. @@ -4056,9 +4130,14 @@ export default class GoTrueClient { ;(async () => { await this.initializePromise - await this._acquireLock(this.lockAcquireTimeout, async () => { - this._emitInitialSession(id) - }) + if (this.lock != null) { + // TODO(v3): remove legacy lock path + await this._acquireLock(this.lockAcquireTimeout, async () => { + this._emitInitialSession(id) + }) + } else { + await this._emitInitialSession(id) + } })() return { data: { subscription } } @@ -4453,7 +4532,10 @@ export default class GoTrueClient { * @param refreshToken A valid refresh token that was returned on login. */ private async _refreshAccessToken(refreshToken: string): Promise { - const debugName = `#_refreshAccessToken(${refreshToken.substring(0, 5)}...)` + // Refresh tokens are long-lived bearer credentials; do NOT include any + // fragment of the token in the debug tag, even when `debug: true` is + // enabled (logs may be forwarded to third-party services). + const debugName = `#_refreshAccessToken()` this._debug(debugName, 'begin') try { @@ -4606,15 +4688,22 @@ export default class GoTrueClient { const { error } = await this._callRefreshToken(currentSession.refresh_token) if (error) { - console.error(error) - - if (!isAuthRetryableFetchError(error)) { - this._debug( - debugName, - 'refresh failed with a non-retryable error, removing the session', - error - ) - await this._removeSession() + // AuthRefreshDiscardedError means a concurrent signOut already + // cleared storage and fired SIGNED_OUT. Don't run _removeSession + // again here, or we'll emit a duplicate SIGNED_OUT. + if (isAuthRefreshDiscardedError(error)) { + this._debug(debugName, 'refresh discarded by commit guard', error) + } else { + console.error(error) + + if (!isAuthRetryableFetchError(error)) { + this._debug( + debugName, + 'refresh failed with a non-retryable error, removing the session', + error + ) + await this._removeSession() + } } } } @@ -4667,18 +4756,85 @@ export default class GoTrueClient { return this.refreshingDeferred.promise } - const debugName = `#_callRefreshToken(${refreshToken.substring(0, 5)}...)` + // Refresh tokens are long-lived bearer credentials; do NOT include any + // fragment of the token in the debug tag, even when `debug: true` is + // enabled (logs may be forwarded to third-party services). + const debugName = `#_callRefreshToken()` this._debug(debugName, 'begin') try { this.refreshingDeferred = new Deferred() + // Snapshot storage before the fetch. The commit guard discards the + // rotated tokens only when a non-null pre-fetch snapshot changed under + // us — typical case: a concurrent `signOut` ran `_removeSession`, or + // another tab's refresh rewrote the slot. Callers passing + // externally-sourced tokens (SSR cookie handoff, multi-account + // switching, `setSession`/`refreshSession({ refresh_token })`) may + // start from a null snapshot OR from a non-null snapshot whose + // refresh_token differs from the one they're hydrating; in both + // cases the guard fires only when storage was *modified between + // snapshots*, not when the input token disagrees with what's stored. + const storedAtStart = (await getItemAsync(this.storage, this.storageKey)) as Session | null + const { data, error } = await this._refreshAccessToken(refreshToken) if (error) throw error if (!data.session) throw new AuthSessionMissingError() + const storedAfter = (await getItemAsync(this.storage, this.storageKey)) as Session | null + const storageChangedUnderUs = + storedAtStart !== null && + (storedAfter === null || storedAfter.refresh_token !== storedAtStart.refresh_token) + + if (storageChangedUnderUs) { + this._debug( + debugName, + 'commit guard: storage changed since refresh started, discarding rotated tokens', + { + // Presence indicators only — never log refresh token fragments, + // even partial. Logs may be forwarded to third-party services. + startedWith: 'present', + nowHolds: storedAfter ? 'replaced' : 'cleared', + } + ) + const discarded: CallRefreshTokenResult = { + data: null, + error: new AuthRefreshDiscardedError(), + } + this.refreshingDeferred.resolve(discarded) + return discarded + } + + // Second leg of the commit guard: close the TOCTOU window between the + // synchronous `storageChangedUnderUs` check and the actual storage + // writes inside `_saveSession`. A concurrent `signOut → _removeSession` + // can land inside `_saveSession`'s `await setItemAsync(...)` yields and + // clear storage just before we overwrite it. Capture the epoch BEFORE + // the save and re-check after; if it advanced, undo the write directly + // (do NOT call `_removeSession` — that would emit a duplicate + // SIGNED_OUT for the concurrent signOut that already fired one). + const epochBeforeSave = this._sessionRemovalEpoch + await this._saveSession(data.session) + + if (this._sessionRemovalEpoch !== epochBeforeSave) { + this._debug( + debugName, + 'commit guard (post-save): _removeSession ran during _saveSession, undoing write' + ) + await removeItemAsync(this.storage, this.storageKey) + if (this.userStorage) { + await removeItemAsync(this.userStorage, this.storageKey + '-user') + } + const discarded: CallRefreshTokenResult = { + data: null, + error: new AuthRefreshDiscardedError(), + } + this.refreshingDeferred.resolve(discarded) + return discarded + } + await this._notifyAllSubscribers('TOKEN_REFRESHED', data.session) const result = { data: data.session, error: null } @@ -4790,6 +4946,11 @@ export default class GoTrueClient { } private async _removeSession() { + // Bump synchronously, BEFORE any `await`, so that `_callRefreshToken`'s + // post-save check sees the increment whenever this method has started — + // even if it hasn't finished. Pairs with the epoch check in + // `_callRefreshToken`. See `_sessionRemovalEpoch` field doc. + this._sessionRemovalEpoch += 1 this._debug('#_removeSession()') this.suppressGetSessionWarning = false @@ -4978,58 +5139,144 @@ export default class GoTrueClient { await this._stopAutoRefresh() } + /** + * Tears down the client's background work: stops the auto-refresh interval, + * removes the `visibilitychange` listener, closes the cross-tab + * `BroadcastChannel`, and clears registered `onAuthStateChange` subscribers. + * + * Call this from cleanup hooks when the client is being replaced before + * its JS realm is destroyed. React Strict Mode and HMR are the common + * cases. Any in-flight `fetch` calls continue to completion and may still + * write to storage; dispose doesn't abort them or erase storage. + * + * Lifecycle caveat: because in-flight refreshes are not aborted, a + * disposed instance can still persist a rotated session to storage after + * `dispose()` returns. A subsequent `createClient` against the same + * `storageKey` will pick up that session on its next read. If you need + * strict isolation between client lifecycles, await any pending auth + * operation before calling `dispose()` (or change the `storageKey` for + * the replacement client). + * + * Safe to call repeatedly. + * + * @category Auth + * + * @example Cleanup on React unmount + * ```ts + * useEffect(() => { + * const client = createClient(...) + * return () => { client.auth.dispose() } + * }, []) + * ``` + */ + async dispose(): Promise { + this._removeVisibilityChangedCallback() + await this._stopAutoRefresh() + this.broadcastChannel?.close() + this.broadcastChannel = null + this.stateChangeEmitters.clear() + } + /** * Runs the auto refresh token tick. */ private async _autoRefreshTokenTick() { this._debug('#_autoRefreshTokenTick()', 'begin') - try { - await this._acquireLock(0, async () => { - try { - const now = Date.now() - + if (this.lock != null) { + // TODO(v3): remove legacy lock path. Uses `_acquireLock(0, ...)` which + // throws `LockAcquireTimeoutError` immediately if the lock is held — + // that's the fail-fast skip path that lets the tick bail out instead + // of queuing behind a long-running operation. + try { + await this._acquireLock(0, async () => { try { - return await this._useSession(async (result) => { - const { - data: { session }, - } = result - - if (!session || !session.refresh_token || !session.expires_at) { - this._debug('#_autoRefreshTokenTick()', 'no session') - return - } + const now = Date.now() + try { + return await this._useSession(async (result) => { + const { + data: { session }, + } = result + + if (!session || !session.refresh_token || !session.expires_at) { + this._debug('#_autoRefreshTokenTick()', 'no session') + return + } - // session will expire in this many ticks (or has already expired if <= 0) - const expiresInTicks = Math.floor( - (session.expires_at * 1000 - now) / AUTO_REFRESH_TICK_DURATION_MS - ) + const expiresInTicks = Math.floor( + (session.expires_at * 1000 - now) / AUTO_REFRESH_TICK_DURATION_MS + ) - this._debug( - '#_autoRefreshTokenTick()', - `access token expires in ${expiresInTicks} ticks, a tick lasts ${AUTO_REFRESH_TICK_DURATION_MS}ms, refresh threshold is ${AUTO_REFRESH_TICK_THRESHOLD} ticks` - ) + this._debug( + '#_autoRefreshTokenTick()', + `access token expires in ${expiresInTicks} ticks, a tick lasts ${AUTO_REFRESH_TICK_DURATION_MS}ms, refresh threshold is ${AUTO_REFRESH_TICK_THRESHOLD} ticks` + ) - if (expiresInTicks <= AUTO_REFRESH_TICK_THRESHOLD) { - await this._callRefreshToken(session.refresh_token) - } - }) - } catch (e) { - console.error( - 'Auto refresh tick failed with error. This is likely a transient error.', - e - ) + if (expiresInTicks <= AUTO_REFRESH_TICK_THRESHOLD) { + await this._callRefreshToken(session.refresh_token) + } + }) + } catch (e) { + console.error( + 'Auto refresh tick failed with error. This is likely a transient error.', + e + ) + } + } finally { + this._debug('#_autoRefreshTokenTick()', 'end') } - } finally { - this._debug('#_autoRefreshTokenTick()', 'end') + }) + } catch (e) { + if (e instanceof LockAcquireTimeoutError) { + this._debug('auto refresh token tick lock not available') + } else { + throw e } - }) - } catch (e) { - if (e instanceof LockAcquireTimeoutError) { - this._debug('auto refresh token tick lock not available') - } else { - throw e } + return + } + + // Lockless default: skip if a refresh is already in flight. + // `_callRefreshToken` also dedupes via the same field; this is just a + // fast-path skip to avoid an unnecessary storage read. + if (this.refreshingDeferred !== null) { + this._debug('#_autoRefreshTokenTick()', 'refresh already in flight, skipping') + return + } + + try { + const now = Date.now() + + try { + await this._useSession(async (result) => { + const { + data: { session }, + } = result + + if (!session || !session.refresh_token || !session.expires_at) { + this._debug('#_autoRefreshTokenTick()', 'no session') + return + } + + // session will expire in this many ticks (or has already expired if <= 0) + const expiresInTicks = Math.floor( + (session.expires_at * 1000 - now) / AUTO_REFRESH_TICK_DURATION_MS + ) + + this._debug( + '#_autoRefreshTokenTick()', + `access token expires in ${expiresInTicks} ticks, a tick lasts ${AUTO_REFRESH_TICK_DURATION_MS}ms, refresh threshold is ${AUTO_REFRESH_TICK_THRESHOLD} ticks` + ) + + if (expiresInTicks <= AUTO_REFRESH_TICK_THRESHOLD) { + await this._callRefreshToken(session.refresh_token) + } + }) + } catch (e) { + console.error('Auto refresh tick failed with error. This is likely a transient error.', e) + } + } finally { + this._debug('#_autoRefreshTokenTick()', 'end') } } @@ -5086,24 +5333,29 @@ export default class GoTrueClient { if (!calledFromInitialize) { // called when the visibility has changed, i.e. the browser // transitioned from hidden -> visible so we need to see if the session - // should be recovered immediately... but to do that we need to acquire - // the lock first asynchronously + // should be recovered await this.initializePromise - await this._acquireLock(this.lockAcquireTimeout, async () => { + if (this.lock != null) { + // TODO(v3): remove legacy lock path + await this._acquireLock(this.lockAcquireTimeout, async () => { + if (document.visibilityState !== 'visible') { + this._debug( + methodName, + 'acquired the lock to recover the session, but the browser visibilityState is no longer visible, aborting' + ) + return + } + await this._recoverAndRefresh() + }) + } else { if (document.visibilityState !== 'visible') { - this._debug( - methodName, - 'acquired the lock to recover the session, but the browser visibilityState is no longer visible, aborting' - ) - - // visibility has changed while waiting for the lock, abort + this._debug(methodName, 'visibilityState is no longer visible, skipping recovery') return } - // recover the session await this._recoverAndRefresh() - }) + } } } else if (document.visibilityState === 'hidden') { if (this.autoRefreshToken) { @@ -5235,7 +5487,7 @@ export default class GoTrueClient { params: MFAVerifyWebauthnParams ): Promise private async _verify(params: MFAVerifyParams): Promise { - return this._acquireLock(this.lockAcquireTimeout, async () => { + const run = async (): Promise => { try { return await this._useSession(async (result) => { const { data: sessionData, error: sessionError } = result @@ -5306,7 +5558,13 @@ export default class GoTrueClient { } throw error } - }) + } + + if (this.lock != null) { + // TODO(v3): remove legacy lock path + return this._acquireLock(this.lockAcquireTimeout, run) + } + return run() } /** @@ -5322,7 +5580,7 @@ export default class GoTrueClient { params: MFAChallengeWebauthnParams ): Promise> private async _challenge(params: MFAChallengeParams): Promise { - return this._acquireLock(this.lockAcquireTimeout, async () => { + const run = async (): Promise => { try { return await this._useSession(async (result) => { const { data: sessionData, error: sessionError } = result @@ -5395,7 +5653,13 @@ export default class GoTrueClient { } throw error } - }) + } + + if (this.lock != null) { + // TODO(v3): remove legacy lock path + return this._acquireLock(this.lockAcquireTimeout, run) + } + return run() } /** @@ -5404,9 +5668,6 @@ export default class GoTrueClient { private async _challengeAndVerify( params: MFAChallengeAndVerifyParams ): Promise { - // both _challenge and _verify independently acquire the lock, so no need - // to acquire it here - const { data: challengeData, error: challengeError } = await this._challenge({ factorId: params.factorId, }) @@ -5425,7 +5686,6 @@ export default class GoTrueClient { * {@see GoTrueMFAApi#listFactors} */ private async _listFactors(): Promise { - // use #getUser instead of #_getUser as the former acquires a lock const { data: { user }, error: userError, diff --git a/packages/core/auth-js/src/lib/errors.ts b/packages/core/auth-js/src/lib/errors.ts index 21fb7886f..eb2520dfa 100644 --- a/packages/core/auth-js/src/lib/errors.ts +++ b/packages/core/auth-js/src/lib/errors.ts @@ -297,6 +297,38 @@ export function isAuthRetryableFetchError(error: unknown): error is AuthRetryabl return isAuthError(error) && error.name === 'AuthRetryableFetchError' } +/** + * Returned when the server rotated a refresh token successfully but the + * client chose not to persist the rotated tokens because the local session + * changed mid-flight. Usually means a concurrent `signOut` cleared storage + * between when the refresh started and when it came back. + * + * Set on the `error` field of the refresh result so callers can tell "we + * got rotated tokens but threw them away" apart from "the refresh failed." + * The rotated session on the server will be picked up on the next refresh + * via GoTrue's parent-of-active path. + * + * @example + * ```ts + * import { isAuthRefreshDiscardedError } from '@supabase/auth-js' + * + * if (isAuthRefreshDiscardedError(error)) { + * // Concurrent signOut/sign-in raced our refresh. Treat as a no-op. + * } + * ``` + */ +export class AuthRefreshDiscardedError extends CustomAuthError { + constructor( + message = 'Refresh result discarded: session state changed mid-flight (e.g., concurrent signOut)' + ) { + super(message, 'AuthRefreshDiscardedError', 409, undefined) + } +} + +export function isAuthRefreshDiscardedError(error: unknown): error is AuthRefreshDiscardedError { + return isAuthError(error) && error.name === 'AuthRefreshDiscardedError' +} + /** * This error is thrown on certain methods when the password used is deemed * weak. Inspect the reasons to identify what password strength rules are diff --git a/packages/core/auth-js/src/lib/locks.ts b/packages/core/auth-js/src/lib/locks.ts index ece1610d0..56b2b44ce 100644 --- a/packages/core/auth-js/src/lib/locks.ts +++ b/packages/core/auth-js/src/lib/locks.ts @@ -1,6 +1,17 @@ +/** + * Lock primitives retained for backwards-compatible imports. The auth client + * coordinates refreshes itself (deduping in-instance callers onto a shared + * in-flight promise) and lets the GoTrue server resolve cross-instance races, + * so it does not invoke any primitive from this module. The functions still + * work for direct callers that need a navigator.locks-backed or in-process + * exclusive lock of their own. + */ + import { supportsLocalStorage } from './helpers' /** + * @deprecated Debug flag for `navigatorLock` / `processLock`. The auth + * client ignores both, so this has no client-side effect. * @experimental */ export const internals = { @@ -18,18 +29,9 @@ export const internals = { /** * An error thrown when a lock cannot be acquired after some amount of time. * - * Use the {@link #isAcquireTimeout} property instead of checking with `instanceof`. - * - * @example - * ```ts - * import { LockAcquireTimeoutError } from '@supabase/auth-js' - * - * class CustomLockError extends LockAcquireTimeoutError { - * constructor() { - * super('Lock timed out') - * } - * } - * ``` + * @deprecated The auth client doesn't acquire locks around auth operations, + * so this error never originates from `supabase.auth.*` calls. Direct callers + * of `navigatorLock` / `processLock` still receive it on acquire timeout. */ export abstract class LockAcquireTimeoutError extends Error { public readonly isAcquireTimeout = true @@ -40,25 +42,15 @@ export abstract class LockAcquireTimeoutError extends Error { } /** - * Error thrown when the browser Navigator Lock API fails to acquire a lock. - * - * @example - * ```ts - * import { NavigatorLockAcquireTimeoutError } from '@supabase/auth-js' - * - * throw new NavigatorLockAcquireTimeoutError('Lock timed out') - * ``` + * @deprecated The auth client doesn't call `navigator.locks`, so this error + * never originates from `supabase.auth.*` calls. Direct callers of + * `navigatorLock` still receive it on acquire timeout. */ export class NavigatorLockAcquireTimeoutError extends LockAcquireTimeoutError {} /** - * Error thrown when the process-level lock helper cannot acquire a lock. - * - * @example - * ```ts - * import { ProcessLockAcquireTimeoutError } from '@supabase/auth-js' - * - * throw new ProcessLockAcquireTimeoutError('Lock timed out') - * ``` + * @deprecated The auth client doesn't run `processLock`, so this error + * never originates from `supabase.auth.*` calls. Direct callers of + * `processLock` still receive it on acquire timeout. */ export class ProcessLockAcquireTimeoutError extends LockAcquireTimeoutError {} @@ -86,12 +78,10 @@ export class ProcessLockAcquireTimeoutError extends LockAcquireTimeoutError {} * will time out after so many milliseconds. An error is * a timeout if it has `isAcquireTimeout` set to true. * @param fn The operation to run once the lock is acquired. - * @example - * ```ts - * await navigatorLock('sync-user', 1000, async () => { - * await refreshSession() - * }) - * ``` + * + * @deprecated The auth client coordinates refreshes itself and the server + * resolves concurrent refresh races, so passing `{ lock: navigatorLock }` + * to it has no effect. You can safely drop the import from your client setup. */ export async function navigatorLock( name: string, @@ -320,6 +310,11 @@ const PROCESS_LOCKS: { [name: string]: Promise } = {} * will time out after so many milliseconds. An error is * a timeout if it has `isAcquireTimeout` set to true. * @param fn The operation to run once the lock is acquired. + * + * @deprecated The auth client coordinates refreshes itself and the server + * resolves concurrent refresh races, so passing `{ lock: processLock }` + * to it has no effect. You can safely drop the import from your client setup. + * * @example * ```ts * await processLock('migrate', 5000, async () => { diff --git a/packages/core/auth-js/src/lib/types.ts b/packages/core/auth-js/src/lib/types.ts index 63382a001..2ddfdd2a2 100644 --- a/packages/core/auth-js/src/lib/types.ts +++ b/packages/core/auth-js/src/lib/types.ts @@ -126,12 +126,17 @@ export type GoTrueClientOptions = { /* If debug messages are emitted. Can be used to inspect the behavior of the library. If set to a function, the provided function will be used instead of `console.log()` to perform the logging. */ debug?: boolean | ((message: string, ...args: any[]) => void) /** - * Provide your own locking mechanism based on the environment. By default, - * `navigatorLock` (Web Locks API) is used in browser environments when - * `persistSession` is true. Falls back to an in-process lock for non-browser - * environments (e.g. React Native). - * - * @experimental + * Provide your own locking mechanism based on the environment. By default + * the client coordinates refreshes itself (single-flight via + * `refreshingDeferred` + commit guard) and relies on the GoTrue server to + * resolve cross-tab refresh races. Passing a custom lock opts into a + * legacy path that wraps every auth operation in your supplied lock — this + * path is preserved for backwards compatibility (typically React Native + * `processLock` or Node multi-process setups). + * + * @deprecated Custom locks still work in v2.x for backwards compatibility. + * The legacy lock path will be removed in v3 — drop this option from your + * constructor options before upgrading. */ lock?: LockFunc /** @@ -145,30 +150,14 @@ export type GoTrueClientOptions = { */ throwOnError?: boolean /** - * The maximum time in milliseconds to wait for acquiring a cross-tab synchronization lock. - * - * When multiple browser tabs or windows use the auth client simultaneously, they coordinate - * via the Web Locks API to prevent race conditions during session refresh and other operations. - * This timeout controls how long to wait before attempting lock recovery. - * - * - **Positive value**: Wait up to this many milliseconds. If the lock is still held, attempt - * automatic recovery by stealing it (the previous holder is evicted, its callback continues - * to completion without exclusive access). This recovers from orphaned locks caused by - * React Strict Mode double-mount, storage API hangs, or aborted operations. - * - **Zero (0)**: Fail immediately if the lock is unavailable; throws `LockAcquireTimeoutError` - * (check `error.isAcquireTimeout === true`). - * - **Negative value**: Wait indefinitely — can cause permanent deadlocks if the lock is orphaned. + * The maximum time in milliseconds to wait for acquiring the custom lock + * supplied via the `lock` option. Only consulted when a custom `lock` is + * passed — the default lockless path doesn't use this timeout. * * @default 5000 * - * @example - * ```ts - * const client = createClient(url, key, { - * auth: { - * lockAcquireTimeout: 5000, // 5 seconds, then steal orphaned lock - * }, - * }) - * ``` + * @deprecated Only used by the legacy lock path. Will be removed in v3 + * along with the `lock` option. */ lockAcquireTimeout?: number diff --git a/packages/core/auth-js/test/GoTrueClient.browser.test.ts b/packages/core/auth-js/test/GoTrueClient.browser.test.ts index 3d1c527c2..c35644016 100644 --- a/packages/core/auth-js/test/GoTrueClient.browser.test.ts +++ b/packages/core/auth-js/test/GoTrueClient.browser.test.ts @@ -650,51 +650,22 @@ describe('GoTrueClient BroadcastChannel', () => { }) }) -describe('Browser locks functionality', () => { - it('should use navigator locks when available', () => { - // Mock navigator.locks - const mockLock = { name: 'test-lock' } - const mockRequest = jest - .fn() - .mockImplementation((_, __, callback) => Promise.resolve(callback(mockLock))) - +describe('Lockless coordination: navigator.locks should NOT be invoked', () => { + it('initializes without touching navigator.locks', async () => { + const mockRequest = jest.fn() Object.defineProperty(navigator, 'locks', { value: { request: mockRequest }, writable: true, }) - // Test navigator locks usage in GoTrueClient const client = getClientWithSpecificStorage({ getItem: jest.fn(), setItem: jest.fn(), removeItem: jest.fn(), }) - - expect(client).toBeDefined() - }) - - it('should handle _acquireLock with empty pendingInLock', async () => { - const client = getClientWithSpecificStorage({ - getItem: jest.fn(), - setItem: jest.fn(), - removeItem: jest.fn(), - }) - - // Mock navigator.locks - const mockLock = { name: 'test-lock' } - const mockRequest = jest - .fn() - .mockImplementation((_, __, callback) => Promise.resolve(callback(mockLock))) - - Object.defineProperty(navigator, 'locks', { - value: { request: mockRequest }, - writable: true, - }) - - // Initialize client to trigger lock acquisition await client.initialize() - expect(client).toBeDefined() + expect(mockRequest).not.toHaveBeenCalled() }) }) @@ -1225,36 +1196,22 @@ describe('Storage and User Storage Combinations', () => { }) }) -describe('Lock Mechanism Branches', () => { - it('should handle lock acquisition timeout', async () => { - const slowLock = jest +describe('Legacy lock opt-in: custom `lock` function is invoked when supplied', () => { + it('a custom `lock` function passed to the constructor IS invoked (legacy path)', async () => { + const customLock = jest .fn() - .mockImplementation(() => new Promise((resolve) => setTimeout(resolve, 100))) - - const client = new (require('../src/GoTrueClient').default)({ - url: 'http://localhost:9999', - autoRefreshToken: false, - lock: slowLock, - }) - - await client.initialize() - expect(client).toBeDefined() - }) - - it('should handle lock release errors', async () => { - const errorLock = jest.fn().mockImplementation(() => ({ - acquire: jest.fn().mockResolvedValue(undefined), - release: jest.fn().mockRejectedValue(new Error('Lock release error')), - })) + .mockImplementation(async (_name: string, _timeout: number, fn: () => Promise) => + fn() + ) const client = new (require('../src/GoTrueClient').default)({ url: 'http://localhost:9999', autoRefreshToken: false, - lock: errorLock, + lock: customLock, }) await client.initialize() - expect(client).toBeDefined() + expect(customLock).toHaveBeenCalled() }) }) diff --git a/packages/core/auth-js/test/GoTrueClient.test.ts b/packages/core/auth-js/test/GoTrueClient.test.ts index 26b1021f1..a201d0b33 100644 --- a/packages/core/auth-js/test/GoTrueClient.test.ts +++ b/packages/core/auth-js/test/GoTrueClient.test.ts @@ -3,7 +3,7 @@ import { JWK, Session } from '../src' import GoTrueClient from '../src/GoTrueClient' import { base64UrlToUint8Array } from '../src/lib/base64url' import { STORAGE_KEY } from '../src/lib/constants' -import { setItemAsync } from '../src/lib/helpers' +import { getItemAsync, setItemAsync } from '../src/lib/helpers' import { memoryLocalStorageAdapter } from '../src/lib/local-storage' import { deserializeCredentialCreationOptions, @@ -57,7 +57,11 @@ describe('GoTrueClient', () => { }) test('should handle custom lock implementation', async () => { - const customLock = jest.fn().mockResolvedValue(undefined) + const customLock = jest + .fn() + .mockImplementation(async (_name: string, _timeout: number, fn: () => Promise) => + fn() + ) const client = new GoTrueClient({ url: 'http://localhost:9999', lock: customLock, @@ -3418,115 +3422,351 @@ describe('SSO Authentication', () => { }) }) -describe('Lock functionality', () => { - test('_acquireLock should execute function when lock is acquired', async () => { - const mockFn = jest.fn().mockResolvedValue('success') - // @ts-expect-error 'Allow access to private _acquireLock' - const result = await authWithSession._acquireLock(1000, mockFn) - expect(result).toBe('success') - expect(mockFn).toHaveBeenCalled() - }) - - test('_acquireLock should throw error when lock acquisition times out', async () => { - let shouldFail = false - const mockLock = jest.fn().mockImplementation(async (name, timeout, fn) => { - if (shouldFail) { - throw new Error('Lock acquisition timeout') - } - return fn() - }) - +describe('Lockless coordination (default) and legacy lock opt-in', () => { + test('default path: no custom `lock` supplied, _acquireLock is not invoked', async () => { const client = new GoTrueClient({ url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON, - lock: mockLock, autoRefreshToken: false, persistSession: false, }) + // @ts-expect-error access private _acquireLock for verification + const acquireSpy = jest.spyOn(client, '_acquireLock') - // Wait for initialization to complete - await new Promise((resolve) => setTimeout(resolve, 100)) - - // Now make the lock fail - shouldFail = true + await client.initialize() + await client.getSession() - const mockFn = jest.fn() - // @ts-expect-error 'Allow access to private _acquireLock' - await expect(client._acquireLock(1000, mockFn)).rejects.toThrow('Lock acquisition timeout') - expect(mockFn).not.toHaveBeenCalled() + // No lock supplied → lockless coordination → _acquireLock is never called + expect(acquireSpy).not.toHaveBeenCalled() }) - test('should use custom lockAcquireTimeout when provided', async () => { - const capturedTimeouts: number[] = [] + test('legacy path: custom `lock` is invoked when supplied', async () => { const mockLock = jest .fn() - .mockImplementation(async (name: string, timeout: number, fn: () => Promise) => { - capturedTimeouts.push(timeout) - return fn() - }) - - const customTimeout = 20000 // 20 seconds (different from default 10s) + .mockImplementation(async (name: string, timeout: number, fn: () => Promise) => + fn() + ) const client = new GoTrueClient({ url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON, lock: mockLock, autoRefreshToken: false, persistSession: false, - lockAcquireTimeout: customTimeout, }) await client.initialize() + await client.getSession() - // Verify that the custom timeout was passed to the lock function + // Custom lock supplied → legacy path → mockLock IS called. expect(mockLock).toHaveBeenCalled() - expect(capturedTimeouts).toContain(customTimeout) }) - test('should use default lockAcquireTimeout (10000ms) when not provided', async () => { - const capturedTimeouts: number[] = [] - const mockLock = jest - .fn() - .mockImplementation(async (name: string, timeout: number, fn: () => Promise) => { - capturedTimeouts.push(timeout) - return fn() - }) + test('`lockAcquireTimeout` option is accepted (lockless path ignores it; legacy uses it)', async () => { + expect( + () => + new GoTrueClient({ + url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON, + autoRefreshToken: false, + persistSession: false, + lockAcquireTimeout: 12345, + }) + ).not.toThrow() + }) + + test('subscriber callback may call other auth methods without deadlock', async () => { + // Auth methods invoked from inside an onAuthStateChange callback complete + // without re-entering a held lock, because notification is not gated by + // one. + const observed: string[] = [] + const { + data: { subscription }, + } = authWithSession.onAuthStateChange(async (event) => { + observed.push(event) + if (event === 'INITIAL_SESSION') { + const { data } = await authWithSession.getSession() + observed.push(`got-session:${data.session ? 'present' : 'null'}`) + } + }) + + // Allow the initial-session emit to flush. + await new Promise((resolve) => setTimeout(resolve, 100)) + + subscription.unsubscribe() + + expect(observed).toContain('INITIAL_SESSION') + expect(observed.some((s) => s.startsWith('got-session:'))).toBe(true) + }) +}) + +describe('dispose() lifecycle', () => { + test('idempotent: safe to call repeatedly', async () => { const client = new GoTrueClient({ url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON, - lock: mockLock, autoRefreshToken: false, persistSession: false, - // lockAcquireTimeout not provided, should default to 5000 }) + await client.initialize() + await expect(client.dispose()).resolves.toBeUndefined() + await expect(client.dispose()).resolves.toBeUndefined() + }) + + test('clears registered onAuthStateChange subscribers', async () => { + const client = new GoTrueClient({ + url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON, + autoRefreshToken: false, + persistSession: false, + }) await client.initialize() - // Verify that the default timeout (5000 = 5 seconds) was used - expect(mockLock).toHaveBeenCalled() - expect(capturedTimeouts).toContain(5000) + const cb = jest.fn() + client.onAuthStateChange(cb) + + // @ts-expect-error access protected field for verification + expect(client.stateChangeEmitters.size).toBeGreaterThan(0) + + await client.dispose() + + // @ts-expect-error access protected field for verification + expect(client.stateChangeEmitters.size).toBe(0) }) - test('should pass negative timeout to lock for indefinite wait', async () => { - const capturedTimeouts: number[] = [] - const mockLock = jest - .fn() - .mockImplementation(async (name: string, timeout: number, fn: () => Promise) => { - capturedTimeouts.push(timeout) - return fn() - }) + test('stops the auto-refresh ticker', async () => { + const client = new GoTrueClient({ + url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON, + autoRefreshToken: true, + persistSession: false, + }) + await client.initialize() + await client.startAutoRefresh() + + // @ts-expect-error access protected field for verification + expect(client.autoRefreshTicker).not.toBeNull() + + await client.dispose() + // @ts-expect-error access protected field for verification + expect(client.autoRefreshTicker).toBeNull() + }) +}) + +describe('Refresh commit guard (signOut-during-refresh race)', () => { + test('discards rotated tokens when storage was cleared mid-flight', async () => { + const storage = memoryLocalStorageAdapter() const client = new GoTrueClient({ url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON, - lock: mockLock, + storage, autoRefreshToken: false, - persistSession: false, - lockAcquireTimeout: -1, // Indefinite wait (not recommended) + persistSession: true, + }) + await client.initialize() + + // Plant a session in storage with refresh_token R1. + const R1 = 'refresh-token-r1' + await setItemAsync(storage, STORAGE_KEY, { + access_token: 'jwt.accesstoken.signature', + refresh_token: R1, + token_type: 'bearer', + expires_in: 1000, + expires_at: Math.floor(Date.now() / 1000) + 1000, + user: { id: 'user-1', email: 'u@example.com' } as any, + } as Session) + + // Simulate a successful refresh that returns rotated tokens (R2) while + // storage was cleared between fetch start and continuation. + // @ts-expect-error access protected for test + client._refreshAccessToken = jest.fn(async () => { + // Concurrent signOut clears storage before our refresh continuation runs. + // @ts-expect-error access protected for test + await client._removeSession() + return { + data: { + session: { + access_token: 'jwt.new.signature', + refresh_token: 'refresh-token-r2', + token_type: 'bearer', + expires_in: 1000, + expires_at: Math.floor(Date.now() / 1000) + 1000, + user: { id: 'user-1', email: 'u@example.com' }, + } as any, + user: { id: 'user-1', email: 'u@example.com' } as any, + }, + error: null, + } as any }) + // @ts-expect-error access protected for test + const result = await client._callRefreshToken(R1) + + // Commit guard discards the rotated tokens. + expect(result.error).not.toBeNull() + expect((result.error as Error).name).toBe('AuthRefreshDiscardedError') + + // Storage stays cleared; rotated tokens were NOT written back. + const stored = await storage.getItem(STORAGE_KEY) + expect(stored).toBeNull() + }) + + test('accepts rotated tokens when storage was empty at fetch start', async () => { + // Regression for the false-positive guard: snapshotting "current storage" + // *after* the fetch and comparing to the input refresh_token would treat + // empty-storage as "cleared mid-flight" and reject. That broke + // setSession/refreshSession({ refresh_token }) for any caller supplying + // externally-sourced tokens against empty storage (SSR cookie-handoff, + // external-token hydration). + const storage = memoryLocalStorageAdapter() + const client = new GoTrueClient({ + url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON, + storage, + autoRefreshToken: false, + persistSession: true, + }) await client.initialize() - // Verify that negative timeout was passed through - expect(mockLock).toHaveBeenCalled() - expect(capturedTimeouts).toContain(-1) + // Storage starts empty — no signOut ever ran. + expect(await storage.getItem(STORAGE_KEY)).toBeNull() + + const rotatedSession = { + access_token: 'jwt.new.signature', + refresh_token: 'refresh-token-r2', + token_type: 'bearer', + expires_in: 1000, + expires_at: Math.floor(Date.now() / 1000) + 1000, + user: { id: 'user-1', email: 'u@example.com' }, + } + + // Refresh succeeds against externally-supplied refresh_token; doesn't + // touch storage itself. The guard must NOT treat the still-empty + // pre-snapshot as a mid-flight clear. + // @ts-expect-error access protected for test + client._refreshAccessToken = jest.fn(async () => ({ + data: { session: rotatedSession, user: rotatedSession.user }, + error: null, + })) + + // @ts-expect-error access protected for test + const result = await client._callRefreshToken('R-external') + + expect(result.error).toBeNull() + expect(result.data?.refresh_token).toBe('refresh-token-r2') + + // Rotated tokens are persisted because storage wasn't cleared under us. + // Read via `getItemAsync` to JSON-parse the stringified Session that + // `_saveSession` wrote through `setItemAsync`. + const stored = (await getItemAsync(storage, STORAGE_KEY)) as Session | null + expect(stored?.refresh_token).toBe('refresh-token-r2') + }) + + test('accepts rotated tokens when storage holds a different session than the input refresh_token', async () => { + // Covers the SSR-hydration / multi-account-switch path: storage already + // holds session A, caller invokes setSession (or refreshSession) with an + // externally-sourced refresh_token that doesn't match A. The guard must + // not interpret "input token differs from stored token" as a mid-flight + // race — nothing was modified between snapshots, so the rotated tokens + // should persist and replace A. + const storage = memoryLocalStorageAdapter() + const client = new GoTrueClient({ + url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON, + storage, + autoRefreshToken: false, + persistSession: true, + }) + await client.initialize() + + // Plant session A (refresh_token RA) — represents "user already signed in". + const RA = 'refresh-token-a' + await setItemAsync(storage, STORAGE_KEY, { + access_token: 'jwt.a.signature', + refresh_token: RA, + token_type: 'bearer', + expires_in: 1000, + expires_at: Math.floor(Date.now() / 1000) + 1000, + user: { id: 'user-a', email: 'a@example.com' } as any, + } as Session) + + // Caller hydrates a different session (refresh_token RB, e.g. from SSR cookie). + // Mock the refresh to return rotated tokens for RB without touching storage. + const rotatedB = { + access_token: 'jwt.b.signature', + refresh_token: 'refresh-token-b-rotated', + token_type: 'bearer', + expires_in: 1000, + expires_at: Math.floor(Date.now() / 1000) + 1000, + user: { id: 'user-b', email: 'b@example.com' }, + } + // @ts-expect-error access protected for test + client._refreshAccessToken = jest.fn(async () => ({ + data: { session: rotatedB, user: rotatedB.user }, + error: null, + })) + + // @ts-expect-error access protected for test + const result = await client._callRefreshToken('refresh-token-b') + + // The rotated tokens were accepted, not discarded by the commit guard. + expect(result.error).toBeNull() + expect(result.data?.refresh_token).toBe('refresh-token-b-rotated') + + // Storage now holds the rotated B session (replaces A). + const stored = (await getItemAsync(storage, STORAGE_KEY)) as Session | null + expect(stored?.refresh_token).toBe('refresh-token-b-rotated') + }) + + test('discards rotated tokens when a different session was written to storage mid-flight', async () => { + // Stronger guard test: storage holds session A at fetch start; while the + // refresh is in flight, *another* tab writes session B to storage; the + // refresh result for A's token must be discarded because storage no + // longer represents the state we were operating on. + const storage = memoryLocalStorageAdapter() + const client = new GoTrueClient({ + url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON, + storage, + autoRefreshToken: false, + persistSession: true, + }) + await client.initialize() + + const RA = 'refresh-token-a' + const sessionA: Session = { + access_token: 'jwt.a.signature', + refresh_token: RA, + token_type: 'bearer', + expires_in: 1000, + expires_at: Math.floor(Date.now() / 1000) + 1000, + user: { id: 'user-a', email: 'a@example.com' } as any, + } + await setItemAsync(storage, STORAGE_KEY, sessionA) + + // Simulate another tab writing session B mid-flight. + // @ts-expect-error access protected for test + client._refreshAccessToken = jest.fn(async () => { + await setItemAsync(storage, STORAGE_KEY, { + ...sessionA, + refresh_token: 'refresh-token-b-from-other-tab', + }) + return { + data: { + session: { + ...sessionA, + access_token: 'jwt.a-rotated.signature', + refresh_token: 'refresh-token-a-rotated', + } as any, + user: sessionA.user as any, + }, + error: null, + } as any + }) + + // @ts-expect-error access protected for test + const result = await client._callRefreshToken(RA) + + expect(result.error).not.toBeNull() + expect((result.error as Error).name).toBe('AuthRefreshDiscardedError') + + // Storage still holds B (what the other tab wrote); A's rotated tokens + // were correctly discarded. + const stored = (await getItemAsync(storage, STORAGE_KEY)) as Session | null + expect(stored?.refresh_token).toBe('refresh-token-b-from-other-tab') }) }) diff --git a/packages/core/supabase-js/src/lib/types.ts b/packages/core/supabase-js/src/lib/types.ts index d19458e9b..c90e35e17 100644 --- a/packages/core/supabase-js/src/lib/types.ts +++ b/packages/core/supabase-js/src/lib/types.ts @@ -182,9 +182,14 @@ export type SupabaseClientOptions = { */ debug?: SupabaseAuthClientOptions['debug'] /** - * Provide your own locking mechanism based on the environment. By default no locking is done at this time. + * Provide your own locking mechanism based on the environment. By default + * the auth client coordinates refreshes itself and the server resolves + * cross-tab races. Passing a custom `lock` opts into a legacy path that + * wraps every auth operation in your supplied lock. * - * @experimental + * @deprecated Custom locks still work in v2.x for backwards compatibility. + * The legacy lock path will be removed in v3 — drop this option from your + * `createClient` options before upgrading. */ lock?: SupabaseAuthClientOptions['lock'] /** @@ -200,11 +205,13 @@ export type SupabaseClientOptions = { */ experimental?: SupabaseAuthClientOptions['experimental'] /** - * Maximum time in milliseconds to wait when acquiring the auth lock before - * stealing it from the previous holder. See `GoTrueClientOptions.lockAcquireTimeout` - * for full semantics (zero fails immediately, negative waits indefinitely). + * Maximum time in milliseconds to wait for acquiring the custom lock + * supplied via `lock`. Only consulted when a custom `lock` is passed. * * @default 5000 + * + * @deprecated Only used by the legacy lock path. Will be removed in v3 + * along with the `lock` option. */ lockAcquireTimeout?: SupabaseAuthClientOptions['lockAcquireTimeout'] /** diff --git a/packages/core/supabase-js/test/unit/SupabaseAuthClient.test.ts b/packages/core/supabase-js/test/unit/SupabaseAuthClient.test.ts index 751057fe7..e2e02c067 100644 --- a/packages/core/supabase-js/test/unit/SupabaseAuthClient.test.ts +++ b/packages/core/supabase-js/test/unit/SupabaseAuthClient.test.ts @@ -58,6 +58,16 @@ test('createClient gates passkey methods when auth.experimental.passkey is not s await expect(supa.auth.passkey.list()).rejects.toThrow(/experimental.*passkey/) }) +// The two tests below verify that `lockAcquireTimeout` flows from +// `createClient({ auth: { lockAcquireTimeout: ... }})` through to the +// constructed `GoTrueClient` instance. The field is `protected`, so we +// cast through `unknown` to a precise shape rather than using `as any`. +// The targeted cast is deliberate: when the legacy lock path is removed in +// v3 (see `// TODO(v3): remove …` markers in `GoTrueClient.ts` and the +// SDK Linear ticket for the v3 cleanup), `grep -rn "lockAcquireTimeout"` +// surfaces both the production code AND these tests together so the +// removal is mechanical. + test('_initSupabaseAuthClient should pass through lockAcquireTimeout option', () => { const client = new SupabaseClient('https://example.supabase.com', 'supabaseKey') const authClient = client['_initSupabaseAuthClient']( @@ -66,14 +76,14 @@ test('_initSupabaseAuthClient should pass through lockAcquireTimeout option', () undefined ) - expect((authClient as any).lockAcquireTimeout).toBe(30_000) + expect((authClient as unknown as { lockAcquireTimeout: number }).lockAcquireTimeout).toBe(30_000) }) test('createClient should accept auth.lockAcquireTimeout and wire it to auth client', () => { const supa = new SupabaseClient('https://example.supabase.com', 'supabaseKey', { auth: { lockAcquireTimeout: 30_000 }, }) - expect((supa.auth as any).lockAcquireTimeout).toBe(30_000) + expect((supa.auth as unknown as { lockAcquireTimeout: number }).lockAcquireTimeout).toBe(30_000) }) test('createClient should accept auth.skipAutoInitialize and wire it to auth client', async () => {