fix: skip retries for non-retryable errors in retryWithBackoff (#1082)

Add shouldRetry predicate to RetryOptions so callers can abort retries
for errors that will never succeed (e.g. 401 WorkflowValidationSkipError).

Previously, retryWithBackoff retried all errors blindly, wasting ~35s on
deterministic failures like workflow validation 401s.

Fixes #1081

Co-authored-by: Claude <noreply@anthropic.com>

src/github/token.ts

@@ -141,8 +141,11 @@ export async function setupGitHubToken(): Promise<string> {
   const permissions = parseAdditionalPermissions();
 
   console.log("Exchanging OIDC token for app token...");
-  const appToken = await retryWithBackoff(() =>
-    exchangeForAppToken(oidcToken, permissions),
+  const appToken = await retryWithBackoff(
+    () => exchangeForAppToken(oidcToken, permissions),
+    {
+      shouldRetry: (error) => !(error instanceof WorkflowValidationSkipError),
+    },
   );
   console.log("App token successfully obtained");
 

src/utils/retry.ts

@@ -3,6 +3,7 @@ export type RetryOptions = {
   initialDelayMs?: number;
   maxDelayMs?: number;
   backoffFactor?: number;
+  shouldRetry?: (error: Error) => boolean;
 };
 
 export async function retryWithBackoff<T>(
@@ -14,6 +15,7 @@ export async function retryWithBackoff<T>(
     initialDelayMs = 5000,
     maxDelayMs = 20000,
     backoffFactor = 2,
+    shouldRetry,
   } = options;
 
   let delayMs = initialDelayMs;
@@ -27,6 +29,11 @@ export async function retryWithBackoff<T>(
       lastError = error instanceof Error ? error : new Error(String(error));
       console.error(`Attempt ${attempt} failed:`, lastError.message);
 
+      if (shouldRetry && !shouldRetry(lastError)) {
+        console.error("Error is not retryable, giving up immediately");
+        throw lastError;
+      }
+
       if (attempt < maxAttempts) {
         console.log(`Retrying in ${delayMs / 1000} seconds...`);
         await new Promise((resolve) => setTimeout(resolve, delayMs));

test/retry.test.ts

@@ -0,0 +1,120 @@
+import { describe, it, expect, beforeEach, afterEach, mock } from "bun:test";
+import { retryWithBackoff } from "../src/utils/retry";
+
+describe("retryWithBackoff", () => {
+  let originalConsoleLog: typeof console.log;
+  let originalConsoleError: typeof console.error;
+
+  beforeEach(() => {
+    originalConsoleLog = console.log;
+    originalConsoleError = console.error;
+    console.log = mock(() => {});
+    console.error = mock(() => {});
+  });
+
+  afterEach(() => {
+    console.log = originalConsoleLog;
+    console.error = originalConsoleError;
+  });
+
+  it("returns the result on first success", async () => {
+    const result = await retryWithBackoff(() => Promise.resolve("ok"), {
+      maxAttempts: 3,
+      initialDelayMs: 1,
+    });
+    expect(result).toBe("ok");
+  });
+
+  it("retries on failure and succeeds", async () => {
+    let attempt = 0;
+    const result = await retryWithBackoff(
+      () => {
+        attempt++;
+        if (attempt < 3) throw new Error("transient");
+        return Promise.resolve("recovered");
+      },
+      { maxAttempts: 3, initialDelayMs: 1 },
+    );
+    expect(result).toBe("recovered");
+    expect(attempt).toBe(3);
+  });
+
+  it("throws after exhausting all attempts", async () => {
+    await expect(
+      retryWithBackoff(() => Promise.reject(new Error("permanent")), {
+        maxAttempts: 2,
+        initialDelayMs: 1,
+      }),
+    ).rejects.toThrow("permanent");
+  });
+
+  it("stops retrying immediately when shouldRetry returns false", async () => {
+    class NonRetryableError extends Error {
+      constructor() {
+        super("non-retryable");
+        this.name = "NonRetryableError";
+      }
+    }
+
+    let attempts = 0;
+    await expect(
+      retryWithBackoff(
+        () => {
+          attempts++;
+          throw new NonRetryableError();
+        },
+        {
+          maxAttempts: 3,
+          initialDelayMs: 1,
+          shouldRetry: (error) => !(error instanceof NonRetryableError),
+        },
+      ),
+    ).rejects.toThrow("non-retryable");
+    expect(attempts).toBe(1);
+  });
+
+  it("continues retrying when shouldRetry returns true", async () => {
+    let attempts = 0;
+    await expect(
+      retryWithBackoff(
+        () => {
+          attempts++;
+          throw new Error("retryable");
+        },
+        {
+          maxAttempts: 3,
+          initialDelayMs: 1,
+          shouldRetry: () => true,
+        },
+      ),
+    ).rejects.toThrow("retryable");
+    expect(attempts).toBe(3);
+  });
+
+  it("preserves the original error when shouldRetry aborts", async () => {
+    class SpecificError extends Error {
+      code = 401;
+      constructor() {
+        super("unauthorized");
+        this.name = "SpecificError";
+      }
+    }
+
+    try {
+      await retryWithBackoff(
+        () => {
+          throw new SpecificError();
+        },
+        {
+          maxAttempts: 3,
+          initialDelayMs: 1,
+          shouldRetry: (error) => !(error instanceof SpecificError),
+        },
+      );
+      expect.unreachable("should have thrown");
+    } catch (error) {
+      expect(error).toBeInstanceOf(SpecificError);
+      expect((error as SpecificError).code).toBe(401);
+    }
+  });
+});