fix: apply expiry buffer before reusing cached tokens#331
fix: apply expiry buffer before reusing cached tokens#331
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughImplements a token expiry safety buffer mechanism in the credentials module. A random buffer value is computed from SDK constants upon token refresh and applied to token expiration checks, causing cached tokens within the buffer window to be proactively refreshed rather than reused. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Related Documentation Checked 8 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
There was a problem hiding this comment.
Pull request overview
This PR updates client-credentials token caching so cached access tokens are treated as expired when they fall within the SDK’s configured expiry threshold + jitter window, reducing the chance of using near-expiry tokens on outbound requests.
Changes:
- Updated
Credentials.getAccessTokenValue()to apply the SDK token-expiry threshold/jitter before reusing a cached token. - Added a regression test ensuring a short-lived token is refreshed on the next call.
- Reused existing expiry-buffer/jitter constants from
SdkConstants.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
credentials/credentials.ts |
Applies expiry threshold+jitter when deciding whether to reuse a cached client-credentials token. |
tests/credentials.test.ts |
Adds a test asserting short-lived cached tokens are proactively refreshed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
credentials/credentials.ts
Outdated
| const tokenExpiryBufferInMs = ( | ||
| SdkConstants.TokenExpiryThresholdBufferInSec + | ||
| (Math.random() * SdkConstants.TokenExpiryJitterInSec) | ||
| ) * 1000; | ||
| const tokenIsValid = !this.accessTokenExpiryDate || ( | ||
| this.accessTokenExpiryDate.getTime() - Date.now() > tokenExpiryBufferInMs | ||
| ); | ||
| if (this.accessToken && tokenIsValid) { |
There was a problem hiding this comment.
tokenExpiryBufferInMs uses Math.random() on every call to getAccessTokenValue(), which can make a cached token flip between “valid” and “expired” across successive calls and cause unnecessary token refreshes. Consider computing/storing the randomized buffer once per fetched token (or pre-adjusting accessTokenExpiryDate when the token is received) so the validity decision is stable for that token.
| import { TelemetryCounters } from "../telemetry/counters"; | ||
| import { TelemetryConfiguration } from "../telemetry/configuration"; | ||
| import { randomUUID } from "crypto"; | ||
| import SdkConstants from "../constants"; |
There was a problem hiding this comment.
This file header states it is auto-generated (“DO NOT EDIT”), but this change introduces a new import. If this file is regenerated as part of the build/release process, these edits may be overwritten; consider updating the generator template/source or adjusting the generation settings/header so the change persists reliably.
| .reply(200, { | ||
| access_token: "short-lived-token", | ||
| expires_in: 120, | ||
| }) |
There was a problem hiding this comment.
This test hardcodes expires_in: 120, which is only “close to expiration” relative to the current buffer/jitter constants. To keep the test resilient if TokenExpiryThresholdBufferInSec/TokenExpiryJitterInSec change, consider stubbing Math.random() and deriving expires_in from the exported constants so the token is guaranteed to fall within the buffer window.
|
Addressed the actionable review comments in commit Implemented:
Validated locally:
On the autogenerated-file/header comment: agreed in principle; this PR is scoped to the runtime bugfix/tests, and generator-template alignment can be tracked separately. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@credentials/credentials.ts`:
- Around line 141-150: The const tokenIsValid declared inside the
CredentialsMethod.ClientCredentials case is not scoped because case clauses fall
through; wrap the entire case body in braces to limit tokenIsValid's scope.
Specifically, in the switch handling CredentialsMethod.ClientCredentials, add a
block { ... } around the logic that computes tokenIsValid (which references
accessTokenExpiryDate and accessTokenExpiryBufferInMs), checks this.accessToken
and tokenIsValid, and returns this.accessToken or calls
this.refreshAccessToken(); this confines the const to that case only.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@credentials/credentials.ts`: - Around line 141-150: The const tokenIsValid declared inside the CredentialsMethod.ClientCredentials case is not scoped because case clauses fall through; wrap the entire case body in braces to limit tokenIsValid's scope. Specifically, in the switch handling CredentialsMethod.ClientCredentials, add a block { ... } around the logic that computes tokenIsValid (which references accessTokenExpiryDate and accessTokenExpiryBufferInMs), checks this.accessToken and tokenIsValid, and returns this.accessToken or calls this.refreshAccessToken(); this confines the const to that case only.credentials/credentials.ts (1)
141-150: Wrapcasebody in a block to scope theconstdeclaration.Biome correctly flags this:
const tokenIsValiddeclared in acaseclause without a block is technically accessible from othercasebranches due to fall-through semantics. Wrap in braces to restrict scope.♻️ Proposed fix
case CredentialsMethod.ClientCredentials: - const tokenIsValid = !this.accessTokenExpiryDate || ( - this.accessTokenExpiryDate.getTime() - Date.now() > this.accessTokenExpiryBufferInMs - ); - if (this.accessToken && tokenIsValid) { - return this.accessToken; - } - - return this.refreshAccessToken(); + { + const tokenIsValid = !this.accessTokenExpiryDate || ( + this.accessTokenExpiryDate.getTime() - Date.now() > this.accessTokenExpiryBufferInMs + ); + if (this.accessToken && tokenIsValid) { + return this.accessToken; + } + + return this.refreshAccessToken(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@credentials/credentials.ts` around lines 141 - 150, The const tokenIsValid declared inside the CredentialsMethod.ClientCredentials case is not scoped because case clauses fall through; wrap the entire case body in braces to limit tokenIsValid's scope. Specifically, in the switch handling CredentialsMethod.ClientCredentials, add a block { ... } around the logic that computes tokenIsValid (which references accessTokenExpiryDate and accessTokenExpiryBufferInMs), checks this.accessToken and tokenIsValid, and returns this.accessToken or calls this.refreshAccessToken(); this confines the const to that case only.
|
Addressed the failing test in commit fbc241b. Root cause:
Fix applied:
Validated locally:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #331 +/- ##
==========================================
+ Coverage 89.67% 89.70% +0.02%
==========================================
Files 25 25
Lines 1492 1496 +4
Branches 279 280 +1
==========================================
+ Hits 1338 1342 +4
Misses 94 94
Partials 60 60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Followed up on an additional Implemented in commit
Validated with:
|
2 participants
Summary
Applies the SDK token-expiry threshold/jitter when deciding whether a cached client-credentials token can be reused.
Changes
expires_in: 120) should be refreshed on the next callgetAccessTokenValue()to treat tokens within the configured threshold/jitter window as expiredSdkConstants.TokenExpiryThresholdBufferInSecandSdkConstants.TokenExpiryJitterInSecPotential Breaking Change
Fixes #330
Summary by CodeRabbit
New Features
Tests