From 31da345c0713d01d3f538a8bf9de9a6baba42f85 Mon Sep 17 00:00:00 2001 From: Henry Mercer Date: Fri, 4 Sep 2026 17:43:40 +0100 Subject: [PATCH] Address review comments Delete each version directory individually so that a symlinked one is skipped rather than removed, take an `ActionState` so the environment is read through `ReadOnlyEnv` rather than the deprecated `getOptionalEnvVar`, let `deleteToolcacheBundles` report its own failure to locate the toolcache instead of having the caller catch it, quote paths in log messages, and rename `HAS_OBTAINED_CODEQL_TOOLS` to `HAS_SET_UP_CODEQL`, which is also set when we find the tools in the toolcache rather than downloading them. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- lib/entry-points.js | 77 +++++++++++++++++++++++++++------------- src/environment.ts | 6 ++-- src/setup-codeql.test.ts | 72 ++++++++++++++++++++++++++----------- src/setup-codeql.ts | 35 ++++++++---------- src/tools-download.ts | 67 ++++++++++++++++++++++++---------- 5 files changed, 170 insertions(+), 87 deletions(-) diff --git a/lib/entry-points.js b/lib/entry-points.js index 087acdfbf..249db17be 100644 --- a/lib/entry-points.js +++ b/lib/entry-points.js @@ -151813,36 +151813,68 @@ function isToolcacheOnWorkspaceFilesystem(logger) { } } async function deleteToolcacheBundles(logger) { - const toolDirectory = getToolcacheToolDirectory(); + let toolDirectory; + try { + toolDirectory = getToolcacheToolDirectory(); + } catch (e) { + logger.info( + `Unable to reclaim disk space from the toolcache: ${getErrorMessage(e)}` + ); + return { deletedVersions: [], failed: true }; + } try { if ((await fs13.promises.lstat(toolDirectory)).isSymbolicLink()) { logger.info( - `Not deleting the CodeQL tools from the toolcache since ${toolDirectory} is a symlink.` + `Not deleting the CodeQL tools from the toolcache since '${toolDirectory}' is a symlink.` ); return { deletedVersions: [], failed: true }; } } catch (e) { if (e?.code === "ENOENT") { logger.debug( - `There are no CodeQL tools at ${toolDirectory} to delete from the toolcache.` + `There are no CodeQL tools at '${toolDirectory}' to delete from the toolcache.` ); return { deletedVersions: [], failed: false }; } logger.info( - `Failed to inspect the CodeQL tools at ${toolDirectory}: ${getErrorMessage(e)}` + `Failed to inspect the CodeQL tools at '${toolDirectory}': ${getErrorMessage(e)}` ); return { deletedVersions: [], failed: true }; } try { - const versions = (await fs13.promises.readdir(toolDirectory, { withFileTypes: true })).filter((entry) => entry.isDirectory()).map((entry) => entry.name).sort(); - await fs13.promises.rm(toolDirectory, { force: true, recursive: true }); - logger.info( - `Deleted the CodeQL tools at ${toolDirectory} from the toolcache to free up disk space. Versions deleted: ${versions.join(", ") || "none"}.` - ); - return { deletedVersions: versions, failed: false }; + const entries = await fs13.promises.readdir(toolDirectory, { + withFileTypes: true + }); + const deletedVersions = []; + let failed = false; + for (const entry of entries) { + const versionDirectory = path12.join(toolDirectory, entry.name); + if (!entry.isDirectory()) { + logger.debug( + `Not deleting '${versionDirectory}' from the toolcache since it is not a directory.` + ); + continue; + } + try { + await fs13.promises.rm(versionDirectory, { + force: true, + recursive: true + }); + deletedVersions.push(entry.name); + logger.info( + `Deleted the CodeQL tools at '${versionDirectory}' from the toolcache to free up disk space.` + ); + } catch (e) { + failed = true; + logger.info( + `Failed to delete the CodeQL tools at '${versionDirectory}' from the toolcache: ${getErrorMessage(e)}` + ); + } + } + return { deletedVersions: deletedVersions.sort(), failed }; } catch (e) { logger.info( - `Failed to delete the CodeQL tools at ${toolDirectory} from the toolcache: ${getErrorMessage(e)}` + `Failed to read the CodeQL tools at '${toolDirectory}' from the toolcache: ${getErrorMessage(e)}` ); return { deletedVersions: [], failed: true }; } @@ -152342,7 +152374,7 @@ var downloadCodeQL = async function(codeqlURL, compressionMethod, maybeBundleVer logger ); const extractedBundlePath = toolcacheInfo?.path ?? getTempExtractionDir(tempDir); - await tryDeleteToolcacheBundles(features, logger); + await tryDeleteToolcacheBundles({ env: getEnv(), features, logger }); const statusReport = await downloadAndExtract( codeqlURL, compressionMethod, @@ -152383,24 +152415,21 @@ function getToolcacheDestinationInfo(maybeBundleVersion, maybeCliVersion, logger } return void 0; } -async function tryDeleteToolcacheBundles(features, logger) { - if (getOptionalEnvVar("CODEQL_ACTION_HAS_OBTAINED_CODEQL_TOOLS" /* HAS_OBTAINED_CODEQL_TOOLS */) !== void 0) { +async function tryDeleteToolcacheBundles({ + env, + features, + logger +}) { + if (env.getOptional("CODEQL_ACTION_HAS_SET_UP_CODEQL" /* HAS_SET_UP_CODEQL */) !== void 0) { logger.debug( - "Not deleting the CodeQL tools from the toolcache since a previous step in this job has already obtained them." + "Not deleting the CodeQL tools from the toolcache since a previous step in this job has already set up CodeQL." ); return; } if (!isGitHubHostedRunner() || !isToolcacheOnWorkspaceFilesystem(logger) || !await features.getValue("cleanup_toolcache_bundles" /* CleanupToolcacheBundles */)) { return; } - let result = { deletedVersions: [], failed: true }; - try { - result = await deleteToolcacheBundles(logger); - } catch (e) { - logger.info( - `Unable to reclaim disk space from the toolcache: ${getErrorMessage(e)}` - ); - } + const result = await deleteToolcacheBundles(logger); addNoLanguageDiagnostic( void 0, makeTelemetryDiagnostic( @@ -152476,7 +152505,7 @@ async function setupCodeQLBundle(toolsInput, apiDetails, tempDir, variant, defau default: assertNever(source); } - core12.exportVariable("CODEQL_ACTION_HAS_OBTAINED_CODEQL_TOOLS" /* HAS_OBTAINED_CODEQL_TOOLS */, "true"); + core12.exportVariable("CODEQL_ACTION_HAS_SET_UP_CODEQL" /* HAS_SET_UP_CODEQL */, "true"); return { codeqlFolder, toolsDownloadStatusReport, diff --git a/src/environment.ts b/src/environment.ts index aa1c3f9e4..bf4bb4f71 100644 --- a/src/environment.ts +++ b/src/environment.ts @@ -64,10 +64,10 @@ export enum EnvVar { HAS_WARNED_ABOUT_DISK_SPACE = "CODEQL_ACTION_HAS_WARNED_ABOUT_DISK_SPACE", /** - * Whether a step in this job has already obtained the CodeQL tools. Steps that run afterwards may - * be holding a path into the toolcache, so we must not delete anything from it. + * Whether a step in this job has already set up CodeQL. Steps that run afterwards may be holding + * a path into the toolcache, so we must not delete anything from it. */ - HAS_OBTAINED_CODEQL_TOOLS = "CODEQL_ACTION_HAS_OBTAINED_CODEQL_TOOLS", + HAS_SET_UP_CODEQL = "CODEQL_ACTION_HAS_SET_UP_CODEQL", /** Whether the `setup-codeql` action has been run. */ SETUP_CODEQL_ACTION_HAS_RUN = "CODEQL_ACTION_SETUP_CODEQL_HAS_RUN", diff --git a/src/setup-codeql.test.ts b/src/setup-codeql.test.ts index a02a2319c..9fab50f00 100644 --- a/src/setup-codeql.test.ts +++ b/src/setup-codeql.test.ts @@ -1199,24 +1199,19 @@ test.serial( ); test.serial( - "downloadCodeQL continues when cleaning up the toolcache throws", + "deleteToolcacheBundles reports a failure when the toolcache location is unknown", async (t) => { - await withTmpDir(async (tmpDir) => { - setupActionsVars(tmpDir, tmpDir); - process.env[ActionsEnvVars.RUNNER_ENVIRONMENT] = "github-hosted"; + delete process.env[ActionsEnvVars.RUNNER_TOOL_CACHE]; - createToolcacheEntry(tmpDir, "CodeQL", CLEANUP_CLI_VERSION); + const result = await toolsDownload.deleteToolcacheBundles( + getRunnerLogger(true), + ); - sinon - .stub(toolsDownload, "deleteToolcacheBundles") - .rejects(new Error("RUNNER_TOOL_CACHE is not set")); - - const cleanupDiagnostic = await runDownloadCodeQL(tmpDir, [ - Feature.CleanupToolcacheBundles, - ]); - - t.deepEqual(cleanupDiagnostic, { deletedVersions: [], failed: true }); - }); + t.deepEqual( + result, + { deletedVersions: [], failed: true }, + "Should report a failure rather than throwing, so the download can continue.", + ); }, ); @@ -1255,7 +1250,7 @@ test.serial( ); test.serial( - "downloadCodeQL does not clean up the toolcache once a step has already obtained the tools", + "downloadCodeQL does not clean up the toolcache once a step has already set up CodeQL", async (t) => { // `.github/workflows/codeql.yml` sets up CodeQL twice and then runs both returned paths. If the // second setup downloads, it must not delete the bundle the first one handed out. @@ -1265,7 +1260,7 @@ test.serial( features: [Feature.CleanupToolcacheBundles], runnerEnvironment: "github-hosted", setUp: () => { - process.env[EnvVar.HAS_OBTAINED_CODEQL_TOOLS] = "true"; + process.env[EnvVar.HAS_SET_UP_CODEQL] = "true"; }, }, ({ cleanupDiagnostic, destinationDirectory, staleDirectory }) => { @@ -1278,11 +1273,11 @@ test.serial( ); test.serial( - "setupCodeQLBundle records that this job has obtained the CodeQL tools", + "setupCodeQLBundle records that this job has set up CodeQL", async (t) => { await withTmpDir(async (tmpDir) => { setupActionsVars(tmpDir, tmpDir); - delete process.env[EnvVar.HAS_OBTAINED_CODEQL_TOOLS]; + delete process.env[EnvVar.HAS_SET_UP_CODEQL]; sinon.stub(setupCodeql, "downloadCodeQL").resolves({ codeqlFolder: "codeql", @@ -1303,7 +1298,7 @@ test.serial( ); t.is( - process.env[EnvVar.HAS_OBTAINED_CODEQL_TOOLS], + process.env[EnvVar.HAS_SET_UP_CODEQL], "true", "A later step must be able to tell that the toolcache is in use.", ); @@ -1402,7 +1397,7 @@ test.serial( ); test.serial( - "isToolcacheOnWorkspaceFilesystem compares the toolcache against the workspace", + "isToolcacheOnWorkspaceFilesystem assumes a different filesystem when it cannot tell", async (t) => { await withTmpDir(async (tmpDir) => { const logger = getRunnerLogger(true); @@ -1419,3 +1414,38 @@ test.serial( }); }, ); + +test.serial( + "downloadCodeQL does not delete through a symlinked version directory", + async (t) => { + await withTmpDir(async (tmpDir) => { + const toolcacheRoot = path.join(tmpDir, "toolcache"); + setupActionsVars(tmpDir, toolcacheRoot); + process.env[ActionsEnvVars.RUNNER_ENVIRONMENT] = "github-hosted"; + + createToolcacheEntry(toolcacheRoot, "CodeQL", CLEANUP_STALE_VERSION); + + // Somewhere outside the toolcache that a version directory points at. + const outsideDirectory = path.join(tmpDir, "outside"); + fs.mkdirSync(outsideDirectory, { recursive: true }); + fs.writeFileSync(path.join(outsideDirectory, "contents"), "x"); + fs.symlinkSync( + outsideDirectory, + path.join(toolcacheRoot, "CodeQL", "9.9.9"), + ); + + const cleanupDiagnostic = await runDownloadCodeQL(toolcacheRoot, [ + Feature.CleanupToolcacheBundles, + ]); + + t.true( + fs.existsSync(path.join(outsideDirectory, "contents")), + "Should not delete anything through a symlinked version directory.", + ); + t.deepEqual(cleanupDiagnostic, { + deletedVersions: [CLEANUP_STALE_VERSION], + failed: false, + }); + }); + }, +); diff --git a/src/setup-codeql.ts b/src/setup-codeql.ts index 193d77341..3a99e04aa 100644 --- a/src/setup-codeql.ts +++ b/src/setup-codeql.ts @@ -8,6 +8,7 @@ import { default as deepEqual } from "fast-deep-equal"; import * as semver from "semver"; import { v4 as uuidV4 } from "uuid"; +import { ActionState } from "./action-common"; import { isAnalyzingPullRequest, isDynamicWorkflow, @@ -21,7 +22,7 @@ import { makeDiagnostic, makeTelemetryDiagnostic, } from "./diagnostics"; -import { EnvVar } from "./environment"; +import { EnvVar, getEnv } from "./environment"; import { CODEQL_VERSION_ZSTD_BUNDLE, CodeQLDefaultVersionInfo, @@ -37,7 +38,6 @@ import { downloadAndExtract, getToolcacheDirectory, isToolcacheOnWorkspaceFilesystem, - ToolcacheCleanupResult, ToolsDownloadStatusReport, writeToolcacheMarkerFile, } from "./tools-download"; @@ -824,7 +824,7 @@ export const downloadCodeQL = async function ( const extractedBundlePath = toolcacheInfo?.path ?? getTempExtractionDir(tempDir); - await tryDeleteToolcacheBundles(features, logger); + await tryDeleteToolcacheBundles({ env: getEnv(), features, logger }); const statusReport = await downloadAndExtract( codeqlURL, @@ -885,16 +885,17 @@ function getToolcacheDestinationInfo( * the toolcache take up space that the analysis could use instead. This holds wherever we extract * the tools we are obtaining, since the toolcache is on that filesystem either way. */ -async function tryDeleteToolcacheBundles( - features: FeatureEnablement, - logger: Logger, -): Promise { - // A step that has already obtained the CodeQL tools may hand out a path into the toolcache that a - // later step runs, so only the first step to obtain them can know that nothing else relies on it. - if (util.getOptionalEnvVar(EnvVar.HAS_OBTAINED_CODEQL_TOOLS) !== undefined) { +async function tryDeleteToolcacheBundles({ + env, + features, + logger, +}: ActionState<["Logger", "ReadOnlyEnv", "FeatureFlags"]>): Promise { + // A step that has already set up CodeQL may hand out a path into the toolcache that a later step + // runs, so only the first step to set it up can know that nothing else relies on the toolcache. + if (env.getOptional(EnvVar.HAS_SET_UP_CODEQL) !== undefined) { logger.debug( "Not deleting the CodeQL tools from the toolcache since a previous step in this job has " + - "already obtained them.", + "already set up CodeQL.", ); return; } @@ -907,15 +908,7 @@ async function tryDeleteToolcacheBundles( return; } - let result: ToolcacheCleanupResult = { deletedVersions: [], failed: true }; - - try { - result = await deleteToolcacheBundles(logger); - } catch (e) { - logger.info( - `Unable to reclaim disk space from the toolcache: ${util.getErrorMessage(e)}`, - ); - } + const result = await deleteToolcacheBundles(logger); addNoLanguageDiagnostic( undefined, @@ -1051,7 +1044,7 @@ export async function setupCodeQLBundle( // Record that this job now has a copy of the CodeQL tools, so that a later step doesn't delete // the toolcache out from under the path we are about to return. - core.exportVariable(EnvVar.HAS_OBTAINED_CODEQL_TOOLS, "true"); + core.exportVariable(EnvVar.HAS_SET_UP_CODEQL, "true"); return { codeqlFolder, diff --git a/src/tools-download.ts b/src/tools-download.ts index 33b3e78cb..cf51c213c 100644 --- a/src/tools-download.ts +++ b/src/tools-download.ts @@ -252,7 +252,7 @@ export interface ToolcacheCleanupResult { } /** - * Deletes the CodeQL tools from the toolcache. + * Deletes every version of the CodeQL tools from the toolcache. * * Only safe to call when we are about to download the tools, since that means we did not resolve * them from the toolcache and so nothing in there is in use by this job. @@ -265,49 +265,80 @@ export interface ToolcacheCleanupResult { export async function deleteToolcacheBundles( logger: Logger, ): Promise { - const toolDirectory = getToolcacheToolDirectory(); + let toolDirectory: string; + + try { + toolDirectory = getToolcacheToolDirectory(); + } catch (e) { + logger.info( + `Unable to reclaim disk space from the toolcache: ${getErrorMessage(e)}`, + ); + return { deletedVersions: [], failed: true }; + } try { // Refuse to follow a symlinked CodeQL directory, so that we can only ever delete paths that are // really inside the toolcache. if ((await fs.promises.lstat(toolDirectory)).isSymbolicLink()) { logger.info( - `Not deleting the CodeQL tools from the toolcache since ${toolDirectory} is a symlink.`, + `Not deleting the CodeQL tools from the toolcache since '${toolDirectory}' is a symlink.`, ); return { deletedVersions: [], failed: true }; } } catch (e: any) { if (e?.code === "ENOENT") { logger.debug( - `There are no CodeQL tools at ${toolDirectory} to delete from the toolcache.`, + `There are no CodeQL tools at '${toolDirectory}' to delete from the toolcache.`, ); return { deletedVersions: [], failed: false }; } logger.info( - `Failed to inspect the CodeQL tools at ${toolDirectory}: ${getErrorMessage(e)}`, + `Failed to inspect the CodeQL tools at '${toolDirectory}': ${getErrorMessage(e)}`, ); return { deletedVersions: [], failed: true }; } try { - const versions = ( - await fs.promises.readdir(toolDirectory, { withFileTypes: true }) - ) - .filter((entry) => entry.isDirectory()) - .map((entry) => entry.name) - .sort(); + const entries = await fs.promises.readdir(toolDirectory, { + withFileTypes: true, + }); - await fs.promises.rm(toolDirectory, { force: true, recursive: true }); + const deletedVersions: string[] = []; + let failed = false; - logger.info( - `Deleted the CodeQL tools at ${toolDirectory} from the toolcache to free up disk space. ` + - `Versions deleted: ${versions.join(", ") || "none"}.`, - ); + for (const entry of entries) { + const versionDirectory = path.join(toolDirectory, entry.name); - return { deletedVersions: versions, failed: false }; + // `isDirectory` is false for a symlink, so we never delete a version directory that is + // really somewhere else. + if (!entry.isDirectory()) { + logger.debug( + `Not deleting '${versionDirectory}' from the toolcache since it is not a directory.`, + ); + continue; + } + + try { + await fs.promises.rm(versionDirectory, { + force: true, + recursive: true, + }); + deletedVersions.push(entry.name); + logger.info( + `Deleted the CodeQL tools at '${versionDirectory}' from the toolcache to free up disk space.`, + ); + } catch (e) { + failed = true; + logger.info( + `Failed to delete the CodeQL tools at '${versionDirectory}' from the toolcache: ${getErrorMessage(e)}`, + ); + } + } + + return { deletedVersions: deletedVersions.sort(), failed }; } catch (e) { logger.info( - `Failed to delete the CodeQL tools at ${toolDirectory} from the toolcache: ${getErrorMessage(e)}`, + `Failed to read the CodeQL tools at '${toolDirectory}' from the toolcache: ${getErrorMessage(e)}`, ); return { deletedVersions: [], failed: true }; }