From 0108c47863462e513824325359d14843b54ea1da Mon Sep 17 00:00:00 2001 From: Nelson Wolf Date: Fri, 17 Jul 2026 10:05:47 -0700 Subject: [PATCH] Retry intermediary 403s in artifact-twirp-client The artifact Results Service's internal Twirp client treats every HTTP 403 as non-retryable, on the assumption it always reflects a genuine authorization failure. In practice its edge/proxy layer intermittently returns its own 403 ("Error from intermediary with HTTP status code 403"), which is a transient infrastructure hiccup rather than an auth decision - the same artifact download succeeds moments later from a parallel job in the same run. isRetryableHttpStatusCode now also retries a 403 when the error message identifies it as coming from an intermediary, while leaving all other 403s (real backend authorization failures) failing fast as before. Fixes the failure mode reported in actions/download-artifact#464. --- .../__tests__/artifact-http-client.test.ts | 85 +++++++++++++++++++ .../internal/shared/artifact-twirp-client.ts | 26 +++++- 2 files changed, 108 insertions(+), 3 deletions(-) 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 {