From 1331773b9afd271389555968e8fe193c3d4234fa Mon Sep 17 00:00:00 2001 From: Henry Mercer Date: Fri, 4 Sep 2026 10:26:09 +0100 Subject: [PATCH] Address Copilot review feedback Run the cleanup even when the download will not be cached in the toolcache, since the toolcache shares a filesystem with the directory we extract to, so freeing it helps either way, and report an error other than the toolcache being absent as a failure rather than as an empty toolcache. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- lib/entry-points.js | 20 ++++++++----- src/setup-codeql.test.ts | 64 +++++++++++++++++++++++++++++++++++++++- src/setup-codeql.ts | 12 ++------ src/tools-download.ts | 14 ++++++--- 4 files changed, 89 insertions(+), 21 deletions(-) diff --git a/lib/entry-points.js b/lib/entry-points.js index 37c35dde0..25d5908a0 100644 --- a/lib/entry-points.js +++ b/lib/entry-points.js @@ -151811,11 +151811,17 @@ async function deleteToolcacheBundles(logger) { ); return { deletedVersions: [], failed: true }; } - } catch { - logger.debug( - `There are no CodeQL tools at ${toolDirectory} to delete from the toolcache.` + } catch (e) { + if (e?.code === "ENOENT") { + logger.debug( + `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)}` ); - return { deletedVersions: [], failed: false }; + return { deletedVersions: [], failed: true }; } try { const versions = (await fs13.promises.readdir(toolDirectory, { withFileTypes: true })).filter((entry) => entry.isDirectory()).map((entry) => entry.name).sort(); @@ -152326,7 +152332,7 @@ var downloadCodeQL = async function(codeqlURL, compressionMethod, maybeBundleVer logger ); const extractedBundlePath = toolcacheInfo?.path ?? getTempExtractionDir(tempDir); - await tryDeleteToolcacheBundles(toolcacheInfo?.version, features, logger); + await tryDeleteToolcacheBundles(features, logger); const statusReport = await downloadAndExtract( codeqlURL, compressionMethod, @@ -152367,14 +152373,14 @@ function getToolcacheDestinationInfo(maybeBundleVersion, maybeCliVersion, logger } return void 0; } -async function tryDeleteToolcacheBundles(destinationVersion, features, logger) { +async function tryDeleteToolcacheBundles(features, logger) { if (getOptionalEnvVar("CODEQL_ACTION_HAS_OBTAINED_CODEQL_TOOLS" /* HAS_OBTAINED_CODEQL_TOOLS */) !== void 0) { logger.debug( "Not deleting the CodeQL tools from the toolcache since a previous step in this job has already obtained them." ); return; } - if (destinationVersion === void 0 || !isGitHubHostedRunner() || !await features.getValue("cleanup_toolcache_bundles" /* CleanupToolcacheBundles */)) { + if (!isGitHubHostedRunner() || !await features.getValue("cleanup_toolcache_bundles" /* CleanupToolcacheBundles */)) { return; } let result = { deletedVersions: [], failed: true }; diff --git a/src/setup-codeql.test.ts b/src/setup-codeql.test.ts index 3bfdaedd2..211e17e2d 100644 --- a/src/setup-codeql.test.ts +++ b/src/setup-codeql.test.ts @@ -974,6 +974,7 @@ function createToolcacheEntry( async function runDownloadCodeQL( toolcacheRoot: string, features: Feature[], + bundleVersion: string | undefined = CLEANUP_BUNDLE_VERSION, ): Promise { sinon .stub(toolsDownload, "downloadAndExtract") @@ -988,7 +989,7 @@ async function runDownloadCodeQL( await setupCodeql.downloadCodeQL( "https://example.com/codeql-bundle.tar.gz", "gzip", - CLEANUP_BUNDLE_VERSION, + bundleVersion, CLEANUP_CLI_VERSION, SAMPLE_DOTCOM_API_DETAILS, undefined, // tarVersion @@ -1309,3 +1310,64 @@ test.serial( }); }, ); + +test.serial( + "downloadCodeQL cleans up the toolcache even when the download will not be cached", + async (t) => { + // A `tools` URL we can't derive a bundle version from is extracted to a temporary directory + // rather than the toolcache, but the toolcache is on the same filesystem, so emptying it still + // frees up space for the analysis. + await withTmpDir(async (tmpDir) => { + setupActionsVars(tmpDir, tmpDir); + process.env[ActionsEnvVars.RUNNER_ENVIRONMENT] = "github-hosted"; + + const staleDirectory = createToolcacheEntry( + tmpDir, + "CodeQL", + CLEANUP_STALE_VERSION, + ); + + const cleanupDiagnostic = await runDownloadCodeQL( + tmpDir, + [Feature.CleanupToolcacheBundles], + undefined, // bundleVersion + ); + + t.false(fs.existsSync(staleDirectory)); + t.deepEqual(cleanupDiagnostic, { + deletedVersions: [CLEANUP_STALE_VERSION], + failed: false, + }); + }); + }, +); + +test.serial( + "downloadCodeQL reports a failure when the toolcache cannot be inspected", + async (t) => { + await withTmpDir(async (tmpDir) => { + setupActionsVars(tmpDir, tmpDir); + process.env[ActionsEnvVars.RUNNER_ENVIRONMENT] = "github-hosted"; + + createToolcacheEntry(tmpDir, "CodeQL", CLEANUP_STALE_VERSION); + + const lstatStub = sinon.stub(fs.promises, "lstat").rejects( + Object.assign(new Error("permission denied"), { + code: "EACCES", + }), + ); + + const cleanupDiagnostic = await runDownloadCodeQL(tmpDir, [ + Feature.CleanupToolcacheBundles, + ]); + + lstatStub.restore(); + + t.deepEqual( + cleanupDiagnostic, + { deletedVersions: [], failed: true }, + "An error other than the toolcache being absent must not be reported as success.", + ); + }); + }, +); diff --git a/src/setup-codeql.ts b/src/setup-codeql.ts index 818fee28c..a7478ed52 100644 --- a/src/setup-codeql.ts +++ b/src/setup-codeql.ts @@ -823,7 +823,7 @@ export const downloadCodeQL = async function ( const extractedBundlePath = toolcacheInfo?.path ?? getTempExtractionDir(tempDir); - await tryDeleteToolcacheBundles(toolcacheInfo?.version, features, logger); + await tryDeleteToolcacheBundles(features, logger); const statusReport = await downloadAndExtract( codeqlURL, @@ -881,13 +881,10 @@ function getToolcacheDestinationInfo( * Reclaims disk space by deleting the CodeQL tools from the toolcache, if enabled. * * On GitHub-hosted runners the toolcache shares a filesystem with the workspace, so tools left in - * the toolcache take up space that the analysis could use instead. - * - * @param destinationVersion The toolcache version number that the tools will be stored under, or - * `undefined` if they will not be stored in the toolcache. + * 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( - destinationVersion: string | undefined, features: FeatureEnablement, logger: Logger, ): Promise { @@ -901,10 +898,7 @@ async function tryDeleteToolcacheBundles( return; } - // If we are not going to add the tools to the toolcache, we are extracting them somewhere else - // and emptying the toolcache would not buy us the space we need. if ( - destinationVersion === undefined || !isGitHubHostedRunner() || !(await features.getValue(Feature.CleanupToolcacheBundles)) ) { diff --git a/src/tools-download.ts b/src/tools-download.ts index d8dc485db..aa31889a9 100644 --- a/src/tools-download.ts +++ b/src/tools-download.ts @@ -255,11 +255,17 @@ export async function deleteToolcacheBundles( ); return { deletedVersions: [], failed: true }; } - } catch { - logger.debug( - `There are no CodeQL tools at ${toolDirectory} to delete from the toolcache.`, + } catch (e: any) { + if (e?.code === "ENOENT") { + logger.debug( + `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)}`, ); - return { deletedVersions: [], failed: false }; + return { deletedVersions: [], failed: true }; } try {