Merge pull request actions#558 from dhadka/dhadka/fix-error-handling

Handle errors representing non-successful http responses in retry logic

packages/cache/RELEASES.md

@@ -21,3 +21,8 @@
 
 ### 1.0.2
 - Use posix archive format to add support for some tools
+
+### 1.0.3
+- Use http-client v1.0.9
+- Fixes error handling so retries are not attempted on non-retryable errors (409 Conflict, for example)
+- Adds 5 second delay between retry attempts

packages/cache/__tests__/requestUtils.test.ts

@@ -1,140 +1,148 @@
 import {retry} from '../src/internal/requestUtils'
+import {HttpClientError} from '@actions/http-client'
 
-interface TestResponse {
+interface ITestResponse {
   statusCode: number
   result: string | null
+  error: Error | null
+}
+
+function TestResponse(
+  action: number | Error,
+  result: string | null = null
+): ITestResponse {
+  if (action instanceof Error) {
+    return {
+      statusCode: -1,
+      result,
+      error: action
+    }
+  } else {
+    return {
+      statusCode: action,
+      result,
+      error: null
+    }
+  }
 }
 
 async function handleResponse(
-  response: TestResponse | undefined
-): Promise<TestResponse> {
+  response: ITestResponse | undefined
+): Promise<ITestResponse> {
   if (!response) {
     // eslint-disable-next-line no-undef
     fail('Retry method called too many times')
   }
 
-  if (response.statusCode === 999) {
-    throw Error('Test Error')
+  if (response.error) {
+    throw response.error
   } else {
     return Promise.resolve(response)
   }
 }
 
 async function testRetryExpectingResult(
-  responses: TestResponse[],
+  responses: ITestResponse[],
   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
+    (response: ITestResponse) => response.statusCode,
+    2, // maxAttempts
+    0 // delay
   )
 
   expect(actualResult.result).toEqual(expectedResult)
 }
 
+async function testRetryConvertingErrorToResult(
+  responses: ITestResponse[],
+  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: ITestResponse) => response.statusCode,
+    2, // maxAttempts
+    0, // delay
+    (e: Error) => {
+      if (e instanceof HttpClientError) {
+        return {
+          statusCode: e.statusCode,
+          result: null,
+          error: null
+        }
+      }
+    }
+  )
+
+  expect(actualResult.statusCode).toEqual(expectedStatus)
+  expect(actualResult.result).toEqual(expectedResult)
+}
+
 async function testRetryExpectingError(
-  responses: TestResponse[]
+  responses: ITestResponse[]
 ): Promise<void> {
   responses = responses.reverse() // Reverse responses since we pop from end
 
   expect(
     retry(
       'test',
       async () => handleResponse(responses.pop()),
-      (response: TestResponse) => response.statusCode
+      (response: ITestResponse) => response.statusCode,
+      2, // maxAttempts,
+      0 // delay
     )
   ).rejects.toBeInstanceOf(Error)
 }
 
 test('retry works on successful response', async () => {
-  await testRetryExpectingResult(
-    [
-      {
-        statusCode: 200,
-        result: 'Ok'
-      }
-    ],
-    'Ok'
-  )
+  await testRetryExpectingResult([TestResponse(200, 'Ok')], 'Ok')
 })
 
 test('retry works after retryable status code', async () => {
   await testRetryExpectingResult(
-    [
-      {
-        statusCode: 503,
-        result: null
-      },
-      {
-        statusCode: 200,
-        result: 'Ok'
-      }
-    ],
+    [TestResponse(503), TestResponse(200, 'Ok')],
     'Ok'
   )
 })
 
 test('retry fails after exhausting retries', async () => {
   await testRetryExpectingError([
-    {
-      statusCode: 503,
-      result: null
-    },
-    {
-      statusCode: 503,
-      result: null
-    },
-    {
-      statusCode: 200,
-      result: 'Ok'
-    }
+    TestResponse(503),
+    TestResponse(503),
+    TestResponse(200, 'Ok')
   ])
 })
 
 test('retry fails after non-retryable status code', async () => {
-  await testRetryExpectingError([
-    {
-      statusCode: 500,
-      result: null
-    },
-    {
-      statusCode: 200,
-      result: 'Ok'
-    }
-  ])
+  await testRetryExpectingError([TestResponse(500), TestResponse(200, 'Ok')])
 })
 
 test('retry works after error', async () => {
   await testRetryExpectingResult(
-    [
-      {
-        statusCode: 999,
-        result: null
-      },
-      {
-        statusCode: 200,
-        result: 'Ok'
-      }
-    ],
+    [TestResponse(new Error('Test error')), TestResponse(200, 'Ok')],
     'Ok'
   )
 })
 
 test('retry returns after client error', async () => {
   await testRetryExpectingResult(
-    [
-      {
-        statusCode: 400,
-        result: null
-      },
-      {
-        statusCode: 200,
-        result: 'Ok'
-      }
-    ],
+    [TestResponse(400), TestResponse(200, 'Ok')],
+    null
+  )
+})
+
+test('retry converts errors to response object', async () => {
+  await testRetryConvertingErrorToResult(
+    [TestResponse(new HttpClientError('Test error', 409))],
+    409,
     null
   )
 })