Skip to content

fix(firebase-frameworks): don't delete an in-flight app on concurrent - #689

Open
leoortizz wants to merge 1 commit into
mainfrom
fix/frameworks-auth-lru-race
Open

leoortizz wants to merge 1 commit into
mainfrom
fix/frameworks-auth-lru-race

Conversation

@leoortizz

Copy link
Copy Markdown
Member

Fixes #685

Problem

firebaseAppsLRU is keyed by uid and disposes with deleteApp. Two separate paths hand a request an app that is already being deleted.

Concurrent requests with the same __session cookie all miss the cache, so each one calls initializeApp and each set() deletes the app the previous request is still signing in with. The losers throw app/app-deleted, handleFactory does not catch, and Express answers 500.

A single request does it too, once the entry goes stale. A stale get() deletes the entry, which fires dispose and deleteApp, then returns that same app. deleteApp is async and nothing awaits it, and the rest of handleAuth is synchronous, so the request never notices. Teardown finishes while the page is awaiting, and the next use of res.locals.firebaseApp throws.

Fix

  • Re-check the cache after the awaited revocation check, so only the first request initializes an app.
  • Drop allowStale.
  • Catch in handleFactory, so an auth failure degrades to an unauthenticated request instead of a 500.

Verification

Next.js 14.2 with the Firebase JS SDK, deployed to Hosting preview channels. Statuses come from the SSR function's own request log.

App Router, the reported scenario: eight <Link>s mount right after sign-in and prefetch at once against a cold entry.

firebase-frameworks at the function
0.11.8 seven 500s with app/app-deleted, one 200
this branch eight 200s

The 0.11.8 latencies match the table in the issue.

The stale path was checked on its own, with a page that awaits getIdToken() and then reads app.options. Once the entry is left idle past its 5 minute LRU_TTL, 0.11.8 serves the stale entry and the page fails with app/app-deleted, while this branch re-initializes and succeeds.

same-uid requests

Concurrent requests for one uid each created their own app, and every
set()
disposed the previous one while it was still signing in. The losers
threw
app/app-deleted and the request ended in a 500. Now:
- the cache is re-checked after the await so only the first request
  creates the app
- allowStale is gone because a stale get() handed back an already
  deleted app
- an auth failure falls through as an unauthenticated request.

Fixes #685

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request removes the allowStale option from the LRU cache, handles concurrent app initialization in handleAuth by checking the cache again after awaiting session cookie verification, and catches authentication errors in handleFactory to treat them as unauthenticated requests. The review feedback highlights a potential race condition and performance bottleneck with concurrent requests executing signInWithCustomToken simultaneously, suggesting caching the sign-in promise on the app instance to prevent redundant operations.

Comment on lines +66 to +67
// A concurrent request may have initialized the app while we awaited
app = firebaseAppsLRU.get(uid);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Concurrency Issue with signInWithCustomToken

While this PR successfully prevents multiple FirebaseApp instances from being initialized concurrently, there is still a potential race condition and performance bottleneck with signInWithCustomToken.

The Problem

When multiple concurrent requests for the same uid arrive:

  1. The first request initializes the FirebaseApp and adds it to the LRU cache.
  2. The subsequent concurrent requests bypass the !app check because they retrieve the newly created app from the LRU cache.
  3. However, because the first request's signInWithCustomToken is asynchronous and has not completed yet, auth.currentUser is still null for all subsequent requests.
  4. As a result, all concurrent requests will pass the auth.currentUser?.uid !== uid check and concurrently call adminAuth.createCustomToken(uid) and signInWithCustomToken(auth, customToken).

This leads to:

  • Multiple redundant network requests to the Firebase Auth backend (Identity Toolkit API) for the same user session.
  • Potential race conditions in the Firebase Auth state listener.
  • Unnecessary CPU overhead from multiple createCustomToken calls.

Suggested Solution

We can cache the active sign-in promise directly on the app instance to ensure that concurrent requests for the same app await the same sign-in operation:

  const auth = getAuth(app);
  if (auth.currentUser?.uid !== uid) {
    // Cache the sign-in promise on the app instance to prevent concurrent sign-ins
    let signInPromise = (app as any)._signInPromise;
    if (!signInPromise) {
      signInPromise = (async () => {
        // TODO(jamesdaniels) get custom claims
        const customToken = await adminAuth
          .createCustomToken(uid)
          .catch((e: Error) => console.error(e.message));
        if (!customToken) return;
        await signInWithCustomToken(auth, customToken);
      })();
      (app as any)._signInPromise = signInPromise;
      
      const cleanUp = () => {
        delete (app as any)._signInPromise;
      };
      signInPromise.then(cleanUp, cleanUp);
    }
    await signInPromise;
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

firebase-frameworks: handleAuth 500s under concurrent same-uid requests (uid-keyed LRU dispose deletes an in-flight Firebase App)

1 participant