From db2162799533caab4cc6e54ebc102be1838822b3 Mon Sep 17 00:00:00 2001
From: Brian Cristante <33549821+brcrista@users.noreply.github.com>
Date: Fri, 4 Jun 2021 17:09:30 -0400
Subject: [PATCH] Retry artifact uploads on HTTP 500 (#833)

* Retry on 500

* bump package version

* fix tests

* Remove spurious change

* fix another test

* Roll back package version
---
 packages/artifact/__tests__/download.test.ts | 8 ++++----
 packages/artifact/__tests__/retry.test.ts    | 6 +++---
 packages/artifact/src/internal/utils.ts      | 3 ++-
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/packages/artifact/__tests__/download.test.ts b/packages/artifact/__tests__/download.test.ts
index 3b6ee3ed..d150d220 100644
--- a/packages/artifact/__tests__/download.test.ts
+++ b/packages/artifact/__tests__/download.test.ts
@@ -71,7 +71,7 @@ describe('Download Tests', () => {
     setupFailedResponse()
     const downloadHttpClient = new DownloadHttpClient()
     expect(downloadHttpClient.listArtifacts()).rejects.toThrow(
-      'List Artifacts failed: Artifact service responded with 500'
+      'List Artifacts failed: Artifact service responded with 400'
     )
   })
 
@@ -113,7 +113,7 @@ describe('Download Tests', () => {
         configVariables.getRuntimeUrl()
       )
     ).rejects.toThrow(
-      `Get Container Items failed: Artifact service responded with 500`
+      `Get Container Items failed: Artifact service responded with 400`
     )
   })
 
@@ -166,7 +166,7 @@ describe('Download Tests', () => {
   it('Test retryable status codes during artifact download', async () => {
     // The first http response should return a retryable status call while the subsequent call should return a 200 so
     // the download should successfully finish
-    const retryableStatusCodes = [429, 502, 503, 504]
+    const retryableStatusCodes = [429, 500, 502, 503, 504]
     for (const statusCode of retryableStatusCodes) {
       const fileContents = Buffer.from('try, try again\n', defaultEncoding)
       const targetPath = path.join(root, `FileC-${statusCode}.txt`)
@@ -468,7 +468,7 @@ describe('Download Tests', () => {
   function setupFailedResponse(): void {
     jest.spyOn(HttpClient.prototype, 'get').mockImplementationOnce(async () => {
       const mockMessage = new http.IncomingMessage(new net.Socket())
-      mockMessage.statusCode = 500
+      mockMessage.statusCode = 400
       return new Promise<HttpClientResponse>(resolve => {
         resolve({
           message: mockMessage,
diff --git a/packages/artifact/__tests__/retry.test.ts b/packages/artifact/__tests__/retry.test.ts
index 102342a9..12c49784 100644
--- a/packages/artifact/__tests__/retry.test.ts
+++ b/packages/artifact/__tests__/retry.test.ts
@@ -107,8 +107,8 @@ test('retry fails after exhausting retries', async () => {
 })
 
 test('retry fails after non-retryable status code', async () => {
-  await testRetry([500, 200], {
-    responseCode: 500,
-    errorMessage: 'test failed: Artifact service responded with 500'
+  await testRetry([400, 200], {
+    responseCode: 400,
+    errorMessage: 'test failed: Artifact service responded with 400'
   })
 })
diff --git a/packages/artifact/src/internal/utils.ts b/packages/artifact/src/internal/utils.ts
index 9e1b0123..8b5ac580 100644
--- a/packages/artifact/src/internal/utils.ts
+++ b/packages/artifact/src/internal/utils.ts
@@ -72,8 +72,9 @@ export function isRetryableStatusCode(statusCode: number | undefined): boolean {
 
   const retryableStatusCodes = [
     HttpCodes.BadGateway,
-    HttpCodes.ServiceUnavailable,
     HttpCodes.GatewayTimeout,
+    HttpCodes.InternalServerError,
+    HttpCodes.ServiceUnavailable,
     HttpCodes.TooManyRequests,
     413 // Payload Too Large
   ]