supabase-js icon indicating copy to clipboard operation
supabase-js copied to clipboard

fix(auth): make _notifyAllSubscribers non-blocking to prevent callback deadlocks

Open 7ttp opened this issue 1 month ago • 2 comments

Summary

Makes _notifyAllSubscribers non-blocking by removing await on subscriber callbacks. This prevents deadlocks when callbacks perform async operations that require the auth lock.

Problem

When a subscriber callback in onAuthStateChange performs async operations like getUser() or getSession(), a deadlock occurs:

  1. Auth method (e.g., signInWithPassword) acquires lock via _acquireLock()
  2. Inside the lock, it calls await _notifyAllSubscribers() which awaits all callbacks
  3. Subscriber callback calls getUser() which tries to acquire the same lock
  4. Lock queues getUser() to wait for current operation
  5. Current operation waits for callback to complete
  6. Deadlock: callback waits for getUser() → waits for lock → waits for callback

Solution

Remove async/await from _notifyAllSubscribers. Callbacks fire synchronously, async callbacks run in background without blocking. Errors still caught and logged.

Related

Extends #2014 - that PR added setTimeout wrapper at one call site (exchangeCodeForSession). This fix makes _notifyAllSubscribers itself non-blocking, fixing all 20+ call sites. closes https://github.com/supabase/supabase-js/issues/2013

Thanks @simplysparsh for the initial finding.

7ttp avatar Jan 12 '26 14:01 7ttp

Coverage Status

coverage: 81.017% (+0.02%) from 80.997% when pulling 60e2a3dedc951d813cc4beee6b5fd8f455de7342 on 7ttp:fix/auth-non-blocking-subscriber-notifications into 09aa10628b00cbdf65ace0f9e8e79237e7c0c1dc on supabase:master.

coveralls avatar Jan 12 '26 15:01 coveralls

@7ttp Thanks for the PR @mandarini @grdsdev Would be great if you can review this.

simplysparsh avatar Jan 18 '26 20:01 simplysparsh

This PR will re-introduce the SSR OAuth bug that #2039 fixes.

The SSR callback is async:

  client.auth.onAuthStateChange(async (event) => {                                       
    await applyServerStorage({...})                                                      
  })                                                                                     

With this PRs non-blocking approach, applyServerStorage won't complete before serverless functions return, causing the same cookie-setting failure as the original setTimeout issue.

We need a different solution that:

  1. Prevents deadlocks when callbacks call getUser() or getSession()
  2. Ensures critical async operations (like cookie-setting) complete before returning
  3. Works in both serverless and long-running server contexts

Potential approaches:

  • Detect if callback is async and warn/error if it awaits other auth methods
  • Document that callbacks MUST NOT await auth methods
  • Refactor locking mechanism to allow reentrant calls during notifications
  • Make cookie-setting synchronous (queue writes instead of awaiting)

mandarini avatar Jan 21 '26 16:01 mandarini

hey @mandarini almost forgot about this, was curious if this aswell is being discussed internally (in the recent lock related discussion) ? or should i continue with the suggested approaches? 😅💚

7ttp avatar Feb 11 '26 17:02 7ttp

let's park it for now @7ttp ! I want to see how the internal discussion goes!! Thank you for ALL the help!!!!! 💚

mandarini avatar Feb 12 '26 09:02 mandarini