Merge pull request #2243 from GhadimiR/main

Don't retry 429s returned from the cache service

packages/cache/RELEASES.md

@@ -1,5 +1,9 @@
 # @actions/cache Releases
 
+### 5.0.3
+
+Prevent retries for rate limited cache operations [2243](https://github.com/actions/toolkit/pull/2243).
+
 ### 5.0.1
 
 - Fix Node.js 24 punycode deprecation warning by updating `@azure/storage-blob` from `^12.13.0` to `^12.29.1` [#2213](https://github.com/actions/toolkit/pull/2213)

packages/cache/__tests__/cacheTwirpClient.test.ts

@@ -0,0 +1,174 @@
+import * as http from 'http'
+import * as net from 'net'
+import {HttpClient} from '@actions/http-client'
+import * as core from '@actions/core'
+import * as config from '../src/internal/config'
+import * as cacheUtils from '../src/internal/cacheUtils'
+import {internalCacheTwirpClient} from '../src/internal/shared/cacheTwirpClient'
+
+jest.mock('@actions/http-client')
+
+const clientOptions = {
+  maxAttempts: 5,
+  retryIntervalMs: 1,
+  retryMultiplier: 1.5
+}
+
+// noopLogs mocks the console.log and core.* functions to prevent output in the console while testing
+const noopLogs = (): void => {
+  jest.spyOn(console, 'log').mockImplementation(() => {})
+  jest.spyOn(core, 'debug').mockImplementation(() => {})
+  jest.spyOn(core, 'info').mockImplementation(() => {})
+  jest.spyOn(core, 'warning').mockImplementation(() => {})
+}
+
+describe('cacheTwirpClient', () => {
+  beforeAll(() => {
+    noopLogs()
+    jest
+      .spyOn(config, 'getCacheServiceURL')
+      .mockReturnValue('http://localhost:8080')
+    jest.spyOn(cacheUtils, 'getRuntimeToken').mockReturnValue('token')
+  })
+
+  beforeEach(() => {
+    jest.clearAllMocks()
+  })
+
+  it('should fail immediately on 429 rate limit without retrying', async () => {
+    const mockPost = jest.fn(() => {
+      const msg = new http.IncomingMessage(new net.Socket())
+      msg.statusCode = 429
+      msg.statusMessage = 'Too Many Requests'
+      return {
+        message: msg,
+        readBody: async () => {
+          return Promise.resolve(`{"ok": false}`)
+        }
+      }
+    })
+
+    ;(HttpClient as unknown as jest.Mock).mockImplementation(() => {
+      return {
+        post: mockPost
+      }
+    })
+
+    const client = internalCacheTwirpClient(clientOptions)
+    await expect(
+      client.CreateCacheEntry({
+        key: 'test-key',
+        version: 'test-version'
+      })
+    ).rejects.toThrow(
+      'Failed to CreateCacheEntry: Rate limited: Failed request: (429) Too Many Requests'
+    )
+
+    // Should only be called once - no retries for 429
+    expect(mockPost).toHaveBeenCalledTimes(1)
+  })
+
+  it('should log warning with retry-after header on 429', async () => {
+    const warningSpy = jest.spyOn(core, 'warning')
+
+    const mockPost = jest.fn(() => {
+      const msg = new http.IncomingMessage(new net.Socket())
+      msg.statusCode = 429
+      msg.statusMessage = 'Too Many Requests'
+      msg.headers = {'retry-after': '60'}
+      return {
+        message: msg,
+        readBody: async () => {
+          return Promise.resolve(`{"ok": false}`)
+        }
+      }
+    })
+
+    ;(HttpClient as unknown as jest.Mock).mockImplementation(() => {
+      return {
+        post: mockPost
+      }
+    })
+
+    const client = internalCacheTwirpClient(clientOptions)
+    await expect(
+      client.CreateCacheEntry({
+        key: 'test-key',
+        version: 'test-version'
+      })
+    ).rejects.toThrow('Rate limited')
+
+    expect(mockPost).toHaveBeenCalledTimes(1)
+    expect(warningSpy).toHaveBeenCalledWith(
+      "You've hit a rate limit, your rate limit will reset in 60 seconds"
+    )
+  })
+
+  it('should not log warning if retry-after header is missing on 429', async () => {
+    const warningSpy = jest.spyOn(core, 'warning')
+
+    const mockPost = jest.fn(() => {
+      const msg = new http.IncomingMessage(new net.Socket())
+      msg.statusCode = 429
+      msg.statusMessage = 'Too Many Requests'
+      // No retry-after header
+      return {
+        message: msg,
+        readBody: async () => {
+          return Promise.resolve(`{"ok": false}`)
+        }
+      }
+    })
+
+    ;(HttpClient as unknown as jest.Mock).mockImplementation(() => {
+      return {
+        post: mockPost
+      }
+    })
+
+    const client = internalCacheTwirpClient(clientOptions)
+    await expect(
+      client.CreateCacheEntry({
+        key: 'test-key',
+        version: 'test-version'
+      })
+    ).rejects.toThrow('Rate limited')
+
+    expect(mockPost).toHaveBeenCalledTimes(1)
+    expect(warningSpy).not.toHaveBeenCalled()
+  })
+
+  it('should not log warning if retry-after header is invalid on 429', async () => {
+    const warningSpy = jest.spyOn(core, 'warning')
+
+    const mockPost = jest.fn(() => {
+      const msg = new http.IncomingMessage(new net.Socket())
+      msg.statusCode = 429
+      msg.statusMessage = 'Too Many Requests'
+      msg.headers = {'retry-after': 'invalid'}
+      return {
+        message: msg,
+        readBody: async () => {
+          return Promise.resolve(`{"ok": false}`)
+        }
+      }
+    })
+
+    ;(HttpClient as unknown as jest.Mock).mockImplementation(() => {
+      return {
+        post: mockPost
+      }
+    })
+
+    const client = internalCacheTwirpClient(clientOptions)
+    await expect(
+      client.CreateCacheEntry({
+        key: 'test-key',
+        version: 'test-version'
+      })
+    ).rejects.toThrow('Rate limited')
+
+    expect(mockPost).toHaveBeenCalledTimes(1)
+    expect(warningSpy).not.toHaveBeenCalled()
+  })
+})

packages/cache/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@actions/cache",
-  "version": "5.0.2",
+  "version": "5.0.3",
   "preview": true,
   "description": "Actions cache lib",
   "keywords": [

packages/cache/src/internal/shared/cacheTwirpClient.ts

@@ -1,6 +1,6 @@
-import {info, debug} from '@actions/core'
+import {info, debug, warning} from '@actions/core'
 import {getUserAgentString} from './user-agent'
-import {NetworkError, UsageError} from './errors'
+import {NetworkError, RateLimitError, UsageError} from './errors'
 import {getCacheServiceURL} from '../config'
 import {getRuntimeToken} from '../cacheUtils'
 import {BearerCredentialHandler} from '@actions/http-client/lib/auth'
@@ -109,6 +109,21 @@ class CacheServiceClient implements Rpc {
 
           errorMessage = `${errorMessage}: ${body.msg}`
         }
+
+        // Handle rate limiting - don't retry, just warn and exit
+        // For more info, see https://docs.github.com/en/actions/reference/limits
+        if (statusCode === HttpCodes.TooManyRequests) {
+          const retryAfterHeader = response.message.headers['retry-after']
+          if (retryAfterHeader) {
+            const parsedSeconds = parseInt(retryAfterHeader, 10)
+            if (!isNaN(parsedSeconds) && parsedSeconds > 0) {
+              warning(
+                `You've hit a rate limit, your rate limit will reset in ${parsedSeconds} seconds`
+              )
+            }
+          }
+          throw new RateLimitError(`Rate limited: ${errorMessage}`)
+        }
       } catch (error) {
         if (error instanceof SyntaxError) {
           debug(`Raw Body: ${rawBody}`)
@@ -118,6 +133,10 @@ class CacheServiceClient implements Rpc {
           throw error
         }
 
+        if (error instanceof RateLimitError) {
+          throw error
+        }
+
         if (NetworkError.isNetworkErrorCode(error?.code)) {
           throw new NetworkError(error?.code)
         }
@@ -162,8 +181,7 @@ class CacheServiceClient implements Rpc {
       HttpCodes.BadGateway,
       HttpCodes.GatewayTimeout,
       HttpCodes.InternalServerError,
-      HttpCodes.ServiceUnavailable,
+      HttpCodes.ServiceUnavailable
-      HttpCodes.TooManyRequests
     ]
 
     return retryableStatusCodes.includes(statusCode)

packages/cache/src/internal/shared/errors.ts

@@ -70,3 +70,10 @@ export class UsageError extends Error {
     return msg.includes('insufficient usage')
   }
 }
+
+export class RateLimitError extends Error {
+  constructor(message: string) {
+    super(message)
+    this.name = 'RateLimitError'
+  }
+}