feat(cache): add cache-mode client behavior (read-denied warning + ACTIONS_CACHE_MODE skip)#2447
feat(cache): add cache-mode client behavior (read-denied warning + ACTIONS_CACHE_MODE skip)#2447philip-gai wants to merge 15 commits into
Conversation
Mirror the existing cache write-denied handling on the restore path. When the receiver refuses a download URL because the run's token has no readable cache scopes, it returns a twirp PermissionDenied (HTTP 403). The twirp client wraps that 403 in a generic Error, so the stable 'cache read denied:' prefix is embedded in the message rather than at the start. - Add CACHE_READ_DENIED_PREFIX and CacheReadDeniedError - Dispatch on the prefix in the restoreCacheV2 catch block (V2 only), log a policy-specific warning, and report a cache miss so the run continues - Add a test mirroring the write-denied coverage
Re-throw CacheReadDeniedError from an inner try/catch around GetCacheEntryDownloadURL and dispatch on typedError.name in the outer catch, matching how saveCacheV2 handles CacheWriteDeniedError.
Extend the read-denied handling to Cache Service v1 so GHES (which forces v1 via _apis/artifactcache) is covered when read-scope enforcement ships there. - Surface the receiver's error body message from getCacheEntry instead of a generic status-code error, so the cache read denied: prefix reaches callers - Re-throw CacheReadDeniedError from restoreCacheV1 and dispatch on it in the outer catch, mirroring restoreCacheV2 and the write-denied v1 handling - Add a v1 read-denied test
There was a problem hiding this comment.
Pull request overview
This PR improves cache restore resilience by detecting policy-driven cache read denials (insufficient read permissions) and surfacing them as a clear core.warning while treating the restore as a cache miss so workflows continue.
Changes:
- Added read-denial detection via
CACHE_READ_DENIED_PREFIXand a dedicatedCacheReadDeniedErrorfor targeted handling. - Updated both cache restore implementations (v1 REST + v2 Twirp/gRPC) to reclassify read-denied failures into a single warning and continue as a miss.
- Adjusted v1
getCacheEntryto selectively surface the receiver’s denial message and added tests + release/version bumps.
Show a summary per file
| File | Description |
|---|---|
| packages/cache/src/internal/cacheHttpClient.ts | Surfaces receiver error message only for cache read denied: so restore paths can reclassify it. |
| packages/cache/src/cache.ts | Adds read-denied prefix + error type and handles read-denied restores as warning + cache miss for both v1/v2. |
| packages/cache/tests/restoreCache.test.ts | Adds v1 tests asserting warning behavior and cache-miss semantics for read denial. |
| packages/cache/tests/restoreCacheV2.test.ts | Adds v2 test asserting warning behavior and cache-miss semantics for wrapped read-denied errors. |
| packages/cache/tests/cacheHttpClient.test.ts | Adds tests ensuring getCacheEntry only surfaces body for read-denied cases. |
| packages/cache/RELEASES.md | Documents the 6.2.0 behavior change. |
| packages/cache/package.json | Bumps @actions/cache version to 6.2.0. |
| packages/cache/package-lock.json | Updates lockfile version metadata to 6.2.0. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Files not reviewed (1)
- packages/cache/package-lock.json: Generated file
- Files reviewed: 7/8 changed files
- Comments generated: 1
- Review effort level: Low
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| if (!isSuccessStatusCode(response.statusCode)) { | ||
| // Only surface the receiver's body for a `cache read denied:` policy denial | ||
| // so callers can dispatch on it; keep the generic message otherwise. | ||
| const errorMessage = response.error?.message |
There was a problem hiding this comment.
it seems odd to me that only have this "fix up the error" message info in the read path. It seams like the write path benefit from it. And a write read only setup seems far more helpful than a write only one in my eyes.
There was a problem hiding this comment.
We already have a block for CacheWriteDeniedError, this is just adding it for the read path whenever the scope is write-only or none and reads are denied
There was a problem hiding this comment.
I think some of my confusion that we have the error fixing up code in both packages/cache/src/internal/cacheHttpClient.ts and packages/cache/src/cache.ts for the read case, while for write we only have it in packages/cache/src/cache.ts (it also does not seen to call out that likely your scope is blocking you. I don't know if there are other cases in the write path that could yield a 403)
There was a problem hiding this comment.
Oh, the write path's reserveCache method already returns a response of statusCode + error to clients, but read's getCacheEntry only throws a plain string Error. I didn't want clients to have to update to a new response structure, so we just throw with the cache read denied: prefix and cache.ts then inspects it and throws it as a CacheReadDeniedError
And we do call it out, with the message cache read denied: token has no readable scopes
| enableCrossOsArchive | ||
| }) | ||
| } catch (error) { | ||
| // The GHES v1 artifact cache service returns HTTP 403 with a |
There was a problem hiding this comment.
Why is the error fixing up logic here and in getCacheEntry?
There was a problem hiding this comment.
I think I responded to this in #2447 (comment) 😄
| if (errorMessage.includes(CACHE_READ_DENIED_PREFIX)) { | ||
| throw new CacheReadDeniedError(errorMessage) | ||
| } | ||
| throw error |
There was a problem hiding this comment.
Right below we we don't throw on a miss, we return undefined when we can't find it the cache, the equivalent to a 404. Do we want to be throwing on 403?
There was a problem hiding this comment.
Yeah, this will just route as to the outer catch the same way it did before I added a catch here, and we want to be able to throw our CacheReadDeniedError and rethrow other errors we may get
…denied handling Address PR review feedback: - Merge the duplicate restore/save skip test.each blocks into single blocks parametrized over ACTIONS_CACHE_SERVICE_V2. - Drop the redundant CacheReadDeniedError catch arms; the typed error is not an HttpClientError so it already falls through to a non-fatal warning. - Clarify why read-denied classification happens both in getCacheEntry and cache.ts (dependency-free internal module cannot import the typed error).
Mirror the read-denied simplification on the save path. CacheWriteDeniedError is not an HttpClientError and its name does not match the ReserveCacheError arm, so it falls through to the same non-fatal warning. Logging behavior is unchanged (warns, never fails the run) and the exported type is still thrown internally for consumers and tests. Also refresh stale doc wording.
The two restoreCache tests exercised the identical warning + cache-miss path now that read-denied is no longer reclassified in the catch, so merge them into one. The read-denied prefix detection that actually branches on the message is covered by getCacheEntry tests in cacheHttpClient.test.ts.
Description
This PR adds client-side cache-mode behavior to
@actions/cache. It combines two related pieces of work:ACTIONS_CACHE_MODEenvironment variable to skip cache operations the effective mode does not permit.Cache-mode values are
none | read | write | write-only(hyphenated). It is a partial lattice, not linear: readable modes = {read, write}, writable modes = {write, write-only}, none = neither. TheACTIONS_CACHE_MODEenv var is provided by the Actions runner, and is gated on the service side. When it is unset or unrecognized, behavior is identical to today (regression-safe).Read-denied warning
When the cache service denies a download because the token has no readable scopes, the run should continue with a clear warning instead of a confusing failure.
CacheReadDeniedErrorand the sharedcache read denied:prefix constant.core.warning, then returns a cache miss.cache write denied:) for consistency.ACTIONS_CACHE_MODE skip
Skip operations the effective cache-mode does not permit, before any tar or network work.
none,write-only).none,read).undefined); save skip returns the existing not-saved sentinel (-1). Neither throws.core.infoline per skip; extra detail (paths, key) is behindACTIONS_STEP_DEBUG.Rationale for skipping restore on
write-only: write-only tokens have no read scope, so a restore would be denied service-side and trip the read-denied warning. Skipping client-side avoids the wasted round-trip and gives a clearer message.Companion changes
ACTIONS_CACHE_MODEinto the step environment.Notes
packages/cache/RELEASES.mdupdated under the 6.2.0 entry.https://github.com/github/actions-persistence/issues/1168