[GUMBOKIM]

Closes #3234.

Summary

A failed token refresh is memoized in promiseToCachedToken_ and replayed to every later caller for
up to ~55 minutes, while a valid token sits in cachedToken_. Every service authenticating through
AuthorizedHttpClient or AuthorizedHttp2Client sees a stale app/invalid-credential.

This is a regression from #2648, which returns the shared promise on the non-refresh path so that
concurrent callers join one refresh — a rejected refresh is now shared the same way. See #3234 for
how RTDB's revocation path produces this in normal operation.

Changes

getToken() returns cachedToken_ when it is further than TOKEN_EXPIRY_THRESHOLD_MILLIS from
expiry, rather than a promiseToCachedToken_ that may hold a rejection.

hasUsableCachedToken() negates the condition shouldRefresh() tested before #2648 added
isRefreshing to it, at the same threshold and the same boundary. The isRefreshing term cancels
out, so the new branch is taken on exactly the calls that took the pre-#2648 read path: this is a
restoration, and only where it was already safe.

The cache read deliberately runs before shouldRefresh(): each predicate reads its own Date.now(),
so the opposite order leaves a window where a token crosses the threshold between reads, neither
branch fires, and the stale promise is returned. The invariant is documented on the helper so it
survives the next edit.

A token inside the refresh threshold still waits for the refresh, as it does today, and the lifetime
of a handed-out token is unchanged: with no refresh in flight, main already serves the same token
through the memoized promise.

Restoring the explicit return Promise.resolve(this.cachedToken_) sketched on #2645 also fixes this,
but moving isRefreshing back out of shouldRefresh() breaks the
Database should gracefully handle errors during token refresh spec. Keeping #2648's structure and
adding the guard is the smaller diff.

Behavior change

When a forced refresh is triggered because the server rejected the current token — RTDB's
revocation path, as opposed to this SDK's own scheduled refresh at src/database/database.ts:169,
which is proactive — a concurrent getToken() now receives that outgoing token until the replacement
lands, instead of joining the refresh. serves the outgoing token while the refresh that replaces it is in flight pins this.

Trade-off: if that forced refresh also fails, callers keep receiving the revoked token until it
reaches the threshold, so they see 401s from each service instead of an explicit
app/invalid-credential. That is the narrow case (revocation plus endpoint failure) against the
common one (any failed refresh taking every service down for ~55 minutes with a valid token in hand).
Happy to revisit if you would rather keep the explicit error.

Nothing changes for a token inside the five-minute refresh threshold: the caller still waits on the
refresh and still sees its error. That window exists so a failing credential surfaces while there is
still time to react, and this change deliberately leaves it intact.

API changes

None. FirebaseAppInternals is not exported API, hasUsableCachedToken() is private, and
npm run api-extractor reports no change to any etc/*.api.md golden file.

Test coverage

Nine tests in a describe('with a stuck or failing credential') block in
test/unit/app/firebase-app.spec.ts. Five fail against main:

• serves a valid cached token while a stalled refresh is still in flight
• keeps serving a valid cached token after a forced refresh is rejected
• serves a cached token that is only just outside the refresh threshold
• leaves the in-flight refresh reachable after serving the cache
• serves the outgoing token while the refresh that replaces it is in flight

The rest pin behavior that must not change — that a token inside the refresh threshold is not
served from the cache, that the caller waits for the refresh and receives its result, that the
credential's error still surfaces there, and that a failed refresh does not stop a later one:

• waits for the in-flight refresh when the cached token is nearly dead
• surfaces the credential error once the cached token is inside the refresh threshold
• does not serve a cached token that is already inside the refresh threshold
• replaces the cached token once a later forced refresh succeeds

I checked the tests actually pin the new branch by mutating it — unreachable branch, shifted
threshold in both directions, > vs >=, dropped null guard, cache served a few microtasks late,
and the cache read overwriting promiseToCachedToken_. Each mutation fails at least one test.

Verification

• npm run build — clean
• npm run build:tests — clean
• npm test — lint clean, 6073 passing, 0 failing
• npm run api-extractor — 18/18, no golden file change

Diff: 2 files changed, +276 insertions, -0 deletions (11 LOC in src, the rest in test).

I also ran the emulator integration suite (auth,database,firestore) on this branch and on main:
81 passing / 65 pending / 0 failing on both, with every individual test reporting the same outcome.

[gemini-code-assist]

Code Review

This pull request updates the getToken method in FirebaseAppInternals to prefer using a valid cached token over an in-flight refresh promise that might contain a failed refresh attempt. It introduces a helper method hasUsableCachedToken to determine if the cached token is still valid and outside the expiration threshold. Additionally, a comprehensive set of unit tests has been added to verify token retrieval behavior under various conditions, such as stalled or failing credentials. I have no feedback to provide as there are no review comments to address.

[GUMBOKIM]

Hi @lahirumaramba, sorry to bother you. Whenever you have a moment, I'd really appreciate a review of this PR.

It fixes #3234, a regression from #2648: one failed token refresh gets cached and replayed as app/invalid-credential to every caller for up to ~55 minutes, even though a valid token is still cached. RTDB's auth-revocation path can trigger this in normal operation. The source change is 11 lines, and the rest is tests.

One small note: CI passed on the original commit, but after I merged main on Sep 19 the workflows ended without running any jobs, so the required build checks are still pending. They may need a re-run or workflow approval on your side.

I'm happy to change anything. Thanks for your time!

[lahirumaramba]

Hey @GUMBOKIM, thank you for your contribution and for the thorough root-cause writeup in #3234! We really appreciate it! Overall the approach and behavior look solid. I added a couple of comments on simplifying the in-flight promise tracking and tightening one of the boundary tests. Let me know what you think!

[lahirumaramba]

Instead of keeping settled (and potentially rejected) promises in promiseToCachedToken_ plus a separate isRefreshing flag and two Date.now() checks, WDYT about only storing the promise while a refresh is actively in flight and clearing it in .finally()?

Something along these lines lets us drop isRefreshing and hasUsableCachedToken(), avoids the Date.now() ordering subtlety, and prevents an earlier overlapping refresh from resetting isRefreshing while a newer forceRefresh is still in flight:

  public getToken(forceRefresh = false): Promise<FirebaseAccessToken> {
    if (!forceRefresh && this.cachedToken_ && !this.shouldRefresh()) {
      return Promise.resolve(this.cachedToken_);
    }
    if (forceRefresh || !this.refreshTokenPromise_) {
      const refreshPromise = this.refreshToken().finally(() => {
        if (this.refreshTokenPromise_ === refreshPromise) {
          this.refreshTokenPromise_ = undefined;
        }
      });
      this.refreshTokenPromise_ = refreshPromise;
    }
    return this.refreshTokenPromise_;
  }

(With shouldRefresh() going back to !this.cachedToken_ || (this.cachedToken_.expirationTime - Date.now()) <= TOKEN_EXPIRY_THRESHOLD_MILLIS.)

[GUMBOKIM]

Agreed, that is much cleaner. Applied in b37042c: promiseToCachedToken_, isRefreshing and hasUsableCachedToken() are gone, shouldRefresh() is back to the plain threshold check, and the promise is cleared in .finally() with the identity check.

Good catch on the overlapping refreshes. I added keeps a newer forced refresh in flight when an older one settles, which fails on the previous commit and passes now.

[lahirumaramba]

nit: TEN_MINUTES_IN_SECONDS without clock.tick(...) leaves 10 minutes on the token (5 minutes outside TOKEN_EXPIRY_THRESHOLD_MILLIS). Should we advance the clock with clock.tick((5 * ONE_MINUTE_IN_MILLISECONDS) - 1000) here (similar to line 436) so this pins the 5m + 1s boundary?

[GUMBOKIM]

Done in b37042c. The test now ticks (5 * ONE_MINUTE_IN_MILLISECONDS) - 1000 so it pins 5m + 1s. I checked that moving the threshold by one second makes it fail.