Visitar URL original
Don't retry 429s returned from the cache service by GhadimiR · Pull Request #2243 · actions/toolkit · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/cache/RELEASES.md
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
174 changes: 174 additions & 0 deletions packages/cache/__tests__/cacheTwirpClient.test.ts
Original file line number Diff line number Diff line change
@@ -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()
})
})
2 changes: 1 addition & 1 deletion packages/cache/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@actions/cache",
"version": "5.0.2",
"version": "5.0.3",
"preview": true,
"description": "Actions cache lib",
"keywords": [
Expand Down
26 changes: 22 additions & 4 deletions packages/cache/src/internal/shared/cacheTwirpClient.ts
Original file line number Diff line number Diff line change
@@ -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'
Expand Down Expand Up @@ -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`
Comment thread
GhadimiR marked this conversation as resolved.
Resolved
)
}
}
throw new RateLimitError(`Rate limited: ${errorMessage}`)
}
} catch (error) {
if (error instanceof SyntaxError) {
debug(`Raw Body: ${rawBody}`)
Expand All @@ -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)
}
Expand Down Expand Up @@ -162,8 +181,7 @@ class CacheServiceClient implements Rpc {
HttpCodes.BadGateway,
HttpCodes.GatewayTimeout,
HttpCodes.InternalServerError,
HttpCodes.ServiceUnavailable,
HttpCodes.TooManyRequests
HttpCodes.ServiceUnavailable
]

return retryableStatusCodes.includes(statusCode)
Expand Down
7 changes: 7 additions & 0 deletions packages/cache/src/internal/shared/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
}
}
Loading