Skip to content
Merged
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.

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
30 changes: 26 additions & 4 deletions lib/entry-points.js

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

152 changes: 151 additions & 1 deletion src/init-action-post-helper.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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),
);

Expand All @@ -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),
);

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

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

Expand All @@ -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,
Expand All @@ -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),
);

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

Expand All @@ -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 {
Expand Down
55 changes: 52 additions & 3 deletions src/init-action-post-helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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.
*/
Expand All @@ -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,
Expand Down Expand Up @@ -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
);
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.

}

/**
* 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
Expand All @@ -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;
Expand Down
Loading
Loading