diff --git a/actions/setup/js/create_issue.test.cjs b/actions/setup/js/create_issue.test.cjs index 1db964e8280..3af75976bbf 100644 --- a/actions/setup/js/create_issue.test.cjs +++ b/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); diff --git a/actions/setup/js/error_recovery.cjs b/actions/setup/js/error_recovery.cjs index 54aea9f24ab..34c443b8890 100644 --- a/actions/setup/js/error_recovery.cjs +++ b/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,18 +131,47 @@ 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|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|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 @@ -141,19 +179,18 @@ function sleep(ms) { 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,6 +245,7 @@ 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; @@ -220,7 +253,7 @@ async function withRetry(operation, config = {}, operationName = "operation") { 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 diff --git a/actions/setup/js/error_recovery.test.cjs b/actions/setup/js/error_recovery.test.cjs index 0ab90f43e2e..ebf7fc0cd68 100644 --- a/actions/setup/js/error_recovery.test.cjs +++ b/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,6 +460,8 @@ 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", () => { @@ -422,6 +469,21 @@ describe("error_recovery", () => { 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", diff --git a/actions/setup/js/pr_review_buffer.test.cjs b/actions/setup/js/pr_review_buffer.test.cjs index ddbf9d00b2b..47fdc0fb4ed 100644 --- a/actions/setup/js/pr_review_buffer.test.cjs +++ b/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();