From 4eeb7bfa5766ad338bb46a271f7ca6beeef68755 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 29 May 2026 14:15:30 -0400 Subject: [PATCH] fix(retry): cap unsafe retry delays --- src/infra/retry.test.ts | 68 +++++++++++++++++++++++++++++++++++++++++ src/infra/retry.ts | 27 ++++++++++++---- 2 files changed, 89 insertions(+), 6 deletions(-) diff --git a/src/infra/retry.test.ts b/src/infra/retry.test.ts index 9f5fe3d2a87..8ec18eb9581 100644 --- a/src/infra/retry.test.ts +++ b/src/infra/retry.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { MAX_SAFE_TIMEOUT_DELAY_MS } from "../utils/timer-delay.js"; import { resolveRetryConfig, retryAsync } from "./retry.js"; const randomMocks = vi.hoisted(() => ({ @@ -193,6 +194,60 @@ describe("retryAsync", () => { expect(fn).toHaveBeenCalledTimes(1); }); + it("falls back to the default attempt count for malformed numeric overloads", async () => { + const fn = vi.fn().mockRejectedValue(new Error("boom")); + await expect(runRetryNumberCase(fn, Number.NaN, 0)).rejects.toThrow("boom"); + expect(fn).toHaveBeenCalledTimes(3); + }); + + it("caps numeric overload delays to the safe timer range", async () => { + vi.clearAllTimers(); + vi.useFakeTimers(); + const timeoutSpy = vi.spyOn(globalThis, "setTimeout"); + const fn = vi.fn().mockRejectedValueOnce(new Error("boom")).mockResolvedValueOnce("ok"); + try { + const promise = retryAsync(fn, 2, 3_000_000_000); + await vi.advanceTimersByTimeAsync(MAX_SAFE_TIMEOUT_DELAY_MS); + await expect(promise).resolves.toBe("ok"); + expect(timeoutSpy).toHaveBeenCalledWith(expect.any(Function), MAX_SAFE_TIMEOUT_DELAY_MS); + } finally { + timeoutSpy.mockRestore(); + vi.clearAllTimers(); + vi.useRealTimers(); + } + }); + + it("keeps overflowed numeric overload backoff delays at the safe timer ceiling", async () => { + vi.clearAllTimers(); + vi.useFakeTimers(); + const timeoutSpy = vi.spyOn(globalThis, "setTimeout"); + const fn = vi + .fn() + .mockRejectedValueOnce(new Error("boom-1")) + .mockRejectedValueOnce(new Error("boom-2")) + .mockResolvedValueOnce("ok"); + try { + const promise = retryAsync(fn, 3, Number.MAX_VALUE); + await vi.advanceTimersByTimeAsync(MAX_SAFE_TIMEOUT_DELAY_MS); + await vi.advanceTimersByTimeAsync(MAX_SAFE_TIMEOUT_DELAY_MS); + await expect(promise).resolves.toBe("ok"); + expect(timeoutSpy).toHaveBeenNthCalledWith( + 1, + expect.any(Function), + MAX_SAFE_TIMEOUT_DELAY_MS, + ); + expect(timeoutSpy).toHaveBeenNthCalledWith( + 2, + expect.any(Function), + MAX_SAFE_TIMEOUT_DELAY_MS, + ); + } finally { + timeoutSpy.mockRestore(); + vi.clearAllTimers(); + vi.useRealTimers(); + } + }); + it.each([ { name: "uses retryAfterMs when provided", @@ -261,6 +316,19 @@ describe("resolveRetryConfig", () => { }, expected: { attempts: 3, minDelayMs: 300, maxDelayMs: 30000, jitter: 1 }, }, + { + name: "caps huge retry delays to the safe timer range", + overrides: { + minDelayMs: 3_000_000_000, + maxDelayMs: 4_000_000_000, + }, + expected: { + attempts: 3, + minDelayMs: MAX_SAFE_TIMEOUT_DELAY_MS, + maxDelayMs: MAX_SAFE_TIMEOUT_DELAY_MS, + jitter: 0, + }, + }, ])("$name", ({ overrides, expected }) => { expect(resolveRetryConfig(undefined, overrides)).toEqual(expected); }); diff --git a/src/infra/retry.ts b/src/infra/retry.ts index 7dad706deaa..5029484f833 100644 --- a/src/infra/retry.ts +++ b/src/infra/retry.ts @@ -1,5 +1,6 @@ import { asFiniteNumber } from "../shared/number-coercion.js"; import { sleep } from "../utils.js"; +import { MAX_SAFE_TIMEOUT_DELAY_MS, resolveSafeTimeoutDelayMs } from "../utils/timer-delay.js"; import { generateSecureFraction } from "./secure-random.js"; export type RetryConfig = { @@ -41,18 +42,32 @@ const clampNumber = (value: unknown, fallback: number, min?: number, max?: numbe return Math.min(Math.max(next, floor), ceiling); }; +function resolveAttemptCount(value: unknown, fallback: number): number { + const candidate = typeof value === "number" && Number.isFinite(value) ? value : fallback; + return Math.max(1, Math.round(candidate)); +} + +function resolveRetryDelayMs(value: number): number { + if (value === Number.POSITIVE_INFINITY) { + return MAX_SAFE_TIMEOUT_DELAY_MS; + } + return resolveSafeTimeoutDelayMs(value, { minMs: 0 }); +} + export function resolveRetryConfig( defaults: Required = DEFAULT_RETRY_CONFIG, overrides?: RetryConfig, ): Required { - const attempts = Math.max(1, Math.round(clampNumber(overrides?.attempts, defaults.attempts, 1))); - const minDelayMs = Math.max( - 0, + const attempts = resolveAttemptCount( + clampNumber(overrides?.attempts, defaults.attempts, 1), + defaults.attempts, + ); + const minDelayMs = resolveRetryDelayMs( Math.round(clampNumber(overrides?.minDelayMs, defaults.minDelayMs, 0)), ); const maxDelayMs = Math.max( minDelayMs, - Math.round(clampNumber(overrides?.maxDelayMs, defaults.maxDelayMs, 0)), + resolveRetryDelayMs(Math.round(clampNumber(overrides?.maxDelayMs, defaults.maxDelayMs, 0))), ); const jitter = clampNumber(overrides?.jitter, defaults.jitter, 0, 1); return { attempts, minDelayMs, maxDelayMs, jitter }; @@ -88,7 +103,7 @@ export async function retryAsync( initialDelayMs = 300, ): Promise { if (typeof attemptsOrOptions === "number") { - const attempts = Math.max(1, Math.round(attemptsOrOptions)); + const attempts = resolveAttemptCount(attemptsOrOptions, DEFAULT_RETRY_CONFIG.attempts); let lastErr: unknown; for (let i = 0; i < attempts; i += 1) { try { @@ -98,7 +113,7 @@ export async function retryAsync( if (i === attempts - 1) { break; } - const delay = initialDelayMs * 2 ** i; + const delay = resolveRetryDelayMs(initialDelayMs * 2 ** i); await sleep(delay); } }