GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — improvement(credentials): keep valid client-credential tokens through refresh outages and pace token retries by failure kind by waleedlatif1 · Pull Request #9925 · simstudioai/sim Conversation
Collaborator
SummaryMakes client-credential token requests more resilient, following the patterns used by MSAL, Entra's resilience guidance and botocore. The changes apply to the per-instance mint cache, the shared coordinated resolver (Shopify, Linear, Atlassian, Vanta) and Vanta itself.
Type of Change
Testing
Checklist
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
…ilures A 429 or 503 token answer now records its Retry-After (delay-seconds or HTTP-date, RFC 9110 section 10.2.3) as an optional retryAfterMs on TokenServiceAccountValidationError, through throwForProviderResponse and the shared client-credentials grant, whose provider classifier receives it. …h outages The per-instance client-credential mint cache now: - serves a cached token that is inside its five-minute early-refresh window but not expired when the refresh fails with an outage or throttling, as MSAL, Azure.Core and botocore do; a refusal of the credentials still fails - retries a quick transient mint failure once after about a second - replays failures by kind instead of a flat 30 s: a refusal 30 s, a throttled answer for its Retry-After (1 s to 5 min), an outage 5 s growing to 30 s while it continues The policy lives in lib/credentials/token-request-resilience.ts so the coordinated resolver can share it. …he coordinated resolver The coordinated client-credentials resolver (Shopify, Linear, Atlassian, Vanta) now applies the same policy as the per-instance mint cache, through the shared row every worker reads: - a quick outage is retried once under the application lock, within the 15 s its waiters allow - a refusal is remembered 30 s, a throttled answer for its Retry-After (1 s to 5 min, Vanta's now included), an outage 5 s growing to 30 s while it lasts - while an outage or throttling is remembered, the stored token for the same secret is served until 30 s before it expires; a refusal fails at once Rolling deploys stay safe: a refusal is stored exactly as before, and outages add only a consecutiveFailures key that previous decoders ignore (both prior decoders are pinned in the integration tests). The replay length rides on the row's existing expiresAt. Expired memos are kept five more minutes so the next failure can extend the backoff; an older build's sweep only resets it. …emembered failures without the lock - A 429 that names no Retry-After is replayed for at least a minute, doubling up to the five-minute cap, instead of the 5 s outage curve. - The outage backoff never undercuts Entra's 5 s first-retry floor through jitter. - Vanta asks for a token at most once per 15 s while exchanges fail (no in-request retry, replay floored), within its documented 5 tokens a minute, through a new optional minExchangeIntervalMs on the coordinated resolver. - A remembered failure and the still-valid fallback are served from committed rows before the application lock is taken, so a refresh outage no longer serializes every resolution behind the lock. - retryAfterMs now only carries a provider's Retry-After; a replayed memo reports its remainder as the additive replayForMs, which the per-instance layer honors exactly. - One decode/verify path for stored tokens; the stored-failure permanence check uses isPermanentTokenFailureCode instead of building an error. …llback gaps - Carry the token endpoint's Retry-After on 429/503 failures from every bespoke client-credential minter (Box, Zoom, DocuSign, NetSuite, Help Scout, Ramp, Jama, Zendesk, Salesforce, Zoho Desk) and through the CrowdStrike, SailPoint and ZoomInfo client errors, so their throttled answers wait as long as the provider asks instead of the fixed backoff. - Recheck the 30 s expiry margin when the per-instance cache falls back to a still-valid token after a slow failed refresh. - Map an unreadable Vanta token body to provider_unavailable so it is remembered for every worker and paced at 15 s like any failed exchange. - Remember a refusal whose code the shared failure row cannot store with an additive `refusal` flag, so a replay never serves the stale token. - Rename the credentials helper to throttledRetryAfterMs so it no longer shares a name with providers/retry.ts. …d redact the fallback log in selector mode
|
There was an error while loading. Please reload this page.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
[High impact] The PR changes retry and token-fallback behavior across several credential workflows. Fix SailPoint's handling of unreadable 429 bodies before merging. Findings
SummaryThe PR keeps still-valid client-credential tokens available through refresh outages and gives refusals, throttling, and outages different retry delays.
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
All reported issues were addressed across 41 files
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…ls, and keep throttle delays through SailPoint and CrowdStrike - A refusal of the credentials drops the per-instance cached token and expires the shared token minted with that secret, so a later outage cannot serve it again. - A previous build's provider_unavailable memo with a non-transient 4xx status replays as a refusal instead of serving the stored token. - The SailPoint saved-credential minter returns the first 429 with its Retry-After instead of retrying until the exchange timeout swallows it. - A CrowdStrike error whose body cannot be read keeps its status and Retry-After. - Vanta logs a malformed or oversized token response apart from an unreadable stream.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@cubic-dev-ai review this PR |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| clientSecret, | ||
| options?.signal ? AbortSignal.any([options.signal, timeout]) : timeout | ||
| options?.signal ? AbortSignal.any([options.signal, timeout]) : timeout, | ||
| { maxThrottleRetries: 0 } |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
SailPoint loses throttle delays
With maxThrottleRetries: 0, the first 429 reaches readBoundedBody. If its body is cut off or exceeds the size limit, the read throws before SailPointAuthError preserves the status and Retry-After. The minter reports a 502 instead, so saved credentials retry after about a second rather than waiting for the provider's delay. This can prolong throttling and keep workflows failing.
Catch body-read failures on non-success responses and preserve the status and header, while still rethrowing caller cancellation.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 41 files
Confidence score: 3/5
credential-service.ts, a local refresh-failure memo can keep returning stale credentials after another worker has refreshed the shared token, causing avoidable auth failures. Check the coordinated token before honoring the memo.client.ts, if reading a 429/503 response body fails, the error loses Retry-After, so the resolver may retry without the server's backoff. Preserve the status and header when constructing SailPointAuthError even if the body read fails.
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/oauth/credential-service.ts">
<violation number="1" location="apps/sim/lib/oauth/credential-service.ts:798">
P2: This local failure memo can hide a recovered shared application token: another credential or worker may successfully refresh the coordinated token, but this credential returns its stale memo before consulting the coordinated resolver. Restrict this memo lookup to non-`sharedToken` providers, whose tokens are actually cached here, and let the coordinated layer pace shared-token failures.</violation>
</file>
<file name="apps/sim/lib/internal/sailpoint/client.ts">
<violation number="1" location="apps/sim/lib/internal/sailpoint/client.ts:237">
P2: SailPoint still loses `Retry-After` when a 429/503 error body cannot be read. Catch the error-body read for non-success responses and construct `SailPointAuthError` from the status and header anyway, so the resolver can apply the provider's backoff.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| const stillValid = () => | ||
| cached && cached.expiresAtMs - Date.now() > TOKEN_EXPIRY_MARGIN_MS ? cached : undefined | ||
|
|
||
| const failedEntry = clientCredentialMintFailureCache.get(cacheIdentity) |
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
P2: This local failure memo can hide a recovered shared application token: another credential or worker may successfully refresh the coordinated token, but this credential returns its stale memo before consulting the coordinated resolver. Restrict this memo lookup to non-sharedToken providers, whose tokens are actually cached here, and let the coordinated layer pace shared-token failures.
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apps/sim/lib/oauth/credential-service.ts, line 798:
<comment>This local failure memo can hide a recovered shared application token: another credential or worker may successfully refresh the coordinated token, but this credential returns its stale memo before consulting the coordinated resolver. Restrict this memo lookup to non-`sharedToken` providers, whose tokens are actually cached here, and let the coordinated layer pace shared-token failures.</comment>
<file context>
@@ -761,29 +786,30 @@ async function resolveMintedServiceAccountToken(
+ const stillValid = () =>
+ cached && cached.expiresAtMs - Date.now() > TOKEN_EXPIRY_MARGIN_MS ? cached : undefined
+
+ const failedEntry = clientCredentialMintFailureCache.get(cacheIdentity)
+ const failed = failedEntry?.secretFingerprint === secretFingerprint ? failedEntry : undefined
+ if (failed && Date.now() < failed.retryAtMs) {
</file context>
| const failedEntry = clientCredentialMintFailureCache.get(cacheIdentity) | |
| const failedEntry = sharedToken ? undefined : clientCredentialMintFailureCache.get(cacheIdentity) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| getSailPointErrorMessage(data, 'Failed to authenticate with SailPoint'), | ||
| response.status | ||
| response.status, | ||
| throttledRetryAfterMs(response) |
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
P2: SailPoint still loses Retry-After when a 429/503 error body cannot be read. Catch the error-body read for non-success responses and construct SailPointAuthError from the status and header anyway, so the resolver can apply the provider's backoff.
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apps/sim/lib/internal/sailpoint/client.ts, line 237:
<comment>SailPoint still loses `Retry-After` when a 429/503 error body cannot be read. Catch the error-body read for non-success responses and construct `SailPointAuthError` from the status and header anyway, so the resolver can apply the provider's backoff.</comment>
<file context>
@@ -228,7 +233,8 @@ export async function requestSailPointToken(
getSailPointErrorMessage(data, 'Failed to authenticate with SailPoint'),
- response.status
+ response.status,
+ throttledRetryAfterMs(response)
)
}
</file context>
Sorry, something went wrong.
There was an error while loading. Please reload this page.
d014680e
Deployed
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.