Merge pull request #4138 from github/henrymercer/bundle-download-errors

Preserve HTTP errors from streaming bundle downloads

Author: henrymercer

lib/entry-points.js

@@ -1,23 +1,27 @@
 import { once } from "events";
+import * as fs from "fs";
+import { ClientRequest, IncomingMessage } from "http";
 import * as path from "path";
 
+import * as core from "@actions/core";
 import * as toolcache from "@actions/tool-cache";
 import test from "ava";
+import { https } from "follow-redirects";
 import nock from "nock";
 import * as sinon from "sinon";
 
 import { getRunnerLogger } from "./logging";
 import * as tar from "./tar";
 import { setupTests } from "./testing-utils";
 import { downloadAndExtract } from "./tools-download";
-import { withTmpDir } from "./util";
+import * as util from "./util";
 
 setupTests(test);
 
 test.serial(
   "downloadAndExtract reports the durations when downloading before extracting",
   async (t) => {
-    await withTmpDir(async (tmpDir) => {
+    await util.withTmpDir(async (tmpDir) => {
       const archivePath = path.join(tmpDir, "codeql-bundle.tar.gz");
       const destination = path.join(tmpDir, "codeql");
       sinon.stub(toolcache, "downloadTool").resolves(archivePath);
@@ -43,13 +47,16 @@ test.serial(
 test.serial(
   "downloadAndExtract falls back to downloading before extracting if streaming fails",
   async (t) => {
-    await withTmpDir(async (tmpDir) => {
+    await util.withTmpDir(async (tmpDir) => {
       sinon.stub(process, "platform").value("linux");
       const archivePath = path.join(tmpDir, "codeql-bundle.tar.zst");
       const destination = path.join(tmpDir, "codeql");
       const downloadTool = sinon
         .stub(toolcache, "downloadTool")
-        .resolves(archivePath);
+        .callsFake(async () => {
+          t.false(fs.existsSync(destination));
+          return archivePath;
+        });
       const extract = sinon.stub(tar, "extract").resolves(destination);
       const extractTarZst = sinon.stub(tar, "extractTarZst").resolves();
       const request = nock("https://example.com")
@@ -78,10 +85,134 @@ test.serial(
   },
 );
 
+test.serial(
+  "downloadAndExtract rethrows a 404 rather than retrying the download",
+  async (t) => {
+    await util.withTmpDir(async (tmpDir) => {
+      sinon.stub(process, "platform").value("linux");
+      const destination = path.join(tmpDir, "codeql");
+      const downloadTool = sinon.stub(toolcache, "downloadTool");
+      const extractTarZst = sinon.stub(tar, "extractTarZst").resolves();
+      const request = nock("https://example.com")
+        .get("/codeql-bundle.tar.zst")
+        .reply(404, "Not found");
+
+      const error = await t.throwsAsync(
+        downloadAndExtract(
+          "https://example.com/codeql-bundle.tar.zst",
+          "zstd",
+          destination,
+          undefined,
+          {},
+          { type: "gnu", version: "1.34" },
+          getRunnerLogger(true),
+        ),
+        {
+          instanceOf: util.HTTPError,
+          message:
+            "Failed to download CodeQL bundle from https://example.com/codeql-bundle.tar.zst. HTTP status code: 404.",
+        },
+      );
+
+      t.is(error?.status, 404);
+      t.true(request.isDone());
+      t.false(extractTarZst.called);
+      t.false(downloadTool.called);
+      t.false(fs.existsSync(destination));
+    });
+  },
+);
+
+test.serial(
+  "downloadAndExtract falls back to downloading before extracting on a server error",
+  async (t) => {
+    await util.withTmpDir(async (tmpDir) => {
+      sinon.stub(process, "platform").value("linux");
+      const archivePath = path.join(tmpDir, "codeql-bundle.tar.zst");
+      const destination = path.join(tmpDir, "codeql");
+      const downloadTool = sinon
+        .stub(toolcache, "downloadTool")
+        .callsFake(async () => {
+          t.false(fs.existsSync(destination));
+          return archivePath;
+        });
+      const extract = sinon.stub(tar, "extract").resolves(destination);
+      const extractTarZst = sinon.stub(tar, "extractTarZst").resolves();
+      const request = nock("https://example.com")
+        .get("/codeql-bundle.tar.zst")
+        .reply(500);
+
+      const statusReport = await downloadAndExtract(
+        "https://example.com/codeql-bundle.tar.zst",
+        "zstd",
+        destination,
+        undefined,
+        {},
+        { type: "gnu", version: "1.34" },
+        getRunnerLogger(true),
+      );
+
+      t.assert(Number.isInteger(statusReport.downloadDurationMs));
+      t.true(request.isDone());
+      t.false(extractTarZst.called);
+      t.true(downloadTool.calledOnce);
+      t.true(extract.calledOnce);
+    });
+  },
+);
+
+test.serial(
+  "downloadAndExtract handles an unknown status as a non-HTTP error",
+  async (t) => {
+    const asHTTPError = sinon.spy(util, "asHTTPError");
+    await util.withTmpDir(async (tmpDir) => {
+      sinon.stub(process, "platform").value("linux");
+      const archivePath = path.join(tmpDir, "codeql-bundle.tar.zst");
+      const destination = path.join(tmpDir, "codeql");
+      const response = sinon.createStubInstance(IncomingMessage);
+      response.statusCode = undefined;
+      sinon
+        .stub(https, "get")
+        .callsArgWith(2, response)
+        .returns(sinon.createStubInstance(ClientRequest));
+      const warning = sinon.stub(core, "warning");
+      const downloadTool = sinon
+        .stub(toolcache, "downloadTool")
+        .resolves(archivePath);
+      const extract = sinon.stub(tar, "extract").resolves(destination);
+      const extractTarZst = sinon.stub(tar, "extractTarZst").resolves();
+
+      await downloadAndExtract(
+        "https://example.com/codeql-bundle.tar.zst",
+        "zstd",
+        destination,
+        undefined,
+        {},
+        { type: "gnu", version: "1.34" },
+        getRunnerLogger(true),
+      );
+
+      t.is(
+        warning.firstCall.args[0],
+        "Failed to download and extract CodeQL bundle using streaming with error: Failed to download CodeQL bundle from https://example.com/codeql-bundle.tar.zst.",
+      );
+      t.true(response.resume.calledOnce);
+      t.false(extractTarZst.called);
+      t.true(downloadTool.calledOnce);
+      t.true(extract.calledOnce);
+    });
+
+    t.true(asHTTPError.calledOnce);
+    t.true(asHTTPError.firstCall.args[0] instanceof Error);
+    t.false(asHTTPError.firstCall.args[0] instanceof util.HTTPError);
+    t.is(asHTTPError.firstCall.returnValue, undefined);
+  },
+);
+
 test.serial(
   "downloadAndExtract reports only the total duration when streaming extraction",
   async (t) => {
-    await withTmpDir(async (tmpDir) => {
+    await util.withTmpDir(async (tmpDir) => {
       sinon.stub(process, "platform").value("linux");
       const downloadTool = sinon.stub(toolcache, "downloadTool");
       const extractTarZst = sinon

src/tools-download.test.ts

@@ -14,7 +14,13 @@ import { ActionState } from "./action-common";
 import { ActionsEnvVars, getEnv, ReadOnlyEnv } from "./environment";
 import { formatDuration, Logger } from "./logging";
 import * as tar from "./tar";
-import { cleanUpPath, getErrorMessage, getRequiredEnvParam } from "./util";
+import {
+  asHTTPError,
+  cleanUpPath,
+  getErrorMessage,
+  getRequiredEnvParam,
+  HTTPError,
+} from "./util";
 
 /**
  * High watermark to use when streaming the download and extraction of the CodeQL tools.
@@ -88,14 +94,20 @@ export async function downloadAndExtract(
       return { totalDurationMs };
     }
   } catch (e) {
+    // If we failed during processing, we want to clean up the destination directory
+    // before we either try again or give up.
+    await cleanUpPath(dest, "CodeQL bundle", logger);
+
+    // Retrying a 404 is pointless: the asset does not exist, so downloading it a different way
+    // will fail in the same way.
+    if (asHTTPError(e)?.status === 404) {
+      throw e;
+    }
+
     core.warning(
       `Failed to download and extract CodeQL bundle using streaming with error: ${getErrorMessage(e)}`,
     );
     core.warning(`Falling back to downloading the bundle before extracting.`);
-
-    // If we failed during processing, we want to clean up the destination directory
-    // before we try again.
-    await cleanUpPath(dest, "CodeQL bundle", logger);
   }
 
   const toolsDownloadStart = performance.now();
@@ -191,9 +203,14 @@ async function downloadAndExtractZstdWithStreaming(
   if (response.statusCode !== 200) {
     // Discard the response body so that the connection can be released.
     response.resume();
-    throw new Error(
-      `Failed to download CodeQL bundle from ${codeqlURL}. HTTP status code: ${response.statusCode}.`,
-    );
+    const baseMessage = `Failed to download CodeQL bundle from ${codeqlURL}.`;
+    if (response.statusCode !== undefined) {
+      throw new HTTPError(
+        `${baseMessage} HTTP status code: ${response.statusCode}.`,
+        response.statusCode,
+      );
+    }
+    throw new Error(baseMessage);
   }
 
   await tar.extractTarZst(response, dest, tarVersion, logger);