Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That approach is also more complex, though it might be interesting to explore later, particularly if we also want to evaluate excluding failures from non-CodeQL Action steps.

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];
Comment thread
mbg marked this conversation as resolved.
Outdated
return (
jobStatus === JobStatus.FailureStatus ||
jobStatus === JobStatus.ConfigErrorStatus
);
Comment on lines +428 to +431

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I expect that it's possible, but the most common case would be the job genuinely failing in some way. It's worth looking into telemetry more once we've tackled the low-hanging fruit, but I'll leave as is for now.

}

/**
* 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" &&
Comment thread
mbg marked this conversation as resolved.
Outdated
!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