From 6797a48e9d26774872936de0490eac699d6f4539 Mon Sep 17 00:00:00 2001 From: eric sciple Date: Sat, 7 Mar 2020 03:15:13 -0500 Subject: [PATCH] dont retry downloadtool on 404 --- .../tool-cache/__tests__/retry-helper.test.ts | 40 ++++++++- .../tool-cache/__tests__/tool-cache.test.ts | 33 +++++++ packages/tool-cache/src/retry-helper.ts | 9 +- packages/tool-cache/src/tool-cache.ts | 86 +++++++++++-------- 4 files changed, 131 insertions(+), 37 deletions(-) diff --git a/packages/tool-cache/__tests__/retry-helper.test.ts b/packages/tool-cache/__tests__/retry-helper.test.ts index cccf00757e..90b2e7c9fc 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 7888fa2adc..42a3e4eafc 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/src/retry-helper.ts b/packages/tool-cache/src/retry-helper.ts index 36bde0bfed..16f8bb7e96 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 67cee42854..ea2b90361d 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 */