From 1e648c999134e8fdf6065ca83ed803aff0cc3b54 Mon Sep 17 00:00:00 2001 From: MK Date: Sat, 30 May 2026 14:46:28 +0800 Subject: [PATCH 1/2] fix(cache): make error classification resilient to minification The catch blocks in cache.ts classified errors with `typedError.name === SomeError.name`. Each error class sets `this.name` to a string literal in its constructor, but the comparison reads the class binding's `.name`. A name-mangling minifier (esbuild/terser/ rolldown without keep-names) renames the class binding, so `SomeError.name` no longer equals the preserved literal. This misroutes a benign ReserveCacheError (a cache reserve race when a build matrix shares one cache key) from core.info to core.warning, surfacing noisy warning annotations. Replace the fragile `=== SomeError.name` comparisons with an `isErrorOfType(error, Ctor, 'Name')` helper that checks `instanceof` (which survives minification because each error sets its prototype via Object.setPrototypeOf) OR the name string literal (which covers errors thrown from a duplicate copy/realm of @actions/cache where instanceof would fail). Add a regression test that simulates a minifier by mangling ReserveCacheError.name and asserts the reserve race is still logged via core.info, never core.warning. --- packages/cache/__tests__/saveCache.test.ts | 55 +++++++++++++++++++++- packages/cache/src/cache.ts | 40 +++++++++++++--- 2 files changed, 87 insertions(+), 8 deletions(-) diff --git a/packages/cache/__tests__/saveCache.test.ts b/packages/cache/__tests__/saveCache.test.ts index e0fd7f201b..3940510c2f 100644 --- a/packages/cache/__tests__/saveCache.test.ts +++ b/packages/cache/__tests__/saveCache.test.ts @@ -1,6 +1,6 @@ import * as core from '@actions/core' import * as path from 'path' -import {saveCache} from '../src/cache' +import {saveCache, ReserveCacheError} from '../src/cache' import * as cacheHttpClient from '../src/internal/cacheHttpClient' import * as cacheUtils from '../src/internal/cacheUtils' import * as config from '../src/internal/config' @@ -221,6 +221,59 @@ test('save with reserve cache failure should fail', async () => { expect(getCompressionMock).toHaveBeenCalledTimes(1) }) +// Regression test for a minification-fragility bug: the catch block classified +// errors with `typedError.name === ReserveCacheError.name`. A name-mangling +// minifier (esbuild/terser/rolldown without keep-names) rewrites the class +// binding, so `ReserveCacheError.name` becomes a short mangled string while +// instances still carry the literal `this.name = 'ReserveCacheError'` from the +// constructor. The comparison then never matches, routing a benign reserve race +// (common in a build matrix sharing one cache key) to core.warning instead of +// core.info. We simulate the minifier by mangling the class's runtime name and +// assert the classification still holds. +test('save with reserve cache failure is logged as info even when the class name is mangled (minification-safe)', async () => { + const paths = ['node_modules'] + const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' + const logInfoMock = jest.spyOn(core, 'info') + const logWarningMock = jest.spyOn(core, 'warning') + + // Simulate what a name-mangling minifier does to the class identifier. + const originalNameDescriptor = Object.getOwnPropertyDescriptor( + ReserveCacheError, + 'name' + ) + Object.defineProperty(ReserveCacheError, 'name', { + value: 'r', + configurable: true + }) + + try { + jest.spyOn(cacheHttpClient, 'reserveCache').mockImplementation(async () => { + const response: TypedResponse = { + statusCode: 500, + result: null, + headers: {} + } + return response + }) + jest + .spyOn(cacheUtils, 'getCompressionMethod') + .mockReturnValueOnce(Promise.resolve(CompressionMethod.Zstd)) + + const cacheId = await saveCache(paths, primaryKey) + + expect(cacheId).toBe(-1) + // A benign reserve race must be info, never a warning annotation. + expect(logInfoMock).toHaveBeenCalledWith( + `Failed to save: Unable to reserve cache with key ${primaryKey}, another job may be creating this cache. More details: undefined` + ) + expect(logWarningMock).not.toHaveBeenCalled() + } finally { + if (originalNameDescriptor) { + Object.defineProperty(ReserveCacheError, 'name', originalNameDescriptor) + } + } +}) + test('save with server error should fail', async () => { const filePath = 'node_modules' const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index 219218b414..e762e7d59e 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -39,6 +39,26 @@ export class FinalizeCacheError extends Error { } } +/** + * Resilient check for one of the error types above that survives both JS + * minification and duplicate copies of this module in a dependency tree. + * + * `instanceof` covers the common single-bundle case (and keeps working under + * minification because each error sets its prototype explicitly), while the + * `name` comparison covers errors thrown from a different copy/realm of + * @actions/cache where `instanceof` would fail. The `name` argument must be a + * string literal: comparing against `ctor.name` breaks under name-mangling + * minifiers (the class binding gets renamed while the constructor's + * `this.name = '...'` literal does not), which is the bug this guards against. + */ +function isErrorOfType( + error: Error, + ctor: new (...args: never[]) => T, + name: string +): boolean { + return error instanceof ctor || error.name === name +} + function checkPaths(paths: string[]): void { if (!paths || paths.length === 0) { throw new ValidationError( @@ -204,7 +224,7 @@ async function restoreCacheV1( return cacheEntry.cacheKey } catch (error) { const typedError = error as Error - if (typedError.name === ValidationError.name) { + if (isErrorOfType(typedError, ValidationError, 'ValidationError')) { throw error } else { // warn on cache restore failure and continue build @@ -336,7 +356,7 @@ async function restoreCacheV2( return response.matchedKey } catch (error) { const typedError = error as Error - if (typedError.name === ValidationError.name) { + if (isErrorOfType(typedError, ValidationError, 'ValidationError')) { throw error } else { // Supress all non-validation cache related errors because caching should be optional @@ -476,9 +496,11 @@ async function saveCacheV1( await cacheHttpClient.saveCache(cacheId, archivePath, '', options) } catch (error) { const typedError = error as Error - if (typedError.name === ValidationError.name) { + if (isErrorOfType(typedError, ValidationError, 'ValidationError')) { throw error - } else if (typedError.name === ReserveCacheError.name) { + } else if ( + isErrorOfType(typedError, ReserveCacheError, 'ReserveCacheError') + ) { core.info(`Failed to save: ${typedError.message}`) } else { // Log server errors (5xx) as errors, all other errors as warnings @@ -621,11 +643,15 @@ async function saveCacheV2( cacheId = parseInt(finalizeResponse.entryId) } catch (error) { const typedError = error as Error - if (typedError.name === ValidationError.name) { + if (isErrorOfType(typedError, ValidationError, 'ValidationError')) { throw error - } else if (typedError.name === ReserveCacheError.name) { + } else if ( + isErrorOfType(typedError, ReserveCacheError, 'ReserveCacheError') + ) { core.info(`Failed to save: ${typedError.message}`) - } else if (typedError.name === FinalizeCacheError.name) { + } else if ( + isErrorOfType(typedError, FinalizeCacheError, 'FinalizeCacheError') + ) { core.warning(typedError.message) } else { // Log server errors (5xx) as errors, all other errors as warnings From dfbae6fcb0e57782b6dda709ea3064f5bf4c7376 Mon Sep 17 00:00:00 2001 From: MK Date: Sat, 30 May 2026 15:17:18 +0800 Subject: [PATCH 2/2] test(cache): cover FinalizeCacheError minify path; document isErrorOfType Follow-up to the minification-resilient error classification: - Add a saveCacheV2 regression test that mangles FinalizeCacheError.name and asserts the FinalizeCacheError branch still fires (logging the bare message) instead of falling through to the generic else branch ("Failed to save: ..."). Previously only the v1 ReserveCacheError path had minify coverage, so a regressed/typo'd literal at the v2 FinalizeCacheError call site would have gone uncaught. - Document the two load-bearing signature choices on isErrorOfType (the boolean return instead of `error is T`, and the generic ``); both avoid narrowing Error to never and breaking the build. --- packages/cache/__tests__/saveCacheV2.test.ts | 76 +++++++++++++++++++- packages/cache/src/cache.ts | 9 +++ 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/packages/cache/__tests__/saveCacheV2.test.ts b/packages/cache/__tests__/saveCacheV2.test.ts index 1916e4f81c..0a2ca02631 100644 --- a/packages/cache/__tests__/saveCacheV2.test.ts +++ b/packages/cache/__tests__/saveCacheV2.test.ts @@ -1,6 +1,6 @@ import * as core from '@actions/core' import * as path from 'path' -import {saveCache} from '../src/cache' +import {saveCache, FinalizeCacheError} from '../src/cache' import * as cacheUtils from '../src/internal/cacheUtils' import {CacheFilename, CompressionMethod} from '../src/internal/constants' import * as config from '../src/internal/config' @@ -509,6 +509,80 @@ test('save with finalize cache entry failure and specific error message', async expect(logWarningMock).toHaveBeenCalledWith(errorMessage) }) +// Regression test for the minification-fragility bug on the v2 path. saveCacheV2 +// classifies a FinalizeCacheError with +// `isErrorOfType(typedError, FinalizeCacheError, 'FinalizeCacheError')`. A +// name-mangling minifier (esbuild/terser/rolldown without keep-names) rewrites +// the FinalizeCacheError class binding, so the pre-fix +// `typedError.name === FinalizeCacheError.name` check would miss it and fall +// through to the generic else branch — logging +// `core.warning('Failed to save: ')` instead of the FinalizeCacheError +// branch's bare `core.warning('')`. We simulate the minifier by mangling +// the class's runtime name and assert the FinalizeCacheError branch still fires. +test('finalize cache failure is classified correctly even when the class name is mangled (minification-safe, v2)', async () => { + const paths = 'node_modules' + const key = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' + const logWarningMock = jest.spyOn(core, 'warning') + const signedUploadURL = 'https://blob-storage.local?signed=true' + const archiveFileSize = 1024 + const errorMessage = + 'Cache entry finalization failed due to concurrent access' + const options: UploadOptions = { + archiveSizeBytes: archiveFileSize, + useAzureSdk: true, + uploadChunkSize: 64 * 1024 * 1024, + uploadConcurrency: 8 + } + + // Simulate what a name-mangling minifier does to the class identifier. + const originalNameDescriptor = Object.getOwnPropertyDescriptor( + FinalizeCacheError, + 'name' + ) + Object.defineProperty(FinalizeCacheError, 'name', { + value: 'f', + configurable: true + }) + + try { + jest + .spyOn(CacheServiceClientJSON.prototype, 'CreateCacheEntry') + .mockReturnValue( + Promise.resolve({ + ok: true, + signedUploadUrl: signedUploadURL, + message: '' + }) + ) + jest.spyOn(cacheHttpClient, 'saveCache').mockResolvedValue() + jest + .spyOn(cacheUtils, 'getCompressionMethod') + .mockReturnValueOnce(Promise.resolve(CompressionMethod.Zstd)) + jest + .spyOn(cacheUtils, 'getArchiveFileSizeInBytes') + .mockReturnValueOnce(archiveFileSize) + jest + .spyOn(CacheServiceClientJSON.prototype, 'FinalizeCacheEntryUpload') + .mockReturnValue( + Promise.resolve({ok: false, entryId: '', message: errorMessage}) + ) + + const cacheId = await saveCache([paths], key, options) + + expect(cacheId).toBe(-1) + // The FinalizeCacheError branch logs the bare message; the generic else + // branch prefixes it with "Failed to save:". Assert the former fired. + expect(logWarningMock).toHaveBeenCalledWith(errorMessage) + expect(logWarningMock).not.toHaveBeenCalledWith( + `Failed to save: ${errorMessage}` + ) + } finally { + if (originalNameDescriptor) { + Object.defineProperty(FinalizeCacheError, 'name', originalNameDescriptor) + } + } +}) + test('save with multiple large caches should succeed in v2 (testing 50GB)', async () => { const paths = ['large-dataset', 'node_modules', 'build-artifacts'] const key = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index e762e7d59e..5753520c12 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -50,6 +50,15 @@ export class FinalizeCacheError extends Error { * string literal: comparing against `ctor.name` breaks under name-mangling * minifiers (the class binding gets renamed while the constructor's * `this.name = '...'` literal does not), which is the bug this guards against. + * + * Two signature choices are load-bearing — do not "simplify" them: + * - It returns `boolean`, not a type predicate (`error is T`). These error + * classes are structurally identical to `Error`, so a predicate narrows + * `typedError` to `never` in the `else` branches and breaks the downstream + * `instanceof HttpClientError` / `.statusCode` checks. + * - The generic `` is required. With a concrete + * `ctor: ... => Error`, `error instanceof ctor` narrows `error` to `never` on + * the right side of the `||`, breaking the `error.name` access. */ function isErrorOfType( error: Error,