From e0c67bfbd14e9ba84bc55ead44bc1c01dc970615 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Mon, 13 Apr 2026 12:00:27 +0200 Subject: [PATCH 1/2] fix(appkit): add jitter to RetryInterceptor exponential backoff RetryInterceptor used pure exponential backoff (initialDelay * 2^attempt), causing all concurrent retries to fire at identical intervals after a simultaneous failure (thundering herd). Now applies full jitter by default: delay * (0.5 + Math.random() * 0.5), producing random delays between 50% and 100% of the base delay. Opt out with retry.jitter = false. Signed-off-by: MarioCadenas --- .../appkit/src/plugin/interceptors/retry.ts | 10 ++- .../appkit/src/plugin/tests/retry.test.ts | 71 ++++++++++++++++++- packages/shared/src/execute.ts | 4 +- 3 files changed, 79 insertions(+), 6 deletions(-) diff --git a/packages/appkit/src/plugin/interceptors/retry.ts b/packages/appkit/src/plugin/interceptors/retry.ts index c0a184b8..682fc6bc 100644 --- a/packages/appkit/src/plugin/interceptors/retry.ts +++ b/packages/appkit/src/plugin/interceptors/retry.ts @@ -9,11 +9,13 @@ export class RetryInterceptor implements ExecutionInterceptor { private attempts: number; private initialDelay: number; private maxDelay: number; + private jitter: boolean; constructor(config: RetryConfig) { this.attempts = config.attempts ?? 3; this.initialDelay = config.initialDelay ?? 1000; this.maxDelay = config.maxDelay ?? 30000; + this.jitter = config.jitter ?? true; } async intercept( @@ -59,11 +61,13 @@ export class RetryInterceptor implements ExecutionInterceptor { } private calculateDelay(attempt: number): number { - // exponential backoff const delay = this.initialDelay * 2 ** (attempt - 1); + const capped = Math.min(delay, this.maxDelay); - // max delay cap - return Math.min(delay, this.maxDelay); + if (!this.jitter) return capped; + + // Full jitter: random value between 50% and 100% of the calculated delay + return capped * (0.5 + Math.random() * 0.5); } private sleep(ms: number): Promise { diff --git a/packages/appkit/src/plugin/tests/retry.test.ts b/packages/appkit/src/plugin/tests/retry.test.ts index 913a8262..ee17e711 100644 --- a/packages/appkit/src/plugin/tests/retry.test.ts +++ b/packages/appkit/src/plugin/tests/retry.test.ts @@ -77,11 +77,12 @@ describe("RetryInterceptor", () => { expect(fn).toHaveBeenCalledTimes(3); }); - test("should use exponential backoff", async () => { + test("should use exponential backoff with jitter disabled", async () => { const config: RetryConfig = { enabled: true, attempts: 4, initialDelay: 1000, + jitter: false, }; const interceptor = new RetryInterceptor(config); const fn = vi @@ -111,7 +112,8 @@ describe("RetryInterceptor", () => { enabled: true, attempts: 10, initialDelay: 1000, - maxDelay: 5000, // Cap at 5 seconds + maxDelay: 5000, + jitter: false, }; const interceptor = new RetryInterceptor(config); const fn = vi.fn().mockRejectedValue(new Error("fail")); @@ -170,4 +172,69 @@ describe("RetryInterceptor", () => { await expect(interceptor.intercept(fn, context)).rejects.toThrow("fail"); expect(fn).toHaveBeenCalledTimes(1); }); + + test("should add jitter to delay by default", async () => { + const config: RetryConfig = { + enabled: true, + attempts: 3, + initialDelay: 1000, + }; + const interceptor = new RetryInterceptor(config); + + vi.spyOn(Math, "random").mockReturnValue(0.5); + + const fn = vi + .fn() + .mockRejectedValueOnce(new Error("fail 1")) + .mockResolvedValue("success"); + + interceptor.intercept(fn, context); + + // With jitter: delay = 1000 * (0.5 + 0.5 * 0.5) = 750ms + await vi.advanceTimersByTimeAsync(749); + expect(fn).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(1); + expect(fn).toHaveBeenCalledTimes(2); + + vi.spyOn(Math, "random").mockRestore(); + }); + + test("should produce delays between 50% and 100% of base delay with jitter", async () => { + const config: RetryConfig = { + enabled: true, + attempts: 3, + initialDelay: 1000, + }; + + // At Math.random() = 0, delay = 1000 * (0.5 + 0) = 500ms (minimum) + vi.spyOn(Math, "random").mockReturnValue(0); + const interceptorMin = new RetryInterceptor(config); + const fnMin = vi + .fn() + .mockRejectedValueOnce(new Error("fail")) + .mockResolvedValue("ok"); + + interceptorMin.intercept(fnMin, context); + await vi.advanceTimersByTimeAsync(499); + expect(fnMin).toHaveBeenCalledTimes(1); + await vi.advanceTimersByTimeAsync(1); + expect(fnMin).toHaveBeenCalledTimes(2); + + // At Math.random() = 1, delay = 1000 * (0.5 + 0.5) = 1000ms (maximum) + vi.spyOn(Math, "random").mockReturnValue(1); + const interceptorMax = new RetryInterceptor(config); + const fnMax = vi + .fn() + .mockRejectedValueOnce(new Error("fail")) + .mockResolvedValue("ok"); + + interceptorMax.intercept(fnMax, context); + await vi.advanceTimersByTimeAsync(999); + expect(fnMax).toHaveBeenCalledTimes(1); + await vi.advanceTimersByTimeAsync(1); + expect(fnMax).toHaveBeenCalledTimes(2); + + vi.spyOn(Math, "random").mockRestore(); + }); }); diff --git a/packages/shared/src/execute.ts b/packages/shared/src/execute.ts index 9221ea2f..13f43802 100644 --- a/packages/shared/src/execute.ts +++ b/packages/shared/src/execute.ts @@ -13,12 +13,14 @@ export interface StreamConfig { maxActiveStreams?: number; } -/** Retry configuration for the RetryInterceptor. Uses exponential backoff between attempts. */ +/** Retry configuration for the RetryInterceptor. Uses exponential backoff with jitter between attempts. */ export interface RetryConfig { enabled?: boolean; attempts?: number; initialDelay?: number; maxDelay?: number; + /** Whether to add random jitter to retry delays to avoid thundering herd. Defaults to `true`. */ + jitter?: boolean; } /** Telemetry configuration for the TelemetryInterceptor. Controls span creation and custom attributes. */ From cb5ca89f6be80b4507774db2fa407ea20d3be59c Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Mon, 13 Apr 2026 12:59:14 +0200 Subject: [PATCH 2/2] fix(appkit): use full jitter and remove jitter opt-out flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR review feedback: - Use true full jitter (capped * Math.random()) giving range [0, capped] instead of half jitter (50%-100%) - Remove jitter boolean flag — jitter is always applied since it is strictly better for production retry behavior Signed-off-by: MarioCadenas --- .../appkit/src/plugin/interceptors/retry.ts | 7 +- .../appkit/src/plugin/tests/retry.test.ts | 90 ++++++++++--------- packages/shared/src/execute.ts | 4 +- 3 files changed, 49 insertions(+), 52 deletions(-) diff --git a/packages/appkit/src/plugin/interceptors/retry.ts b/packages/appkit/src/plugin/interceptors/retry.ts index 682fc6bc..435e0fde 100644 --- a/packages/appkit/src/plugin/interceptors/retry.ts +++ b/packages/appkit/src/plugin/interceptors/retry.ts @@ -9,13 +9,11 @@ export class RetryInterceptor implements ExecutionInterceptor { private attempts: number; private initialDelay: number; private maxDelay: number; - private jitter: boolean; constructor(config: RetryConfig) { this.attempts = config.attempts ?? 3; this.initialDelay = config.initialDelay ?? 1000; this.maxDelay = config.maxDelay ?? 30000; - this.jitter = config.jitter ?? true; } async intercept( @@ -64,10 +62,7 @@ export class RetryInterceptor implements ExecutionInterceptor { const delay = this.initialDelay * 2 ** (attempt - 1); const capped = Math.min(delay, this.maxDelay); - if (!this.jitter) return capped; - - // Full jitter: random value between 50% and 100% of the calculated delay - return capped * (0.5 + Math.random() * 0.5); + return capped * Math.random(); } private sleep(ms: number): Promise { diff --git a/packages/appkit/src/plugin/tests/retry.test.ts b/packages/appkit/src/plugin/tests/retry.test.ts index ee17e711..b897c585 100644 --- a/packages/appkit/src/plugin/tests/retry.test.ts +++ b/packages/appkit/src/plugin/tests/retry.test.ts @@ -77,13 +77,14 @@ describe("RetryInterceptor", () => { expect(fn).toHaveBeenCalledTimes(3); }); - test("should use exponential backoff with jitter disabled", async () => { + test("should use exponential backoff", async () => { const config: RetryConfig = { enabled: true, attempts: 4, initialDelay: 1000, - jitter: false, }; + + vi.spyOn(Math, "random").mockReturnValue(1); const interceptor = new RetryInterceptor(config); const fn = vi .fn() @@ -94,17 +95,20 @@ describe("RetryInterceptor", () => { interceptor.intercept(fn, context); - // First retry: 1000ms delay (2^0 * 1000) + // With Math.random() = 1, jitter multiplier is 1x (no reduction) + // First retry: 1000ms delay (2^0 * 1000 * 1) await vi.advanceTimersByTimeAsync(1000); expect(fn).toHaveBeenCalledTimes(2); - // Second retry: 2000ms delay (2^1 * 1000) + // Second retry: 2000ms delay (2^1 * 1000 * 1) await vi.advanceTimersByTimeAsync(2000); expect(fn).toHaveBeenCalledTimes(3); - // Third retry: 4000ms delay (2^2 * 1000) + // Third retry: 4000ms delay (2^2 * 1000 * 1) await vi.advanceTimersByTimeAsync(4000); expect(fn).toHaveBeenCalledTimes(4); + + vi.spyOn(Math, "random").mockRestore(); }); test("should respect maxDelay cap", async () => { @@ -113,21 +117,23 @@ describe("RetryInterceptor", () => { attempts: 10, initialDelay: 1000, maxDelay: 5000, - jitter: false, }; + + vi.spyOn(Math, "random").mockReturnValue(1); const interceptor = new RetryInterceptor(config); const fn = vi.fn().mockRejectedValue(new Error("fail")); interceptor.intercept(fn, context); - // After 3 retries, delay should be capped at maxDelay - // Attempt 4 would normally be 8000ms (2^3 * 1000), but capped at 5000ms + // With Math.random() = 1, delays are at their maximum await vi.advanceTimersByTimeAsync(1000); // 1st retry await vi.advanceTimersByTimeAsync(2000); // 2nd retry await vi.advanceTimersByTimeAsync(4000); // 3rd retry - await vi.advanceTimersByTimeAsync(5000); // 4th retry (capped) + await vi.advanceTimersByTimeAsync(5000); // 4th retry (capped at maxDelay) expect(fn).toHaveBeenCalledTimes(5); + + vi.spyOn(Math, "random").mockRestore(); }); test("should not retry if signal is aborted", async () => { @@ -173,41 +179,14 @@ describe("RetryInterceptor", () => { expect(fn).toHaveBeenCalledTimes(1); }); - test("should add jitter to delay by default", async () => { - const config: RetryConfig = { - enabled: true, - attempts: 3, - initialDelay: 1000, - }; - const interceptor = new RetryInterceptor(config); - - vi.spyOn(Math, "random").mockReturnValue(0.5); - - const fn = vi - .fn() - .mockRejectedValueOnce(new Error("fail 1")) - .mockResolvedValue("success"); - - interceptor.intercept(fn, context); - - // With jitter: delay = 1000 * (0.5 + 0.5 * 0.5) = 750ms - await vi.advanceTimersByTimeAsync(749); - expect(fn).toHaveBeenCalledTimes(1); - - await vi.advanceTimersByTimeAsync(1); - expect(fn).toHaveBeenCalledTimes(2); - - vi.spyOn(Math, "random").mockRestore(); - }); - - test("should produce delays between 50% and 100% of base delay with jitter", async () => { + test("should apply full jitter: delay between 0 and capped value", async () => { const config: RetryConfig = { enabled: true, attempts: 3, initialDelay: 1000, }; - // At Math.random() = 0, delay = 1000 * (0.5 + 0) = 500ms (minimum) + // At Math.random() = 0, delay = 1000 * 0 = 0ms (minimum) vi.spyOn(Math, "random").mockReturnValue(0); const interceptorMin = new RetryInterceptor(config); const fnMin = vi @@ -215,13 +194,12 @@ describe("RetryInterceptor", () => { .mockRejectedValueOnce(new Error("fail")) .mockResolvedValue("ok"); - interceptorMin.intercept(fnMin, context); - await vi.advanceTimersByTimeAsync(499); - expect(fnMin).toHaveBeenCalledTimes(1); - await vi.advanceTimersByTimeAsync(1); + const promiseMin = interceptorMin.intercept(fnMin, context); + await vi.advanceTimersByTimeAsync(0); + await promiseMin; expect(fnMin).toHaveBeenCalledTimes(2); - // At Math.random() = 1, delay = 1000 * (0.5 + 0.5) = 1000ms (maximum) + // At Math.random() = 1, delay = 1000 * 1 = 1000ms (maximum) vi.spyOn(Math, "random").mockReturnValue(1); const interceptorMax = new RetryInterceptor(config); const fnMax = vi @@ -237,4 +215,30 @@ describe("RetryInterceptor", () => { vi.spyOn(Math, "random").mockRestore(); }); + + test("should produce jittered delay at midpoint", async () => { + const config: RetryConfig = { + enabled: true, + attempts: 3, + initialDelay: 1000, + }; + + vi.spyOn(Math, "random").mockReturnValue(0.5); + const interceptor = new RetryInterceptor(config); + const fn = vi + .fn() + .mockRejectedValueOnce(new Error("fail")) + .mockResolvedValue("success"); + + interceptor.intercept(fn, context); + + // delay = 1000 * 0.5 = 500ms + await vi.advanceTimersByTimeAsync(499); + expect(fn).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(1); + expect(fn).toHaveBeenCalledTimes(2); + + vi.spyOn(Math, "random").mockRestore(); + }); }); diff --git a/packages/shared/src/execute.ts b/packages/shared/src/execute.ts index 13f43802..62ac45bf 100644 --- a/packages/shared/src/execute.ts +++ b/packages/shared/src/execute.ts @@ -13,14 +13,12 @@ export interface StreamConfig { maxActiveStreams?: number; } -/** Retry configuration for the RetryInterceptor. Uses exponential backoff with jitter between attempts. */ +/** Retry configuration for the RetryInterceptor. Uses exponential backoff with full jitter between attempts. */ export interface RetryConfig { enabled?: boolean; attempts?: number; initialDelay?: number; maxDelay?: number; - /** Whether to add random jitter to retry delays to avoid thundering herd. Defaults to `true`. */ - jitter?: boolean; } /** Telemetry configuration for the TelemetryInterceptor. Controls span creation and custom attributes. */