diff --git a/packages/artifact/__tests__/artifact-http-client.test.ts b/packages/artifact/__tests__/artifact-http-client.test.ts index 84a50a6f4a..e8898cfd8e 100644 --- a/packages/artifact/__tests__/artifact-http-client.test.ts +++ b/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(() => { diff --git a/packages/artifact/src/internal/shared/artifact-twirp-client.ts b/packages/artifact/src/internal/shared/artifact-twirp-client.ts index 04958a7840..0d3101ca55 100644 --- a/packages/artifact/src/internal/shared/artifact-twirp-client.ts +++ b/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 {