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

@@ -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)):

__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<typeof internalArtifactTwirpClient>[0]
+    ): Promise<unknown> =>
+      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', '<html>rate limited</html>'))
+
+      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])
+    })
+  })
 })

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",

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@actions/artifact",
-  "version": "6.2.2",
+  "version": "6.3.0",
   "preview": true,
   "description": "Actions artifact lib",
   "keywords": [

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<void> {
     return new Promise(resolve => setTimeout(resolve, milliseconds))
   }