Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions init/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never nice to have to feed this in through an extra action input, but I don't see a better approach for getting hold of the job status. An alternative option might be to set CODEQL_ACTION_STEP_(init|analyze|...) environment variables / state that we set to e.g. starting when the respective action starts and then to success or failure depending on the outcome. That should then allow us to identify which step started, succeeded, or failed (gracefully or not). For the overlay status, we could then check that all available environment variables with a CODEQL_ACTION_STEP_ prefix are success and none are starting or failure. The downside is that it wouldn't catch if the failure isn't related to what happens in CodeQL Action steps, or we fail to even set the starting value.

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
Expand Down
18 changes: 15 additions & 3 deletions lib/entry-points.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

95 changes: 95 additions & 0 deletions src/init-action-post-helper.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ 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,
Expand Down Expand Up @@ -58,6 +59,7 @@ test.serial("init-post action with debug mode off", async (t) => {
createTestConfig({ debugMode: false }),
parseRepositoryNwo("github/codeql-action"),
createFeatures([]),
"success",
getRunnerLogger(true),
);

Expand All @@ -80,6 +82,7 @@ test.serial("init-post action with debug mode on", async (t) => {
createTestConfig({ debugMode: true }),
parseRepositoryNwo("github/codeql-action"),
createFeatures([]),
"success",
getRunnerLogger(true),
);

Expand Down Expand Up @@ -375,6 +378,7 @@ test.serial(
}),
parseRepositoryNwo("github/codeql-action"),
createFeatures([Feature.OverlayAnalysisStatusSave]),
"success",
getRunnerLogger(true),
);

Expand Down Expand Up @@ -443,6 +447,7 @@ test.serial(
}),
parseRepositoryNwo("github/codeql-action"),
createFeatures([]),
"success",
getRunnerLogger(true),
);

Expand Down Expand Up @@ -480,6 +485,7 @@ test.serial("does not save overlay status when build successful", async (t) => {
}),
parseRepositoryNwo("github/codeql-action"),
createFeatures([Feature.OverlayAnalysisStatusSave]),
"success",
getRunnerLogger(true),
);

Expand Down Expand Up @@ -517,6 +523,7 @@ test.serial(
}),
parseRepositoryNwo("github/codeql-action"),
createFeatures([]),
"success",
getRunnerLogger(true),
);

Expand All @@ -528,6 +535,94 @@ test.serial(
},
);

/**
* Runs `uploadFailureInfo` for an overlay-base job that did not complete successfully, for a job
* that the Actions runtime environment reports as cancelled.
*/
async function testCancelledOverlayJob({
jobStatus = "cancelled",
codeQlReportedError = false,
}: {
jobStatus?: string;
codeQlReportedError?: boolean;
} = {}) {
return await util.withTmpDir(async (tmpDir) => {
setupActionsVars(tmpDir, tmpDir);
delete process.env[EnvVar.ANALYZE_DID_COMPLETE_SUCCESSFULLY];
if (codeQlReportedError) {
process.env[EnvVar.JOB_STATUS] = JobStatus.FailureStatus;
} else {
delete process.env[EnvVar.JOB_STATUS];
}

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,
getRunnerLogger(true),
);

return { saveOverlayStatusStub };
});
}

test.serial(
"does not save overlay status when the job was cancelled",
async (t) => {
const { saveOverlayStatusStub } = await testCancelledOverlayJob();

t.true(
saveOverlayStatusStub.notCalled,
"a cancellation tells us nothing about whether the analysis would have succeeded",
);
},
);

test.serial(
"saves overlay status when the job failed rather than being cancelled",
async (t) => {
const { saveOverlayStatusStub } = await testCancelledOverlayJob({
jobStatus: "failure",
});

t.true(
saveOverlayStatusStub.calledOnce,
"only cancellations are treated as unrelated to the analysis",
);
},
);

test.serial(
"saves overlay status when a CodeQL Action reported an error before the run was cancelled",
async (t) => {
const { saveOverlayStatusStub } = await testCancelledOverlayJob({
codeQlReportedError: true,
});

t.true(
saveOverlayStatusStub.calledOnce,
"the analysis genuinely failed, even though the run was later cancelled",
);
},
);

function createTestWorkflow(
steps: workflow.WorkflowJobStep[],
): workflow.Workflow {
Expand Down
34 changes: 33 additions & 1 deletion src/init-action-post-helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ 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 logger The logger to use.
* @returns The results of uploading the SARIF file for the failure.
*/
Expand All @@ -331,9 +332,10 @@ export async function uploadFailureInfo(
config: Config,
repositoryNwo: RepositoryNwo,
features: FeatureEnablement,
jobStatus: string | undefined,
logger: Logger,
): Promise<UploadFailedSarifResult> {
await recordOverlayStatus(codeql, config, features, logger);
await recordOverlayStatus(codeql, config, features, jobStatus, logger);

const uploadFailedSarifResult = await tryUploadSarifIfRunFailed(
config,
Expand Down Expand Up @@ -412,6 +414,21 @@ 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(): boolean {
const jobStatus = process.env[EnvVar.JOB_STATUS];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a ReadOnlyEnv parameter and use getOptional.

return (
jobStatus === JobStatus.FailureStatus ||
jobStatus === JobStatus.ConfigErrorStatus
);
Comment on lines +426 to +429

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we end up with one of these if the workflow job was cancelled? E.g. because it caused a thread abort style exception to be thrown at an inconvenient moment?

}

/**
* 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
Expand All @@ -421,6 +438,7 @@ async function recordOverlayStatus(
codeql: CodeQL,
config: Config,
features: FeatureEnablement,
jobStatus: string | undefined,
logger: Logger,
) {
if (
Expand All @@ -431,6 +449,20 @@ async function recordOverlayStatus(
return;
}

// A cancelled run tells us nothing about whether the analysis would have succeeded, so recording
// a failure would disable overlay analysis needlessly. Note that we still record a failure if one
// of our own Actions reported an error before the run was cancelled.
if (
jobStatus?.trim().toLowerCase() === "cancelled" &&

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than checking for the case where we don't want to record the overlay status, would it make sense to make this a check for the cases where we do want to record the overlay status?

That would be more defensive and guard against the hypothetical case of a new status value that we don't recognise.

I would also expect that we'd have a jobStatus value here unless something broke badly?

!didCodeQlReportError()
) {
logger.info(
"Not recording an improved incremental analysis failure for this job because the workflow " +
"run was cancelled.",
);
return;
}

const checkRunIdInput = actionsUtil.getOptionalInput("check-run-id");
const checkRunId =
checkRunIdInput !== undefined ? parseInt(checkRunIdInput, 10) : undefined;
Expand Down
7 changes: 7 additions & 0 deletions src/init-action-post.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import * as core from "@actions/core";

import {
restoreInputs,
getOptionalInput,
getTemporaryDirectory,
printDebugLogs,
} from "./actions-util";
Expand Down Expand Up @@ -55,6 +56,11 @@ async function run(startedAt: Date) {
| undefined;
let dependencyCachingUsage: DependencyCachingUsageReport | undefined;
try {
// Read the job status before restoring inputs, since it is provided by the Actions runtime
// environment for this step and would otherwise be overwritten by the value that the `init`
// Action saw, which is always a success.
const jobStatus = getOptionalInput("job-status");

// Restore inputs from `init` Action.
restoreInputs();

Expand Down Expand Up @@ -84,6 +90,7 @@ async function run(startedAt: Date) {
config,
repositoryNwo,
features,
jobStatus,
logger,
);

Expand Down
Loading