Retry intermediary 403s in artifact-twirp-client

packages/artifact/__tests__/artifact-http-client.test.ts

@@ -234,6 +234,91 @@ describe('artifact-http-client', () => {
     expect(mockPost).toHaveBeenCalledTimes(1)
   })
 
+  it('should retry a 403 from an intermediary', async () => {
+    const mockPost = jest
+      .fn(() => {
+        const msgSucceeded = new http.IncomingMessage(new net.Socket())
+        msgSucceeded.statusCode = 200
+        return {
+          message: msgSucceeded,
+          readBody: async () => {
+            return Promise.resolve(
+              `{"ok": true, "signedUploadUrl": "http://localhost:8080/upload"}`
+            )
+          }
+        }
+      })
+      .mockImplementationOnce(() => {
+        const msgFailed = new http.IncomingMessage(new net.Socket())
+        msgFailed.statusCode = 403
+        msgFailed.statusMessage = 'Forbidden'
+        return {
+          message: msgFailed,
+          readBody: async () => {
+            return Promise.resolve(
+              `{"msg": "Error from intermediary with HTTP status code 403 \\"Forbidden\\""}`
+            )
+          }
+        }
+      })
+    const mockHttpClient = (
+      HttpClient as unknown as jest.Mock
+    ).mockImplementation(() => {
+      return {
+        post: mockPost
+      }
+    })
+
+    const client = internalArtifactTwirpClient(clientOptions)
+    const artifact = await client.CreateArtifact({
+      workflowRunBackendId: '1234',
+      workflowJobRunBackendId: '5678',
+      name: 'artifact',
+      version: 4
+    })
+
+    expect(mockHttpClient).toHaveBeenCalledTimes(1)
+    expect(artifact).toBeDefined()
+    expect(artifact.ok).toBe(true)
+    expect(artifact.signedUploadUrl).toBe('http://localhost:8080/upload')
+    expect(mockPost).toHaveBeenCalledTimes(2)
+  })
+
+  it('should fail immediately on a 403 that is not from an intermediary', async () => {
+    const mockPost = jest.fn(() => {
+      const msgFailed = new http.IncomingMessage(new net.Socket())
+      msgFailed.statusCode = 403
+      msgFailed.statusMessage = 'Forbidden'
+      return {
+        message: msgFailed,
+        readBody: async () => {
+          return Promise.resolve(`{"ok": false}`)
+        }
+      }
+    })
+
+    const mockHttpClient = (
+      HttpClient as unknown as jest.Mock
+    ).mockImplementation(() => {
+      return {
+        post: mockPost
+      }
+    })
+    const client = internalArtifactTwirpClient(clientOptions)
+    await expect(async () => {
+      await client.CreateArtifact({
+        workflowRunBackendId: '1234',
+        workflowJobRunBackendId: '5678',
+        name: 'artifact',
+        version: 4
+      })
+    }).rejects.toThrowError(
+      'Received non-retryable error: Failed request: (403) Forbidden'
+    )
+    expect(mockHttpClient).toHaveBeenCalledTimes(1)
+    expect(mockPost).toHaveBeenCalledTimes(1)
+  })
+
   it('should fail with a descriptive error', async () => {
     // 409 duplicate error
     const mockPost = jest.fn(() => {

packages/artifact/src/internal/shared/artifact-twirp-client.ts

@@ -92,7 +92,6 @@ class ArtifactHttpClient implements Rpc {
         if (this.isSuccessStatusCode(statusCode)) {
           return {response, body}
         }
-        isRetryable = this.isRetryableHttpStatusCode(statusCode)
         errorMessage = `Failed request: (${statusCode}) ${response.message.statusMessage}`
         if (body.msg) {
           if (UsageError.isUsageErrorMessage(body.msg)) {
@@ -101,6 +100,7 @@ class ArtifactHttpClient implements Rpc {
 
           errorMessage = `${errorMessage}: ${body.msg}`
         }
+        isRetryable = this.isRetryableHttpStatusCode(statusCode, errorMessage)
       } catch (error) {
         if (error instanceof SyntaxError) {
           debug(`Raw Body: ${rawBody}`)
@@ -147,7 +147,10 @@ class ArtifactHttpClient implements Rpc {
     return statusCode >= 200 && statusCode < 300
   }
 
-  isRetryableHttpStatusCode(statusCode?: number): boolean {
+  isRetryableHttpStatusCode(
+    statusCode?: number,
+    errorMessage?: string
+  ): boolean {
     if (!statusCode) return false
 
     const retryableStatusCodes = [
@@ -158,7 +161,24 @@ class ArtifactHttpClient implements Rpc {
       HttpCodes.TooManyRequests
     ]
 
-    return retryableStatusCodes.includes(statusCode)
+    if (retryableStatusCodes.includes(statusCode)) {
+      return true
+    }
+
+    // A 403 from the artifact Results Service usually means a genuine
+    // authorization failure (e.g. an expired runtime token, or an artifact
+    // outside the run's scope), which should fail fast rather than retry.
+    // However, the service's edge/proxy layer can also intermittently
+    // return a 403 of its own, distinguishable by a body identifying it as
+    // coming from an intermediary rather than the backend (e.g. "Error from
+    // intermediary with HTTP status code 403"). That flavor is a transient
+    // infrastructure hiccup, not an authorization decision, so it's safe -
+    // and worthwhile - to retry. See actions/download-artifact#464.
+    if (statusCode === HttpCodes.Forbidden && errorMessage) {
+      return /error from intermediary/i.test(errorMessage)
+    }
+
+    return false
   }
 
   async sleep(milliseconds: number): Promise<void> {