dont retry downloadtool on 404

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/)
+  })
 })

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')
+    }
+  })
 })
 
 /**

packages/tool-cache/src/retry-helper.ts

@@ -21,13 +21,20 @@ export class RetryHelper {
     }
   }
 
-  async execute<T>(action: () => Promise<T>): Promise<T> {
+  async execute<T>(
+    action: () => Promise<T>,
+    isRetryable?: (e: Error) => boolean
+  ): Promise<T> {
     let attempt = 1
     while (attempt < this.maxAttempts) {
       // Try
       try {
         return await action()
       } catch (err) {
+        if (isRetryable && !isRetryable(err)) {
+          throw err
+        }
+
         core.info(err.message)
       }
 

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<string> {
-  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<number>(
+  const minSeconds = _getGlobal<number>(
     'TEST_DOWNLOAD_TOOL_RETRY_MIN_SECONDS',
     10
   )
-  const maxSeconds = getGlobal<number>(
+  const maxSeconds = _getGlobal<number>(
     '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<string> {
 
   // 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<string> {
   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<string> {
   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<T>(key: string, defaultValue: T): T {
+function _getGlobal<T>(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 */