fix(util-retry): correct attempts count on StandardRetryStrategy (#4891)

Author: srchase

packages/util-retry/src/ConfiguredRetryStrategy.spec.ts

@@ -5,13 +5,13 @@ describe(ConfiguredRetryStrategy.name, () => {
     const strategy = new ConfiguredRetryStrategy(5, (attempt) => attempt * 1000);
 
     const token = await strategy.acquireInitialRetryToken("");
-    token.getRetryCount = () => 4;
+    token.getRetryCount = () => 3;
 
     const retryToken = await strategy.refreshRetryTokenForRetry(token, {
       errorType: "TRANSIENT",
     });
 
-    expect(retryToken.getRetryCount()).toBe(5);
-    expect(retryToken.getRetryDelay()).toBe(5000);
+    expect(retryToken.getRetryCount()).toBe(4);
+    expect(retryToken.getRetryDelay()).toBe(4000);
   });
 });

packages/util-retry/src/StandardRetryStrategy.spec.ts

@@ -22,7 +22,7 @@ describe(StandardRetryStrategy.name, () => {
   });
 
   afterEach(() => {
-    jest.clearAllMocks;
+    jest.clearAllMocks();
   });
 
   it("sets maxAttemptsProvider as a class member variable", () => {
@@ -65,47 +65,49 @@ describe(StandardRetryStrategy.name, () => {
       expect(getRetryCount).toHaveBeenCalledTimes(3);
     });
 
-    it("throws when attempts exceeds maxAttempts", async () => {
+    it("disables any retries when maxAttempts is 1", async () => {
       const mockRetryToken = {
-        getRetryCount: () => 2,
+        getRetryCount: () => 0,
         getRetryTokenCount: (errorInfo: any) => 1,
       };
       (createDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken);
-      const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(1));
+      const retryStrategy = new StandardRetryStrategy(1);
       const token = await retryStrategy.acquireInitialRetryToken(retryTokenScope);
       try {
         await retryStrategy.refreshRetryTokenForRetry(token, errorInfo);
+        fail(`expected ${noRetryTokenAvailableError}`);
       } catch (error) {
         expect(error).toStrictEqual(noRetryTokenAvailableError);
       }
     });
 
-    it("throws when attempts exceeds default max attempts (3)", async () => {
+    it("throws when attempts exceeds maxAttempts", async () => {
       const mockRetryToken = {
-        getRetryCount: () => 5,
+        getRetryCount: () => 2,
         getRetryTokenCount: (errorInfo: any) => 1,
       };
       (createDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken);
-      const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(5));
+      const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(1));
       const token = await retryStrategy.acquireInitialRetryToken(retryTokenScope);
       try {
         await retryStrategy.refreshRetryTokenForRetry(token, errorInfo);
+        fail(`expected ${noRetryTokenAvailableError}`);
       } catch (error) {
         expect(error).toStrictEqual(noRetryTokenAvailableError);
       }
     });
 
-    it("throws when no tokens are available", async () => {
+    it("throws when attempts exceeds default max attempts (3)", async () => {
       const mockRetryToken = {
-        getRetryCount: () => 0,
+        getRetryCount: () => 5,
         getRetryTokenCount: (errorInfo: any) => 1,
-        hasRetryTokens: (errorType: RetryErrorType) => false,
       };
       (createDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken);
-      const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(maxAttempts));
+      const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(5));
       const token = await retryStrategy.acquireInitialRetryToken(retryTokenScope);
       try {
         await retryStrategy.refreshRetryTokenForRetry(token, errorInfo);
+        fail(`expected ${noRetryTokenAvailableError}`);
       } catch (error) {
         expect(error).toStrictEqual(noRetryTokenAvailableError);
       }
@@ -125,6 +127,7 @@ describe(StandardRetryStrategy.name, () => {
       } as RetryErrorInfo;
       try {
         await retryStrategy.refreshRetryTokenForRetry(token, errorInfo);
+        fail(`expected ${noRetryTokenAvailableError}`);
       } catch (error) {
         expect(error).toStrictEqual(noRetryTokenAvailableError);
       }

packages/util-retry/src/StandardRetryStrategy.ts

@@ -86,7 +86,7 @@ export class StandardRetryStrategy implements RetryStrategyV2 {
   }
 
   private shouldRetry(tokenToRenew: StandardRetryToken, errorInfo: RetryErrorInfo, maxAttempts: number): boolean {
-    const attempts = tokenToRenew.getRetryCount();
+    const attempts = tokenToRenew.getRetryCount() + 1;
 
     return (
       attempts < maxAttempts &&

private/aws-client-retry-test/src/ClientRetryTest.spec.ts

@@ -89,21 +89,23 @@ describe("util-retry integration tests", () => {
     } catch (error) {
       expect(error).toStrictEqual(expectedException);
       expect(error.$metadata.httpStatusCode).toBe(429);
-      expect(error.$metadata.attempts).toBe(4);
+      expect(error.$metadata.attempts).toBe(3);
       expect(error.$metadata.totalRetryDelay).toBeGreaterThan(0);
     }
   });
 
   it("should use a shared capacity for retries", async () => {
     const expectedInitialCapacity = 500;
     const expectedDrainPerAttempt = 5;
-    const expectedRetryAttemptsPerRequest = 7;
+    const expectedRetryAttemptsPerRequest = 6;
+    // Set maxAttempts to one more than the number of retries (initial request + retries)
+    const maxAttempts = 7;
     const delayPerRetry = 1;
     const expectedRequests = 4;
     const expectedRemainingCapacity =
       expectedInitialCapacity - expectedDrainPerAttempt * expectedRetryAttemptsPerRequest * expectedRequests;
 
-    const retryStrategy = new ConfiguredRetryStrategy(expectedRetryAttemptsPerRequest, delayPerRetry);
+    const retryStrategy = new ConfiguredRetryStrategy(maxAttempts, delayPerRetry);
     const s3 = new S3({
       requestHandler: new MockRequestHandler(),
       retryStrategy,