actions · ericsciple · Mar 5, 2020 · Mar 4, 2020 · ericsciple · Mar 5, 2020
diff --git a/packages/artifact/package-lock.json b/packages/artifact/package-lock.json
diff --git a/packages/tool-cache/RELEASES.md b/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)

diff --git a/packages/tool-cache/__tests__/retry-helper.test.ts b/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/)
+  })
+})
diff --git a/packages/tool-cache/__tests__/tool-cache.test.ts b/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
+  }
+}
diff --git a/packages/tool-cache/package.json b/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",

diff --git a/packages/tool-cache/src/retry-helper.ts b/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))
+  }
+}