test(e2e): bound BAPI retries and make fake user/org teardown best-effort

.changeset/bounded-bapi-retries.md

@@ -0,0 +1,2 @@
+---
+---

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' });

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<string, unknown>; organizations?: Record<string, unknown> } = {}) {
+  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();
+  });
+});

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<string>() };
@@ -38,6 +43,7 @@ export function printRetrySummary(): void {
 }
 
 async function retryOnFailure<T>(firstAttempt: Promise<T>, fn: () => Promise<T>, path: string): Promise<T> {
+  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<T>(firstAttempt: Promise<T>, fn: () => Promise<T>,
       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`,
       );

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<unknown>): Promise<void> {
+  let timer: ReturnType<typeof setTimeout> | 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<T>(operation: string, fn: () => Promise<T>): Promise<T> {
   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<void>;
 };
 
@@ -76,7 +111,10 @@ export type FakeUserWithEmail = FakeUser & { email: string };
 export type FakeOrganization = {
   name: string;
   organization: { id: string };
-  delete: () => Promise<Organization>;
+  /**
+   * Best-effort cleanup: resolves even if the deletion fails or times out.
+   */
+  delete: () => Promise<void>;
 };
 
 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) => {