diff --git a/packages/cache/__tests__/cache.test.ts b/packages/cache/__tests__/cache.test.ts index 8037530554..b3c7f43070 100644 --- a/packages/cache/__tests__/cache.test.ts +++ b/packages/cache/__tests__/cache.test.ts @@ -1,14 +1,69 @@ import * as cache from '../src/cache' -test('isFeatureAvailable returns true if server url is set', () => { - try { +describe('isFeatureAvailable', () => { + const originalEnv = process.env + + beforeEach(() => { + jest.resetModules() + process.env = {...originalEnv} + // Clean cache-related environment variables + delete process.env['ACTIONS_CACHE_URL'] + delete process.env['ACTIONS_RESULTS_URL'] + delete process.env['ACTIONS_CACHE_SERVICE_V2'] + delete process.env['GITHUB_SERVER_URL'] + }) + + afterAll(() => { + process.env = originalEnv + }) + + test('returns true for cache service v1 when ACTIONS_CACHE_URL is set', () => { process.env['ACTIONS_CACHE_URL'] = 'http://cache.com' expect(cache.isFeatureAvailable()).toBe(true) - } finally { - delete process.env['ACTIONS_CACHE_URL'] - } -}) + }) + + test('returns false for cache service v1 when only ACTIONS_RESULTS_URL is set', () => { + process.env['ACTIONS_RESULTS_URL'] = 'http://results.com' + expect(cache.isFeatureAvailable()).toBe(false) + }) + + test('returns true for cache service v1 when both URLs are set', () => { + process.env['ACTIONS_CACHE_URL'] = 'http://cache.com' + process.env['ACTIONS_RESULTS_URL'] = 'http://results.com' + expect(cache.isFeatureAvailable()).toBe(true) + }) + + test('returns true for cache service v2 when ACTIONS_RESULTS_URL is set', () => { + process.env['ACTIONS_CACHE_SERVICE_V2'] = 'true' + process.env['ACTIONS_RESULTS_URL'] = 'http://results.com' + expect(cache.isFeatureAvailable()).toBe(true) + }) + + test('returns false for cache service v2 when only ACTIONS_CACHE_URL is set', () => { + process.env['ACTIONS_CACHE_SERVICE_V2'] = 'true' + process.env['ACTIONS_CACHE_URL'] = 'http://cache.com' + expect(cache.isFeatureAvailable()).toBe(false) + }) + + test('returns false when no cache URLs are set', () => { + expect(cache.isFeatureAvailable()).toBe(false) + }) + + test('returns false for cache service v2 when no URLs are set', () => { + process.env['ACTIONS_CACHE_SERVICE_V2'] = 'true' + expect(cache.isFeatureAvailable()).toBe(false) + }) + + test('returns true for GHES with v1 even when v2 flag is set', () => { + process.env['GITHUB_SERVER_URL'] = 'https://my-enterprise.github.com' + process.env['ACTIONS_CACHE_SERVICE_V2'] = 'true' + process.env['ACTIONS_CACHE_URL'] = 'http://cache.com' + expect(cache.isFeatureAvailable()).toBe(true) + }) -test('isFeatureAvailable returns false if server url is not set', () => { - expect(cache.isFeatureAvailable()).toBe(false) + test('returns false for GHES with only ACTIONS_RESULTS_URL', () => { + process.env['GITHUB_SERVER_URL'] = 'https://my-enterprise.github.com' + process.env['ACTIONS_RESULTS_URL'] = 'http://results.com' + expect(cache.isFeatureAvailable()).toBe(false) + }) }) diff --git a/packages/cache/__tests__/restoreCache.test.ts b/packages/cache/__tests__/restoreCache.test.ts index 7992490e5a..4b5c6f865f 100644 --- a/packages/cache/__tests__/restoreCache.test.ts +++ b/packages/cache/__tests__/restoreCache.test.ts @@ -6,6 +6,8 @@ import * as cacheUtils from '../src/internal/cacheUtils' import {CacheFilename, CompressionMethod} from '../src/internal/constants' import {ArtifactCacheEntry} from '../src/internal/contracts' import * as tar from '../src/internal/tar' +import {HttpClientError} from '@actions/http-client' +import {CacheServiceClientJSON} from '../src/generated/results/api/v1/cache.twirp-client' jest.mock('../src/internal/cacheHttpClient') jest.mock('../src/internal/cacheUtils') @@ -73,18 +75,28 @@ test('restore with no cache found', async () => { test('restore with server error should fail', async () => { const paths = ['node_modules'] const key = 'node-test' - const logWarningMock = jest.spyOn(core, 'warning') + const logErrorMock = jest.spyOn(core, 'error') - jest.spyOn(cacheHttpClient, 'getCacheEntry').mockImplementation(() => { - throw new Error('HTTP Error Occurred') - }) + // Set cache service to V2 to test error logging for server errors + process.env['ACTIONS_CACHE_SERVICE_V2'] = 'true' + process.env['ACTIONS_RESULTS_URL'] = 'https://results.local/' + + jest + .spyOn(CacheServiceClientJSON.prototype, 'GetCacheEntryDownloadURL') + .mockImplementation(() => { + throw new HttpClientError('HTTP Error Occurred', 500) + }) const cacheKey = await restoreCache(paths, key) expect(cacheKey).toBe(undefined) - expect(logWarningMock).toHaveBeenCalledTimes(1) - expect(logWarningMock).toHaveBeenCalledWith( + expect(logErrorMock).toHaveBeenCalledTimes(1) + expect(logErrorMock).toHaveBeenCalledWith( 'Failed to restore: HTTP Error Occurred' ) + + // Clean up environment + delete process.env['ACTIONS_CACHE_SERVICE_V2'] + delete process.env['ACTIONS_RESULTS_URL'] }) test('restore with restore keys and no cache found', async () => { diff --git a/packages/cache/__tests__/restoreCacheV2.test.ts b/packages/cache/__tests__/restoreCacheV2.test.ts index 485b8aebce..e24d1df4b4 100644 --- a/packages/cache/__tests__/restoreCacheV2.test.ts +++ b/packages/cache/__tests__/restoreCacheV2.test.ts @@ -8,6 +8,7 @@ import {restoreCache} from '../src/cache' import {CacheFilename, CompressionMethod} from '../src/internal/constants' import {CacheServiceClientJSON} from '../src/generated/results/api/v1/cache.twirp-client' import {DownloadOptions} from '../src/options' +import {HttpClientError} from '@actions/http-client' jest.mock('../src/internal/cacheHttpClient') jest.mock('../src/internal/cacheUtils') @@ -95,18 +96,18 @@ test('restore with no cache found', async () => { test('restore with server error should fail', async () => { const paths = ['node_modules'] const key = 'node-test' - const logWarningMock = jest.spyOn(core, 'warning') + const logErrorMock = jest.spyOn(core, 'error') jest .spyOn(CacheServiceClientJSON.prototype, 'GetCacheEntryDownloadURL') .mockImplementation(() => { - throw new Error('HTTP Error Occurred') + throw new HttpClientError('HTTP Error Occurred', 500) }) const cacheKey = await restoreCache(paths, key) expect(cacheKey).toBe(undefined) - expect(logWarningMock).toHaveBeenCalledTimes(1) - expect(logWarningMock).toHaveBeenCalledWith( + expect(logErrorMock).toHaveBeenCalledTimes(1) + expect(logErrorMock).toHaveBeenCalledWith( 'Failed to restore: HTTP Error Occurred' ) }) diff --git a/packages/cache/__tests__/saveCache.test.ts b/packages/cache/__tests__/saveCache.test.ts index e5ed695b1f..ab5d380351 100644 --- a/packages/cache/__tests__/saveCache.test.ts +++ b/packages/cache/__tests__/saveCache.test.ts @@ -7,11 +7,12 @@ import * as config from '../src/internal/config' import {CacheFilename, CompressionMethod} from '../src/internal/constants' import * as tar from '../src/internal/tar' import {TypedResponse} from '@actions/http-client/lib/interfaces' +import {HttpClientError} from '@actions/http-client' import { ReserveCacheResponse, ITypedResponseWithError } from '../src/internal/contracts' -import {HttpClientError} from '@actions/http-client' +import {CacheServiceClientJSON} from '../src/generated/results/api/v1/cache.twirp-client' jest.mock('../src/internal/cacheHttpClient') jest.mock('../src/internal/cacheUtils') @@ -223,45 +224,48 @@ test('save with reserve cache failure should fail', async () => { test('save with server error should fail', async () => { const filePath = 'node_modules' const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' - const cachePaths = [path.resolve(filePath)] + const logErrorMock = jest.spyOn(core, 'error') const logWarningMock = jest.spyOn(core, 'warning') - const cacheId = 4 - const reserveCacheMock = jest - .spyOn(cacheHttpClient, 'reserveCache') - .mockImplementation(async () => { - const response: TypedResponse = { - statusCode: 500, - result: {cacheId}, - headers: {} - } - return response - }) + + // Mock cache service version to V2 + const getCacheServiceVersionMock = jest.spyOn(config, 'getCacheServiceVersion').mockReturnValue('v2') + + // Mock V2 CreateCacheEntry to succeed + const createCacheEntryMock = jest + .spyOn(CacheServiceClientJSON.prototype, 'CreateCacheEntry') + .mockReturnValue( + Promise.resolve({ok: true, signedUploadUrl: 'https://blob-storage.local?signed=true'}) + ) + + // Mock the FinalizeCacheEntryUpload to succeed (since the error should happen in saveCache) + const finalizeCacheEntryMock = jest + .spyOn(CacheServiceClientJSON.prototype, 'FinalizeCacheEntryUpload') + .mockReturnValue(Promise.resolve({ok: true, entryId: '4'})) const createTarMock = jest.spyOn(tar, 'createTar') + // Mock the saveCache call to throw a server error const saveCacheMock = jest .spyOn(cacheHttpClient, 'saveCache') .mockImplementationOnce(() => { - throw new Error('HTTP Error Occurred') + throw new HttpClientError('HTTP Error Occurred', 500) }) + const compression = CompressionMethod.Zstd const getCompressionMock = jest .spyOn(cacheUtils, 'getCompressionMethod') .mockReturnValueOnce(Promise.resolve(compression)) await saveCache([filePath], primaryKey) - expect(logWarningMock).toHaveBeenCalledTimes(1) - expect(logWarningMock).toHaveBeenCalledWith( + + expect(logErrorMock).toHaveBeenCalledTimes(1) + expect(logErrorMock).toHaveBeenCalledWith( 'Failed to save: HTTP Error Occurred' ) - expect(reserveCacheMock).toHaveBeenCalledTimes(1) - expect(reserveCacheMock).toHaveBeenCalledWith(primaryKey, [filePath], { - cacheSize: undefined, - compressionMethod: compression, - enableCrossOsArchive: false - }) + expect(createCacheEntryMock).toHaveBeenCalledTimes(1) const archiveFolder = '/foo/bar' + const cachePaths = [path.resolve(filePath)] const archiveFile = path.join(archiveFolder, CacheFilename.Zstd) expect(createTarMock).toHaveBeenCalledTimes(1) expect(createTarMock).toHaveBeenCalledWith( @@ -270,13 +274,11 @@ test('save with server error should fail', async () => { compression ) expect(saveCacheMock).toHaveBeenCalledTimes(1) - expect(saveCacheMock).toHaveBeenCalledWith( - cacheId, - archiveFile, - '', - undefined - ) expect(getCompressionMock).toHaveBeenCalledTimes(1) + expect(getCompressionMock).toHaveBeenCalledTimes(1) + + // Restore the getCacheServiceVersion mock to its original state + getCacheServiceVersionMock.mockRestore() }) test('save with valid inputs uploads a cache', async () => { diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index f7b2d1937e..ba4cf1c768 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -13,6 +13,7 @@ import { GetCacheEntryDownloadURLRequest } from './generated/results/api/v1/cache' import {CacheFileSizeLimit} from './internal/constants' +import {HttpClientError} from '@actions/http-client' export class ValidationError extends Error { constructor(message: string) { super(message) @@ -57,7 +58,18 @@ function checkKey(key: string): void { * @returns boolean return true if Actions cache service feature is available, otherwise false */ export function isFeatureAvailable(): boolean { - return !!process.env['ACTIONS_CACHE_URL'] + const cacheServiceVersion = getCacheServiceVersion() + + // Check availability based on cache service version + switch (cacheServiceVersion) { + case 'v2': + // For v2, we need ACTIONS_RESULTS_URL + return !!process.env['ACTIONS_RESULTS_URL'] + case 'v1': + default: + // For v1, we only need ACTIONS_CACHE_URL + return !!process.env['ACTIONS_CACHE_URL'] + } } /** @@ -186,8 +198,17 @@ async function restoreCacheV1( if (typedError.name === ValidationError.name) { throw error } else { - // Supress all non-validation cache related errors because caching should be optional - core.warning(`Failed to restore: ${(error as Error).message}`) + // warn on cache restore failure and continue build + // Log server errors (5xx) as errors, all other errors as warnings + if ( + typedError instanceof HttpClientError && + typeof typedError.statusCode === 'number' && + typedError.statusCode >= 500 + ) { + core.error(`Failed to restore: ${(error as Error).message}`) + } else { + core.warning(`Failed to restore: ${(error as Error).message}`) + } } } finally { // Try to delete the archive to save space @@ -305,7 +326,16 @@ async function restoreCacheV2( throw error } else { // Supress all non-validation cache related errors because caching should be optional - core.warning(`Failed to restore: ${(error as Error).message}`) + // Log server errors (5xx) as errors, all other errors as warnings + if ( + typedError instanceof HttpClientError && + typeof typedError.statusCode === 'number' && + typedError.statusCode >= 500 + ) { + core.error(`Failed to restore: ${(error as Error).message}`) + } else { + core.warning(`Failed to restore: ${(error as Error).message}`) + } } } finally { try { @@ -437,7 +467,16 @@ async function saveCacheV1( } else if (typedError.name === ReserveCacheError.name) { core.info(`Failed to save: ${typedError.message}`) } else { - core.warning(`Failed to save: ${typedError.message}`) + // Log server errors (5xx) as errors, all other errors as warnings + if ( + typedError instanceof HttpClientError && + typeof typedError.statusCode === 'number' && + typedError.statusCode >= 500 + ) { + core.error(`Failed to save: ${typedError.message}`) + } else { + core.warning(`Failed to save: ${typedError.message}`) + } } } finally { // Try to delete the archive to save space @@ -576,7 +615,16 @@ async function saveCacheV2( } else if (typedError.name === ReserveCacheError.name) { core.info(`Failed to save: ${typedError.message}`) } else { - core.warning(`Failed to save: ${typedError.message}`) + // Log server errors (5xx) as errors, all other errors as warnings + if ( + typedError instanceof HttpClientError && + typeof typedError.statusCode === 'number' && + typedError.statusCode >= 500 + ) { + core.error(`Failed to save: ${typedError.message}`) + } else { + core.warning(`Failed to save: ${typedError.message}`) + } } } finally { // Try to delete the archive to save space