From 5859d7172e5f4281198f6d74eeb9fe96fb27a23d Mon Sep 17 00:00:00 2001 From: eric sciple Date: Mon, 9 Mar 2020 14:35:53 -0400 Subject: [PATCH] only retry downloadtool on 500s and 408 and 429 (#373) --- packages/tool-cache/RELEASES.md | 4 + .../tool-cache/__tests__/retry-helper.test.ts | 40 ++++++++- .../tool-cache/__tests__/tool-cache.test.ts | 33 +++++++ packages/tool-cache/package.json | 2 +- packages/tool-cache/src/retry-helper.ts | 9 +- packages/tool-cache/src/tool-cache.ts | 86 +++++++++++-------- 6 files changed, 136 insertions(+), 38 deletions(-) diff --git a/packages/tool-cache/RELEASES.md b/packages/tool-cache/RELEASES.md index aeafeffd..7541b4a7 100644 --- a/packages/tool-cache/RELEASES.md +++ b/packages/tool-cache/RELEASES.md @@ -1,5 +1,9 @@ # @actions/tool-cache Releases +### 1.3.3 + +- [Update downloadTool to only retry 500s and 408 and 429](https://github.com/actions/toolkit/pull/373) + ### 1.3.2 - [Update downloadTool with better error handling and retries](https://github.com/actions/toolkit/pull/369) diff --git a/packages/tool-cache/__tests__/retry-helper.test.ts b/packages/tool-cache/__tests__/retry-helper.test.ts index cccf0075..90b2e7c9 100644 --- a/packages/tool-cache/__tests__/retry-helper.test.ts +++ b/packages/tool-cache/__tests__/retry-helper.test.ts @@ -66,7 +66,7 @@ describe('retry-helper tests', () => { expect(info[3]).toMatch(/Waiting .+ seconds before trying again/) }) - it('all attempts fail succeeds', async () => { + it('all attempts fail', async () => { let attempts = 0 let error: Error = (null as unknown) as Error try { @@ -84,4 +84,42 @@ describe('retry-helper tests', () => { expect(info[2]).toBe('some error 2') expect(info[3]).toMatch(/Waiting .+ seconds before trying again/) }) + + it('checks retryable after first attempt', async () => { + let attempts = 0 + let error: Error = (null as unknown) as Error + try { + await retryHelper.execute( + async () => { + throw new Error(`some error ${++attempts}`) + }, + () => false + ) + } catch (err) { + error = err + } + expect(error.message).toBe('some error 1') + expect(attempts).toBe(1) + expect(info).toHaveLength(0) + }) + + it('checks retryable after second attempt', async () => { + let attempts = 0 + let error: Error = (null as unknown) as Error + try { + await retryHelper.execute( + async () => { + throw new Error(`some error ${++attempts}`) + }, + (e: Error) => e.message === 'some error 1' + ) + } catch (err) { + error = err + } + expect(error.message).toBe('some error 2') + expect(attempts).toBe(2) + expect(info).toHaveLength(2) + expect(info[0]).toBe('some error 1') + expect(info[1]).toMatch(/Waiting .+ seconds before trying again/) + }) }) diff --git a/packages/tool-cache/__tests__/tool-cache.test.ts b/packages/tool-cache/__tests__/tool-cache.test.ts index 7888fa2a..42a3e4ea 100644 --- a/packages/tool-cache/__tests__/tool-cache.test.ts +++ b/packages/tool-cache/__tests__/tool-cache.test.ts @@ -619,6 +619,39 @@ describe('@actions/tool-cache', function() { expect(err.toString()).toContain('502') } }) + + it('retries 429s', async function() { + nock('http://example.com') + .get('/too-many-requests-429') + .times(2) + .reply(429, undefined) + nock('http://example.com') + .get('/too-many-requests-429') + .reply(500, undefined) + + try { + const statusCodeUrl = 'http://example.com/too-many-requests-429' + await tc.downloadTool(statusCodeUrl) + } catch (err) { + expect(err.toString()).toContain('500') + } + }) + + it("doesn't retry 404", async function() { + nock('http://example.com') + .get('/not-found-404') + .reply(404, undefined) + nock('http://example.com') + .get('/not-found-404') + .reply(500, undefined) + + try { + const statusCodeUrl = 'http://example.com/not-found-404' + await tc.downloadTool(statusCodeUrl) + } catch (err) { + expect(err.toString()).toContain('404') + } + }) }) /** diff --git a/packages/tool-cache/package.json b/packages/tool-cache/package.json index 7d2bf1e1..585e4038 100644 --- a/packages/tool-cache/package.json +++ b/packages/tool-cache/package.json @@ -1,6 +1,6 @@ { "name": "@actions/tool-cache", - "version": "1.3.2", + "version": "1.3.3", "description": "Actions tool-cache lib", "keywords": [ "github", diff --git a/packages/tool-cache/src/retry-helper.ts b/packages/tool-cache/src/retry-helper.ts index 36bde0bf..16f8bb7e 100644 --- a/packages/tool-cache/src/retry-helper.ts +++ b/packages/tool-cache/src/retry-helper.ts @@ -21,13 +21,20 @@ export class RetryHelper { } } - async execute(action: () => Promise): Promise { + async execute( + action: () => Promise, + isRetryable?: (e: Error) => boolean + ): Promise { let attempt = 1 while (attempt < this.maxAttempts) { // Try try { return await action() } catch (err) { + if (isRetryable && !isRetryable(err)) { + throw err + } + core.info(err.message) } diff --git a/packages/tool-cache/src/tool-cache.ts b/packages/tool-cache/src/tool-cache.ts index 67cee428..ea2b9036 100644 --- a/packages/tool-cache/src/tool-cache.ts +++ b/packages/tool-cache/src/tool-cache.ts @@ -23,30 +23,6 @@ export class HTTPError extends Error { const IS_WINDOWS = process.platform === 'win32' const userAgent = 'actions/tool-cache' -// On load grab temp directory and cache directory and remove them from env (currently don't want to expose this) -let tempDirectory: string = process.env['RUNNER_TEMP'] || '' -let cacheRoot: string = process.env['RUNNER_TOOL_CACHE'] || '' -// If directories not found, place them in common temp locations -if (!tempDirectory || !cacheRoot) { - let baseLocation: string - if (IS_WINDOWS) { - // On windows use the USERPROFILE env variable - baseLocation = process.env['USERPROFILE'] || 'C:\\' - } else { - if (process.platform === 'darwin') { - baseLocation = '/Users' - } else { - baseLocation = '/home' - } - } - if (!tempDirectory) { - tempDirectory = path.join(baseLocation, 'actions', 'temp') - } - if (!cacheRoot) { - cacheRoot = path.join(baseLocation, 'actions', 'cache') - } -} - /** * Download a tool from an url and stream it into a file * @@ -58,23 +34,40 @@ export async function downloadTool( url: string, dest?: string ): Promise { - dest = dest || path.join(tempDirectory, uuidV4()) + dest = dest || path.join(_getTempDirectory(), uuidV4()) await io.mkdirP(path.dirname(dest)) core.debug(`Downloading ${url}`) core.debug(`Destination ${dest}`) const maxAttempts = 3 - const minSeconds = getGlobal( + const minSeconds = _getGlobal( 'TEST_DOWNLOAD_TOOL_RETRY_MIN_SECONDS', 10 ) - const maxSeconds = getGlobal( + const maxSeconds = _getGlobal( 'TEST_DOWNLOAD_TOOL_RETRY_MAX_SECONDS', 20 ) const retryHelper = new RetryHelper(maxAttempts, minSeconds, maxSeconds) return await retryHelper.execute( - async () => await downloadToolAttempt(url, dest || '') + async () => { + return await downloadToolAttempt(url, dest || '') + }, + (err: Error) => { + if (err instanceof HTTPError && err.httpStatusCode) { + // Don't retry anything less than 500, except 408 Request Timeout and 429 Too Many Requests + if ( + err.httpStatusCode < 500 && + err.httpStatusCode !== 408 && + err.httpStatusCode !== 429 + ) { + return false + } + } + + // Otherwise retry + return true + } ) } @@ -98,7 +91,7 @@ async function downloadToolAttempt(url: string, dest: string): Promise { // Download the response body const pipeline = util.promisify(stream.pipeline) - const responseMessageFactory = getGlobal<() => stream.Readable>( + const responseMessageFactory = _getGlobal<() => stream.Readable>( 'TEST_DOWNLOAD_TOOL_RESPONSE_MESSAGE_FACTORY', () => response.message ) @@ -417,7 +410,12 @@ export function find( let toolPath = '' if (versionSpec) { versionSpec = semver.clean(versionSpec) || '' - const cachePath = path.join(cacheRoot, toolName, versionSpec, arch) + const cachePath = path.join( + _getCacheDirectory(), + toolName, + versionSpec, + arch + ) core.debug(`checking cache: ${cachePath}`) if (fs.existsSync(cachePath) && fs.existsSync(`${cachePath}.complete`)) { core.debug(`Found tool in cache ${toolName} ${versionSpec} ${arch}`) @@ -439,7 +437,7 @@ export function findAllVersions(toolName: string, arch?: string): string[] { const versions: string[] = [] arch = arch || os.arch() - const toolPath = path.join(cacheRoot, toolName) + const toolPath = path.join(_getCacheDirectory(), toolName) if (fs.existsSync(toolPath)) { const children: string[] = fs.readdirSync(toolPath) @@ -459,7 +457,7 @@ export function findAllVersions(toolName: string, arch?: string): string[] { async function _createExtractFolder(dest?: string): Promise { if (!dest) { // create a temp dir - dest = path.join(tempDirectory, uuidV4()) + dest = path.join(_getTempDirectory(), uuidV4()) } await io.mkdirP(dest) return dest @@ -471,7 +469,7 @@ async function _createToolPath( arch?: string ): Promise { const folderPath = path.join( - cacheRoot, + _getCacheDirectory(), tool, semver.clean(version) || version, arch || '' @@ -486,7 +484,7 @@ async function _createToolPath( function _completeToolPath(tool: string, version: string, arch?: string): void { const folderPath = path.join( - cacheRoot, + _getCacheDirectory(), tool, semver.clean(version) || version, arch || '' @@ -533,10 +531,28 @@ function _evaluateVersions(versions: string[], versionSpec: string): string { return version } +/** + * Gets RUNNER_TOOL_CACHE + */ +function _getCacheDirectory(): string { + const cacheDirectory = process.env['RUNNER_TOOL_CACHE'] || '' + ok(cacheDirectory, 'Expected RUNNER_TOOL_CACHE to be defined') + return cacheDirectory +} + +/** + * Gets RUNNER_TEMP + */ +function _getTempDirectory(): string { + const tempDirectory = process.env['RUNNER_TEMP'] || '' + ok(tempDirectory, 'Expected RUNNER_TEMP to be defined') + return tempDirectory +} + /** * Gets a global variable */ -function getGlobal(key: string, defaultValue: T): T { +function _getGlobal(key: string, defaultValue: T): T { /* eslint-disable @typescript-eslint/no-explicit-any */ const value = (global as any)[key] as T | undefined /* eslint-enable @typescript-eslint/no-explicit-any */