From 1ef26b239071067a8d6fe5953e413fc512fba8b1 Mon Sep 17 00:00:00 2001
From: Dave Hadka <dhadka@github.com>
Date: Mon, 17 Aug 2020 10:48:15 -0500
Subject: [PATCH] Handle errors representing non-successful http responses in
 retry logic

---
 packages/cache/__tests__/requestUtils.test.ts | 48 ++++++++++++++++++-
 packages/cache/src/internal/requestUtils.ts   | 48 ++++++++++++++-----
 2 files changed, 84 insertions(+), 12 deletions(-)

diff --git a/packages/cache/__tests__/requestUtils.test.ts b/packages/cache/__tests__/requestUtils.test.ts
index 27fef955..0d78320d 100644
--- a/packages/cache/__tests__/requestUtils.test.ts
+++ b/packages/cache/__tests__/requestUtils.test.ts
@@ -13,8 +13,16 @@ async function handleResponse(
     fail('Retry method called too many times')
   }
 
-  if (response.statusCode === 999) {
+  // Status codes >= 600 will throw an Error instead of returning a response object. This
+  // mimics the behavior of the http-client *Json methods, which throw an error on any
+  // non-successful status codes.  Values in the 6xx, 7xx, and 8xx range are converted
+  // to the corresponding 3xx, 4xx, and 5xx status code.
+  if (response.statusCode >= 900) {
     throw Error('Test Error')
+  } else if (response.statusCode >= 600) {
+    const error: any = Error('Test Error with Status Code')
+    error['statusCode'] = response.statusCode - 300
+    throw error
   } else {
     return Promise.resolve(response)
   }
@@ -35,6 +43,31 @@ async function testRetryExpectingResult(
   expect(actualResult.result).toEqual(expectedResult)
 }
 
+async function testRetryConvertingErrorToResult(
+  responses: TestResponse[],
+  expectedStatus: number,
+  expectedResult: string | null
+): Promise<void> {
+  responses = responses.reverse() // Reverse responses since we pop from end
+
+  const actualResult = await retry(
+    'test',
+    async () => handleResponse(responses.pop()),
+    (response: TestResponse) => response.statusCode,
+    2,
+    (e: Error) => {
+      const error: any = e
+      return {
+        statusCode: error['statusCode'],
+        result: error['result']
+      }
+    }
+  )
+
+  expect(actualResult.statusCode).toEqual(expectedStatus)
+  expect(actualResult.result).toEqual(expectedResult)
+}
+
 async function testRetryExpectingError(
   responses: TestResponse[]
 ): Promise<void> {
@@ -138,3 +171,16 @@ test('retry returns after client error', async () => {
     null
   )
 })
+
+test('retry converts errors to response object', async () => {
+  await testRetryConvertingErrorToResult(
+    [
+      {
+        statusCode: 709, // throw a 409 Conflict error
+        result: null
+      }
+    ],
+    409,
+    null
+  )
+})
diff --git a/packages/cache/src/internal/requestUtils.ts b/packages/cache/src/internal/requestUtils.ts
index c72728cb..d489458b 100644
--- a/packages/cache/src/internal/requestUtils.ts
+++ b/packages/cache/src/internal/requestUtils.ts
@@ -35,30 +35,41 @@ export async function retry<T>(
   name: string,
   method: () => Promise<T>,
   getStatusCode: (arg0: T) => number | undefined,
-  maxAttempts = 2
+  maxAttempts = 2,
+  onError: ((arg0: Error) => T | undefined) | undefined = undefined
 ): Promise<T> {
-  let response: T | undefined = undefined
-  let statusCode: number | undefined = undefined
-  let isRetryable = false
   let errorMessage = ''
   let attempt = 1
 
   while (attempt <= maxAttempts) {
+    let response: T | undefined = undefined
+    let statusCode: number | undefined = undefined
+    let isRetryable = false
+
     try {
       response = await method()
+    } catch (error) {
+      if (onError) {
+        response = onError(error)
+      }
+
+      isRetryable = true
+      errorMessage = error.message
+    }
+
+    if (response) {
       statusCode = getStatusCode(response)
 
       if (!isServerErrorStatusCode(statusCode)) {
         return response
       }
-
-      isRetryable = isRetryableStatusCode(statusCode)
-      errorMessage = `Cache service responded with ${statusCode}`
-    } catch (error) {
-      isRetryable = true
-      errorMessage = error.message
     }
 
+    if (statusCode) {
+      isRetryable = isRetryableStatusCode(statusCode)
+      errorMessage = `Cache service responded with ${statusCode}`
+    }
+    
     core.debug(
       `${name} - Attempt ${attempt} of ${maxAttempts} failed with error: ${errorMessage}`
     )
@@ -83,7 +94,22 @@ export async function retryTypedResponse<T>(
     name,
     method,
     (response: ITypedResponse<T>) => response.statusCode,
-    maxAttempts
+    maxAttempts,
+    // If the error object contains the statusCode property, extract it and return
+    // an ITypedResponse<T> so it can be processed by the retry logic.  Explicitly
+    // casting Error object to any to workaround missing property errors.
+    (e: Error) => {
+      const error : any = e
+      if (error['statusCode']) {
+        return {
+          statusCode: error['statusCode'],
+          result: null,
+          headers: {}
+        }
+      } else {
+        return undefined
+      }
+    }
   )
 }