Repository navigation
improvement(credentials): keep valid client-credential tokens through refresh outages and pace token retries by failure kind - #9925
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
[High impact] The PR appears safe to merge, with no new actionable issues found. SummaryAdds shared retry rules for client-credential token requests and keeps still-valid tokens available during refresh outages.
|
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 41 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 41 files
Confidence score: 3/5
- In
apps/sim/lib/internal/sailpoint/client.ts, an unreadable 4xx error body can be treated as invalid credentials and delete a still-valid cached token during a refresh outage. Keep body-read failures distinct so the outage fallback can run.
Prompt for AI agents (unresolved issues)
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/internal/sailpoint/client.ts">
<violation number="1" location="apps/sim/lib/internal/sailpoint/client.ts:230">
P2: An unreadable SailPoint error body on a 4xx is misclassified as `invalid_credentials`, so refresh outages can delete a still-valid cached token instead of using outage fallback. Preserve the body-read failure separately and map it as an unavailable/invalid-response failure while retaining the response status and `Retry-After`.
(Based on your team's feedback about provider response body failures.)</violation>
</file>
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
…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
…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.
…rror bodies and replay only coordinated failures locally - A SailPoint token error whose body cannot be read keeps its status and Retry-After, as CrowdStrike's does; caller cancellation still propagates. - A shared-token (Vanta) mint failure is replayed per instance only when the coordinated layer remembers it (replayForMs); a failure it does not remember asks again at once.
c6c8771 to
2a94a62
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
…during the failed refresh
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 41 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
Summary
Makes 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.
Still-valid tokens keep working. Tokens are refreshed early, inside the 5-minute window before expiry. If that refresh fails with an outage or throttling, the cached or stored token is served until 30 s before it expires. That margin is checked again after the failed refresh, so a slow attempt cannot carry the token past it. A refusal of the credentials still fails at once.
Retry timing depends on the kind of failure (shared policy in
lib/credentials/token-request-resilience.ts):Retry-Afteris honoured (seconds or HTTP-date), clamped to 1 s to 5 min. Without one, wait 1 min, doubling up to 5 min.Retry-After is carried on errors from every client-credential minter.
TokenServiceAccountValidationErrorgets an optionalretryAfterMs, read bythrottledRetryAfterMs(429/503 only). It is filled in by:throwForProviderResponse,exchangeClientCredentialsGrant, Shopify and VantaA replayed failure carries an optional
replayForMs, which the per-instance layer honours exactly.Coordinated resolver:
provider_unavailable) gets an additiverefusal: trueflag, so a replay never serves the stale token.throttled: trueflag, so the throttling streak continues across workers and stays separate from an outage streak.Single-flight is unchanged:
coalesceLocallyper instance, the advisory lock across workers.The new fallback warning leaves out the credential id in selector privacy mode, like every other log in
credential-service.ts.Type of Change
Testing
credential-service-cancellation.test.ts,errors.test.tsandclient-credentials-grant.test.ts:helpscout.test.ts).coordinated-client-token.integration.tsandvanta-token.integration.ts, run one file at a time:{code,status}. An outage memo adds onlyconsecutiveFailures, a throttle memo addsconsecutiveFailuresandthrottled, and an unstorable refusal adds onlyrefusal; the old decoders ignore all three. The replay length rides on the existingexpiresAt.lock_timeout(2 s budget + about 1.2 s pause + one 10 s attempt).Checklist