fix: fix deno download retry logic (#104)

* fix: fix deno download retry logic

* chore: try different windows deno binary

* fix: cleanup on error, should also fix windows tests maybe

* chore: deduplicate code

* chore: close file

* chore: fix rm

* chore: fix node 12

* chore: fix review

* Update test/downloader.ts

src/downloader.ts

@@ -1,5 +1,6 @@
-import fs from 'fs'
+import { createWriteStream, promises as fs } from 'fs'
 import path from 'path'
+import { promisify } from 'util'
 
 import fetch from 'node-fetch'
 import StreamZip from 'node-stream-zip'
@@ -9,57 +10,63 @@ import semver from 'semver'
 import { Logger } from './logger.js'
 import { getBinaryExtension, getPlatformTarget } from './platform.js'
 
-const download = async (targetDirectory: string, versionRange: string, logger: Logger) => {
+const downloadWithRetry = async (targetDirectory: string, versionRange: string, logger: Logger) =>
+  await pRetry(async () => await download(targetDirectory, versionRange), {
+    retries: 3,
+    onFailedAttempt: (error) => {
+      logger.system('Deno download with retry failed', error)
+    },
+  })
+
+const download = async (targetDirectory: string, versionRange: string) => {
   const zipPath = path.join(targetDirectory, 'deno-cli-latest.zip')
-  const data = await downloadVersionWithRetry(versionRange, logger)
+  const data = await downloadVersion(versionRange)
   const binaryName = `deno${getBinaryExtension()}`
   const binaryPath = path.join(targetDirectory, binaryName)
-  const file = fs.createWriteStream(zipPath)
-
-  await new Promise((resolve, reject) => {
-    data.pipe(file)
-    data.on('error', reject)
-    file.on('finish', resolve)
-  })
-
-  await extractBinaryFromZip(zipPath, binaryPath, binaryName)
+  const file = createWriteStream(zipPath)
 
   try {
-    await fs.promises.unlink(zipPath)
-  } catch {
-    // no-op
+    await new Promise((resolve, reject) => {
+      data.pipe(file)
+      data.on('error', reject)
+      file.on('finish', resolve)
+    })
+
+    await extractBinaryFromZip(zipPath, binaryPath, binaryName)
+
+    return binaryPath
+  } finally {
+    // Try closing and deleting the zip file in any case, error or not
+    await promisify(file.close.bind(file))()
+
+    try {
+      await fs.unlink(zipPath)
+    } catch {
+      // no-op
+    }
   }
-
-  return binaryPath
 }
 
 const downloadVersion = async (versionRange: string) => {
   const version = await getLatestVersionForRange(versionRange)
   const url = getReleaseURL(version)
   const res = await fetch(url)
 
-  if (res.body === null) {
-    throw new Error('Could not download Deno')
+  // eslint-disable-next-line no-magic-numbers
+  if (res.body === null || res.status < 200 || res.status > 299) {
+    throw new Error(`Download failed with status code ${res.status}`)
   }
 
   return res.body
 }
 
-const downloadVersionWithRetry = async (versionRange: string, logger: Logger) =>
-  await pRetry(async () => await downloadVersion(versionRange), {
-    retries: 3,
-    onFailedAttempt: (error) => {
-      logger.system('Deno CLI download retry attempt error', error)
-    },
-  })
-
 const extractBinaryFromZip = async (zipPath: string, binaryPath: string, binaryName: string) => {
   const { async: StreamZipAsync } = StreamZip
   const zip = new StreamZipAsync({ file: zipPath })
 
   await zip.extract(binaryName, binaryPath)
   await zip.close()
-  await fs.promises.chmod(binaryPath, '755')
+  await fs.chmod(binaryPath, '755')
 }
 
 const getLatestVersion = async () => {
@@ -106,4 +113,4 @@ const getReleaseURL = (version: string) => {
   return `https://dl.deno.land/release/v${version}/deno-${target}.zip`
 }
 
-export { download }
+export { downloadWithRetry as download }

test/downloader.ts

@@ -0,0 +1,150 @@
+import { promises as fs } from 'fs'
+import { platform } from 'process'
+import { PassThrough } from 'stream'
+
+// eslint-disable-next-line ava/use-test
+import testFn, { TestFn } from 'ava'
+import { execa } from 'execa'
+import nock from 'nock'
+import tmp from 'tmp-promise'
+
+import { download } from '../src/downloader.js'
+import { getLogger } from '../src/logger.js'
+import { getPlatformTarget } from '../src/platform.js'
+
+const testLogger = getLogger(() => {
+})
+
+const streamError = () => {
+  const stream = new PassThrough()
+  setTimeout(() => stream.emit('data', 'zipcontent'), 100)
+  setTimeout(() => stream.emit('error', new Error('stream error')), 200)
+
+  return stream
+}
+
+interface Context {
+  tmpDir: string
+}
+const test = testFn as TestFn<Context>
+
+test.beforeEach(async (t) => {
+  const tmpDir = await tmp.dir()
+  t.context = { tmpDir: tmpDir.path }
+})
+
+test.afterEach(async (t) => {
+  await fs.rmdir(t.context.tmpDir, { recursive: true })
+})
+
+test.serial('tries downloading binary up to 4 times', async (t) => {
+  t.timeout(15_000)
+  nock.disableNetConnect()
+
+  const version = '99.99.99'
+  const mockURL = 'https://dl.deno.land:443'
+  const target = getPlatformTarget()
+  const zipPath = `/release/v${version}/deno-${target}.zip`
+  const latestVersionMock = nock(mockURL)
+    .get('/release-latest.txt')
+    .reply(200, `v${version}\n`)
+
+    // first attempt
+    .get(zipPath)
+    .reply(500)
+
+    // second attempt
+    .get(zipPath)
+    .reply(500)
+
+    // third attempt
+    .get(zipPath)
+    .reply(500)
+
+    // fourth attempt
+    .get(zipPath)
+    // 1 second delay
+    .delayBody(1000)
+    .replyWithFile(200, platform === 'win32' ? './test/fixtures/deno.win.zip' : './test/fixtures/deno.zip', {
+      'Content-Type': 'application/zip',
+    })
+
+  const deno = await download(t.context.tmpDir, `^${version}`, testLogger)
+
+  t.true(latestVersionMock.isDone())
+  t.truthy(deno)
+
+  const res = await execa(deno)
+  t.is(res.stdout, 'hello')
+})
+
+test.serial('fails downloading binary after 4th time', async (t) => {
+  t.timeout(15_000)
+  nock.disableNetConnect()
+
+  const version = '99.99.99'
+  const mockURL = 'https://dl.deno.land:443'
+  const target = getPlatformTarget()
+  const zipPath = `/release/v${version}/deno-${target}.zip`
+  const latestVersionMock = nock(mockURL)
+    .get('/release-latest.txt')
+    .reply(200, `v${version}\n`)
+
+    // first attempt
+    .get(zipPath)
+    .reply(500)
+
+    // second attempt
+    .get(zipPath)
+    .reply(500)
+
+    // third attempt
+    .get(zipPath)
+    .reply(500)
+
+    // fourth attempt
+    .get(zipPath)
+    .reply(500)
+
+  await t.throwsAsync(() => download(t.context.tmpDir, `^${version}`, testLogger), {
+    message: /Download failed with status code 500/,
+  })
+
+  t.true(latestVersionMock.isDone())
+})
+
+test.serial('fails downloading if response stream throws error', async (t) => {
+  t.timeout(15_000)
+  nock.disableNetConnect()
+
+  const version = '99.99.99'
+  const mockURL = 'https://dl.deno.land:443'
+  const target = getPlatformTarget()
+  const zipPath = `/release/v${version}/deno-${target}.zip`
+
+  const latestVersionMock = nock(mockURL)
+    .get('/release-latest.txt')
+    .reply(200, `v${version}\n`)
+
+    // first attempt
+    .get(zipPath)
+    .reply(200, streamError)
+
+    // second attempt
+    .get(zipPath)
+    .reply(200, streamError)
+
+    // third attempt
+    .get(zipPath)
+    .reply(200, streamError)
+
+    // fourth attempt
+    .get(zipPath)
+    .reply(200, streamError)
+
+  await t.throwsAsync(() => download(t.context.tmpDir, `^${version}`, testLogger), {
+    message: /stream error/,
+  })
+
+  t.true(latestVersionMock.isDone())
+})