fix(app): stop replaying a failed token refresh to later callers - #3235
Open
GUMBOKIM wants to merge 1 commit into
Open
fix(app): stop replaying a failed token refresh to later callers#3235GUMBOKIM wants to merge 1 commit into
GUMBOKIM wants to merge 1 commit into
Conversation
A failed refresh is memoized in `promiseToCachedToken_` and returned to every later caller for up to ~55 minutes, while a valid token sits in `cachedToken_`. `getToken()` now serves the cached token when it is further than TOKEN_EXPIRY_THRESHOLD_MILLIS from expiry, restoring the read path that existed before firebase#2648. The cache read runs before shouldRefresh() because each reads its own Date.now(); in the opposite order a token crossing the threshold between the two reads would satisfy neither. Closes firebase#3234
Contributor
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3234.
Summary
A failed token refresh is memoized in
promiseToCachedToken_and replayed to every later caller forup to ~55 minutes, while a valid token sits in
cachedToken_. Every service authenticating throughAuthorizedHttpClientorAuthorizedHttp2Clientsees a staleapp/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()returnscachedToken_when it is further thanTOKEN_EXPIRY_THRESHOLD_MILLISfromexpiry, rather than a
promiseToCachedToken_that may hold a rejection.hasUsableCachedToken()negates the conditionshouldRefresh()tested before #2648 addedisRefreshingto it, at the same threshold and the same boundary. TheisRefreshingterm cancelsout, 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 ownDate.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,
mainalready serves the same tokenthrough the memoized promise.
Restoring the explicit
return Promise.resolve(this.cachedToken_)sketched on #2645 also fixes this,but moving
isRefreshingback out ofshouldRefresh()breaks theDatabase should gracefully handle errors during token refreshspec. Keeping #2648's structure andadding 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 replacementlands, instead of joining the refresh.
serves the outgoing token while the refresh that replaces it is in flightpins 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 thecommon 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.
FirebaseAppInternalsis not exported API,hasUsableCachedToken()isprivate, andnpm run api-extractorreports no change to anyetc/*.api.mdgolden file.Test coverage
Nine tests in a
describe('with a stuck or failing credential')block intest/unit/app/firebase-app.spec.ts. Five fail againstmain: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:
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— cleannpm run build:tests— cleannpm test— lint clean, 6073 passing, 0 failingnpm run api-extractor— 18/18, no golden file changeDiff: 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 onmain:81 passing / 65 pending / 0 failing on both, with every individual test reporting the same outcome.