fix: demote benign cache reserve-race to info instead of warning - #75
Merged
fengmk2 merged 2 commits intoMay 29, 2026
Merged
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
In a build matrix, jobs that share a cache key (same OS/arch/lockfile
across different Node versions) race to save it in the post step. Only
one job can reserve a given key; the losers get cacheId === -1 from
@actions/cache, which is expected and benign. setup-vp was emitting
`warning("Cache save failed or was skipped.")` on that path, producing
noisy warning annotations even though caching worked correctly.
Demote that branch to an info message. A genuinely thrown error from
saveCache still warns, since that is a real, unexpected failure.
Adds src/cache-save.test.ts covering the reserve-race (no warning),
success, and thrown-error paths.
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Pull request overview
Demotes the benign cacheId === -1 outcome from saveCacheAction (which happens when concurrent matrix jobs race to reserve the same cache key) from a warning annotation to an info log, removing noisy CI annotations. Adds unit tests for saveCache covering the reserve-race, success, and thrown-error paths. Rebuilds dist/index.mjs to match.
Changes:
- Replace warning(...) with info(...) in the cacheId === -1 branch of saveCache.
- Add src/cache-save.test.ts with three test cases for saveCache.
- Regenerate dist/index.mjs to include the updated message.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/cache-save.ts | Demotes benign reserve-race log from warning to info with clearer message. |
| src/cache-save.test.ts | New tests mocking @actions/cache and @actions/core to verify no-warn/success/throw paths. |
| dist/index.mjs | Rebuilt bundle reflecting the source change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
…ce at info
Even after demoting setup-vp's own message, CI still showed a warning:
"Failed to save: Unable to reserve cache with key …, another job may be
creating this cache." That comes from @actions/cache itself.
Root cause is in our bundle, not the library. @actions/cache picks the log
level with `error.name === ReserveCacheError.name`. Plain `minify: true`
mangles the class binding to a short identifier, so `ReserveCacheError.name`
evaluates to the mangled name (e.g. "e") while the instance keeps its
`this.name = "ReserveCacheError"` literal. The comparison is always false,
so the benign reserve-race ReserveCacheError falls through to the default
branch and is logged via core.warning instead of core.info.
Configure pack.minify with mangle.keepNames (function + class) so oxc emits
named class expressions (`X = class ReserveCacheError extends Error {…}`),
preserving `X.name`. The comparison now matches and the reserve race is
logged at info — no warning annotation.
Adds src/bundle.test.ts to guard the built dist against a regression to
plain `minify: true` (asserts ReserveCacheError / ValidationError /
FinalizeCacheError survive as named classes).
fengmk2
commented
May 29, 2026
Comment thread
vite.config.ts
| // cache reserve race in a build matrix) to core.warning instead of core.info. | ||
| minify: { | ||
| compress: true, | ||
| mangle: { keepNames: { function: true, class: true } }, |
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment.
TIL
Sorry, something went wrong.
fengmk2
added a commit
to node-modules/urllib
that referenced
this pull request
May 29, 2026
setup-vp 6cb180a additionally fixes the @actions/cache "Failed to save: Unable to reserve cache…" warning, which was caused by class-name mangling in the action bundle (error.name === ReserveCacheError.name never matched). Temporary SHA pin — revert to @v1 once voidzero-dev/setup-vp#75 is merged and the v1 tag is re-cut.
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.
Problem
Consumers running a build matrix see noisy warning annotations from setup-vp's cache save step, e.g. in node-modules/urllib:
These are harmless — caching works correctly.
Root cause
The cache key is vite-plus-{OS}-{arch}-{lockfile-type}-{hash} and intentionally does not include the Node.js version (the package-manager store is Node-version-independent, so one shared cache per OS/arch/lockfile is correct and desirable). When a matrix runs several Node versions per OS, those jobs compute the identical key and race to save it in the post step. GitHub's cache backend lets only one job reserve a given key; the losers get cacheId === -1 from @actions/cache.
That -1 is expected and benign, but cache-save.ts was emitting warning("Cache save failed or was skipped.") on it — and @actions/cache already logs the specific reason itself, so this was redundant noise surfaced as a warning annotation.
Change
Verification
Note
The companion Failed to save: Unable to reserve cache… message comes from @actions/cache itself; v6.0.1 (already on main) demotes it to info. Both warnings disappear for consumers once a new v1 tag is cut that includes this change and the v6.0.1 bump (the current v1 predates both).