Delete CLI output-cache file during init - #4097
Conversation
There was a problem hiding this comment.
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.
Pull request overview
Prevents stale CodeQL CLI output caches from persisting across runs on self-hosted runners.
Changes:
- Exports the command-cache path helper.
- Deletes the cache during initialization.
- Updates generated JavaScript.
Show a summary per file
| File | Description |
|---|---|
src/init-action.ts |
Adds cache deletion during initialization. |
src/cli/output-cache.ts |
Exports the cache-path helper. |
lib/entry-points.js |
Generated artifact; excluded from review. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Files excluded by content exclusion policy (1)
- lib/entry-points.js
- Files reviewed: 2/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| // Delete the CLI output-cache file if it exists, to avoid | ||
| // accidentally reusing a stale version from a previous run. | ||
| try { | ||
| fs.unlinkSync(getCommandCacheFilePath(actionState.env)); |
mbg
left a comment
mbg
left a comment
There was a problem hiding this comment.
Copilot raised a valid point, but there is also another big issue with the approach here:
The init Action is not necessarily the first to run. We also have start-proxy (in Default Setup), setup-codeql, and upload-sarif. In theory, any of them might be exposed to a stale cache file. While not all of these may use the CLI commands that we are currently/planning to cache, it would be easy to shoot ourselves in the foot in the future by only deleting the file in init. In general, we don't know which action comes first (while there is some logic to try and inspect the workflow file, that's just a best-effort implementation and doesn't cover all cases).
I think a potentially more straight-forward solution is to take advantage of my recent change to the Job UUID generation. The Job UUID is expected to be the same for a given job and the new logic in getJobUUID ensures that we either generate one if there isn't one already, or use the existing one that we generated for the workflow job.
I think a straight-forward solution to the problem here is therefore to store that UUID in the output cache file and compare it to the current job UUID when loading the output cache from disk. If they don't match, discard the content from the output cache and delete the file.
| // Delete the CLI output-cache file if it exists, to avoid | ||
| // accidentally reusing a stale version from a previous run. | ||
| try { | ||
| fs.unlinkSync(getCommandCacheFilePath(actionState.env)); |
|
Yup, makes sense! I'll go with the UUID then. |
The CLI output-cache file, which was introduced in #4081, is not meant to be used between runs—only steps. On non-ephemeral (i.e. self-hosted) runners, if the temp directories are not cleared between runs, the Action may load a previous ("stale") cache file, which may lead to unexpected results if the CLI has changed.
To prevent that from happening, with this PR, the Action will proactively delete the cache file at the expected location early in the
initstep, if one exists.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
dynamicworkflows (Default Setup, Code Quality, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist