diff --git a/__tests__/cache-restore.test.ts b/__tests__/cache-restore.test.ts index dbf77f6..037c008 100644 --- a/__tests__/cache-restore.test.ts +++ b/__tests__/cache-restore.test.ts @@ -2,7 +2,6 @@ import {jest, describe, it, expect, beforeEach, afterEach} from '@jest/globals'; import fs from 'fs'; -import path from 'path'; jest.unstable_mockModule('@actions/cache', () => ({ saveCache: jest.fn(), @@ -88,10 +87,6 @@ describe('restoreCache', () => { ]; const originalEnv = {...process.env}; const originalArch = Object.getOwnPropertyDescriptor(process, 'arch')!; - const originalPlatform = Object.getOwnPropertyDescriptor( - process, - 'platform' - )!; beforeEach(() => { jest.resetAllMocks(); @@ -102,7 +97,6 @@ describe('restoreCache', () => { ImageOS: 'ubuntu24' }; Object.defineProperty(process, 'arch', {value: 'x64'}); - Object.defineProperty(process, 'platform', {value: 'linux'}); hashFilesSpy.mockResolvedValue('file_hash'); getCacheDirectoryPathSpy.mockResolvedValue(directories); restoreCacheSpy.mockResolvedValue(undefined); @@ -112,7 +106,6 @@ describe('restoreCache', () => { afterEach(() => { process.env = {...originalEnv}; Object.defineProperty(process, 'arch', originalArch); - Object.defineProperty(process, 'platform', originalPlatform); jest.restoreAllMocks(); }); @@ -153,15 +146,9 @@ describe('restoreCache', () => { expect(infoSpy).toHaveBeenCalledWith('build cache is not found'); }); - it.each([ - {platform: 'linux', firstFails: false}, - {platform: 'linux', firstFails: true}, - {platform: 'win32', firstFails: false}, - {platform: 'win32', firstFails: true} - ])( - 'starts both restores concurrently on $platform and waits for both (first fails=$firstFails)', - async ({platform, firstFails}) => { - Object.defineProperty(process, 'platform', {value: platform}); + it.each([false, true])( + 'restores entries sequentially and waits for both (first fails=%s)', + async firstFails => { const started = [deferred(), deferred()]; const release = [deferred(), deferred()]; let completed = false; @@ -179,10 +166,14 @@ describe('restoreCache', () => { completed = true; }); try { - await Promise.all(started.map(signal => signal.promise)); + await started[0].promise; + expect(restoreCacheSpy).toHaveBeenCalledTimes(1); + expect(core.saveState).not.toHaveBeenCalled(); + expect(setOutputSpy).not.toHaveBeenCalledWith('cache-hit', true); + release[0].resolve(); - // Let the first completion propagate while the second stays blocked. - await new Promise(resolve => setImmediate(resolve)); + await started[1].promise; + expect(restoreCacheSpy).toHaveBeenCalledTimes(2); expect(completed).toBe(false); expect(core.saveState).not.toHaveBeenCalled(); expect(setOutputSpy).not.toHaveBeenCalledWith('cache-hit', true); @@ -196,38 +187,6 @@ describe('restoreCache', () => { } ); - it.each(['linux', 'win32'])( - 'restores overlapping directories sequentially on %s', - async platform => { - Object.defineProperty(process, 'platform', {value: platform}); - getCacheDirectoryPathSpy.mockResolvedValue([ - directories[0], - {kind: 'build', path: path.join(modulePath, 'build')} - ]); - const firstStarted = deferred(); - const releaseFirst = deferred(); - restoreCacheSpy.mockImplementation(async (paths, key) => { - if (paths[0] === modulePath) { - firstStarted.resolve(); - await releaseFirst.promise; - } - return key; - }); - - const pending = restore(); - try { - await firstStarted.promise; - expect(restoreCacheSpy).toHaveBeenCalledTimes(1); - } finally { - releaseFirst.resolve(); - await pending; - } - - expect(restoreCacheSpy).toHaveBeenCalledTimes(2); - expect(setOutputSpy).toHaveBeenLastCalledWith('cache-hit', true); - } - ); - it.each([ {modules: false, build: false}, {modules: true, build: false}, diff --git a/__tests__/cache-save.test.ts b/__tests__/cache-save.test.ts index 28682d3..488b06b 100644 --- a/__tests__/cache-save.test.ts +++ b/__tests__/cache-save.test.ts @@ -41,16 +41,17 @@ function deferred() { describe('cache-save', () => { let entries: CacheEntry[]; - let getBooleanInputSpy: jest.Mock; - let getStateSpy: jest.Mock; - let infoSpy: jest.Mock; - let warningSpy: jest.Mock; - let debugSpy: jest.Mock; - let setFailedSpy: jest.Mock; - let saveCacheSpy: jest.Mock; - let existsSpy: jest.Mock; + const getBooleanInputSpy = jest.mocked(core.getBooleanInput); + const getStateSpy = jest.mocked(core.getState); + const infoSpy = jest.mocked(core.info); + const warningSpy = jest.mocked(core.warning); + const debugSpy = jest.mocked(core.debug); + const setFailedSpy = jest.mocked(core.setFailed); + const saveCacheSpy = jest.mocked(cache.saveCache); + const existsSpy = jest.mocked(fs.existsSync); beforeEach(() => { + jest.resetAllMocks(); entries = [ { kind: 'modules', @@ -64,36 +65,14 @@ describe('cache-save', () => { } ]; - getBooleanInputSpy = core.getBooleanInput as jest.Mock< - typeof core.getBooleanInput - >; getBooleanInputSpy.mockReturnValue(true); - - getStateSpy = core.getState as jest.Mock; getStateSpy.mockImplementation(() => JSON.stringify(entries)); - - infoSpy = core.info as jest.Mock; - infoSpy.mockImplementation(() => undefined); - - warningSpy = core.warning as jest.Mock; - warningSpy.mockImplementation(() => undefined); - - debugSpy = core.debug as jest.Mock; - debugSpy.mockImplementation(() => undefined); - - setFailedSpy = core.setFailed as jest.Mock; - setFailedSpy.mockImplementation(() => undefined); - - saveCacheSpy = cache.saveCache as jest.Mock; - saveCacheSpy.mockImplementation(() => Promise.resolve(0)); - - existsSpy = fs.existsSync as jest.Mock; - existsSpy.mockImplementation(() => true); + saveCacheSpy.mockResolvedValue(0); + existsSpy.mockReturnValue(true); }); afterEach(() => { jest.restoreAllMocks(); - jest.clearAllMocks(); }); it('does not save cache when the cache input is false', async () => { @@ -146,7 +125,7 @@ describe('cache-save', () => { ); it('does not save cache when there are no cache folders on the disk', async () => { - existsSpy.mockImplementation(() => false); + existsSpy.mockReturnValue(false); await run(); diff --git a/__tests__/cache-utils.test.ts b/__tests__/cache-utils.test.ts index 688eebf..8fdb5a8 100644 --- a/__tests__/cache-utils.test.ts +++ b/__tests__/cache-utils.test.ts @@ -1,14 +1,4 @@ -import { - jest, - describe, - it, - expect, - beforeEach, - afterEach, - afterAll -} from '@jest/globals'; -import fs from 'fs'; -import path from 'path'; +import {jest, describe, it, expect, beforeEach, afterAll} from '@jest/globals'; jest.unstable_mockModule('@actions/exec', () => ({ exec: jest.fn(), @@ -212,139 +202,6 @@ describe('getCacheDirectoryPath', () => { }); }); -describe('canRestoreCachesInParallel', () => { - const originalPlatform = Object.getOwnPropertyDescriptor( - process, - 'platform' - )!; - const directoryStat = fs.lstatSync(process.cwd()); - - beforeEach(() => { - Object.defineProperty(process, 'platform', {value: 'linux'}); - jest.spyOn(fs, 'lstatSync').mockReturnValue(directoryStat); - jest - .spyOn(fs, 'realpathSync') - .mockImplementation(directory => path.resolve(directory.toString())); - }); - - afterEach(() => { - Object.defineProperty(process, 'platform', originalPlatform); - jest.restoreAllMocks(); - }); - - it.each([ - {modules: 'cache/modules', build: 'cache/build', parallel: true}, - {modules: 'cache', build: 'cache-build', parallel: true}, - {modules: 'cache', build: 'cache', parallel: false}, - {modules: 'cache', build: 'cache/build', parallel: false}, - {modules: 'cache/modules', build: 'cache', parallel: false}, - {modules: 'cache', build: 'cache/../cache', parallel: false} - ])( - 'checks overlapping paths: $modules and $build', - ({modules, build, parallel}) => { - expect( - cacheUtils.canRestoreCachesInParallel([ - {kind: 'modules', path: modules}, - {kind: 'build', path: build} - ]) - ).toBe(parallel); - } - ); - - it('allows concurrent Windows restores with disjoint paths', () => { - Object.defineProperty(process, 'platform', {value: 'win32'}); - - expect( - cacheUtils.canRestoreCachesInParallel([ - {kind: 'modules', path: 'cache/modules'}, - {kind: 'build', path: 'cache/build'} - ]) - ).toBe(true); - }); - - it.each(['darwin', 'win32'])( - 'handles case-insensitive paths conservatively on %s', - platform => { - Object.defineProperty(process, 'platform', {value: platform}); - - expect( - cacheUtils.canRestoreCachesInParallel([ - {kind: 'modules', path: 'cache'}, - {kind: 'build', path: 'CACHE/build'} - ]) - ).toBe(false); - } - ); - - it.each([true, false])( - 'serializes symlink aliases with missing cache directories (overlap=%s)', - overlap => { - const alias = path.resolve('alias'); - const target = path.resolve('target'); - jest.spyOn(fs, 'lstatSync').mockImplementation(directory => { - if (directory !== alias && directory !== target) { - throw Object.assign(new Error('Missing path'), {code: 'ENOENT'}); - } - return directoryStat; - }); - jest.spyOn(fs, 'realpathSync').mockReturnValue(target); - - expect( - cacheUtils.canRestoreCachesInParallel([ - {kind: 'modules', path: path.join(alias, 'modules')}, - { - kind: 'build', - path: path.join(target, overlap ? 'modules/build' : 'build') - } - ]) - ).toBe(false); - } - ); - - it.each(['nested alias', 'alias through module cache'])( - 'serializes restores when the %s resolves outside the module cache', - scenario => { - const modules = path.resolve('modules'); - const target = path.resolve('external-build'); - const alias = path.join(modules, 'build-link'); - const build = - scenario === 'nested alias' - ? path.join(alias, 'cache') - : path.resolve('build-alias', 'cache'); - jest.spyOn(fs, 'realpathSync').mockImplementation(directory => { - const absolute = path.resolve(directory.toString()); - if (absolute === build) { - return path.join(target, 'cache'); - } - if (absolute === path.dirname(build)) { - return scenario === 'nested alias' ? target : alias; - } - return absolute; - }); - - expect( - cacheUtils.canRestoreCachesInParallel([ - {kind: 'modules', path: modules}, - {kind: 'build', path: build} - ]) - ).toBe(false); - } - ); - - it('falls back to sequential restores when path resolution fails', () => { - jest.spyOn(fs, 'realpathSync').mockImplementation(() => { - throw new Error('Cannot resolve path'); - }); - - expect( - cacheUtils.canRestoreCachesInParallel([ - {kind: 'modules', path: 'cache/modules'}, - {kind: 'build', path: 'cache/build'} - ]) - ).toBe(false); - }); -}); - describe('isCacheFeatureAvailable', () => { //Arrange const isFeatureAvailableSpy = cache.isFeatureAvailable as jest.Mock< diff --git a/dist/cache-save/index.js b/dist/cache-save/index.js index cba62e5..dbee425 100644 --- a/dist/cache-save/index.js +++ b/dist/cache-save/index.js @@ -96684,35 +96684,33 @@ const cachePackages = async () => { info('Cache keys were not generated. Please check the log messages above for more errors or information'); return; } - await Promise.all(entries.map(async (entry) => { - try { - await saveCacheEntry(entry); - } - catch (error) { - const message = error instanceof Error ? error.message : String(error); - warning(`Save ${entry.kind} cache failed: ${message}`); - } - })); + await Promise.all(entries.map(saveCacheEntry)); }; const saveCacheEntry = async (entry) => { - const { kind, path, primaryKey, matchedKey } = entry; - if (primaryKey === matchedKey) { - info(`Cache hit occurred on the primary key ${primaryKey}, not saving cache.`); - return; + try { + const { kind, path, primaryKey, matchedKey } = entry; + if (primaryKey === matchedKey) { + info(`Cache hit occurred on the primary key ${primaryKey}, not saving cache.`); + return; + } + if (!external_fs_default().existsSync(path)) { + logWarning(`${kind} cache folder doesn't exist on disk: ${path}`); + return; + } + const cacheId = await cache_saveCache([path], primaryKey); + if (cacheId === -1) { + // saveCache returns -1 without throwing when the cache was not saved, e.g. + // a reserve collision or a read-only token (fork PR). @actions/cache has + // already logged the reason at the appropriate severity, so just trace it. + core_debug(`Cache was not saved for the key: ${primaryKey}`); + return; + } + info(`Cache saved with the key: ${primaryKey}`); } - if (!external_fs_default().existsSync(path)) { - logWarning(`${kind} cache folder doesn't exist on disk: ${path}`); - return; + catch (error) { + const message = error instanceof Error ? error.message : String(error); + warning(`Save ${entry.kind} cache failed: ${message}`); } - const cacheId = await cache_saveCache([path], primaryKey); - if (cacheId === -1) { - // saveCache returns -1 without throwing when the cache was not saved, e.g. - // a reserve collision or a read-only token (fork PR). @actions/cache has - // already logged the reason at the appropriate severity, so just trace it. - core_debug(`Cache was not saved for the key: ${primaryKey}`); - return; - } - info(`Cache saved with the key: ${primaryKey}`); }; function logWarning(message) { const warningPrefix = '[warning]'; diff --git a/dist/setup/index.js b/dist/setup/index.js index 9f2b106..a0cfd4e 100644 --- a/dist/setup/index.js +++ b/dist/setup/index.js @@ -97991,8 +97991,6 @@ const supportedPackageManagers = { - - const getCommandOutput = async (toolCommand) => { let { stdout, stderr, exitCode } = await getExecOutput(toolCommand, undefined, { ignoreReturnCode: true }); if (exitCode) { @@ -98031,58 +98029,6 @@ const getCacheDirectoryPath = async (packageManagerInfo) => { } return cachePaths; }; -function canRestoreCachesInParallel(directories) { - try { - const paths = directories.map(directory => { - const resolvedPath = resolveCachePath(directory.path); - // Conservatively handle case-insensitive Windows/macOS filesystems. - return process.platform === 'darwin' || process.platform === 'win32' - ? resolvedPath.toLowerCase() - : resolvedPath; - }); - // A cache archive can replace symlinks traversed by the other restore. - // Canonical destinations alone do not capture that extraction footprint. - if (paths.some((resolvedPath, index) => { - const absolutePath = external_path_default().resolve(directories[index].path); - const normalizedPath = process.platform === 'darwin' || process.platform === 'win32' - ? absolutePath.toLowerCase() - : absolutePath; - return resolvedPath !== normalizedPath; - })) { - return false; - } - return paths.every((parent, index) => paths.every((child, otherIndex) => { - if (index === otherIndex) { - return true; - } - const relative = external_path_default().relative(parent, child); - return (relative === '..' || - relative.startsWith(`..${(external_path_default()).sep}`) || - external_path_default().isAbsolute(relative)); - })); - } - catch { - // If the paths cannot be resolved safely, avoid concurrent extraction. - return false; - } -} -function resolveCachePath(directory) { - const absolutePath = external_path_default().resolve(directory); - try { - external_fs_default().lstatSync(absolutePath); - } - catch (error) { - const parent = external_path_default().dirname(absolutePath); - if (error.code !== 'ENOENT' || - parent === absolutePath) { - throw error; - } - // Cold cache directories may not exist yet. Resolve their existing - // ancestors so symlink aliases still participate in the overlap check. - return external_path_default().join(resolveCachePath(parent), external_path_default().basename(absolutePath)); - } - return external_fs_default().realpathSync(absolutePath); -} function cache_utils_isGhes() { const ghUrl = new URL(process.env['GITHUB_SERVER_URL'] || 'https://github.com'); const hostname = ghUrl.hostname.trimEnd().toUpperCase(); @@ -98135,7 +98081,7 @@ const cache_restore_restoreCache = async (versionSpec, packageManager, cacheDepe ...directory, primaryKey: primaryKeys[directory.kind] })); - const restoreEntry = async (entry) => { + for (const entry of entries) { core_debug(`${entry.kind} primary key is ${entry.primaryKey}`); try { entry.matchedKey = await restoreCache([entry.path], entry.primaryKey); @@ -98150,14 +98096,6 @@ const cache_restore_restoreCache = async (versionSpec, packageManager, cacheDepe const message = error instanceof Error ? error.message : String(error); warning(`Restore ${entry.kind} cache failed: ${message}`); } - }; - if (canRestoreCachesInParallel(entries)) { - await Promise.all(entries.map(restoreEntry)); - } - else { - for (const entry of entries) { - await restoreEntry(entry); - } } // Preserve the resolved paths so the post step uses the same cache versions, // even if a later step changes the Go environment. diff --git a/docs/advanced-usage.md b/docs/advanced-usage.md index 36f0093..fb14834 100644 --- a/docs/advanced-usage.md +++ b/docs/advanced-usage.md @@ -248,7 +248,7 @@ The action restores and saves two independent cache entries: Both entries use the content hash of files listed by `cache-dependency-path`, or the root `go.mod` by default. Adding source files or build settings to that input invalidates both entries when those files change. Custom cache locations and the cache service's normal access restrictions still apply. -Each entry is restored and saved independently. An exact hit is not uploaded again. A failure for one entry does not prevent caching the other. The `cache-hit` output is `true` only when both entries are restored on their primary keys. +Each entry is restored and saved independently. Restores run sequentially to avoid concurrent extraction into overlapping custom cache directories. An exact hit is not uploaded again. A failure for one entry does not prevent caching the other. The `cache-hit` output is `true` only when both entries are restored on their primary keys. ### Caching in monorepos diff --git a/src/cache-restore.ts b/src/cache-restore.ts index 3701425..7135c43 100644 --- a/src/cache-restore.ts +++ b/src/cache-restore.ts @@ -8,7 +8,6 @@ import {State, Outputs} from './constants.js'; import {PackageManagerInfo} from './package-managers.js'; import { CacheEntry, - canRestoreCachesInParallel, getCacheDirectoryPath, getPackageManagerInfo } from './cache-utils.js'; @@ -49,7 +48,7 @@ export const restoreCache = async ( primaryKey: primaryKeys[directory.kind] })); - const restoreEntry = async (entry: CacheEntry) => { + for (const entry of entries) { core.debug(`${entry.kind} primary key is ${entry.primaryKey}`); try { entry.matchedKey = await cache.restoreCache( @@ -65,14 +64,6 @@ export const restoreCache = async ( const message = error instanceof Error ? error.message : String(error); core.warning(`Restore ${entry.kind} cache failed: ${message}`); } - }; - - if (canRestoreCachesInParallel(entries)) { - await Promise.all(entries.map(restoreEntry)); - } else { - for (const entry of entries) { - await restoreEntry(entry); - } } // Preserve the resolved paths so the post step uses the same cache versions, diff --git a/src/cache-save.ts b/src/cache-save.ts index 41c96e4..db81a10 100644 --- a/src/cache-save.ts +++ b/src/cache-save.ts @@ -49,41 +49,37 @@ const cachePackages = async () => { return; } - await Promise.all( - entries.map(async entry => { - try { - await saveCacheEntry(entry); - } catch (error) { - const message = error instanceof Error ? error.message : String(error); - core.warning(`Save ${entry.kind} cache failed: ${message}`); - } - }) - ); + await Promise.all(entries.map(saveCacheEntry)); }; const saveCacheEntry = async (entry: CacheEntry) => { - const {kind, path, primaryKey, matchedKey} = entry; - if (primaryKey === matchedKey) { - core.info( - `Cache hit occurred on the primary key ${primaryKey}, not saving cache.` - ); - return; - } + try { + const {kind, path, primaryKey, matchedKey} = entry; + if (primaryKey === matchedKey) { + core.info( + `Cache hit occurred on the primary key ${primaryKey}, not saving cache.` + ); + return; + } - if (!fs.existsSync(path)) { - logWarning(`${kind} cache folder doesn't exist on disk: ${path}`); - return; - } + if (!fs.existsSync(path)) { + logWarning(`${kind} cache folder doesn't exist on disk: ${path}`); + return; + } - const cacheId = await cache.saveCache([path], primaryKey); - if (cacheId === -1) { - // saveCache returns -1 without throwing when the cache was not saved, e.g. - // a reserve collision or a read-only token (fork PR). @actions/cache has - // already logged the reason at the appropriate severity, so just trace it. - core.debug(`Cache was not saved for the key: ${primaryKey}`); - return; + const cacheId = await cache.saveCache([path], primaryKey); + if (cacheId === -1) { + // saveCache returns -1 without throwing when the cache was not saved, e.g. + // a reserve collision or a read-only token (fork PR). @actions/cache has + // already logged the reason at the appropriate severity, so just trace it. + core.debug(`Cache was not saved for the key: ${primaryKey}`); + return; + } + core.info(`Cache saved with the key: ${primaryKey}`); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + core.warning(`Save ${entry.kind} cache failed: ${message}`); } - core.info(`Cache saved with the key: ${primaryKey}`); }; function logWarning(message: string): void { diff --git a/src/cache-utils.ts b/src/cache-utils.ts index 3e774ea..bef05da 100644 --- a/src/cache-utils.ts +++ b/src/cache-utils.ts @@ -1,8 +1,6 @@ import * as cache from '@actions/cache'; import * as core from '@actions/core'; import * as exec from '@actions/exec'; -import fs from 'fs'; -import path from 'path'; import { supportedPackageManagers, PackageManagerInfo, @@ -75,69 +73,6 @@ export const getCacheDirectoryPath = async ( return cachePaths; }; -export function canRestoreCachesInParallel( - directories: CacheDirectory[] -): boolean { - try { - const paths = directories.map(directory => { - const resolvedPath = resolveCachePath(directory.path); - // Conservatively handle case-insensitive Windows/macOS filesystems. - return process.platform === 'darwin' || process.platform === 'win32' - ? resolvedPath.toLowerCase() - : resolvedPath; - }); - // A cache archive can replace symlinks traversed by the other restore. - // Canonical destinations alone do not capture that extraction footprint. - if ( - paths.some((resolvedPath, index) => { - const absolutePath = path.resolve(directories[index].path); - const normalizedPath = - process.platform === 'darwin' || process.platform === 'win32' - ? absolutePath.toLowerCase() - : absolutePath; - return resolvedPath !== normalizedPath; - }) - ) { - return false; - } - return paths.every((parent, index) => - paths.every((child, otherIndex) => { - if (index === otherIndex) { - return true; - } - const relative = path.relative(parent, child); - return ( - relative === '..' || - relative.startsWith(`..${path.sep}`) || - path.isAbsolute(relative) - ); - }) - ); - } catch { - // If the paths cannot be resolved safely, avoid concurrent extraction. - return false; - } -} - -function resolveCachePath(directory: string): string { - const absolutePath = path.resolve(directory); - try { - fs.lstatSync(absolutePath); - } catch (error) { - const parent = path.dirname(absolutePath); - if ( - (error as NodeJS.ErrnoException).code !== 'ENOENT' || - parent === absolutePath - ) { - throw error; - } - // Cold cache directories may not exist yet. Resolve their existing - // ancestors so symlink aliases still participate in the overlap check. - return path.join(resolveCachePath(parent), path.basename(absolutePath)); - } - return fs.realpathSync(absolutePath); -} - export function isGhes(): boolean { const ghUrl = new URL( process.env['GITHUB_SERVER_URL'] || 'https://github.com'