update downloadTool to handle errors from response stream and retry (#369)

packages/artifact/package-lock.json

@@ -4,11 +4,6 @@
   "lockfileVersion": 1,
   "requires": true,
   "dependencies": {
-    "@actions/core": {
-      "version": "1.2.1",
-      "resolved": "https://registry.npmjs.org/@actions/core/-/core-1.2.1.tgz",
-      "integrity": "sha512-xD+CQd9p4lU7ZfRqmUcbJpqR+Ss51rJRVeXMyOLrZQImN9/8Sy/BEUBnHO/UKD3z03R686PVTLfEPmkropGuLw=="
-    },
     "@actions/http-client": {
       "version": "1.0.6",
       "resolved": "https://registry.npmjs.org/@actions/http-client/-/http-client-1.0.6.tgz",

packages/tool-cache/RELEASES.md

@@ -1,5 +1,9 @@
 # @actions/tool-cache Releases
 
+### 1.3.2
+
+- [Update downloadTool with better error handling and retries](https://github.com/actions/toolkit/pull/369)
+
 ### 1.3.1
 
 - [Increase http-client min version](https://github.com/actions/toolkit/pull/314)

packages/tool-cache/__tests__/retry-helper.test.ts

@@ -0,0 +1,87 @@
+import * as core from '@actions/core'
+import {RetryHelper} from '../src/retry-helper'
+
+let info: string[]
+let retryHelper: RetryHelper
+
+describe('retry-helper tests', () => {
+  beforeAll(() => {
+    // Mock @actions/core info()
+    jest.spyOn(core, 'info').mockImplementation((message: string) => {
+      info.push(message)
+    })
+
+    retryHelper = new RetryHelper(3, 0, 0)
+  })
+
+  beforeEach(() => {
+    // Reset info
+    info = []
+  })
+
+  afterAll(() => {
+    // Restore
+    jest.restoreAllMocks()
+  })
+
+  it('first attempt succeeds', async () => {
+    const actual = await retryHelper.execute(async () => {
+      return 'some result'
+    })
+    expect(actual).toBe('some result')
+    expect(info).toHaveLength(0)
+  })
+
+  it('second attempt succeeds', async () => {
+    let attempts = 0
+    const actual = await retryHelper.execute(async () => {
+      if (++attempts === 1) {
+        throw new Error('some error')
+      }
+
+      return Promise.resolve('some result')
+    })
+    expect(attempts).toBe(2)
+    expect(actual).toBe('some result')
+    expect(info).toHaveLength(2)
+    expect(info[0]).toBe('some error')
+    expect(info[1]).toMatch(/Waiting .+ seconds before trying again/)
+  })
+
+  it('third attempt succeeds', async () => {
+    let attempts = 0
+    const actual = await retryHelper.execute(async () => {
+      if (++attempts < 3) {
+        throw new Error(`some error ${attempts}`)
+      }
+
+      return Promise.resolve('some result')
+    })
+    expect(attempts).toBe(3)
+    expect(actual).toBe('some result')
+    expect(info).toHaveLength(4)
+    expect(info[0]).toBe('some error 1')
+    expect(info[1]).toMatch(/Waiting .+ seconds before trying again/)
+    expect(info[2]).toBe('some error 2')
+    expect(info[3]).toMatch(/Waiting .+ seconds before trying again/)
+  })
+
+  it('all attempts fail succeeds', async () => {
+    let attempts = 0
+    let error: Error = (null as unknown) as Error
+    try {
+      await retryHelper.execute(() => {
+        throw new Error(`some error ${++attempts}`)
+      })
+    } catch (err) {
+      error = err
+    }
+    expect(error.message).toBe('some error 3')
+    expect(attempts).toBe(3)
+    expect(info).toHaveLength(4)
+    expect(info[0]).toBe('some error 1')
+    expect(info[1]).toMatch(/Waiting .+ seconds before trying again/)
+    expect(info[2]).toBe('some error 2')
+    expect(info[3]).toMatch(/Waiting .+ seconds before trying again/)
+  })
+})

packages/tool-cache/__tests__/tool-cache.test.ts

@@ -2,6 +2,7 @@ import * as fs from 'fs'
 import * as path from 'path'
 import * as io from '@actions/io'
 import * as exec from '@actions/exec'
+import * as stream from 'stream'
 import nock from 'nock'
 
 const cachePath = path.join(__dirname, 'CACHE')
@@ -24,6 +25,8 @@ describe('@actions/tool-cache', function() {
         username: 'abc',
         password: 'def'
       })
+    setGlobal('TEST_DOWNLOAD_TOOL_RETRY_MIN_SECONDS', 0)
+    setGlobal('TEST_DOWNLOAD_TOOL_RETRY_MAX_SECONDS', 0)
   })
 
   beforeEach(async function() {
@@ -33,9 +36,15 @@ describe('@actions/tool-cache', function() {
     await io.mkdirP(tempPath)
   })
 
+  afterEach(function() {
+    setResponseMessageFactory(undefined)
+  })
+
   afterAll(async function() {
     await io.rmRF(tempPath)
     await io.rmRF(cachePath)
+    setGlobal('TEST_DOWNLOAD_TOOL_RETRY_MIN_SECONDS', undefined)
+    setGlobal('TEST_DOWNLOAD_TOOL_RETRY_MAX_SECONDS', undefined)
   })
 
   it('downloads a 35 byte file', async () => {
@@ -90,6 +99,7 @@ describe('@actions/tool-cache', function() {
 
   it('downloads a 35 byte file after a redirect', async () => {
     nock('http://example.com')
+      .persist()
       .get('/redirect-to')
       .reply(303, undefined, {
         location: 'http://example.com/bytes/35'
@@ -103,8 +113,75 @@ describe('@actions/tool-cache', function() {
     expect(fs.statSync(downPath).size).toBe(35)
   })
 
+  it('handles error from response message stream', async () => {
+    nock('http://example.com')
+      .persist()
+      .get('/error-from-response-message-stream')
+      .reply(200, {})
+
+    setResponseMessageFactory(() => {
+      const readStream = new stream.Readable()
+      /* eslint-disable @typescript-eslint/unbound-method */
+      readStream._read = () => {
+        readStream.destroy(new Error('uh oh'))
+      }
+      /* eslint-enable @typescript-eslint/unbound-method */
+      return readStream
+    })
+
+    let error = new Error('unexpected')
+    try {
+      await tc.downloadTool(
+        'http://example.com/error-from-response-message-stream'
+      )
+    } catch (err) {
+      error = err
+    }
+
+    expect(error).not.toBeUndefined()
+    expect(error.message).toBe('uh oh')
+  })
+
+  it('retries error from response message stream', async () => {
+    nock('http://example.com')
+      .persist()
+      .get('/retries-error-from-response-message-stream')
+      .reply(200, {})
+
+    /* eslint-disable @typescript-eslint/unbound-method */
+    let attempt = 1
+    setResponseMessageFactory(() => {
+      const readStream = new stream.Readable()
+      let failsafe = 0
+      readStream._read = () => {
+        // First attempt fails
+        if (attempt++ === 1) {
+          readStream.destroy(new Error('uh oh'))
+          return
+        }
+
+        // Write some data
+        if (failsafe++ === 0) {
+          readStream.push(''.padEnd(35))
+          readStream.push(null) // no more data
+        }
+      }
+
+      return readStream
+    })
+    /* eslint-enable @typescript-eslint/unbound-method */
+
+    const downPath = await tc.downloadTool(
+      'http://example.com/retries-error-from-response-message-stream'
+    )
+
+    expect(fs.existsSync(downPath)).toBeTruthy()
+    expect(fs.statSync(downPath).size).toBe(35)
+  })
+
   it('has status code in exception dictionary for HTTP error code responses', async () => {
     nock('http://example.com')
+      .persist()
       .get('/bytes/bad')
       .reply(400, {
         username: 'bad',
@@ -124,6 +201,7 @@ describe('@actions/tool-cache', function() {
 
   it('works with redirect code 302', async function() {
     nock('http://example.com')
+      .persist()
       .get('/redirect-to')
       .reply(302, undefined, {
         location: 'http://example.com/bytes/35'
@@ -542,3 +620,27 @@ describe('@actions/tool-cache', function() {
     }
   })
 })
+
+/**
+ * Sets up a mock response body for downloadTool. This function works around a limitation with
+ * nock when the response stream emits an error.
+ */
+function setResponseMessageFactory(
+  factory: (() => stream.Readable) | undefined
+): void {
+  setGlobal('TEST_DOWNLOAD_TOOL_RESPONSE_MESSAGE_FACTORY', factory)
+}
+
+/**
+ * Sets a global variable
+ */
+function setGlobal<T>(key: string, value: T | undefined): void {
+  /* eslint-disable @typescript-eslint/no-explicit-any */
+  const g = global as any
+  /* eslint-enable @typescript-eslint/no-explicit-any */
+  if (value === undefined) {
+    delete g[key]
+  } else {
+    g[key] = value
+  }
+}

packages/tool-cache/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@actions/tool-cache",
-  "version": "1.3.1",
+  "version": "1.3.2",
   "description": "Actions tool-cache lib",
   "keywords": [
     "github",

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

@@ -0,0 +1,55 @@
+import * as core from '@actions/core'
+
+/**
+ * Internal class for retries
+ */
+export class RetryHelper {
+  private maxAttempts: number
+  private minSeconds: number
+  private maxSeconds: number
+
+  constructor(maxAttempts: number, minSeconds: number, maxSeconds: number) {
+    if (maxAttempts < 1) {
+      throw new Error('max attempts should be greater than or equal to 1')
+    }
+
+    this.maxAttempts = maxAttempts
+    this.minSeconds = Math.floor(minSeconds)
+    this.maxSeconds = Math.floor(maxSeconds)
+    if (this.minSeconds > this.maxSeconds) {
+      throw new Error('min seconds should be less than or equal to max seconds')
+    }
+  }
+
+  async execute<T>(action: () => Promise<T>): Promise<T> {
+    let attempt = 1
+    while (attempt < this.maxAttempts) {
+      // Try
+      try {
+        return await action()
+      } catch (err) {
+        core.info(err.message)
+      }
+
+      // Sleep
+      const seconds = this.getSleepAmount()
+      core.info(`Waiting ${seconds} seconds before trying again`)
+      await this.sleep(seconds)
+      attempt++
+    }
+
+    // Last attempt
+    return await action()
+  }
+
+  private getSleepAmount(): number {
+    return (
+      Math.floor(Math.random() * (this.maxSeconds - this.minSeconds + 1)) +
+      this.minSeconds
+    )
+  }
+
+  private async sleep(seconds: number): Promise<void> {
+    return new Promise(resolve => setTimeout(resolve, seconds * 1000))
+  }
+}

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

@@ -5,10 +5,13 @@ import * as os from 'os'
 import * as path from 'path'
 import * as httpm from '@actions/http-client'
 import * as semver from 'semver'
+import * as stream from 'stream'
+import * as util from 'util'
 import uuidV4 from 'uuid/v4'
 import {exec} from '@actions/exec/lib/exec'
 import {ExecOptions} from '@actions/exec/lib/interfaces'
 import {ok} from 'assert'
+import {RetryHelper} from './retry-helper'
 
 export class HTTPError extends Error {
   constructor(readonly httpStatusCode: number | undefined) {
@@ -55,24 +58,36 @@ export async function downloadTool(
   url: string,
   dest?: string
 ): Promise<string> {
-  // Wrap in a promise so that we can resolve from within stream callbacks
-  return new Promise<string>(async (resolve, reject) => {
-    try {
-      const http = new httpm.HttpClient(userAgent, [], {
-        allowRetries: true,
-        maxRetries: 3
-      })
   dest = dest || path.join(tempDirectory, uuidV4())
   await io.mkdirP(path.dirname(dest))
   core.debug(`Downloading ${url}`)
-      core.debug(`Downloading ${dest}`)
+  core.debug(`Destination ${dest}`)
 
+  const maxAttempts = 3
+  const minSeconds = getGlobal<number>(
+    'TEST_DOWNLOAD_TOOL_RETRY_MIN_SECONDS',
+    10
+  )
+  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 function downloadToolAttempt(url: string, dest: string): Promise<string> {
   if (fs.existsSync(dest)) {
     throw new Error(`Destination file path ${dest} already exists`)
   }
 
+  // Get the response headers
+  const http = new httpm.HttpClient(userAgent, [], {
+    allowRetries: false
+  })
   const response: httpm.HttpClientResponse = await http.get(url)
-
   if (response.message.statusCode !== 200) {
     const err = new HTTPError(response.message.statusCode)
     core.debug(
@@ -81,29 +96,30 @@ export async function downloadTool(
     throw err
   }
 
-      const file: NodeJS.WritableStream = fs.createWriteStream(dest)
+  // Download the response body
-      file.on('open', async () => {
+  const pipeline = util.promisify(stream.pipeline)
-        try {
+  const responseMessageFactory = getGlobal<() => stream.Readable>(
-          const stream = response.message.pipe(file)
+    'TEST_DOWNLOAD_TOOL_RESPONSE_MESSAGE_FACTORY',
-          stream.on('close', () => {
+    () => response.message
-            core.debug('download complete')
-            resolve(dest)
-          })
-        } catch (err) {
-          core.debug(
-            `Failed to download from "${url}". Code(${response.message.statusCode}) Message(${response.message.statusMessage})`
   )
-          reject(err)
+  const readStream = responseMessageFactory()
-        }
+  let succeeded = false
-      })
+  try {
-      file.on('error', err => {
+    await pipeline(readStream, fs.createWriteStream(dest))
-        file.end()
+    core.debug('download complete')
-        reject(err)
+    succeeded = true
-      })
+    return dest
+  } finally {
+    // Error, delete dest before retry
+    if (!succeeded) {
+      core.debug('download failed')
+      try {
+        await io.rmRF(dest)
       } catch (err) {
-      reject(err)
+        core.debug(`Failed to delete '${dest}'. ${err.message}`)
+      }
+    }
   }
-  })
 }
 
 /**
@@ -516,3 +532,13 @@ function _evaluateVersions(versions: string[], versionSpec: string): string {
 
   return version
 }
+
+/**
+ * Gets a global variable
+ */
+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 */
+  return value !== undefined ? value : defaultValue
+}