diff --git a/packages/cache/RELEASES.md b/packages/cache/RELEASES.md index 08d484e0..3acb9fd6 100644 --- a/packages/cache/RELEASES.md +++ b/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) diff --git a/packages/cache/__tests__/cacheTwirpClient.test.ts b/packages/cache/__tests__/cacheTwirpClient.test.ts new file mode 100644 index 00000000..d74bfd3a --- /dev/null +++ b/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() + }) +}) diff --git a/packages/cache/package.json b/packages/cache/package.json index ca2e145c..eb7531ed 100644 --- a/packages/cache/package.json +++ b/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": [ diff --git a/packages/cache/src/internal/shared/cacheTwirpClient.ts b/packages/cache/src/internal/shared/cacheTwirpClient.ts index f6c2af61..a6e2d0ad 100644 --- a/packages/cache/src/internal/shared/cacheTwirpClient.ts +++ b/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.TooManyRequests + HttpCodes.ServiceUnavailable ] return retryableStatusCodes.includes(statusCode) diff --git a/packages/cache/src/internal/shared/errors.ts b/packages/cache/src/internal/shared/errors.ts index 9ec29f6b..5e90a299 100644 --- a/packages/cache/src/internal/shared/errors.ts +++ b/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' + } +}