fix(toolkit-lib): retry backoff has no jitter, retries stay in lockstep

packages/@aws-cdk/toolkit-lib/lib/api/aws-auth/sdk.ts

@@ -1265,7 +1265,7 @@ function makeSdkLoggerSafeByBindingThis(logger: ISdkLogger): ISdkLogger {
 }
 
 /**
- * Build an exponential-backoff delay function whose result is capped at a maximum.
+ * Build a jittered exponential-backoff delay function whose result is capped at a maximum.
  *
  * An uncapped exponential backoff (`base * 2^attempt`) grows without bound as the
  * number of retries increases. For the retry counts we use on the CloudFormation
@@ -1276,9 +1276,20 @@ function makeSdkLoggerSafeByBindingThis(logger: ISdkLogger): ISdkLogger {
  * time to a few minutes at worst, while still giving the SDK plenty of
  * opportunity to ride out transient server problems or throttling.
  *
+ * The delay is jittered because `ConfiguredRetryStrategy` replaces the SDK's own
+ * jittered backoff with whatever function it is handed. Without jitter, requests
+ * that were throttled at the same moment retry at the same moments, and a herd of
+ * pollers stays a herd instead of dispersing over the retry window. We use equal
+ * jitter - a random point in `[floor(delay/2), delay - 1]` - rather than the SDK's
+ * full jitter, so that the worst case stays under the capped delay above and the
+ * wait before a retry never collapses to nearly zero.
+ *
  * @param baseMs - base delay in milliseconds (used in `baseMs * 2^attempt`)
  * @param capMs - maximum delay in milliseconds; any computed delay larger than this is clamped
  */
 export function cappedExponentialBackoff(baseMs: number, capMs: number): (attempt: number) => number {
-  return (attempt: number) => Math.min(baseMs * (2 ** attempt), capMs);
+  return (attempt: number) => {
+    const delay = Math.min(baseMs * (2 ** attempt), capMs);
+    return Math.floor(delay / 2 + Math.random() * (delay / 2));
+  };
 }

packages/@aws-cdk/toolkit-lib/test/api/aws-auth/sdk-retry.test.ts

@@ -1,7 +1,29 @@
 import { cappedExponentialBackoff } from '../../../lib/api/aws-auth/private';
 
+/**
+ * The top of the jitter window is the delay the schedule asks for, so mocking
+ * `Math.random()` to 1 lets the schedule be asserted without jitter arithmetic.
+ */
+function mockTopOfJitterWindow() {
+  jest.spyOn(Math, 'random').mockReturnValue(1);
+}
+
+/**
+ * The delay for `attempt` before jitter is applied
+ */
+function scheduled(attempt: number, baseMs = 1000, capMs = 15_000) {
+  return Math.min(baseMs * (2 ** attempt), capMs);
+}
+
+afterEach(() => {
+  // `clearMocks` does not restore a spy's implementation, and two tests below rely
+  // on real randomness.
+  jest.restoreAllMocks();
+});
+
 describe(cappedExponentialBackoff, () => {
   test('returns exponentially growing delays while below the cap', () => {
+    mockTopOfJitterWindow();
     const backoff = cappedExponentialBackoff(1000, 15_000);
 
     expect(backoff(1)).toBe(2000);
@@ -10,6 +32,7 @@ describe(cappedExponentialBackoff, () => {
   });
 
   test('clamps delays to the provided maximum', () => {
+    mockTopOfJitterWindow();
     const backoff = cappedExponentialBackoff(1000, 15_000);
 
     // Uncapped would be 16_000, 32_000, 1024 * 1000, 2048 * 1000 etc.
@@ -19,10 +42,42 @@ describe(cappedExponentialBackoff, () => {
     expect(backoff(20)).toBe(15_000);
   });
 
+  test('never waits less than half the scheduled delay', () => {
+    jest.spyOn(Math, 'random').mockReturnValue(0);
+    const backoff = cappedExponentialBackoff(1000, 15_000);
+
+    expect(backoff(1)).toBe(1000);
+    expect(backoff(4)).toBe(7500);
+    expect(backoff(20)).toBe(7500);
+  });
+
+  test('stays within the jitter window for every attempt', () => {
+    const backoff = cappedExponentialBackoff(1000, 15_000);
+
+    for (let attempt = 1; attempt <= 10; attempt++) {
+      for (let i = 0; i < 100; i++) {
+        const delay = backoff(attempt);
+        expect(delay).toBeGreaterThanOrEqual(Math.floor(scheduled(attempt) / 2));
+        expect(delay).toBeLessThan(scheduled(attempt));
+      }
+    }
+  });
+
+  test('disperses callers that retry at the same moment', () => {
+    // Requests throttled together retry together, so identical inputs must not
+    // produce identical delays; otherwise the herd stays synchronized.
+    const backoff = cappedExponentialBackoff(1000, 15_000);
+
+    const delays = new Set(Array.from({ length: 50 }, () => backoff(4)));
+
+    expect(delays.size).toBeGreaterThan(1);
+  });
+
   test('bounds total retry time for the CloudFormation client configuration', () => {
     // This mirrors the actual production config: 7 retries, 1s base, 15s cap.
     // Without the cap the total retry time was ~34 minutes, which manifests as
     // a hang to CLI users when CloudFormation returns InternalFailure.
+    mockTopOfJitterWindow();
     const backoff = cappedExponentialBackoff(1000, 15_000);
 
     let total = 0;
@@ -35,10 +90,11 @@ describe(cappedExponentialBackoff, () => {
     expect(total).toBeLessThan(120_000);
   });
 
-  test('can produce delays smaller than the base when base is small', () => {
+  test('scales the schedule with the configured base', () => {
+    mockTopOfJitterWindow();
     const backoff = cappedExponentialBackoff(100, 10_000);
 
-    expect(backoff(0)).toBe(100);
     expect(backoff(1)).toBe(200);
+    expect(backoff(2)).toBe(400);
   });
 });