From 0b529ec7903f8fb5c11015c5ecc76554982c85bf Mon Sep 17 00:00:00 2001 From: Max Yinger Date: Fri, 14 Aug 2026 15:11:20 -0600 Subject: [PATCH] test(e2e): bound BAPI retries and make fake user/org teardown best-effort Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/bounded-bapi-retries.md | 2 + .../__tests__/retryableClerkClient.test.ts | 50 +++++++++++---- .../testUtils/__tests__/usersService.test.ts | 63 +++++++++++++++++++ integration/testUtils/retryableClerkClient.ts | 17 ++++- integration/testUtils/usersService.ts | 48 ++++++++++++-- 5 files changed, 161 insertions(+), 19 deletions(-) create mode 100644 .changeset/bounded-bapi-retries.md create mode 100644 integration/testUtils/__tests__/usersService.test.ts diff --git a/.changeset/bounded-bapi-retries.md b/.changeset/bounded-bapi-retries.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/bounded-bapi-retries.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/integration/testUtils/__tests__/retryableClerkClient.test.ts b/integration/testUtils/__tests__/retryableClerkClient.test.ts index 4fa5d718475..efe63145ee2 100644 --- a/integration/testUtils/__tests__/retryableClerkClient.test.ts +++ b/integration/testUtils/__tests__/retryableClerkClient.test.ts @@ -47,26 +47,50 @@ describe('withRetry', () => { }); describe('retryOnFailure — retryable status codes', () => { - it.each([429, 502, 503, 504])('retries on status %d up to MAX_RETRIES then throws', async status => { - const error = makeClerkAPIError(status); + it.each([429, 502, 503, 504])( + 'retries on status %d until the retry budget is exhausted, then throws', + async status => { + vi.spyOn(Math, 'random').mockReturnValue(0); + const error = makeClerkAPIError(status); + const mock = mockDeferredReject(error); + const client = makeMockClient({ getUser: mock }); + const wrapped = withRetry(client); + + const promise = (wrapped.users as any).getUser('user_123'); + + // Attach handler before advancing timers to avoid unhandled rejection + const expectation = expect(promise).rejects.toBe(error); + + // Backoff of 1s + 2s + 4s + 8s = 15s elapsed; the next 16s delay would exceed the 20s budget + for (const delayMs of [1000, 2000, 4000, 8000]) { + await vi.advanceTimersByTimeAsync(delayMs); + } + await vi.advanceTimersByTimeAsync(0); + + await expectation; + + // 1 initial call + 4 retries = 5 total + expect(mock).toHaveBeenCalledTimes(5); + }, + ); + + it('gives up once the elapsed time plus the next delay would exceed the total budget', async () => { + const error = makeClerkAPIError(429, { retryAfter: 60 }); const mock = mockDeferredReject(error); const client = makeMockClient({ getUser: mock }); const wrapped = withRetry(client); const promise = (wrapped.users as any).getUser('user_123'); - - // Attach handler before advancing timers to avoid unhandled rejection const expectation = expect(promise).rejects.toBe(error); - // Advance through all 6 attempts (initial + 5 retries) - for (let i = 0; i < 6; i++) { - await vi.advanceTimersByTimeAsync(60_000); - } + // Two capped 10s waits exhaust the 20s budget, so the third failure is not retried + await vi.advanceTimersByTimeAsync(10_000); + await vi.advanceTimersByTimeAsync(10_000); + await vi.advanceTimersByTimeAsync(0); await expectation; - // 1 initial call + 5 retries = 6 total - expect(mock).toHaveBeenCalledTimes(6); + expect(mock).toHaveBeenCalledTimes(3); }); it('succeeds on retry after transient failure', async () => { @@ -158,7 +182,7 @@ describe('withRetry', () => { expect(mock).toHaveBeenCalledTimes(2); }); - it('caps retryAfter delay at MAX_RETRY_DELAY_MS (30s)', async () => { + it('caps retryAfter delay at MAX_RETRY_DELAY_MS (10s)', async () => { const error = makeClerkAPIError(429, { retryAfter: 60 }); const mock = vi .fn() @@ -169,8 +193,8 @@ describe('withRetry', () => { const promise = (wrapped.users as any).getUser('user_123'); - // Even though retryAfter is 60s, delay should be capped at 30s - await vi.advanceTimersByTimeAsync(30_000); + // Even though retryAfter is 60s, delay should be capped at 10s + await vi.advanceTimersByTimeAsync(10_000); await vi.advanceTimersByTimeAsync(0); await expect(promise).resolves.toEqual({ id: 'user_123' }); diff --git a/integration/testUtils/__tests__/usersService.test.ts b/integration/testUtils/__tests__/usersService.test.ts new file mode 100644 index 00000000000..2706bcf9fcb --- /dev/null +++ b/integration/testUtils/__tests__/usersService.test.ts @@ -0,0 +1,63 @@ +import type { ClerkClient } from '@clerk/backend'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { createUserService } from '../usersService'; + +const fakePlaywrightTest = { + info: () => ({ file: 'basic.test.ts', line: 24, title: 'a test', titlePath: ['a test'] }), +}; + +function makeMockClient(overrides: { users?: Record; organizations?: Record } = {}) { + return { + users: { + getUserList: vi.fn().mockResolvedValue({ data: [{ id: 'user_123' }] }), + deleteUser: vi.fn().mockResolvedValue({}), + ...overrides.users, + }, + organizations: { + createOrganization: vi.fn().mockResolvedValue({ id: 'org_123' }), + deleteOrganization: vi.fn().mockResolvedValue({}), + ...overrides.organizations, + }, + } as unknown as ClerkClient; +} + +describe('best-effort teardown', () => { + beforeEach(() => { + vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); + vi.spyOn(console, 'log').mockImplementation(() => {}); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + + it('resolves when deleting the user fails', async () => { + const client = makeMockClient({ users: { deleteUser: vi.fn().mockRejectedValue(new Error('429')) } }); + const fakeUser = createUserService(client).createFakeUser(fakePlaywrightTest); + + await expect(fakeUser.deleteIfExists()).resolves.toBeUndefined(); + }); + + it('resolves when deleting the user does not finish in time', async () => { + vi.useFakeTimers(); + const client = makeMockClient({ users: { deleteUser: vi.fn(() => new Promise(() => {})) } }); + const fakeUser = createUserService(client).createFakeUser(fakePlaywrightTest); + + const promise = fakeUser.deleteIfExists(); + await vi.advanceTimersByTimeAsync(5_000); + + await expect(promise).resolves.toBeUndefined(); + }); + + it('resolves when deleting the organization fails', async () => { + const client = makeMockClient({ + organizations: { deleteOrganization: vi.fn().mockRejectedValue(new Error('429')) }, + }); + const fakeOrganization = await createUserService(client).createFakeOrganization('user_123'); + + await expect(fakeOrganization.delete()).resolves.toBeUndefined(); + }); +}); diff --git a/integration/testUtils/retryableClerkClient.ts b/integration/testUtils/retryableClerkClient.ts index fc2ccb40920..1beb0db2c3f 100644 --- a/integration/testUtils/retryableClerkClient.ts +++ b/integration/testUtils/retryableClerkClient.ts @@ -4,7 +4,12 @@ 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 MAX_RETRY_DELAY_MS = 10_000; +/** + * Playwright's default test/hook timeout is 30s, so a single call must never be able to + * out-wait it — otherwise a rate limited instance surfaces as an opaque hook timeout. + */ +const MAX_TOTAL_RETRY_MS = 20_000; const RETRYABLE_STATUS_CODES = new Set([429, 502, 503, 504]); const retryStats = { totalRetries: 0, callsRetried: new Set() }; @@ -38,6 +43,7 @@ export function printRetrySummary(): void { } async function retryOnFailure(firstAttempt: Promise, fn: () => Promise, path: string): Promise { + const startedAt = Date.now(); for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) { try { return attempt === 0 ? await firstAttempt : await fn(); @@ -46,8 +52,15 @@ async function retryOnFailure(firstAttempt: Promise, fn: () => Promise, if (!isRetryable || attempt === MAX_RETRIES) { throw error; } - recordRetry(path); const delayMs = getRetryDelay(error, attempt); + const elapsedMs = Date.now() - startedAt; + if (elapsedMs + delayMs > MAX_TOTAL_RETRY_MS) { + console.warn( + `[Retry] ${error.status} for ${path}, giving up after ${Math.round(elapsedMs)}ms (retry budget of ${MAX_TOTAL_RETRY_MS}ms exhausted)`, + ); + throw error; + } + recordRetry(path); console.warn( `[Retry] ${error.status} for ${path}, attempt ${attempt + 1}/${MAX_RETRIES}, waiting ${Math.round(delayMs)}ms`, ); diff --git a/integration/testUtils/usersService.ts b/integration/testUtils/usersService.ts index d24973ed4bb..7736c1cec8c 100644 --- a/integration/testUtils/usersService.ts +++ b/integration/testUtils/usersService.ts @@ -1,10 +1,42 @@ -import type { APIKey, ClerkClient, Organization, User } from '@clerk/backend'; +import type { APIKey, ClerkClient, User } from '@clerk/backend'; import { faker } from '@faker-js/faker'; import type { TestInfo } from '@playwright/test'; import { fakerPassword, hash } from '../models/helpers'; import { getE2ERunMarker } from './e2eRun'; +/** + * Leftover users are reaped by the scheduled `Cleanup e2e instances` workflow, so a slow or failing + * teardown must never fail the suite (or out-wait Playwright's 30s hook timeout) on its own. + */ +const TEARDOWN_TIMEOUT_MS = 5_000; + +async function bestEffortCleanup(operation: string, fn: () => Promise): Promise { + let timer: ReturnType | undefined; + const work = fn().then(() => 'done' as const); + work.catch(() => {}); + + try { + const result = await Promise.race([ + work, + new Promise<'timeout'>(resolve => { + timer = setTimeout(() => resolve('timeout'), TEARDOWN_TIMEOUT_MS); + }), + ]); + if (result === 'timeout') { + console.warn( + `[usersService] ${operation} did not finish within ${TEARDOWN_TIMEOUT_MS}ms, leaving it to the scheduled e2e cleanup`, + ); + } + } catch (e: any) { + console.warn( + `[usersService] ${operation} failed (${e?.status ?? 'unknown status'}: ${e?.message}), leaving it to the scheduled e2e cleanup`, + ); + } finally { + clearTimeout(timer); + } +} + async function withErrorLogging(operation: string, fn: () => Promise): Promise { try { return await fn(); @@ -68,6 +100,9 @@ export type FakeUser = { username?: string; phoneNumber?: string; privateMetadata?: UserPrivateMetadata; + /** + * Best-effort cleanup: resolves even if the deletion fails or times out. + */ deleteIfExists: () => Promise; }; @@ -76,7 +111,10 @@ export type FakeUserWithEmail = FakeUser & { email: string }; export type FakeOrganization = { name: string; organization: { id: string }; - delete: () => Promise; + /** + * Best-effort cleanup: resolves even if the deletion fails or times out. + */ + delete: () => Promise; }; export type FakeAPIKey = { @@ -154,7 +192,7 @@ export const createUserService = (clerkClient: ClerkClient) => { line, ...(runMarker ? { e2eRunMarker: runMarker } : {}), }, - deleteIfExists: () => self.deleteIfExists({ email, phoneNumber }), + deleteIfExists: () => bestEffortCleanup('deleteIfExists', () => self.deleteIfExists({ email, phoneNumber })), }; }, createBapiUser: async fakeUser => { @@ -246,7 +284,9 @@ export const createUserService = (clerkClient: ClerkClient) => { name, organization, delete: () => - withErrorLogging('deleteOrganization', () => clerkClient.organizations.deleteOrganization(organization.id)), + bestEffortCleanup('deleteOrganization', () => + withErrorLogging('deleteOrganization', () => clerkClient.organizations.deleteOrganization(organization.id)), + ), } satisfies FakeOrganization; }, createFakeAPIKey: async (userId: string) => {