Merge pull request #4122 from github/henrymercer/friendly-potato

Don't record an overlay status when the job was cancelled

Author: henrymercer

init/action.yml

@@ -164,6 +164,13 @@ inputs:
       [Internal] The ID of the check run, as provided by the Actions runtime environment. Do not set this value manually.
     default: ${{ job.check_run_id }}
     required: false
+  job-status:
+    description: >-
+      [Internal] The status of the job, as provided by the Actions runtime environment. This is how the
+      post step learns whether the job as a whole succeeded, failed, or was cancelled. Do not set this
+      value manually.
+    default: ${{ job.status }}
+    required: false
 outputs:
   codeql-path:
     description: The path of the CodeQL binary used for analysis

lib/entry-points.js

@@ -15,10 +15,12 @@ import { getRunnerLogger } from "./logging";
 import { OverlayDatabaseMode } from "./overlay/overlay-database-mode";
 import * as overlayStatus from "./overlay/status";
 import { parseRepositoryNwo } from "./repository";
+import { JobStatus } from "./status-report";
 import {
   createFeatures,
   createTestConfig,
   DEFAULT_ACTIONS_VARS,
+  getTestEnv,
   makeMacro,
   makeVersionInfo,
   RecordingLogger,
@@ -58,6 +60,8 @@ test.serial("init-post action with debug mode off", async (t) => {
       createTestConfig({ debugMode: false }),
       parseRepositoryNwo("github/codeql-action"),
       createFeatures([]),
+      "success",
+      getTestEnv(),
       getRunnerLogger(true),
     );
 
@@ -80,6 +84,8 @@ test.serial("init-post action with debug mode on", async (t) => {
       createTestConfig({ debugMode: true }),
       parseRepositoryNwo("github/codeql-action"),
       createFeatures([]),
+      "success",
+      getTestEnv(),
       getRunnerLogger(true),
     );
 
@@ -375,6 +381,8 @@ test.serial(
         }),
         parseRepositoryNwo("github/codeql-action"),
         createFeatures([Feature.OverlayAnalysisStatusSave]),
+        "success",
+        getTestEnv(),
         getRunnerLogger(true),
       );
 
@@ -443,6 +451,8 @@ test.serial(
         }),
         parseRepositoryNwo("github/codeql-action"),
         createFeatures([]),
+        "success",
+        getTestEnv(),
         getRunnerLogger(true),
       );
 
@@ -457,8 +467,13 @@ test.serial(
 test.serial("does not save overlay status when build successful", async (t) => {
   return await util.withTmpDir(async (tmpDir) => {
     setupActionsVars(tmpDir, tmpDir);
-    // Mark analyze as having completed successfully.
+    // Mark analyze as having completed successfully. `tryUploadSarifIfRunFailed` reads this from
+    // the process environment, while `recordOverlayStatus` reads it from the environment it is
+    // given.
     process.env[EnvVar.ANALYZE_DID_COMPLETE_SUCCESSFULLY] = "true";
+    const env = getTestEnv({
+      [EnvVar.ANALYZE_DID_COMPLETE_SUCCESSFULLY]: "true",
+    });
 
     sinon.stub(util, "checkDiskUsage").resolves({
       numAvailableBytes: 100 * NUM_BYTES_PER_GIB,
@@ -480,6 +495,8 @@ test.serial("does not save overlay status when build successful", async (t) => {
       }),
       parseRepositoryNwo("github/codeql-action"),
       createFeatures([Feature.OverlayAnalysisStatusSave]),
+      "success",
+      env,
       getRunnerLogger(true),
     );
 
@@ -517,6 +534,8 @@ test.serial(
         }),
         parseRepositoryNwo("github/codeql-action"),
         createFeatures([]),
+        "success",
+        getTestEnv(),
         getRunnerLogger(true),
       );
 
@@ -528,6 +547,137 @@ test.serial(
   },
 );
 
+/**
+ * Runs `uploadFailureInfo` for an overlay-base job that did not complete successfully, with the
+ * given job status from the Actions runtime environment.
+ */
+async function runOverlayPostStep({
+  jobStatus,
+  codeQlReportedError = false,
+}: {
+  jobStatus: string | undefined;
+  codeQlReportedError?: boolean;
+}) {
+  return await util.withTmpDir(async (tmpDir) => {
+    setupActionsVars(tmpDir, tmpDir);
+    delete process.env[EnvVar.ANALYZE_DID_COMPLETE_SUCCESSFULLY];
+    const env = getTestEnv(
+      codeQlReportedError
+        ? { [EnvVar.JOB_STATUS]: JobStatus.FailureStatus }
+        : {},
+    );
+
+    sinon.stub(util, "checkDiskUsage").resolves({
+      numAvailableBytes: 100 * NUM_BYTES_PER_GIB,
+      numTotalBytes: 200 * NUM_BYTES_PER_GIB,
+    });
+
+    const saveOverlayStatusStub = sinon
+      .stub(overlayStatus, "saveOverlayStatus")
+      .resolves(true);
+
+    await initActionPostHelper.uploadFailureInfo(
+      sinon.spy(),
+      sinon.spy(),
+      codeql.createStubCodeQL({}),
+      createTestConfig({
+        debugMode: false,
+        languages: ["javascript"],
+        overlayDatabaseMode: OverlayDatabaseMode.OverlayBase,
+      }),
+      parseRepositoryNwo("github/codeql-action"),
+      createFeatures([Feature.OverlayAnalysisStatusSave]),
+      jobStatus,
+      env,
+      getRunnerLogger(true),
+    );
+
+    return { saveOverlayStatusStub };
+  });
+}
+
+test.serial(
+  "does not save overlay status when the job was cancelled",
+  async (t) => {
+    const { saveOverlayStatusStub } = await runOverlayPostStep({
+      jobStatus: "cancelled",
+    });
+
+    t.true(
+      saveOverlayStatusStub.notCalled,
+      "a cancellation tells us nothing about whether the analysis would have succeeded",
+    );
+  },
+);
+
+test.serial(
+  "does not save overlay status when the job status is not recognised",
+  async (t) => {
+    const { saveOverlayStatusStub } = await runOverlayPostStep({
+      jobStatus: "some-new-status",
+    });
+
+    t.true(
+      saveOverlayStatusStub.notCalled,
+      "a status we do not recognise tells us nothing about whether the analysis would have succeeded",
+    );
+  },
+);
+
+test.serial(
+  "does not save overlay status when the job status is unavailable",
+  async (t) => {
+    const { saveOverlayStatusStub } = await runOverlayPostStep({
+      jobStatus: undefined,
+    });
+
+    t.true(
+      saveOverlayStatusStub.notCalled,
+      "without a job status we cannot tell whether the analysis would have succeeded",
+    );
+  },
+);
+
+test.serial(
+  "saves overlay status when the job failed rather than being cancelled",
+  async (t) => {
+    const { saveOverlayStatusStub } = await runOverlayPostStep({
+      jobStatus: "failure",
+    });
+
+    t.true(
+      saveOverlayStatusStub.calledOnce,
+      "a failed job indicates that the analysis itself failed",
+    );
+  },
+);
+
+test.serial("saves overlay status when the job succeeded", async (t) => {
+  const { saveOverlayStatusStub } = await runOverlayPostStep({
+    jobStatus: "success",
+  });
+
+  t.true(
+    saveOverlayStatusStub.calledOnce,
+    "the analysis did not complete successfully even though the job as a whole succeeded",
+  );
+});
+
+test.serial(
+  "saves overlay status when a CodeQL Action reported an error before the run was cancelled",
+  async (t) => {
+    const { saveOverlayStatusStub } = await runOverlayPostStep({
+      jobStatus: "cancelled",
+      codeQlReportedError: true,
+    });
+
+    t.true(
+      saveOverlayStatusStub.calledOnce,
+      "the analysis genuinely failed, even though the run was later cancelled",
+    );
+  },
+);
+
 function createTestWorkflow(
   steps: workflow.WorkflowJobStep[],
 ): workflow.Workflow {

src/init-action-post-helper.test.ts

@@ -18,7 +18,7 @@ import {
   sanitizeArtifactName,
 } from "./debug-artifacts";
 import * as dependencyCaching from "./dependency-caching";
-import { EnvVar } from "./environment";
+import { EnvVar, ReadOnlyEnv } from "./environment";
 import { Feature, FeatureEnablement } from "./feature-flags";
 import { Logger } from "./logging";
 import { OverlayDatabaseMode } from "./overlay/overlay-database-mode";
@@ -316,6 +316,8 @@ export async function tryUploadSarifIfRunFailed(
  * @param config The CodeQL Action configuration.
  * @param repositoryNwo The name and owner of the repository.
  * @param features Information about enabled features.
+ * @param jobStatus The status of the job, as reported by the Actions runtime environment.
+ * @param env The environment to read variables from.
  * @param logger The logger to use.
  * @returns The results of uploading the SARIF file for the failure.
  */
@@ -331,9 +333,11 @@ export async function uploadFailureInfo(
   config: Config,
   repositoryNwo: RepositoryNwo,
   features: FeatureEnablement,
+  jobStatus: string | undefined,
+  env: ReadOnlyEnv,
   logger: Logger,
 ): Promise<UploadFailedSarifResult> {
-  await recordOverlayStatus(codeql, config, features, logger);
+  await recordOverlayStatus(codeql, config, features, jobStatus, env, logger);
 
   const uploadFailedSarifResult = await tryUploadSarifIfRunFailed(
     config,
@@ -412,6 +416,37 @@ export async function uploadFailureInfo(
   return uploadFailedSarifResult;
 }
 
+/**
+ * Whether one of the CodeQL Actions reported an error for this job, which means the analysis
+ * genuinely failed.
+ *
+ * Note that the converse does not hold: an Action that is terminated abruptly, or that fails before
+ * it can gather telemetry, does not get to report anything.
+ */
+function didCodeQlReportError(env: ReadOnlyEnv): boolean {
+  const jobStatus = env.getOptional(EnvVar.JOB_STATUS);
+  return (
+    jobStatus === JobStatus.FailureStatus ||
+    jobStatus === JobStatus.ConfigErrorStatus
+  );
+}
+
+/**
+ * Whether the job status tells us anything about whether the analysis itself would have succeeded.
+ *
+ * We check for the statuses we know to be meaningful rather than excluding the ones that are not,
+ * so that a status we do not recognise is treated as inconclusive.
+ */
+function isConclusiveJobStatus(jobStatus: string | undefined): boolean {
+  switch (jobStatus?.trim().toLowerCase()) {
+    case "failure":
+    case "success":
+      return true;
+    default:
+      return false;
+  }
+}
+
 /**
  * If overlay base database creation was attempted but the analysis did not complete
  * successfully, save the failure status to the Actions cache so that subsequent runs
@@ -421,16 +456,30 @@ async function recordOverlayStatus(
   codeql: CodeQL,
   config: Config,
   features: FeatureEnablement,
+  jobStatus: string | undefined,
+  env: ReadOnlyEnv,
   logger: Logger,
 ) {
   if (
     config.overlayDatabaseMode !== OverlayDatabaseMode.OverlayBase ||
-    process.env[EnvVar.ANALYZE_DID_COMPLETE_SUCCESSFULLY] === "true" ||
+    env.getOptional(EnvVar.ANALYZE_DID_COMPLETE_SUCCESSFULLY) === "true" ||
     !(await features.getValue(Feature.OverlayAnalysisStatusSave))
   ) {
     return;
   }
 
+  // Only record a failure when the job outcome tells us something about the analysis. A cancelled
+  // job, or a status we do not recognise, says nothing about whether the analysis would have
+  // succeeded, so recording a failure would disable overlay analysis needlessly. We still record
+  // one if a CodeQL Action reported an error before the job ended.
+  if (!isConclusiveJobStatus(jobStatus) && !didCodeQlReportError(env)) {
+    logger.info(
+      "Not recording an improved incremental analysis failure for this job because the job " +
+        `status (${jobStatus ?? "unset"}) does not tell us whether the analysis itself failed.`,
+    );
+    return;
+  }
+
   const checkRunIdInput = actionsUtil.getOptionalInput("check-run-id");
   const checkRunId =
     checkRunIdInput !== undefined ? parseInt(checkRunIdInput, 10) : undefined;