chore(backend): Fix JWKS cache #3321
Conversation
🦋 Changeset detectedLatest commit: 698cc4c The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| if (jwksCacheTtlInMs >= 0) { | ||
| setTimeout(() => { | ||
| if (jwk) { | ||
| delete cache[jwk.kid]; | ||
| } else { | ||
| cache = {}; | ||
| } | ||
| }, jwksCacheTtlInMs); | ||
| } |
There was a problem hiding this comment.
We already have a hard-coded 5 minute TTL for the values in the jwks cache. This setTimeout appears to be resulting in a scenario where the cache is cleared by the timeout here immediately after the 5 minute TTL is hit, so the cache remains empty for the remainder of the 5 minute TTL.
| @@ -116,11 +106,10 @@ export async function loadClerkJWKFromRemote({ | |||
| apiUrl = API_URL, | |||
| apiVersion = API_VERSION, | |||
| kid, | |||
| jwksCacheTtlInMs = JWKS_CACHE_TTL_MS, | |||
There was a problem hiding this comment.
This doesn't seem to be functionally doing anything if beyond 5 minutes (except for introducing this bug), as we have the hardcoded 5 minute max cache time.
| @@ -184,5 +185,38 @@ export default (QUnit: QUnit) => { | |||
| } | |||
| } | |||
| }); | |||
|
|
|||
| test('cache TTLs do not conflict', async assert => { | |||
There was a problem hiding this comment.
This a really specific test case for the observed bug. I confirmed that this test fails on main and passes on this branch. Not sure if it's worthwhile to keep around for a long time with the removal of the timer-based cache eviction. 🤔
There was a problem hiding this comment.
☁️ The reason behind adding the MAX_CACHE_LAST_UPDATED_AT_SECONDS to be 5 minutes is to refresh the cache if the requested kid is missing from the cache and the cache is older than 5 minutes. This is used to cover cases where the kid is missing in the cache but exists in the jwks endpoint.
There may be some issue in the existing code and it may not be clear, but i think that we shouldn't deprecate the jwksCacheTtlInMs. Those two are meant to be used for different reasons and i think that we should always provide a way to configure the cache TTL instead of hard-coding the value.
Therefore i believe we should revert this PR and apply the appropriate fix to resolve the issue with the setTimeout.
Also, we are using a similar approach in our ClerkJS to handle session token cache, should we avoid using the setTimeout there and clear the cache if it's expired to avoid a similar race condition?
cc: @BRKalow , @nikosdouvlis
|
|
||
| if (isExpired) { | ||
| cache = {}; | ||
| } |
There was a problem hiding this comment.
I think we should not mutate the cache in this function. We should use the result of this function and clear the cache in the same place where we set it.
With this change we have converted a pure function that is supposed to return a boolean to a function with side-effects. This may cause issues in the future. I think we need to keep separate the pure logic and the functions used to determine the state from the functions mutating it.
cc: @BRKalow , @nikosdouvlis
There was a problem hiding this comment.
I agree that a predicate function shouldn't mutate external state
| @@ -208,7 +196,19 @@ async function fetchJWKSFromBAPI(apiUrl: string, key: string, apiVersion: string | |||
| } | |||
|
|
|||
| function cacheHasExpired() { | |||
| return Date.now() - lastUpdatedAt >= MAX_CACHE_LAST_UPDATED_AT_SECONDS * 1000; | |||
There was a problem hiding this comment.
@dimkl do you remember why this was capped at 5minutes? Also, why is this defined in seconds but the other constant defined in ms? We should probably align the two in any case
There was a problem hiding this comment.
@nikosdouvlis I agree that we should align all of them to be in MS. It was capped at 5 minutes because we considered that it would be safe to expect that in 5 minutes all the systems related to the JWKs would have been updated to include a new instance or an update to the JWKs initiated by our Clerk API (eg Edge cache, read-only replicas, ...)
There was a problem hiding this comment.
That makes sense but we probably need to document jwksCacheTtlInMs better if we don't deprecate it as the prop is somewhat confusing
Description
Fixes a bug in the JWKS cache where it was getting cleared but not treated as expired.
Checklist
npm testruns as expected.npm run buildruns as expected.Type of change