fix(auth): add configurable lock acquisition timeout to prevent deadlocks

[yoshifumi-kondo]

Summary

• Add lockAcquireTimeout option to GoTrueClientOptions (default: 60000ms)
• Replace infinite lock timeout (-1) with configurable timeout to prevent production deadlocks
• Users can customize timeout or set negative for infinite wait (not recommended)

Fixes #1594

Test plan

• Build passes (npx nx build auth-js)
• Existing tests pass
• Manual testing with concurrent browser tabs

[coveralls]

coverage: 95.367% (+15.0%) from 80.341%
when pulling 550db2d on yoshifumi-kondo:fix/issue-1594-lock-acquire-timeout
into d3d05f8 on supabase:master.

[mandarini]

Thanks for this PR! This addresses a real pain point for users experiencing deadlocks (especially on Android Chrome). The implementation is clean and follows existing patterns.

Requested Changes

1. Enhanced JSDoc Documentation

The current JSDoc could be more explicit about error handling and edge cases:

/**
 * The maximum time in milliseconds to wait for a lock to be acquired.
 * If the lock cannot be acquired within this time, a `LockAcquireTimeoutError` is
 thrown.
 * You can catch this by checking `error.isAcquireTimeout === true`.
 *
 * - **Positive value**: Wait up to this many milliseconds before timing out
 * - **Zero (0)**: Fail immediately if the lock is unavailable
 * - **Negative value**: Wait indefinitely (not recommended - can cause deadlocks)
 *
 * Defaults to 60000 (1 minute).
 */
lockAcquireTimeout?: number

2. Add Tests for Timeout Behavior

Please add tests to verify the timeout functionality works as expected. For example:

it('should throw LockAcquireTimeoutError when lock acquisition times out', async
() => {
  // Test that a short timeout actually throws the expected error
})

it('should use custom lockAcquireTimeout when provided', async () => {
  // Test that the option is respected
})

3. Document User Recovery

When a timeout occurs, what should users do? Consider adding guidance. Something like: "If you encounter frequent timeouts, check for held locks via navigator.locks.query() and consider clearing browser data or restarting the browser."

Some questions to think about

1. Default timeout value: Is 60 seconds the right default? That's a long time for a user to wait on a white screen. Would 30s or even 10s be more appropriate? Curious about the reasoning.

2. Error handling guidance: When this timeout error is thrown, most apps probably won't handle it gracefully. Should we consider:

   • Logging a helpful console message with recovery suggestions?
   • Adding a retry mechanism with exponential backoff?

Overall this is a solid fix for a frustrating issue. Just needs a bit more documentation and test coverage. Thanks for tackling this and for contributing to Supabase! 💚

[yoshifumi-kondo]

@mandarini
Thank you for the thoughtful review and clear feedback! I've addressed all the requested changes.

1. Enhanced JSDoc Documentation

Added comprehensive JSDoc to lockAcquireTimeout option in types.ts:

/**
 * 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 for the lock before failing.
 *
 * If the lock cannot be acquired within this time, a `LockAcquireTimeoutError` is thrown.
 * You can catch this by checking `error.isAcquireTimeout === true`.
 *
 * - **Positive value**: Wait up to this many milliseconds before timing out
 * - **Zero (0)**: Fail immediately if the lock is unavailable
 * - **Negative value**: Wait indefinitely (not recommended - can cause deadlocks)
 *
 * @default 10000
 *
 * @example
 * try {
 *   await client.auth.getSession()
 * } catch (error) {
 *   if (error.isAcquireTimeout) {
 *     // Lock held by another tab/instance, or a previous operation is stuck.
 *     // Consider: closing other tabs, increasing timeout, or restarting the browser.
 *     console.error('Could not acquire lock within timeout period.')
 *   }
 * }
 */

2. Add Tests for Timeout Behavior

Added tests in two locations:

locks.test.ts (actual timeout behavior):

• should throw LockAcquireTimeoutError when lock acquisition times out — verifies real timeout with isAcquireTimeout: true
• should fail immediately when acquireTimeout is 0 and lock is held

GoTrueClient.test.ts (configuration integration):

• should use custom lockAcquireTimeout when provided
• should use default lockAcquireTimeout (10000ms) when not provided
• should pass negative timeout to lock for indefinite wait

3. Document User Recovery

Added recovery guidance in:

• JSDoc example shows how to catch and handle the error
• console.warn message in processLock includes actionable guidance:

Lock "{name}" acquisition timed out after {timeout}ms.
This may be caused by another operation holding the lock.
Consider increasing lockAcquireTimeout or checking for stuck operations.

4. Answers to Questions

Q: Is 60 seconds too long as default?

I initially chose 60 seconds to account for edge cases like slow networks or heavily loaded devices where lock operations might take longer than usual.

However, your point about user experience is valid — in actual deadlock situations, 60 seconds is far too long for users to wait on a white screen. Since lock operations typically complete in milliseconds, a shorter timeout still provides adequate buffer for edge cases while giving users faster feedback.

Changed to 10 seconds (10000ms) as a balance between these considerations.

Q: Should we add console logging?

Added console.warn to processLock when timeout occurs. Includes lock name and timeout value for debugging context.

Q: Should we add a retry mechanism?

For now, users can implement custom retry logic by catching error.isAcquireTimeout === true.

I believe automatic retry should be considered in a follow-up PR because:

1. This PR's scope is deadlock prevention via configurable timeout (Issue [Auth] GoTrueClient uses infinite timeouts for lock acquisition causing deadlocks in production #1594)
2. Retry mechanism requires careful design decisions (retry count, backoff strategy, configurability)
3. Different use cases have different retry requirements

If there's interest, I'm happy to open a follow-up issue to discuss retry mechanism design.

[Owez]

Thanks for this :) any idea what release this'll be in? Right now we're using the noop workaround

[tadeumaia]

My React Native app is exploding with timeout errors now. What is the best practice for RN? should i hunt all that is causing deadlocks or the previous behavior good enough for apps?

[mandarini]

@tadeumaia what version are you using? Do you think this PR caused your issue?

[tadeumaia]

@mandarini Yeah, when I updated to latest I started getting a bunch of warning for @supabase/gotrue-js: Lock "lock:sb-tvjkvpkckfxgxdhyggiy-auth-token" acquisition timed out after 10000ms. This may be caused by another operation holding the lock. Consider increasing lockAcquireTimeout or checking for stuck operations.

I'm on "@supabase/supabase-js": "^2.90.1" was on "^2.87.1", before.

Just no idea what the best practice and if the lock is necessary for RN or only for browser (multi tab stuff) so I should disable it or hunt for places where I might be calling locks too much.

[Owez]

I've been trying out the new canary version in prod and it seems to have mainly fixed the main issue of deadlocks, but I get the feeling like theres something odd going on internally for it to be causing it. I still seem to be getting the occasional total lockout still, but its performing better than before (say 1 per day vs ~5). Could be a fault on my end, though it doesn't feel seemless.

Total vibes tell me its multiple open sessions at the same time, but I'm assuming getSession is locally JWT checking so that shouldn't matter

Though I'm just an end user and probs can't reproduce on a public repo 😔