feat(e2e): add retry for transient BAPI errors in integration tests#8081
Open
jacekradko wants to merge 12 commits intomainfrom
Open
feat(e2e): add retry for transient BAPI errors in integration tests#8081jacekradko wants to merge 12 commits intomainfrom
jacekradko wants to merge 12 commits intomainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
jacekradko
changed the title
Contributor
📝 WalkthroughWalkthroughA new retry utility was added at 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
jacekradko
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration/testUtils/retryableClerkClient.ts`:
- Around line 1-92: Add unit tests for the retry utility focusing on behavior in
createProxy/retryOnFailure/getRetryDelay/withRetry: write tests that mock a
ClerkClient method to return a rejected Promise with status 429/502/503/504 and
assert it is retried up to MAX_RETRIES (use jest.spyOn on the mocked method to
count calls), tests that an error with a non-retryable status does not retry, a
test that when isClerkAPIResponseError-style error includes retryAfter the delay
uses retryAfter seconds (use jest.useFakeTimers and advanceTimersByTime to
validate wait), a test that once MAX_RETRIES is reached the original error is
thrown, and a test that synchronous (non-Promise) methods are passed through
unwrapped; also assert console.warn/log calls (or printRetrySummary output) to
verify retry reporting. Use the exported withRetry to wrap a thin mocked client
and reference the functions/classes by name: withRetry, createProxy,
retryOnFailure, getRetryDelay, and printRetrySummary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 653cffb3-c354-4e65-b206-18f8dce194e6
📒 Files selected for processing (1)
integration/testUtils/retryableClerkClient.ts
Comment on lines
+1
to
+92
| import type { ClerkClient } from '@clerk/backend'; | ||
| import { isClerkAPIResponseError } from '@clerk/shared/error'; | ||
|
|
||
| const MAX_RETRIES = 5; | ||
| const BASE_DELAY_MS = 1000; | ||
| const JITTER_MAX_MS = 500; | ||
| const MAX_RETRY_DELAY_MS = 30_000; | ||
| const RETRYABLE_STATUS_CODES = new Set([429, 502, 503, 504]); | ||
|
|
||
| const retryStats = { totalRetries: 0, callsRetried: new Set<string>() }; | ||
|
|
||
| function sleep(ms: number): Promise<void> { | ||
| return new Promise(resolve => setTimeout(resolve, ms)); | ||
| } | ||
|
|
||
| function getRetryDelay(error: unknown, attempt: number): number { | ||
| if (isClerkAPIResponseError(error) && typeof error.retryAfter === 'number') { | ||
| return Math.min(error.retryAfter * 1000, MAX_RETRY_DELAY_MS); | ||
| } | ||
| return BASE_DELAY_MS * Math.pow(2, attempt) + Math.random() * JITTER_MAX_MS; | ||
| } | ||
|
|
||
| function recordRetry(path: string): void { | ||
| retryStats.totalRetries++; | ||
| retryStats.callsRetried.add(path); | ||
| } | ||
|
|
||
| export function printRetrySummary(): void { | ||
| if (retryStats.totalRetries === 0) { | ||
| console.log('[Retry] No retries occurred during this run.'); | ||
| return; | ||
| } | ||
| const methods = [...retryStats.callsRetried].join(', '); | ||
| console.warn( | ||
| `[Retry] Summary: ${retryStats.totalRetries} retries across ${retryStats.callsRetried.size} API calls (${methods})`, | ||
| ); | ||
| } | ||
|
|
||
| async function retryOnFailure<T>(firstAttempt: Promise<T>, fn: () => Promise<T>, path: string): Promise<T> { | ||
| for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) { | ||
| try { | ||
| return attempt === 0 ? await firstAttempt : await fn(); | ||
| } catch (error) { | ||
| const isRetryable = isClerkAPIResponseError(error) && RETRYABLE_STATUS_CODES.has(error.status); | ||
| if (!isRetryable || attempt === MAX_RETRIES) { | ||
| throw error; | ||
| } | ||
| recordRetry(path); | ||
| const delayMs = getRetryDelay(error, attempt); | ||
| console.warn( | ||
| `[Retry] ${error.status} for ${path}, attempt ${attempt + 1}/${MAX_RETRIES}, waiting ${Math.round(delayMs)}ms`, | ||
| ); | ||
| await sleep(delayMs); | ||
| } | ||
| } | ||
| // Unreachable, but satisfies TypeScript | ||
| throw new Error('Unreachable'); | ||
| } | ||
|
|
||
| function createProxy(target: unknown, path: string[] = []): unknown { | ||
| if (target === null || (typeof target !== 'object' && typeof target !== 'function')) { | ||
| return target; | ||
| } | ||
|
|
||
| return new Proxy(target as object, { | ||
| get(obj, prop, receiver) { | ||
| if (typeof prop === 'symbol') { | ||
| return Reflect.get(obj, prop, receiver); | ||
| } | ||
| const value = Reflect.get(obj, prop, receiver); | ||
| if (typeof value === 'function') { | ||
| return (...args: unknown[]) => { | ||
| const result = value.apply(obj, args); | ||
| // Only wrap promises (async API calls), pass through sync returns | ||
| if (result && typeof result === 'object' && typeof result.then === 'function') { | ||
| const fullPath = [...path, prop].join('.'); | ||
| return retryOnFailure(result, () => value.apply(obj, args), fullPath); | ||
| } | ||
| return result; | ||
| }; | ||
| } | ||
| if (typeof value === 'object' && value !== null) { | ||
| return createProxy(value, [...path, prop]); | ||
| } | ||
| return value; | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| export function withRetry(client: ClerkClient): ClerkClient { | ||
| return createProxy(client) as ClerkClient; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if tests exist for retryableClerkClient
fd -t f 'retryableClerkClient' --extension ts --extension test.ts
rg -l 'retryableClerkClient|withRetry' --glob '*test*' --glob '*spec*'Repository: clerk/javascript
Length of output: 105
Add tests for the retry utility.
This new utility contains significant retry logic (exponential backoff, retry conditions, status code filtering, proxy-based interception) that should have test coverage. Consider tests for:
- Retry behavior on 429/502/503/504 status codes
- No retry on other error codes
retryAfterheader handling- Max retry limit enforcement
- Proxy correctly wraps async calls but passes through sync calls
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@integration/testUtils/retryableClerkClient.ts` around lines 1 - 92, Add unit
tests for the retry utility focusing on behavior in
createProxy/retryOnFailure/getRetryDelay/withRetry: write tests that mock a
ClerkClient method to return a rejected Promise with status 429/502/503/504 and
assert it is retried up to MAX_RETRIES (use jest.spyOn on the mocked method to
count calls), tests that an error with a non-retryable status does not retry, a
test that when isClerkAPIResponseError-style error includes retryAfter the delay
uses retryAfter seconds (use jest.useFakeTimers and advanceTimersByTime to
validate wait), a test that once MAX_RETRIES is reached the original error is
thrown, and a test that synchronous (non-Promise) methods are passed through
unwrapped; also assert console.warn/log calls (or printRetrySummary output) to
verify retry reporting. Use the exported withRetry to wrap a thin mocked client
and reference the functions/classes by name: withRetry, createProxy,
retryOnFailure, getRetryDelay, and printRetrySummary.
tmilewski
approved these changes
Mar 17, 2026
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
2 participants
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.
Summary
withRetry) around theClerkClientused in integration test setup/teardownRetry-Afterheaders (capped at 30s), max 5 retriescreateClerkClientcall site intestUtils/index.ts— zero changes to individual service files or testsMotivation
Multiple concurrent CI runs share the same Clerk development instances, causing 429 rate limit errors and occasional 5xx transient failures (e.g. 503 Service Unavailable) that fail integration tests. Instead of failing, tests will now automatically retry with backoff, logging
[Retry] 503 for users.createUser, attempt 1/5, waiting 1234msfor CI observability.Test plan
pnpm buildpasses[Retry]messages to confirm the wrapper activates on real errorsSummary by CodeRabbit