Skip uploading dependency caches if we know they exist#3296
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the dependency caching mechanism by skipping cache uploads when the cache key was previously restored, avoiding unnecessary expensive operations like calculating cache sizes and API calls.
Key changes:
- Tracks restored cache keys during cache restoration and stores them in the config
- Checks if a cache with the same key was restored before attempting to upload, skipping the upload if it was
- Adds comprehensive unit test coverage for
uploadDependencyCacheswhich was previously untested
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/config-utils.ts | Adds dependencyCachingRestoredKeys field to the Config interface to track restored cache keys |
| src/testing-utils.ts | Updates createTestConfig helper to include the new dependencyCachingRestoredKeys field |
| src/init-action.ts | Captures restored cache keys from downloadDependencyCaches and stores them in config |
| src/dependency-caching.ts | Adds DownloadDependencyCachesResult interface; modifies downloadDependencyCaches to return restored keys; optimizes uploadDependencyCaches to skip upload for known cache keys |
| src/dependency-caching.test.ts | Adds comprehensive unit tests for uploadDependencyCaches covering various scenarios (no config, no hash, duplicate, empty, stored, error handling) |
| src/config-utils.test.ts | Refactors tests to use createTestConfig helper instead of manually constructing config objects |
| lib/init-action.js | Auto-generated JavaScript from TypeScript source |
| lib/analyze-action.js | Auto-generated JavaScript from TypeScript source |
755b23c to
b665de6
Compare
b665de6 to
1ed85b4
Compare
This was referenced Nov 24, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR modifies
uploadDependencyCachesto avoid attempting to create and upload a dependency cache if a cache with the same key was previously restored, because that means we know that it already exists. Cache keys are intended to uniquely identify the contents of a cache and, based on that principle, it does not make sense to upload a cache with a key that already exists. Note that this is also not a change in behaviour here: ordinarily, the Cache API would refuse to accept a cache upload if a cache with the same key already exists. This change simply optimisesuploadDependencyCachesso that we don't perform expensive tasks when we can avoid them before the API tells us that the cache already exists.This is accomplished in the following way:
initaction, we now keep track of the cache keys that we restored.This PR also adds general unit test coverage for
uploadDependencyCacheswhich was missing so far.We need to continue to check for
ReserveCacheErrors when uploading caches, because there are two scenarios where we may still try to upload a cache that already exists:Some other notes:
initto match the cache key we compute inuploadDependencyCaches, because our analysis should not modify any project files.uploadDependencyCachesto ensure that it really corresponds to what we are about to put into the cache. This hasn't changed in this PR.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
analysis-kinds: code-scanning).analysis-kinds: code-quality).How did/will you validate this change?
.test.tsfiles).pr-checks).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?
Merge / deployment checklist