Skip to content

Delete unused CodeQL bundles from the toolcache before downloading - #4124

Open
henrymercer wants to merge 8 commits into
mainfrom
henrymercer/toolcache-bundle-cleanup
Open

Delete unused CodeQL bundles from the toolcache before downloading#4124
henrymercer wants to merge 8 commits into
mainfrom
henrymercer/toolcache-bundle-cleanup

Conversation

@henrymercer

@henrymercer henrymercer commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

When a job downloads a CodeQL bundle, the toolcache is often already holding a different one that the job will not use, typically the bundle baked into the runner image. On GitHub-hosted runners the toolcache sits on the same filesystem we measure for the disk space check, so that unused bundle takes around 2 GB away from the analysis.

This adds a feature flag, cleanup_toolcache_bundles, that empties $RUNNER_TOOL_CACHE/CodeQL immediately before we download a bundle. It is off by default.

Emptying it before the download rather than after means the space also covers the archive during extraction. We only get as far as downloading when the tools were not resolved from the toolcache, and a lookup there requires both the version directory and its completion marker, so nothing left behind is usable by the job.

That stops holding once a step has obtained the tools, because a later step may run a path it was given, as our own codeql.yml does. The job records that it has the tools and skips the cleanup from then on.

The cleanup only runs when RUNNER_ENVIRONMENT reports github-hosted, so we never delete from a toolcache that outlives the job, and only when the toolcache is on the same filesystem as the workspace, since otherwise the space we free is not space the analysis can use. Windows runner images are laid out that way, keeping the toolcache on C: while the workspace is on D:. Deletion is confined to the CodeQL directory, refuses to follow a symlink, and logs failures rather than throwing.

This also renames isHostedRunner to looksLikeHostedRunner, with no behaviour change. It infers hostedness from the runner name and filesystem layout, so it can disagree with the new isGitHubHostedRunner check. Its callers are deciding whether caching is worthwhile, where the inferred answer is arguably the one they want, so switching them would be a behaviour change and is left for a separate PR.

Risk assessment

  • Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Which use cases does this change impact?

Workflow types:

  • Advanced setup - Impacts users who have custom CodeQL workflows.
  • Managed - Impacts users with dynamic workflows (Default Setup, Code Quality, ...).

Products:

  • Code Scanning - The changes impact analyses when analysis-kinds: code-scanning.
  • Code Quality - The changes impact analyses when analysis-kinds: code-quality.
  • Other first-party - The changes impact other first-party analyses.
  • Third-party analyses - The changes affect the upload-sarif action.

The cleanup sits on the shared bundle download path, so it can run for anything that obtains the CodeQL tools.

Environments:

  • Dotcom - Impacts CodeQL workflows on github.com and/or GitHub Enterprise Cloud with Data Residency.

How did/will you validate this change?

  • End-to-end tests - I am depending on PR checks (i.e. tests in pr-checks).
  • Unit tests - I am depending on unit test coverage (i.e. tests in .test.ts files).
  • Other - Please provide details.

Unit tests cover the flag being off, a non-hosted runner, an unreported runner environment, a toolcache on a different filesystem to the workspace, a hosted runner with bundles present, a download that will not be cached in the toolcache, a step having already obtained the tools, deletion failing, a toolcache we cannot inspect, and a symlinked CodeQL directory. The multi-language PR check runs with the flag enabled, so the cleanup happens ahead of a real build and analysis of seven languages.

This has not been run on a real hosted runner outside the PR checks, and the disk saving is inferred from bundle size rather than measured, so the rollout should confirm it.

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

  • Feature flags - All new or changed code paths can be fully disabled with corresponding feature flags.

The rename is not behind the flag, but it has no behaviour change.

How will you know if something goes wrong after this change is released?

  • Telemetry - I rely on existing telemetry or have made changes to the telemetry.

A codeql-action/toolcache-bundle-cleanup diagnostic reports which versions were deleted and whether the attempt failed. Note that diagnostics are only flushed by the init action, so a cleanup under the standalone setup-codeql action reports nothing. That is pre-existing behaviour for diagnostics raised from setup-codeql.ts, but it does leave that entry point unmeasurable.

Are there any special considerations for merging or releasing this change?

  • No special considerations - This change can be merged at any time.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Consider adding a changelog entry for this change.
  • Confirm the readme and docs have been updated if necessary.

henrymercer and others added 4 commits September 4, 2026 10:12
`isHostedRunner` infers hostedness from the runner name and the toolcache path, so it also matches self-hosted runners that are configured to resemble hosted ones. Rename it to `looksLikeHostedRunner` so callers can see they are getting a heuristic, and add `isGitHubHostedRunner`, which reads the `RUNNER_ENVIRONMENT` value the Actions service reports. The existing callers keep the heuristic, so there is no behaviour change.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
`deleteToolcacheBundles` removes `$RUNNER_TOOL_CACHE/CodeQL` and reports which versions were there. It refuses to follow a symlinked CodeQL directory so that it can only ever delete paths that are really inside the toolcache, and reports failures rather than throwing. Not called yet.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
When we download a bundle the toolcache often already holds a different one that the job will not use, and on GitHub-hosted runners it shares a filesystem with the workspace, so it takes space away from the analysis. Empty the toolcache before downloading, which also frees space for the archive during extraction, and which is safe because getting as far as a download means the tools were not resolved from the toolcache. Skip this once a step has obtained the tools, since a later step may run a path it was given, and gate it on the runner being GitHub-hosted and on a feature flag that is off by default.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The check installs the tools from a URL, so most versions in its matrix are downloaded rather than found in the toolcache, which is when the cleanup runs, and it then builds and analyses seven languages, so a bundle we damaged on the way in would show up.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@henrymercer
henrymercer requested a balanced review from Copilot September 4, 2026 09:15
@github-actions github-actions Bot added the size/L May be hard to review label Sep 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

  • Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.

Copilot review overview

🟡 Changes recommended

Two moderate issues affect cleanup coverage and failure telemetry.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 2 Medium severity

New issues introduced by this change (2)
Severity Finding
Medium severity src/​setup-codeql.ts — This condition excludes supported explicit tools URLs that do not contain a /codeql-bundle-.../
Medium severity src/​tools-download.ts — The catch also handles permission errors, I/O failures, and other lstat errors, but reports all…
What changed in this PR

Adds feature-flagged cleanup of unused CodeQL toolcache bundles before downloads on GitHub-hosted runners.

Changes:

  • Adds guarded toolcache cleanup with telemetry and tests.
  • Tracks whether a job already obtained CodeQL tools.
  • Clarifies hosted-runner detection APIs.
File Review
src/​util.ts Renames and documents the hosted-runner heuristic.
src/​tools-download.ts Implements cleanup; non-ENOENT inspection errors must be reported as failures.
src/​setup-codeql.ts Integrates cleanup; the current condition incorrectly excludes some explicit download URLs.
src/​setup-codeql.test.ts Covers cleanup behavior and safeguards.
src/​feature-flags.ts Defines the cleanup feature flag.
src/​environment.ts Adds job-scoped tool acquisition state.
src/​config-utils.ts Updates renamed heuristic usage.
src/​caching-utils.ts Updates renamed heuristic usage.
src/​actions-util.ts Adds service-reported hosted-runner detection.
pr-checks/​checks/​multi-language-autodetect.yml Enables cleanup in integration checks.
Files excluded by content exclusion policy (2)
  • .github/workflows/__multi-language-autodetect.yml
  • lib/entry-points.js

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/setup-codeql.ts Outdated
// 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 ||
Comment thread src/tools-download.ts Outdated
henrymercer and others added 3 commits September 4, 2026 10:18
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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>
Some runner images keep the toolcache on a different volume to the workspace, so deleting the tools there frees up disk space that the analysis cannot use, and costs a later step that wanted them in the toolcache a download. Windows runners are laid out this way, with the toolcache on `C:` and the workspace on `D:`.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@henrymercer
henrymercer marked this pull request as ready for review September 4, 2026 10:19
@henrymercer
henrymercer requested a review from a team as a code owner September 4, 2026 10:19
@github-actions github-actions Bot added size/XL May be very hard to review and removed size/L May be hard to review labels Sep 4, 2026
@mbg
mbg self-requested a review September 4, 2026 11:20
@mbg mbg self-assigned this Sep 4, 2026

@mbg mbg left a comment

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.

This mostly looks good and makes a lot of sense, thank you! I don't think I noticed anything overly critical, but found a few minor-ish points, some of which we should probably address.

Comment thread src/caching-utils.ts
Comment thread src/tools-download.ts Outdated
.map((entry) => entry.name)
.sort();

await fs.promises.rm(toolDirectory, { force: true, recursive: true });

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.

To be a bit extra safe, do you think it would make sense to perform the symlink check for each of the specific version directories as well before deleting them individually to make sure that the specific version directories aren't symlinks?

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.

This would be odd on a runner that claims to be GitHub-hosted, but can do. In most cases this shouldn't slow things down much as we expect exactly one CodeQL entry in the toolcache.

Comment thread src/tools-download.ts Outdated
return { deletedVersions: versions, failed: false };
} catch (e) {
logger.info(
`Failed to delete the CodeQL tools at ${toolDirectory} from the toolcache: ${getErrorMessage(e)}`,

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.

Minor: Consider putting ${toolDirectory} in quotes to clearly delimit the start and end of the value. Similarly elsewhere in log messages.

Comment thread src/setup-codeql.ts Outdated
Comment on lines +891 to +892
features: FeatureEnablement,
logger: Logger,

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.

Minor: Would be nice if this used ActionState<["Logger", "Features"]>. Although not available at the call site, you can just pass { logger, features } in there.

Comment thread src/setup-codeql.ts Outdated
): Promise<void> {
// 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) {

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.

If following the suggestion for ActionState above, add Env/ReadOnlyEnv to the list of state features. Otherwise add either as an explicit parameter. Then use (action.)env.getOptional instead of the deprecated getOptionalEnvVar. Use getEnv() as argument to tryDeleteToolcacheBundles if needed.

Comment thread src/setup-codeql.ts Outdated
Comment on lines +918 to +922
} catch (e) {
logger.info(
`Unable to reclaim disk space from the toolcache: ${util.getErrorMessage(e)}`,
);
}

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.

I think this will mostly just always result in a redundant log message since most of the body of deleteToolcacheBundles is wrapped in its own try/catch blocks with appropriate log messages. Consider whether:

  • The call to getToolcacheToolDirectory in deleteToolcacheBundles could be wrapped in its own try/catch to avoid this one here; or
  • The entire body of deleteToolcacheBundles could be wrapped in one; or
  • The existing catch blocks in deleteToolcacheBundles should log at debug level instead.

Comment thread src/setup-codeql.ts Outdated
Comment on lines +1056 to +1058
// 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");

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.

I am wondering a bit about the logic / naming of this. The name implies that we have obtained our own bundle (and stored it in the toolcache), but that's not necessarily what would have happened if we reach this point (e.g. because the bundle was already in the toolcache).

Consider whether it would be better to name this environment variable something else that's more representative of the logic here (e.g. HAS_SETUP_CODEQL) so that we (or someone unfamiliar with this) don't get confused/mislead when reviewing logs that contain a HAS_OBTAINED_CODEQL_TOOLS=true or only set this environment variable if we have actually obtained our own bundle.

Comment thread src/setup-codeql.test.ts Outdated
);

test.serial(
"isToolcacheOnWorkspaceFilesystem compares the toolcache against the workspace",

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.

Minor: Change this to be more descriptive of what the test actually does, i.e. checks that we assume the toolcache is not on the same filesystem if we can't tell.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL May be very hard to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants