Harden withRetry for transient fetch failures

actions/setup/js/create_issue.test.cjs

@@ -1517,7 +1517,7 @@ describe("create_issue", () => {
       expect(mockGithub.rest.issues.create).toHaveBeenCalledTimes(6);
     });
 
-    it("should have retry delays that never exceed maxDelayMs + jitterMs", async () => {
+    it("should have retry delays that never exceed maxDelayMs", async () => {
       const setTimeoutSpy = vi.spyOn(globalThis, "setTimeout");
 
       mockGithub.rest.issues.create = vi
@@ -1541,9 +1541,8 @@ describe("create_issue", () => {
       await vi.runAllTimersAsync();
       await resultPromise;
 
-      // create_issue uses RATE_LIMIT_RETRY_CONFIG: { initialDelayMs: 15000, maxDelayMs: 240000, jitterMs: 5000 }
-      // Maximum possible delay per retry = maxDelayMs + jitterMs = 245000ms
-      const maxBound = 245000;
+      // create_issue uses RATE_LIMIT_RETRY_CONFIG with maxDelayMs = 240000.
+      const maxBound = 240000;
       // Filter out short setTimeout calls (e.g. from test infrastructure) to isolate retry delays
       const sleepDelays = setTimeoutSpy.mock.calls.filter(([, ms]) => ms > 1000).map(([, ms]) => ms);
 

actions/setup/js/error_recovery.cjs

@@ -60,6 +60,8 @@ const RATE_LIMIT_RETRY_CONFIG = {
  * @type {string[]}
  */
 const RATE_LIMIT_INDICATORS = ["rate limit", "secondary rate limit", "abuse detection", "too many requests"];
+const TRANSIENT_HTTP_STATUSES = new Set([408, 425, 429, 500, 502, 503, 504]);
+const MAX_TIMER_DELAY_MS = 2 ** 31 - 1;
 
 /**
  * @param {string} messageLower - Lower-cased error message
@@ -77,6 +79,7 @@ function hasRateLimitIndicator(messageLower) {
 function isTransientError(error) {
   const errorMsg = getErrorMessage(error);
   const errorMsgLower = errorMsg.trimStart().toLowerCase();
+  const status = error?.response?.status ?? error?.status ?? null;
 
   // GitHub REST APIs may crash and return an HTML error page (e.g. the "Unicorn!"
   // 500 page) instead of JSON. Detect this by checking for an HTML doctype at the
@@ -92,6 +95,12 @@ function isTransientError(error) {
     return true;
   }
 
+  // Fetch and Octokit errors do not consistently include the status text in
+  // their message, so classify standard transient HTTP statuses directly.
+  if (TRANSIENT_HTTP_STATUSES.has(Number(status))) {
+    return true;
+  }
+
   // Network-related errors that are likely transient
   const transientPatterns = [
     "network",
@@ -122,38 +131,66 @@ function sleep(ms) {
 }
 
 /**
- * Extract the Retry-After delay in milliseconds from a GitHub API rate-limit error.
+ * Read a response header from either a Fetch Headers instance or a plain object.
+ * @param {Headers|Record<string, any>|null|undefined} headers
+ * @param {string} name
+ * @returns {any}
+ */
+function getHeader(headers, name) {
+  if (!headers) return null;
+  if (typeof headers.get === "function") {
+    return headers.get(name);
+  }
+  const matchingKey = Object.keys(headers).find(key => key.toLowerCase() === name);
+  return matchingKey === undefined ? null : headers[matchingKey];
+}
+
+/**
+ * Determine whether an error response represents a GitHub rate-limit condition.
+ * @param {any} error - The error to classify
+ * @returns {{headers: Headers|Record<string, any>|null, isRateLimit: boolean}}
+ */
+function getRateLimitErrorDetails(error) {
+  const status = error?.response?.status ?? error?.status ?? null;
+  const headers = error?.response?.headers ?? error?.headers ?? null;
+  const remainingHeader = getHeader(headers, "x-ratelimit-remaining");
+  const remainingExhausted = remainingHeader != null && parseInt(remainingHeader, 10) === 0;
+  return { headers, isRateLimit: status === 429 || (status === 403 && remainingExhausted) };
+}
+
+/**
+ * Extract the Retry-After delay in milliseconds from a retryable HTTP error.
  *
- * Only applies when the response status indicates a rate-limit condition:
+ * Applies when the response status indicates a rate-limit condition or service
+ * unavailability:
  *   - HTTP 429 (Too Many Requests)
  *   - HTTP 403 with `x-ratelimit-remaining: 0` (GitHub secondary rate limit)
+ *   - HTTP 503 (Service Unavailable)
  *
  * In those cases GitHub returns one of two headers:
  *   - `retry-after`       – integer seconds OR HTTP-date to wait until
  *   - `x-ratelimit-reset` – Unix timestamp (seconds) when the quota resets
  *
- * For any other status (5xx transient errors, etc.) returns null so normal
- * exponential backoff applies.
+ * For any other status returns null so normal exponential backoff applies.
  *
  * @param {any} error - The error object from a failed GitHub API call
  * @returns {number|null} Milliseconds to wait, or null if not a rate-limit response
  */
 function getRetryAfterMs(error) {
   // Octokit surfaces response headers via error.response.headers or error.headers
   const status = error?.response?.status ?? error?.status ?? null;
-  const headers = error?.response?.headers ?? error?.headers ?? null;
+  const { headers, isRateLimit } = getRateLimitErrorDetails(error);
   if (!headers) return null;
 
   // Only honour rate-limit headers for genuine rate-limit responses.
   // GitHub uses 429 for primary rate limits and 403 for secondary rate limits
   // (the latter always sets x-ratelimit-remaining to "0").
-  const remainingHeader = headers["x-ratelimit-remaining"];
-  const isRateLimitStatus = status === 429 || (status === 403 && remainingHeader != null && parseInt(remainingHeader, 10) === 0);
+  const retryAfter = getHeader(headers, "retry-after");
+  const supportsRetryAfter = isRateLimit || status === 503;
 
-  if (!isRateLimitStatus) return null;
+  if (!supportsRetryAfter) return null;
 
   // retry-after: number of seconds OR HTTP-date (highest priority)
-  const retryAfter = headers["retry-after"];
   if (retryAfter != null) {
     const seconds = parseInt(retryAfter, 10);
     if (!Number.isNaN(seconds) && seconds > 0) {
@@ -169,8 +206,8 @@ function getRetryAfterMs(error) {
   }
 
   // x-ratelimit-reset: Unix timestamp — derive wait time from clock delta
-  const resetAt = headers["x-ratelimit-reset"];
-  if (resetAt != null) {
+  const resetAt = getHeader(headers, "x-ratelimit-reset");
+  if (isRateLimit && resetAt != null) {
     const resetTimestampMs = parseInt(resetAt, 10) * 1000;
     if (!Number.isNaN(resetTimestampMs)) {
       const waitMs = resetTimestampMs - Date.now();
@@ -189,12 +226,7 @@ function getRetryAfterMs(error) {
  * @returns {boolean} True when the error indicates primary or secondary rate limiting
  */
 function isRateLimitError(error) {
-  const status = error?.response?.status ?? error?.status ?? null;
-  const headers = error?.response?.headers ?? error?.headers ?? null;
-  const remainingHeader = headers?.["x-ratelimit-remaining"];
-  const retryAfterHeader = headers?.["retry-after"];
-  const hasRateLimitHeaders = status === 403 && (retryAfterHeader != null || (remainingHeader != null && parseInt(remainingHeader, 10) === 0));
-  if (status === 429 || hasRateLimitHeaders) {
+  if (getRateLimitErrorDetails(error).isRateLimit) {
     return true;
   }
 
@@ -213,14 +245,15 @@ function isRateLimitError(error) {
  */
 async function withRetry(operation, config = {}, operationName = "operation") {
   const fullConfig = { ...DEFAULT_RETRY_CONFIG, ...config };
+  validateRetryConfig(operation, fullConfig);
   let lastError;
   let delay = fullConfig.initialDelayMs;
 
   for (let attempt = 0; attempt <= fullConfig.maxRetries; attempt++) {
     try {
       if (attempt > 0) {
         const jitter = fullConfig.jitterMs > 0 ? Math.floor(Math.random() * fullConfig.jitterMs) : 0;
-        const delayWithJitter = delay + jitter;
+        const delayWithJitter = Math.min(delay + jitter, fullConfig.maxDelayMs);
         core.info(`Retry attempt ${attempt}/${fullConfig.maxRetries} for ${operationName} after ${delayWithJitter}ms delay`);
         logRetryEvent(lastError, operationName, attempt, delayWithJitter);
         await sleep(delayWithJitter);
@@ -297,6 +330,35 @@ async function withRetry(operation, config = {}, operationName = "operation") {
   throw lastError;
 }
 
+/**
+ * Reject invalid retry configuration before starting an operation.
+ * @param {unknown} operation
+ * @param {RetryConfig} config
+ */
+function validateRetryConfig(operation, config) {
+  if (typeof operation !== "function") {
+    throw new TypeError("Retry operation must be a function");
+  }
+
+  for (const key of ["maxRetries", "initialDelayMs", "maxDelayMs", "jitterMs"]) {
+    const value = config[key];
+    if (!Number.isSafeInteger(value) || value < 0) {
+      throw new RangeError(`Retry configuration ${key} must be a non-negative safe integer`);
+    }
+  }
+  for (const key of ["initialDelayMs", "maxDelayMs", "jitterMs"]) {
+    if (config[key] > MAX_TIMER_DELAY_MS) {
+      throw new RangeError(`Retry configuration ${key} must not exceed ${MAX_TIMER_DELAY_MS}`);
+    }
+  }
+  if (!Number.isFinite(config.backoffMultiplier) || config.backoffMultiplier < 1) {
+    throw new RangeError("Retry configuration backoffMultiplier must be a finite number greater than or equal to 1");
+  }
+  if (typeof config.shouldRetry !== "function") {
+    throw new TypeError("Retry configuration shouldRetry must be a function");
+  }
+}
+
 /**
  * Enhance an error with additional context for better debugging
  * @param {any} error - The original error

actions/setup/js/error_recovery.test.cjs

@@ -57,6 +57,16 @@ describe("error_recovery", () => {
       expect(isTransientError({ response: { status: 429 }, message: "Request failed" })).toBe(true);
     });
 
+    it.each([408, 425, 500, 502, 503, 504])("should identify HTTP %i as transient without relying on message text", status => {
+      expect(isTransientError({ status, message: "Request failed" })).toBe(true);
+      expect(isTransientError({ response: { status }, message: "Request failed" })).toBe(true);
+    });
+
+    it("should not identify non-transient HTTP statuses as transient", () => {
+      expect(isTransientError({ status: 400, message: "Request failed" })).toBe(false);
+      expect(isTransientError({ status: 404, message: "Request failed" })).toBe(false);
+    });
+
     it("should not identify validation errors as transient", () => {
       expect(isTransientError(new Error("Invalid input"))).toBe(false);
       expect(isTransientError(new Error("Field is required"))).toBe(false);
@@ -122,10 +132,10 @@ describe("error_recovery", () => {
       expect(operation).toHaveBeenCalledTimes(4); // Initial + 3 retries
     });
 
-    it("should emit E010 for exhausted retries on 403 with retry-after header", async () => {
+    it("should emit E010 for exhausted retries on a secondary rate limit", async () => {
       const rateLimitError = {
         message: "secondary rate limit",
-        response: { status: 403, headers: { "retry-after": "1" } },
+        response: { status: 403, headers: { "retry-after": "1", "x-ratelimit-remaining": "0" } },
       };
       const operation = vi.fn().mockRejectedValue(rateLimitError);
 
@@ -235,6 +245,41 @@ describe("error_recovery", () => {
       // Base delay after first failure: 100 * 2 = 200ms, no jitter
       expect(core.info).toHaveBeenCalledWith(expect.stringContaining("after 200ms delay"));
     });
+
+    it("should cap the delay after adding jitter", async () => {
+      const randomSpy = vi.spyOn(Math, "random").mockReturnValue(0.99);
+      const operation = vi.fn().mockRejectedValueOnce(new Error("Network timeout")).mockResolvedValue("success");
+
+      await withRetry(operation, { maxRetries: 1, initialDelayMs: 1000, backoffMultiplier: 2, maxDelayMs: 2000, jitterMs: 1000 }, "test-operation");
+
+      expect(core.info).toHaveBeenCalledWith(expect.stringContaining("after 2000ms delay"));
+      randomSpy.mockRestore();
+    });
+
+    it.each([
+      ["maxRetries", -1],
+      ["initialDelayMs", Number.NaN],
+      ["maxDelayMs", Number.POSITIVE_INFINITY],
+      ["maxDelayMs", 2 ** 31],
+      ["jitterMs", 1.5],
+      ["backoffMultiplier", 0],
+    ])("should reject invalid %s configuration", async (key, value) => {
+      const operation = vi.fn();
+
+      await expect(withRetry(operation, { [key]: value }, "test-operation")).rejects.toThrow(`Retry configuration ${key}`);
+      expect(operation).not.toHaveBeenCalled();
+    });
+
+    it("should accept the largest supported timer delay", async () => {
+      const operation = vi.fn().mockResolvedValue("success");
+
+      await expect(withRetry(operation, { maxDelayMs: 2 ** 31 - 1 }, "test-operation")).resolves.toBe("success");
+    });
+
+    it("should reject a non-function retry predicate", async () => {
+      // @ts-expect-error Deliberately exercise runtime input validation.
+      await expect(withRetry(vi.fn(), { shouldRetry: true }, "test-operation")).rejects.toThrow("Retry configuration shouldRetry must be a function");
+    });
   });
 
   describe("enhanceError", () => {
@@ -415,13 +460,30 @@ describe("error_recovery", () => {
       // 403 without x-ratelimit-remaining: 0 is not a rate-limit response
       const error403 = { response: { status: 403, headers: { "retry-after": "60", "x-ratelimit-remaining": "100" } } };
       expect(getRetryAfterMs(error403)).toBeNull();
+      const secondaryRateLimitWithoutRemaining = { response: { status: 403, headers: { "retry-after": "60" } } };
+      expect(getRetryAfterMs(secondaryRateLimitWithoutRemaining)).toBeNull();
     });
 
     it("should extract retry-after seconds from response headers on 429", () => {
       const error = { response: { status: 429, headers: { "retry-after": "60" } } };
       expect(getRetryAfterMs(error)).toBe(60000);
     });
 
+    it("should extract retry-after seconds from a 403 secondary rate-limit response", () => {
+      const error = { response: { status: 403, headers: { "retry-after": "30", "x-ratelimit-remaining": "0" } } };
+      expect(getRetryAfterMs(error)).toBe(30000);
+    });
+
+    it("should extract retry-after seconds from a 503 response", () => {
+      const error = { response: { status: 503, headers: { "retry-after": "15" } } };
+      expect(getRetryAfterMs(error)).toBe(15000);
+    });
+
+    it("should read case-insensitive headers from a Fetch Headers instance", () => {
+      const error = { response: { status: 429, headers: new Headers({ "Retry-After": "20" }) } };
+      expect(getRetryAfterMs(error)).toBe(20000);
+    });
+
     it("should extract retry-after seconds from top-level headers on 429", () => {
       const error = { status: 429, headers: { "retry-after": "30" } };
       expect(getRetryAfterMs(error)).toBe(30000);
@@ -511,6 +573,19 @@ describe("error_recovery", () => {
       expect(core.info).toHaveBeenCalledWith(expect.stringContaining("Retry-After header detected for test-operation: next retry will wait 1000ms"));
     });
 
+    it("should use Retry-After delay for a status-only 503 response", async () => {
+      const retryAfterError = {
+        message: "Request failed",
+        response: { status: 503, headers: { "retry-after": "1" } },
+      };
+      const operation = vi.fn().mockRejectedValueOnce(retryAfterError).mockResolvedValue("success");
+
+      const result = await withRetry(operation, { maxRetries: 1, initialDelayMs: 10, backoffMultiplier: 2, jitterMs: 0 }, "test-operation");
+
+      expect(result).toBe("success");
+      expect(core.info).toHaveBeenCalledWith(expect.stringContaining("Retry-After header detected for test-operation: next retry will wait 1000ms"));
+    });
+
     it("should fall back to exponential backoff for non-rate-limit errors (502)", async () => {
       const transientError = {
         message: "502 bad gateway",

actions/setup/js/pr_review_buffer.test.cjs

@@ -1227,13 +1227,13 @@ describe("pr_review_buffer (factory pattern)", () => {
         });
 
         const lockedError = Object.assign(new Error("lock prevents review"), { status: 422 });
-        const otherError = Object.assign(new Error("internal server error"), { status: 500 });
+        const otherError = Object.assign(new Error("bad request"), { status: 400 });
         mockGithub.rest.pulls.createReview.mockRejectedValueOnce(lockedError).mockRejectedValueOnce(otherError);
 
         const result = await buffer.submitReview();
 
         expect(result.success).toBe(false);
-        expect(result.error).toContain("internal server error");
+        expect(result.error).toContain("bad request");
         expect(mockGithub.rest.pulls.createReview).toHaveBeenCalledTimes(2);
       } finally {
         setTimeoutSpy.mockRestore();