fix: demote benign cache reserve-race to info instead of warning#75
Merged
Conversation
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.
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(...)withinfo(...)in thecacheId === -1branch ofsaveCache. - Add
src/cache-save.test.tswith three test cases forsaveCache. - Regenerate
dist/index.mjsto 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.
…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
| // cache reserve race in a build matrix) to core.warning instead of core.info. | ||
| minify: { | ||
| compress: true, | ||
| mangle: { keepNames: { function: true, class: true } }, |
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 getcacheId === -1from@actions/cache.That
-1is expected and benign, butcache-save.tswas emittingwarning("Cache save failed or was skipped.")on it — and@actions/cachealready logs the specific reason itself, so this was redundant noise surfaced as a warning annotation.Change
cacheId === -1branch fromwarning(...)toinfo(...).catchblock'swarning(...)for genuinely thrown errors — a real, unexpected failure should still warn.src/cache-save.test.tscovering the reserve-race (no warning), success, and thrown-error paths.Verification
vp test run→ 147 passed (8 files), including the new cache-save testsvp run check→ cleanvp run typecheck→ cleanvp run build→dist/index.mjsrebuilt; old"Cache save failed or was skipped"string goneNote
The companion
Failed to save: Unable to reserve cache…message comes from@actions/cacheitself; v6.0.1 (already onmain) demotes it toinfo. Both warnings disappear for consumers once a newv1tag is cut that includes this change and the v6.0.1 bump (the currentv1predates both).