mirror of
https://github.com/actions/toolkit.git
synced 2026-08-07 00:00:18 +02:00
feat(cache): add cache-mode client behavior (read-denied warning + ACTIONS_CACHE_MODE skip) (#2447)
* feat(cache): surface cache read-denied as a distinct restore warning 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 * chore(cache): trim comments, bump to 6.2.0, add RELEASES entry * refactor(cache): dispatch read-denied by error name to mirror write path Re-throw CacheReadDeniedError from an inner try/catch around GetCacheEntryDownloadURL and dispatch on typedError.name in the outer catch, matching how saveCacheV2 handles CacheWriteDeniedError. * feat(cache): handle read-denied on the v1 restore path 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 * refactor(cache): only surface receiver body for read-denied on v1 * test(cache): assert getCacheEntry only surfaces body for read-denied * test(cache): cover non-read-denied getCacheEntry passthrough on v1 * refactor(cache): share read-denied prefix via constants to avoid drift * feat(cache): skip restore/save per ACTIONS_CACHE_MODE * test(cache): expand ACTIONS_CACHE_MODE skip coverage across v1/v2 and unknown modes * fix copilot pr feedback Co-authored-by: Copilot Autofix powered by AI <[email protected]> * docs(cache): remove internal reference from cache-mode comment * test(cache): merge redundant cache-mode skip tests and simplify read-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). * refactor(cache): drop redundant CacheWriteDeniedError catch arms 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. * test(cache): collapse redundant restore getCacheEntry-failure tests 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. --------- Co-authored-by: Copilot Autofix powered by AI <[email protected]>
This commit is contained in:
co-authored by
Copilot Autofix powered by AI
parent
0786132e6a
commit
ffdc20ef92
+34
-1
@@ -1,8 +1,10 @@
|
||||
import {downloadCache} from '../src/internal/cacheHttpClient'
|
||||
import {downloadCache, getCacheEntry} from '../src/internal/cacheHttpClient'
|
||||
import {getCacheVersion} from '../src/internal/cacheUtils'
|
||||
import {CompressionMethod} from '../src/internal/constants'
|
||||
import * as downloadUtils from '../src/internal/downloadUtils'
|
||||
import * as requestUtils from '../src/internal/requestUtils'
|
||||
import {DownloadOptions, getDownloadOptions} from '../src/options'
|
||||
import {HttpClientError} from '@actions/http-client'
|
||||
|
||||
jest.mock('../src/internal/downloadUtils')
|
||||
|
||||
@@ -57,6 +59,37 @@ test('getCacheVersion with enableCrossOsArchive as false returns version on wind
|
||||
}
|
||||
})
|
||||
|
||||
test('getCacheEntry throws a generic status-code error for non-read-denied failures', async () => {
|
||||
// Regression: a non read-denied failure must NOT leak the server's body
|
||||
// message; it should surface the generic status-code error.
|
||||
jest.spyOn(requestUtils, 'retryTypedResponse').mockResolvedValue({
|
||||
statusCode: 403,
|
||||
result: null,
|
||||
headers: {},
|
||||
error: new HttpClientError('some other server detail', 403)
|
||||
})
|
||||
|
||||
await expect(getCacheEntry(['key'], ['node_modules'])).rejects.toThrow(
|
||||
'Cache service responded with 403'
|
||||
)
|
||||
})
|
||||
|
||||
test('getCacheEntry surfaces the body message for a cache read denial', async () => {
|
||||
jest.spyOn(requestUtils, 'retryTypedResponse').mockResolvedValue({
|
||||
statusCode: 403,
|
||||
result: null,
|
||||
headers: {},
|
||||
error: new HttpClientError(
|
||||
'cache read denied: token has no readable scopes',
|
||||
403
|
||||
)
|
||||
})
|
||||
|
||||
await expect(getCacheEntry(['key'], ['node_modules'])).rejects.toThrow(
|
||||
'cache read denied: token has no readable scopes'
|
||||
)
|
||||
})
|
||||
|
||||
test('downloadCache uses http-client for non-Azure URLs', async () => {
|
||||
const downloadCacheHttpClientMock = jest.spyOn(
|
||||
downloadUtils,
|
||||
|
||||
+34
@@ -23,3 +23,37 @@ test('isGhes returns false for ghe.localhost', () => {
|
||||
process.env.GITHUB_SERVER_URL = 'https://my.domain.ghe.localhost'
|
||||
expect(config.isGhes()).toBe(false)
|
||||
})
|
||||
|
||||
describe('cache-mode helpers', () => {
|
||||
const original = process.env.ACTIONS_CACHE_MODE
|
||||
|
||||
afterEach(() => {
|
||||
if (original === undefined) {
|
||||
delete process.env.ACTIONS_CACHE_MODE
|
||||
} else {
|
||||
process.env.ACTIONS_CACHE_MODE = original
|
||||
}
|
||||
})
|
||||
|
||||
test('getCacheMode normalizes whitespace and case', () => {
|
||||
process.env.ACTIONS_CACHE_MODE = ' Write-Only '
|
||||
expect(config.getCacheMode()).toBe('write-only')
|
||||
})
|
||||
|
||||
test('getCacheMode returns empty string when unset', () => {
|
||||
delete process.env.ACTIONS_CACHE_MODE
|
||||
expect(config.getCacheMode()).toBe('')
|
||||
})
|
||||
|
||||
test.each([
|
||||
['', true, true],
|
||||
['read', true, false],
|
||||
['write', true, true],
|
||||
['write-only', false, true],
|
||||
['none', false, false],
|
||||
['garbage', true, true]
|
||||
])("mode '%s' -> readable=%s writable=%s", (mode, readable, writable) => {
|
||||
expect(config.isCacheReadable(mode)).toBe(readable)
|
||||
expect(config.isCacheWritable(mode)).toBe(writable)
|
||||
})
|
||||
})
|
||||
|
||||
+87
@@ -99,6 +99,93 @@ test('restore with server error should fail', async () => {
|
||||
delete process.env['ACTIONS_RESULTS_URL']
|
||||
})
|
||||
|
||||
test('restore surfaces a getCacheEntry failure as a warning and reports a cache miss', async () => {
|
||||
// restoreCache treats any getCacheEntry failure (read-denied or otherwise)
|
||||
// as a non-fatal warning and a cache miss so the workflow continues. The
|
||||
// read-denied prefix detection itself is covered in cacheHttpClient.test.ts.
|
||||
const paths = ['node_modules']
|
||||
const key = 'node-test'
|
||||
const logErrorMock = jest.spyOn(core, 'error')
|
||||
const logWarningMock = jest.spyOn(core, 'warning')
|
||||
const message = 'cache read denied: token has no readable scopes'
|
||||
|
||||
jest.spyOn(cacheHttpClient, 'getCacheEntry').mockImplementation(async () => {
|
||||
throw new Error(message)
|
||||
})
|
||||
|
||||
const cacheKey = await restoreCache(paths, key)
|
||||
expect(cacheKey).toBe(undefined)
|
||||
expect(logErrorMock).not.toHaveBeenCalled()
|
||||
expect(logWarningMock).toHaveBeenCalledWith(`Failed to restore: ${message}`)
|
||||
expect(logWarningMock).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
describe('restore cache-mode gating', () => {
|
||||
const originalMode = process.env.ACTIONS_CACHE_MODE
|
||||
const originalV2 = process.env.ACTIONS_CACHE_SERVICE_V2
|
||||
|
||||
const restoreEnv = (key: string, value: string | undefined): void => {
|
||||
if (value === undefined) {
|
||||
delete process.env[key]
|
||||
} else {
|
||||
process.env[key] = value
|
||||
}
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
restoreEnv('ACTIONS_CACHE_MODE', originalMode)
|
||||
restoreEnv('ACTIONS_CACHE_SERVICE_V2', originalV2)
|
||||
})
|
||||
|
||||
// The skip short-circuits before v1/v2 dispatch, so it applies regardless of
|
||||
// the ACTIONS_CACHE_SERVICE_V2 feature flag.
|
||||
test.each([
|
||||
['none', undefined],
|
||||
['none', 'true'],
|
||||
['write-only', undefined],
|
||||
['write-only', 'true']
|
||||
])(
|
||||
"mode '%s' skips restore with ACTIONS_CACHE_SERVICE_V2=%s",
|
||||
async (mode, v2) => {
|
||||
process.env.ACTIONS_CACHE_MODE = mode
|
||||
restoreEnv('ACTIONS_CACHE_SERVICE_V2', v2)
|
||||
const logInfoMock = jest.spyOn(core, 'info')
|
||||
const getCacheEntryMock = jest.spyOn(cacheHttpClient, 'getCacheEntry')
|
||||
|
||||
const cacheKey = await restoreCache(['node_modules'], 'node-test')
|
||||
|
||||
expect(cacheKey).toBe(undefined)
|
||||
expect(getCacheEntryMock).not.toHaveBeenCalled()
|
||||
expect(logInfoMock).toHaveBeenCalledTimes(1)
|
||||
expect(logInfoMock).toHaveBeenCalledWith(
|
||||
`Cache restore skipped: the effective cache-mode '${mode}' does not permit reads.`
|
||||
)
|
||||
}
|
||||
)
|
||||
|
||||
test.each(['read', 'write', '', 'garbage'])(
|
||||
"mode '%s' does not skip restore",
|
||||
async mode => {
|
||||
if (mode === '') {
|
||||
delete process.env.ACTIONS_CACHE_MODE
|
||||
} else {
|
||||
process.env.ACTIONS_CACHE_MODE = mode
|
||||
}
|
||||
const logInfoMock = jest.spyOn(core, 'info')
|
||||
const getCacheEntryMock = jest
|
||||
.spyOn(cacheHttpClient, 'getCacheEntry')
|
||||
.mockResolvedValue(null as never)
|
||||
|
||||
await restoreCache(['node_modules'], 'node-test')
|
||||
|
||||
expect(getCacheEntryMock).toHaveBeenCalledTimes(1)
|
||||
expect(logInfoMock).not.toHaveBeenCalledWith(
|
||||
expect.stringContaining('Cache restore skipped')
|
||||
)
|
||||
}
|
||||
)
|
||||
})
|
||||
|
||||
test('restore with restore keys and no cache found', async () => {
|
||||
const paths = ['node_modules']
|
||||
const key = 'node-test'
|
||||
|
||||
+40
@@ -33,6 +33,19 @@ beforeAll(() => {
|
||||
// Ensure that we're using v2 for these tests
|
||||
jest.spyOn(config, 'getCacheServiceVersion').mockReturnValue('v2')
|
||||
|
||||
// config is auto-mocked; use the real cache-mode helpers so gating reflects
|
||||
// ACTIONS_CACHE_MODE and unset stays permissive.
|
||||
const actualConfig = jest.requireActual('../src/internal/config')
|
||||
jest
|
||||
.spyOn(config, 'getCacheMode')
|
||||
.mockImplementation(actualConfig.getCacheMode)
|
||||
jest
|
||||
.spyOn(config, 'isCacheReadable')
|
||||
.mockImplementation(actualConfig.isCacheReadable)
|
||||
jest
|
||||
.spyOn(config, 'isCacheWritable')
|
||||
.mockImplementation(actualConfig.isCacheWritable)
|
||||
|
||||
logDebugMock = jest.spyOn(core, 'debug')
|
||||
logInfoMock = jest.spyOn(core, 'info')
|
||||
})
|
||||
@@ -112,6 +125,33 @@ test('restore with server error should fail', async () => {
|
||||
)
|
||||
})
|
||||
|
||||
test('restore denied by read-only token logs warning and reports cache miss', async () => {
|
||||
// The receiver returns twirp PermissionDenied (403) when the run's token has
|
||||
// no readable cache scopes; the client wraps it so the `cache read denied:`
|
||||
// prefix arrives embedded. Expect a single warning (not error) and a miss.
|
||||
const paths = ['node_modules']
|
||||
const key = 'node-test'
|
||||
const logErrorMock = jest.spyOn(core, 'error')
|
||||
const logWarningMock = jest.spyOn(core, 'warning')
|
||||
const wrappedDeniedMessage =
|
||||
'Failed to GetCacheEntryDownloadURL: Received non-retryable error: ' +
|
||||
'Failed request: (403) Forbidden: cache read denied: token has no readable scopes'
|
||||
|
||||
jest
|
||||
.spyOn(CacheServiceClientJSON.prototype, 'GetCacheEntryDownloadURL')
|
||||
.mockImplementation(() => {
|
||||
throw new Error(wrappedDeniedMessage)
|
||||
})
|
||||
|
||||
const cacheKey = await restoreCache(paths, key)
|
||||
expect(cacheKey).toBe(undefined)
|
||||
expect(logErrorMock).not.toHaveBeenCalled()
|
||||
expect(logWarningMock).toHaveBeenCalledWith(
|
||||
`Failed to restore: ${wrappedDeniedMessage}`
|
||||
)
|
||||
expect(logWarningMock).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
test('restore with restore keys and no cache found', async () => {
|
||||
const paths = ['node_modules']
|
||||
const key = 'node-test'
|
||||
|
||||
+81
@@ -35,6 +35,18 @@ beforeAll(() => {
|
||||
jest.spyOn(cacheUtils, 'createTempDirectory').mockImplementation(async () => {
|
||||
return Promise.resolve('/foo/bar')
|
||||
})
|
||||
// config is auto-mocked; use the real cache-mode helpers so gating reflects
|
||||
// ACTIONS_CACHE_MODE and unset stays permissive.
|
||||
const actualConfig = jest.requireActual('../src/internal/config')
|
||||
jest
|
||||
.spyOn(config, 'getCacheMode')
|
||||
.mockImplementation(actualConfig.getCacheMode)
|
||||
jest
|
||||
.spyOn(config, 'isCacheReadable')
|
||||
.mockImplementation(actualConfig.isCacheReadable)
|
||||
jest
|
||||
.spyOn(config, 'isCacheWritable')
|
||||
.mockImplementation(actualConfig.isCacheWritable)
|
||||
})
|
||||
|
||||
test('save with missing input should fail', async () => {
|
||||
@@ -45,6 +57,75 @@ test('save with missing input should fail', async () => {
|
||||
)
|
||||
})
|
||||
|
||||
describe('save cache-mode gating', () => {
|
||||
const originalMode = process.env.ACTIONS_CACHE_MODE
|
||||
const originalV2 = process.env.ACTIONS_CACHE_SERVICE_V2
|
||||
|
||||
const restoreEnv = (key: string, value: string | undefined): void => {
|
||||
if (value === undefined) {
|
||||
delete process.env[key]
|
||||
} else {
|
||||
process.env[key] = value
|
||||
}
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
restoreEnv('ACTIONS_CACHE_MODE', originalMode)
|
||||
restoreEnv('ACTIONS_CACHE_SERVICE_V2', originalV2)
|
||||
})
|
||||
|
||||
// The skip short-circuits before v1/v2 dispatch, so it applies regardless of
|
||||
// the ACTIONS_CACHE_SERVICE_V2 feature flag.
|
||||
test.each([
|
||||
['read', undefined],
|
||||
['read', 'true'],
|
||||
['none', undefined],
|
||||
['none', 'true']
|
||||
])(
|
||||
"mode '%s' skips save with ACTIONS_CACHE_SERVICE_V2=%s",
|
||||
async (mode, v2) => {
|
||||
process.env.ACTIONS_CACHE_MODE = mode
|
||||
restoreEnv('ACTIONS_CACHE_SERVICE_V2', v2)
|
||||
const logInfoMock = jest.spyOn(core, 'info')
|
||||
const resolvePathsMock = jest.spyOn(cacheUtils, 'resolvePaths')
|
||||
|
||||
const cacheId = await saveCache(['node_modules'], 'node-test')
|
||||
|
||||
expect(cacheId).toBe(-1)
|
||||
expect(resolvePathsMock).not.toHaveBeenCalled()
|
||||
expect(logInfoMock).toHaveBeenCalledTimes(1)
|
||||
expect(logInfoMock).toHaveBeenCalledWith(
|
||||
`Cache save skipped: the effective cache-mode '${mode}' does not permit writes.`
|
||||
)
|
||||
}
|
||||
)
|
||||
|
||||
test.each(['write', 'write-only', '', 'garbage'])(
|
||||
"mode '%s' does not skip save",
|
||||
async mode => {
|
||||
if (mode === '') {
|
||||
delete process.env.ACTIONS_CACHE_MODE
|
||||
} else {
|
||||
process.env.ACTIONS_CACHE_MODE = mode
|
||||
}
|
||||
const logInfoMock = jest.spyOn(core, 'info')
|
||||
const resolvePathsMock = jest.spyOn(cacheUtils, 'resolvePaths')
|
||||
|
||||
try {
|
||||
await saveCache(['node_modules'], 'node-test')
|
||||
} catch {
|
||||
// Downstream client is not fully mocked here; we only assert the guard
|
||||
// let execution proceed past it.
|
||||
}
|
||||
|
||||
expect(resolvePathsMock).toHaveBeenCalled()
|
||||
expect(logInfoMock).not.toHaveBeenCalledWith(
|
||||
expect.stringContaining('Cache save skipped')
|
||||
)
|
||||
}
|
||||
)
|
||||
})
|
||||
|
||||
test('save with large cache outputs should fail', async () => {
|
||||
const filePath = 'node_modules'
|
||||
const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43'
|
||||
|
||||
Reference in New Issue
Block a user