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

@@ -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 };

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<toolsDownload.ToolcacheCleanupResult | undefined> {
   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.",
+      );
+    });
+  },
+);

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<void> {
@@ -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))
   ) {

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 {