Merge pull request #3409 from github/mbg/start-proxy/make-unique-artifact

Ensure that proxy log artifacts have unique names

lib/analyze-action-post.js

@@ -125716,10 +125716,26 @@ async function uploadCombinedSarifArtifacts(logger, gitHubVariant, codeQlVersion
     });
   }
 }
-async function uploadDebugArtifacts(logger, toUpload, rootDir, artifactName, ghVariant, codeQlVersion) {
-  if (toUpload.length === 0) {
-    return "no-artifacts-to-upload";
+function getArtifactSuffix(matrix) {
+  let suffix = "";
+  if (matrix) {
+    try {
+      const matrixObject = JSON.parse(matrix);
+      if (matrixObject !== null && typeof matrixObject === "object") {
+        for (const matrixKey of Object.keys(matrixObject).sort())
+          suffix += `-${matrixObject[matrixKey]}`;
+      } else {
+        core12.warning("User-specified `matrix` input is not an object.");
+      }
+    } catch {
+      core12.warning(
+        "Could not parse user-specified `matrix` input into JSON. The debug artifact will not be named with the user's `matrix` input."
+      );
+    }
   }
+  return suffix;
+}
+async function uploadDebugArtifacts(logger, toUpload, rootDir, artifactName, ghVariant, codeQlVersion) {
   const uploadSupported = isSafeArtifactUpload(codeQlVersion);
   if (!uploadSupported) {
     core12.info(
@@ -125727,24 +125743,17 @@ async function uploadDebugArtifacts(logger, toUpload, rootDir, artifactName, ghV
     );
     return "upload-not-supported";
   }
+  return uploadArtifacts(logger, toUpload, rootDir, artifactName, ghVariant);
+}
+async function uploadArtifacts(logger, toUpload, rootDir, artifactName, ghVariant) {
+  if (toUpload.length === 0) {
+    return "no-artifacts-to-upload";
+  }
   if (isInTestMode()) {
     await scanArtifactsForTokens(toUpload, logger);
     core12.exportVariable("CODEQL_ACTION_ARTIFACT_SCAN_FINISHED", "true");
   }
-  let suffix = "";
-  const matrix = getOptionalInput("matrix");
-  if (matrix) {
-    try {
-      for (const [, matrixVal] of Object.entries(
-        JSON.parse(matrix)
-      ).sort())
-        suffix += `-${matrixVal}`;
-    } catch {
-      core12.info(
-        "Could not parse user-specified `matrix` input into JSON. The debug artifact will not be named with the user's `matrix` input."
-      );
-    }
-  }
+  const suffix = getArtifactSuffix(getOptionalInput("matrix"));
   const artifactUploader = await getArtifactUploaderClient(logger, ghVariant);
   try {
     await artifactUploader.uploadArtifact(

lib/init-action-post.js

@@ -130453,10 +130453,26 @@ async function tryUploadAllAvailableDebugArtifacts(codeql, config, logger, codeQ
     );
   }
 }
-async function uploadDebugArtifacts(logger, toUpload, rootDir, artifactName, ghVariant, codeQlVersion) {
-  if (toUpload.length === 0) {
-    return "no-artifacts-to-upload";
+function getArtifactSuffix(matrix) {
+  let suffix = "";
+  if (matrix) {
+    try {
+      const matrixObject = JSON.parse(matrix);
+      if (matrixObject !== null && typeof matrixObject === "object") {
+        for (const matrixKey of Object.keys(matrixObject).sort())
+          suffix += `-${matrixObject[matrixKey]}`;
+      } else {
+        core12.warning("User-specified `matrix` input is not an object.");
+      }
+    } catch {
+      core12.warning(
+        "Could not parse user-specified `matrix` input into JSON. The debug artifact will not be named with the user's `matrix` input."
+      );
+    }
   }
+  return suffix;
+}
+async function uploadDebugArtifacts(logger, toUpload, rootDir, artifactName, ghVariant, codeQlVersion) {
   const uploadSupported = isSafeArtifactUpload(codeQlVersion);
   if (!uploadSupported) {
     core12.info(
@@ -130464,24 +130480,17 @@ async function uploadDebugArtifacts(logger, toUpload, rootDir, artifactName, ghV
     );
     return "upload-not-supported";
   }
+  return uploadArtifacts(logger, toUpload, rootDir, artifactName, ghVariant);
+}
+async function uploadArtifacts(logger, toUpload, rootDir, artifactName, ghVariant) {
+  if (toUpload.length === 0) {
+    return "no-artifacts-to-upload";
+  }
   if (isInTestMode()) {
     await scanArtifactsForTokens(toUpload, logger);
     core12.exportVariable("CODEQL_ACTION_ARTIFACT_SCAN_FINISHED", "true");
   }
-  let suffix = "";
-  const matrix = getOptionalInput("matrix");
-  if (matrix) {
-    try {
-      for (const [, matrixVal] of Object.entries(
-        JSON.parse(matrix)
-      ).sort())
-        suffix += `-${matrixVal}`;
-    } catch {
-      core12.info(
-        "Could not parse user-specified `matrix` input into JSON. The debug artifact will not be named with the user's `matrix` input."
-      );
-    }
-  }
+  const suffix = getArtifactSuffix(getOptionalInput("matrix"));
   const artifactUploader = await getArtifactUploaderClient(logger, ghVariant);
   try {
     await artifactUploader.uploadArtifact(

lib/upload-sarif-action-post.js

@@ -124642,10 +124642,26 @@ async function uploadCombinedSarifArtifacts(logger, gitHubVariant, codeQlVersion
     });
   }
 }
-async function uploadDebugArtifacts(logger, toUpload, rootDir, artifactName, ghVariant, codeQlVersion) {
-  if (toUpload.length === 0) {
-    return "no-artifacts-to-upload";
+function getArtifactSuffix(matrix) {
+  let suffix = "";
+  if (matrix) {
+    try {
+      const matrixObject = JSON.parse(matrix);
+      if (matrixObject !== null && typeof matrixObject === "object") {
+        for (const matrixKey of Object.keys(matrixObject).sort())
+          suffix += `-${matrixObject[matrixKey]}`;
+      } else {
+        core12.warning("User-specified `matrix` input is not an object.");
+      }
+    } catch {
+      core12.warning(
+        "Could not parse user-specified `matrix` input into JSON. The debug artifact will not be named with the user's `matrix` input."
+      );
+    }
   }
+  return suffix;
+}
+async function uploadDebugArtifacts(logger, toUpload, rootDir, artifactName, ghVariant, codeQlVersion) {
   const uploadSupported = isSafeArtifactUpload(codeQlVersion);
   if (!uploadSupported) {
     core12.info(
@@ -124653,24 +124669,17 @@ async function uploadDebugArtifacts(logger, toUpload, rootDir, artifactName, ghV
     );
     return "upload-not-supported";
   }
+  return uploadArtifacts(logger, toUpload, rootDir, artifactName, ghVariant);
+}
+async function uploadArtifacts(logger, toUpload, rootDir, artifactName, ghVariant) {
+  if (toUpload.length === 0) {
+    return "no-artifacts-to-upload";
+  }
   if (isInTestMode()) {
     await scanArtifactsForTokens(toUpload, logger);
     core12.exportVariable("CODEQL_ACTION_ARTIFACT_SCAN_FINISHED", "true");
   }
-  let suffix = "";
-  const matrix = getOptionalInput("matrix");
-  if (matrix) {
-    try {
-      for (const [, matrixVal] of Object.entries(
-        JSON.parse(matrix)
-      ).sort())
-        suffix += `-${matrixVal}`;
-    } catch {
-      core12.info(
-        "Could not parse user-specified `matrix` input into JSON. The debug artifact will not be named with the user's `matrix` input."
-      );
-    }
-  }
+  const suffix = getArtifactSuffix(getOptionalInput("matrix"));
   const artifactUploader = await getArtifactUploaderClient(logger, ghVariant);
   try {
     await artifactUploader.uploadArtifact(

src/debug-artifacts.test.ts

@@ -24,6 +24,37 @@ test("sanitizeArtifactName", (t) => {
   );
 });
 
+test("getArtifactSuffix", (t) => {
+  // No suffix if there's no `matrix` input, it is invalid, or has no keys.
+  t.is(debugArtifacts.getArtifactSuffix(undefined), "");
+  t.is(debugArtifacts.getArtifactSuffix(""), "");
+  t.is(debugArtifacts.getArtifactSuffix("invalid json"), "");
+  t.is(debugArtifacts.getArtifactSuffix("{}"), "");
+  t.is(debugArtifacts.getArtifactSuffix("null"), "");
+  t.is(debugArtifacts.getArtifactSuffix("123"), "");
+  t.is(debugArtifacts.getArtifactSuffix('"string"'), "");
+
+  // Suffixes for non-empty, valid `matrix` inputs.
+  const testMatrices = [
+    { matrix: { language: "go" }, expected: "-go" },
+    {
+      matrix: { language: "javascript", "build-mode": "none" },
+      expected: "-none-javascript",
+    },
+    {
+      matrix: { "build-mode": "none", language: "javascript" },
+      expected: "-none-javascript",
+    },
+  ];
+
+  for (const testMatrix of testMatrices) {
+    const suffix = debugArtifacts.getArtifactSuffix(
+      JSON.stringify(testMatrix.matrix),
+    );
+    t.is(suffix, testMatrix.expected);
+  }
+});
+
 // These next tests check the correctness of the logic to determine whether or not
 // artifacts are uploaded in debug mode. Since it's not easy to mock the actual
 // call to upload an artifact, we just check that we get an "upload-failed" result,

src/debug-artifacts.ts

@@ -246,6 +246,42 @@ export async function tryUploadAllAvailableDebugArtifacts(
   }
 }
 
+/**
+ * When a build matrix is used, multiple different jobs arising from the matrix may attempt to upload
+ * workflow artifacts with the same base name. In that case, only one of the uploads will succeed and
+ * the others will fail. This function inspects the matrix object to compute a suffix for the artifact
+ * name that uniquely identifies the matrix values of the current job to avoid name clashes.
+ *
+ * @param matrix A stringified JSON value, usually the value of the `matrix` input.
+ * @returns A suffix that uniquely identifies the `matrix` value for the current job, or `""` if there
+ * is no matrix value.
+ */
+export function getArtifactSuffix(matrix: string | undefined): string {
+  let suffix = "";
+  if (matrix) {
+    try {
+      const matrixObject = JSON.parse(matrix);
+      if (matrixObject !== null && typeof matrixObject === "object") {
+        for (const matrixKey of Object.keys(matrixObject as object).sort())
+          suffix += `-${matrixObject[matrixKey]}`;
+      } else {
+        core.warning("User-specified `matrix` input is not an object.");
+      }
+    } catch {
+      core.warning(
+        "Could not parse user-specified `matrix` input into JSON. The debug artifact will not be named with the user's `matrix` input.",
+      );
+    }
+  }
+  return suffix;
+}
+
+// Enumerates different, possible outcomes for artifact uploads.
+export type UploadArtifactsResult =
+  | "no-artifacts-to-upload"
+  | "upload-successful"
+  | "upload-failed";
+
 export async function uploadDebugArtifacts(
   logger: Logger,
   toUpload: string[],
@@ -253,15 +289,7 @@ export async function uploadDebugArtifacts(
   artifactName: string,
   ghVariant: GitHubVariant,
   codeQlVersion: string | undefined,
-): Promise<
-  | "no-artifacts-to-upload"
-  | "upload-successful"
-  | "upload-failed"
-  | "upload-not-supported"
-> {
-  if (toUpload.length === 0) {
-    return "no-artifacts-to-upload";
-  }
+): Promise<UploadArtifactsResult | "upload-not-supported"> {
   const uploadSupported = isSafeArtifactUpload(codeQlVersion);
 
   if (!uploadSupported) {
@@ -271,6 +299,31 @@ export async function uploadDebugArtifacts(
     return "upload-not-supported";
   }
 
+  return uploadArtifacts(logger, toUpload, rootDir, artifactName, ghVariant);
+}
+
+/**
+ * Uploads the specified files as a single workflow artifact.
+ *
+ * @param logger The logger to use.
+ * @param toUpload The list of paths to include in the artifact.
+ * @param rootDir The root directory of the paths to include.
+ * @param artifactName The base name for the artifact.
+ * @param ghVariant The GitHub variant.
+ *
+ * @returns The outcome of the attempt to create and upload the artifact.
+ */
+export async function uploadArtifacts(
+  logger: Logger,
+  toUpload: string[],
+  rootDir: string,
+  artifactName: string,
+  ghVariant: GitHubVariant,
+): Promise<UploadArtifactsResult> {
+  if (toUpload.length === 0) {
+    return "no-artifacts-to-upload";
+  }
+
   // When running in test mode, perform a best effort scan of the debug artifacts. The artifact
   // scanner is basic and not reliable or fast enough for production use, but it can help catch
   // some issues early.
@@ -279,21 +332,7 @@ export async function uploadDebugArtifacts(
     core.exportVariable("CODEQL_ACTION_ARTIFACT_SCAN_FINISHED", "true");
   }
 
-  let suffix = "";
-  const matrix = getOptionalInput("matrix");
-  if (matrix) {
-    try {
-      for (const [, matrixVal] of Object.entries(
-        JSON.parse(matrix) as any[][],
-      ).sort())
-        suffix += `-${matrixVal}`;
-    } catch {
-      core.info(
-        "Could not parse user-specified `matrix` input into JSON. The debug artifact will not be named with the user's `matrix` input.",
-      );
-    }
-  }
-
+  const suffix = getArtifactSuffix(getOptionalInput("matrix"));
   const artifactUploader = await getArtifactUploaderClient(logger, ghVariant);
 
   try {

src/start-proxy-action-post.ts

@@ -8,7 +8,7 @@ import * as core from "@actions/core";
 import * as actionsUtil from "./actions-util";
 import { getGitHubVersion } from "./api-client";
 import * as configUtils from "./config-utils";
-import { getArtifactUploaderClient } from "./debug-artifacts";
+import { uploadArtifacts } from "./debug-artifacts";
 import { getActionsLogger } from "./logging";
 import { checkGitHubVersionInRange, getErrorMessage } from "./util";
 
@@ -44,19 +44,12 @@ async function runWrapper() {
       const gitHubVersion = await getGitHubVersion();
       checkGitHubVersionInRange(gitHubVersion, logger);
 
-      const artifactUploader = await getArtifactUploaderClient(
+      await uploadArtifacts(
         logger,
-        gitHubVersion.type,
-      );
-
-      await artifactUploader.uploadArtifact(
-        "proxy-log-file",
         [logFilePath],
         actionsUtil.getTemporaryDirectory(),
-        {
-          // ensure we don't keep the debug artifacts around for too long since they can be large.
-          retentionDays: 7,
-        },
+        "proxy-log-file",
+        gitHubVersion.type,
       );
     }
   } catch (error) {