From 110ff4d59f2d64716542c8fe6059d49ec419e1d8 Mon Sep 17 00:00:00 2001 From: tunc-d <46646394+tunc-d@users.noreply.github.com> Date: Tue, 6 Oct 2026 11:39:25 +0200 Subject: [PATCH] artifact: honor Retry-After on HTTP 429 and prepare 6.3.0 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- RELEASES.md | 5 + __tests__/artifact-http-client.test.ts | 245 +++++++++++++++++++ package-lock.json | 4 +- package.json | 2 +- src/internal/shared/artifact-twirp-client.ts | 33 ++- 5 files changed, 284 insertions(+), 5 deletions(-) diff --git a/RELEASES.md b/RELEASES.md index 5dbc962..26184c8 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -1,5 +1,10 @@ # @actions/artifact Releases +## 6.3.0 + +- Honor positive integer `Retry-After` seconds on HTTP 429 responses, including when the response body is not valid JSON. Unsupported or missing values use exponential backoff. +- Increase the initial fallback retry wait to 8 seconds and limit accumulated retry sleep to 120 seconds. + ## 6.2.2 - Bump dependency versions ([#2393](https://github.com/actions/toolkit/pull/2393)): diff --git a/__tests__/artifact-http-client.test.ts b/__tests__/artifact-http-client.test.ts index 84a50a6..64efcb8 100644 --- a/__tests__/artifact-http-client.test.ts +++ b/__tests__/artifact-http-client.test.ts @@ -345,4 +345,249 @@ describe('artifact-http-client', () => { expect(mockHttpClient).toHaveBeenCalledTimes(1) expect(mockPost).toHaveBeenCalledTimes(1) }) + + describe('retry waits', () => { + let sleepSpy: jest.SpyInstance + let randomSpy: jest.SpyInstance | undefined + + const createArtifact = async ( + options?: Parameters[0] + ): Promise => + internalArtifactTwirpClient(options).CreateArtifact({ + workflowRunBackendId: '1234', + workflowJobRunBackendId: '5678', + name: 'artifact', + version: 4 + }) + + const response = ( + statusCode: number, + statusMessage: string, + headers: http.IncomingHttpHeaders = {}, + body = `{"ok": false}` + ): object => { + const msg = new http.IncomingMessage(new net.Socket()) + msg.statusCode = statusCode + msg.statusMessage = statusMessage + msg.headers = headers + return {message: msg, readBody: async () => Promise.resolve(body)} + } + + const rateLimited = (retryAfter?: string, body = `{"ok": false}`): object => + response( + 429, + 'Too Many Requests', + retryAfter === undefined ? {} : {'retry-after': retryAfter}, + body + ) + + const serverError = (): object => response(500, 'Internal Server Error') + + const mockResponses = (...responses: object[]): jest.Mock => { + const mockPost = jest.fn(() => response(200, 'OK', {}, `{"ok": true}`)) + for (const r of responses) { + mockPost.mockImplementationOnce(() => r) + } + ;(HttpClient as unknown as jest.Mock).mockImplementation(() => ({ + post: mockPost + })) + return mockPost + } + + const sleepTimes = (): number[] => + sleepSpy.mock.calls.map(call => call[1] as number) + + const sum = (values: number[]): number => + values.reduce((total, value) => total + value, 0) + + beforeEach(() => { + sleepSpy = jest.spyOn(global, 'setTimeout').mockImplementation((( + callback: () => void + ) => { + callback() + return undefined + }) as unknown as typeof setTimeout) + }) + + afterEach(() => { + sleepSpy.mockRestore() + randomSpy?.mockRestore() + randomSpy = undefined + }) + + it.each([ + ['30', 30000], + ['010', 10000], + ['10 ', 10000], + [' 10 ', 10000] + ])('should wait for a Retry-After of %p on 429', async (header, wait) => { + const mockPost = mockResponses(rateLimited(header)) + + await createArtifact() + + expect(mockPost).toHaveBeenCalledTimes(2) + expect(sleepTimes()).toEqual([wait]) + }) + + it('should read Retry-After before parsing the body', async () => { + mockResponses(rateLimited('20', 'rate limited')) + + await createArtifact() + + expect(sleepTimes()).toEqual([20000]) + }) + + it.each([ + ['missing', undefined], + ['empty', ''], + ['whitespace', ' '], + ['zero', '0'], + ['padded zero', '000'], + ['negative', '-5'], + ['fractional', '1.5'], + ['decimal', '1.0'], + ['a positive sign', '+5'], + ['hexadecimal', '0x10'], + ['scientific notation', '1e2'], + ['trailing junk', '10seconds'], + ['prefix junk', 'seconds10'], + ['embedded whitespace', '1 0'], + ['non-numeric', 'abc'], + ['an HTTP-date', 'Wed, 21 Oct 2015 07:28:00 GMT'] + ])('should use backoff when Retry-After is %s', async (_, header) => { + mockResponses(rateLimited(header)) + + await createArtifact() + + expect(sleepTimes()).toEqual([8000]) + }) + + it.each([ + [503, 'Service Unavailable'], + [500, 'Internal Server Error'], + [502, 'Bad Gateway'], + [504, 'Gateway Timeout'] + ])('should ignore Retry-After on %s', async (statusCode, statusMessage) => { + mockResponses(response(statusCode, statusMessage, {'retry-after': '30'})) + + await createArtifact() + + expect(sleepTimes()).toEqual([8000]) + }) + + it('should fail without sleeping once the retry timeout is exhausted', async () => { + const mockPost = mockResponses( + rateLimited('60'), + rateLimited('60'), + rateLimited('60') + ) + + await expect(createArtifact()).rejects.toThrow( + 'Failed to CreateArtifact: Retry wait of 60000 ms would exceed the maximum total retry wait of 120000 ms: Failed request: (429) Too Many Requests' + ) + expect(mockPost).toHaveBeenCalledTimes(3) + expect(sleepTimes()).toEqual([60000, 60000]) + }) + + it.each([ + ['a 121s Retry-After', [rateLimited('121')], [], 1], + [ + 'a 113s Retry-After after an 8s backoff', + [serverError(), rateLimited('113')], + [8000], + 2 + ] + ])( + 'should fail before sleeping when %s would exceed the retry timeout', + async (_, responses, waits, attempts) => { + const mockPost = mockResponses(...responses) + + await expect(createArtifact()).rejects.toThrow( + 'would exceed the maximum total retry wait of 120000 ms' + ) + expect(sleepTimes()).toEqual(waits) + expect(mockPost).toHaveBeenCalledTimes(attempts) + } + ) + + it('should allow a wait that exactly fills the retry timeout', async () => { + const mockPost = mockResponses(rateLimited('120')) + + await createArtifact() + + expect(sleepTimes()).toEqual([120000]) + expect(mockPost).toHaveBeenCalledTimes(2) + }) + + it('should allow mixed waits that exactly fill the retry timeout', async () => { + const mockPost = mockResponses(serverError(), rateLimited('112')) + + await createArtifact() + + expect(sleepTimes()).toEqual([8000, 112000]) + expect(mockPost).toHaveBeenCalledTimes(3) + }) + + it('should use backoff after a 429 without reusing its Retry-After', async () => { + randomSpy = jest.spyOn(Math, 'random').mockReturnValue(0) + const mockPost = mockResponses(rateLimited('30'), serverError()) + + await createArtifact() + + expect(sleepTimes()).toEqual([30000, 12000]) + expect(mockPost).toHaveBeenCalledTimes(3) + }) + + it('should stop after five attempts without sleeping after the last failure', async () => { + const mockPost = mockResponses( + rateLimited('30'), + rateLimited('30'), + rateLimited('30'), + rateLimited('30'), + rateLimited('30') + ) + + await expect(createArtifact()).rejects.toThrow( + 'Failed to make request after 5 attempts: Failed request: (429) Too Many Requests' + ) + + expect(mockPost).toHaveBeenCalledTimes(5) + expect(sleepTimes()).toEqual([30000, 30000, 30000, 30000]) + }) + + it.each([ + ['minimum', 0, [8000, 12000, 18000, 27000]], + ['maximum', 0.9999999, [8000, 17999, 26999, 40499]] + ])( + 'should use the %s default backoff waits within the retry timeout', + async (_, random, waits) => { + randomSpy = jest.spyOn(Math, 'random').mockReturnValue(random) + const mockPost = mockResponses( + serverError(), + serverError(), + serverError(), + serverError() + ) + + await createArtifact() + + expect(mockPost).toHaveBeenCalledTimes(5) + expect(sleepTimes()).toEqual(waits) + expect(sum(sleepTimes())).toBeGreaterThanOrEqual(60000) + expect(sum(sleepTimes())).toBeLessThanOrEqual(110000) + } + ) + + it('should fail when a custom backoff wait exceeds the retry timeout', async () => { + randomSpy = jest.spyOn(Math, 'random').mockReturnValue(0) + mockResponses(serverError(), serverError(), serverError()) + + await expect( + createArtifact({retryIntervalMs: 10000, retryMultiplier: 3}) + ).rejects.toThrow( + 'Retry wait of 90000 ms would exceed the maximum total retry wait of 120000 ms: Failed request: (500) Internal Server Error' + ) + expect(sleepTimes()).toEqual([10000, 30000]) + }) + }) }) diff --git a/package-lock.json b/package-lock.json index 01cf77b..eb3f82d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@actions/artifact", - "version": "6.2.2", + "version": "6.3.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@actions/artifact", - "version": "6.2.2", + "version": "6.3.0", "license": "MIT", "dependencies": { "@actions/core": "^3.0.1", diff --git a/package.json b/package.json index 38bf353..4d99ef8 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@actions/artifact", - "version": "6.2.2", + "version": "6.3.0", "preview": true, "description": "Actions artifact lib", "keywords": [ diff --git a/src/internal/shared/artifact-twirp-client.ts b/src/internal/shared/artifact-twirp-client.ts index 04958a7..e48d1a7 100644 --- a/src/internal/shared/artifact-twirp-client.ts +++ b/src/internal/shared/artifact-twirp-client.ts @@ -21,8 +21,9 @@ class ArtifactHttpClient implements Rpc { private httpClient: HttpClient private baseUrl: string private maxAttempts = 5 - private baseRetryIntervalMilliseconds = 3000 + private baseRetryIntervalMilliseconds = 8000 private retryMultiplier = 1.5 + private retryTimeoutMilliseconds = 120000 constructor( userAgent: string, @@ -77,12 +78,17 @@ class ArtifactHttpClient implements Rpc { let attempt = 0 let errorMessage = '' let rawBody = '' + let totalRetryWaitMilliseconds = 0 while (attempt < this.maxAttempts) { let isRetryable = false + let retryAfterSeconds: number | undefined try { const response = await operation() const statusCode = response.message.statusCode + if (statusCode === HttpCodes.TooManyRequests) { + retryAfterSeconds = this.getRetryAfterSeconds(response) + } rawBody = await response.readBody() debug(`[Response] - ${response.message.statusCode}`) debug(`Headers: ${JSON.stringify(response.message.headers, null, 2)}`) @@ -129,13 +135,24 @@ class ArtifactHttpClient implements Rpc { } const retryTimeMilliseconds = - this.getExponentialRetryTimeMilliseconds(attempt) + retryAfterSeconds !== undefined + ? retryAfterSeconds * 1000 + : this.getExponentialRetryTimeMilliseconds(attempt) + if ( + totalRetryWaitMilliseconds + retryTimeMilliseconds > + this.retryTimeoutMilliseconds + ) { + throw new Error( + `Retry wait of ${retryTimeMilliseconds} ms would exceed the maximum total retry wait of ${this.retryTimeoutMilliseconds} ms: ${errorMessage}` + ) + } info( `Attempt ${attempt + 1} of ${ this.maxAttempts } failed with error: ${errorMessage}. Retrying request in ${retryTimeMilliseconds} ms...` ) await this.sleep(retryTimeMilliseconds) + totalRetryWaitMilliseconds += retryTimeMilliseconds attempt++ } @@ -161,6 +178,18 @@ class ArtifactHttpClient implements Rpc { return retryableStatusCodes.includes(statusCode) } + // Only positive integer seconds are supported, not HTTP-date values. + getRetryAfterSeconds(response: HttpClientResponse): number | undefined { + const header = response.message.headers['retry-after'] + const value = (Array.isArray(header) ? header[0] : header)?.trim() + if (value === undefined || !/^\d+$/.test(value)) { + return undefined + } + + const parsed = parseInt(value, 10) + return !isNaN(parsed) && parsed > 0 ? parsed : undefined + } + async sleep(milliseconds: number): Promise { return new Promise(resolve => setTimeout(resolve, milliseconds)) }