From 78e3d71e19f152406214491601b48e7ca552acfa Mon Sep 17 00:00:00 2001 From: Jason Ginchereau Date: Thu, 11 Jun 2026 17:01:56 -0700 Subject: [PATCH] Handle cache write error due to read-only token --- packages/cache/__tests__/saveCache.test.ts | 50 +++++++++++++++ packages/cache/__tests__/saveCacheV2.test.ts | 64 +++++++++++++++++++ packages/cache/src/cache.ts | 66 +++++++++++++++++++- 3 files changed, 179 insertions(+), 1 deletion(-) diff --git a/packages/cache/__tests__/saveCache.test.ts b/packages/cache/__tests__/saveCache.test.ts index e0fd7f20..f747da1a 100644 --- a/packages/cache/__tests__/saveCache.test.ts +++ b/packages/cache/__tests__/saveCache.test.ts @@ -221,6 +221,56 @@ test('save with reserve cache failure should fail', async () => { expect(getCompressionMock).toHaveBeenCalledTimes(1) }) +test('save with reserve cache denied by read-only token logs warning (not info)', async () => { + // V1 path: when the legacy ReserveCache REST call returns an error message + // starting with the stable `cache write denied:` prefix, the toolkit must + // surface it as a `core.warning` (not the usual `core.info` used for the + // generic "another job may be creating this cache" contention case). + const paths = ['node_modules'] + const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' + const deniedMessage = + 'cache write denied: read-only token issued for untrusted trigger' + const logInfoMock = jest.spyOn(core, 'info') + const logWarningMock = jest.spyOn(core, 'warning') + + const reserveCacheMock = jest + .spyOn(cacheHttpClient, 'reserveCache') + .mockImplementation(async () => { + const response: ITypedResponseWithError = { + statusCode: 403, + result: null, + headers: {}, + error: new HttpClientError(deniedMessage, 403) + } + return response + }) + + const createTarMock = jest.spyOn(tar, 'createTar') + const saveCacheMock = jest.spyOn(cacheHttpClient, 'saveCache') + const compression = CompressionMethod.Zstd + const getCompressionMock = jest + .spyOn(cacheUtils, 'getCompressionMethod') + .mockReturnValueOnce(Promise.resolve(compression)) + + const cacheId = await saveCache(paths, primaryKey) + expect(cacheId).toBe(-1) + + // The generic "another job may be creating this cache" info log MUST NOT + // fire — this is a policy denial, not a contention case. + expect(logInfoMock).not.toHaveBeenCalledWith( + expect.stringContaining('another job may be creating this cache') + ) + // A single warning carrying the stable prefix is what the customer sees. + expect(logWarningMock).toHaveBeenCalledWith( + `Failed to save: Unable to reserve cache with key ${primaryKey}. More details: ${deniedMessage}` + ) + + expect(reserveCacheMock).toHaveBeenCalledTimes(1) + expect(createTarMock).toHaveBeenCalledTimes(1) + expect(saveCacheMock).toHaveBeenCalledTimes(0) + expect(getCompressionMock).toHaveBeenCalledTimes(1) +}) + test('save with server error should fail', async () => { const filePath = 'node_modules' const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' diff --git a/packages/cache/__tests__/saveCacheV2.test.ts b/packages/cache/__tests__/saveCacheV2.test.ts index 1916e4f8..c292bec3 100644 --- a/packages/cache/__tests__/saveCacheV2.test.ts +++ b/packages/cache/__tests__/saveCacheV2.test.ts @@ -100,6 +100,70 @@ test('create cache entry failure on non-ok response', async () => { expect(saveCacheMock).toHaveBeenCalledTimes(0) }) +test('create cache entry denied by read-only token logs single warning and skips inner warning', async () => { + // When the receiver signals a policy-driven write denial (token was + // downgraded to read-only), the toolkit should: + // 1. Suppress the inner `Cache reservation failed: ...` warning so the + // runner log is not noisy with duplicate messages. + // 2. Emit a single outer `Failed to save: Unable to reserve cache with + // key ${key}. More details: cache write denied: ...` at `warning` + // level (not `info`), so the customer can see why caching stopped. + const paths = ['node_modules'] + const key = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' + const deniedMessage = + 'cache write denied: read-only token issued for untrusted trigger' + const infoLogMock = jest.spyOn(core, 'info') + const warningLogMock = jest.spyOn(core, 'warning') + + const createCacheEntryMock = jest + .spyOn(CacheServiceClientJSON.prototype, 'CreateCacheEntry') + .mockResolvedValue({ok: false, signedUploadUrl: '', message: deniedMessage}) + + const createTarMock = jest.spyOn(tar, 'createTar') + const finalizeCacheEntryMock = jest.spyOn( + CacheServiceClientJSON.prototype, + 'FinalizeCacheEntryUpload' + ) + const compression = CompressionMethod.Zstd + const getCompressionMock = jest + .spyOn(cacheUtils, 'getCompressionMethod') + .mockResolvedValueOnce(compression) + const archiveFileSize = 1024 + jest + .spyOn(cacheUtils, 'getArchiveFileSizeInBytes') + .mockReturnValueOnce(archiveFileSize) + const cacheVersion = cacheUtils.getCacheVersion(paths, compression) + const saveCacheMock = jest.spyOn(cacheHttpClient, 'saveCache') + + const cacheId = await saveCache(paths, key) + expect(cacheId).toBe(-1) + + // No "another job may be creating this cache" info log: this is the + // policy-denial path, not a contention path. + expect(infoLogMock).not.toHaveBeenCalledWith( + `Failed to save: Unable to reserve cache with key ${key}, another job may be creating this cache.` + ) + // No inner "Cache reservation failed:" warning either: the outer arm owns + // the customer-facing message in this scenario. + expect(warningLogMock).not.toHaveBeenCalledWith( + `Cache reservation failed: ${deniedMessage}` + ) + // One outer warning that includes the stable "cache write denied:" prefix + // so the runner UI and post-action consumers can dispatch on it. + expect(warningLogMock).toHaveBeenCalledWith( + `Failed to save: Unable to reserve cache with key ${key}. More details: ${deniedMessage}` + ) + + expect(createCacheEntryMock).toHaveBeenCalledWith({ + key, + version: cacheVersion + }) + expect(createTarMock).toHaveBeenCalledTimes(1) + expect(getCompressionMock).toHaveBeenCalledTimes(1) + expect(finalizeCacheEntryMock).toHaveBeenCalledTimes(0) + expect(saveCacheMock).toHaveBeenCalledTimes(0) +}) + test('create cache entry fails on rejected promise', async () => { const paths = ['node_modules'] const key = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index 219218b4..3f66c6f5 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -31,6 +31,36 @@ export class ReserveCacheError extends Error { } } +/** + * Stable prefix the receiver writes into the cache reservation response when + * the issuer downgraded the cache token to read-only (for example, because + * the run was triggered by an untrusted event). saveCacheV1 / saveCacheV2 + * dispatch on this prefix to re-classify the failure as a + * CacheWriteDeniedError so consumers (and the outer catch arm) can + * distinguish a policy denial from other reservation failures. + */ +export const CACHE_WRITE_DENIED_PREFIX = 'cache write denied:' + +/** + * Raised when the cache backend refuses to reserve a writable cache entry + * because the JWT issued for this run was scoped read-only (for example, the + * run was triggered by an event the repository administrator classified as + * untrusted). The error message is forwarded verbatim from the receiver and + * always begins with `cache write denied:`. + * + * Extends ReserveCacheError for source-compatibility: existing + * `instanceof ReserveCacheError` checks and `typedError.name === + * ReserveCacheError.name` paths keep working, while consumers that want to + * distinguish the policy case can match on this subclass. + */ +export class CacheWriteDeniedError extends ReserveCacheError { + constructor(message: string) { + super(message) + this.name = 'CacheWriteDeniedError' + Object.setPrototypeOf(this, CacheWriteDeniedError.prototype) + } +} + export class FinalizeCacheError extends Error { constructor(message: string) { super(message) @@ -467,6 +497,18 @@ async function saveCacheV1( )} MB (${archiveFileSize} B) is over the data cap limit, not saving cache.` ) } else { + // Inspect the receiver's error message before deciding which error to + // throw. A message starting with the stable `cache write denied:` + // prefix indicates the issuer downgraded the token to read-only + // (policy denial), not a contention case, so we surface it as a + // CacheWriteDeniedError which the outer catch arm logs at warning + // level. + const detailMessage = reserveCacheResponse?.error?.message + if (detailMessage?.startsWith(CACHE_WRITE_DENIED_PREFIX)) { + throw new CacheWriteDeniedError( + `Unable to reserve cache with key ${key}. More details: ${detailMessage}` + ) + } throw new ReserveCacheError( `Unable to reserve cache with key ${key}, another job may be creating this cache. More details: ${reserveCacheResponse?.error?.message}` ) @@ -478,6 +520,11 @@ async function saveCacheV1( const typedError = error as Error if (typedError.name === ValidationError.name) { throw error + } else if (typedError.name === CacheWriteDeniedError.name) { + // Cache write was denied by policy (read-only token). Surface to the + // customer at warning level so it is visible in the workflow log + // without failing the run. + core.warning(`Failed to save: ${typedError.message}`) } else if (typedError.name === ReserveCacheError.name) { core.info(`Failed to save: ${typedError.message}`) } else { @@ -578,7 +625,13 @@ async function saveCacheV2( try { const response = await twirpClient.CreateCacheEntry(request) if (!response.ok) { - if (response.message) { + // Skip the redundant inner warning when the receiver signalled a + // policy denial: the outer catch arm below will log a single + // customer-facing warning. + if ( + response.message && + !response.message.startsWith(CACHE_WRITE_DENIED_PREFIX) + ) { core.warning(`Cache reservation failed: ${response.message}`) } throw new Error(response.message || 'Response was not ok') @@ -586,6 +639,12 @@ async function saveCacheV2( signedUploadUrl = response.signedUploadUrl } catch (error) { core.debug(`Failed to reserve cache: ${error}`) + const errorMessage = (error as Error)?.message ?? '' + if (errorMessage.startsWith(CACHE_WRITE_DENIED_PREFIX)) { + throw new CacheWriteDeniedError( + `Unable to reserve cache with key ${key}. More details: ${errorMessage}` + ) + } throw new ReserveCacheError( `Unable to reserve cache with key ${key}, another job may be creating this cache.` ) @@ -623,6 +682,11 @@ async function saveCacheV2( const typedError = error as Error if (typedError.name === ValidationError.name) { throw error + } else if (typedError.name === CacheWriteDeniedError.name) { + // Cache write was denied by policy (read-only token). Surface to the + // customer at warning level so it is visible in the workflow log + // without failing the run. + core.warning(`Failed to save: ${typedError.message}`) } else if (typedError.name === ReserveCacheError.name) { core.info(`Failed to save: ${typedError.message}`) } else if (typedError.name === FinalizeCacheError.name) {