diff --git a/packages/cache/RELEASES.md b/packages/cache/RELEASES.md index 08d484e0..140f17db 100644 --- a/packages/cache/RELEASES.md +++ b/packages/cache/RELEASES.md @@ -1,5 +1,9 @@ # @actions/cache Releases +### 5.0.2 + +Fail cache saves on rate limit errors from the cache service to prevent slowing down impacted runs + ### 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/src/internal/shared/cacheTwirpClient.ts b/packages/cache/src/internal/shared/cacheTwirpClient.ts index ade1b8c9..72bd6c0a 100644 --- a/packages/cache/src/internal/shared/cacheTwirpClient.ts +++ b/packages/cache/src/internal/shared/cacheTwirpClient.ts @@ -136,6 +136,11 @@ class CacheServiceClient implements Rpc { throw new NetworkError(error?.code) } + // Re-throw rate limit errors without retry + if (error.message?.startsWith('Rate limited:')) { + throw error + } + isRetryable = true errorMessage = error.message }