diff --git a/common/changes/@microsoft/rush/iclanton-build-cache-download-lock_2026-07-18-00-51-14.json b/common/changes/@microsoft/rush/iclanton-build-cache-download-lock_2026-07-18-00-51-14.json new file mode 100644 index 0000000000..1d2feb9fe0 --- /dev/null +++ b/common/changes/@microsoft/rush/iclanton-build-cache-download-lock_2026-07-18-00-51-14.json @@ -0,0 +1,11 @@ +{ + "changes": [ + { + "comment": "Avoid redundant downloads when multiple local Rush processes race to restore the same build cache entry (when useDirectFileTransfersForBuildCache is enabled).", + "type": "none", + "packageName": "@microsoft/rush" + } + ], + "packageName": "@microsoft/rush", + "email": "198982749+Copilot@users.noreply.github.com" +} \ No newline at end of file diff --git a/libraries/rush-lib/src/logic/buildCache/OperationBuildCache.ts b/libraries/rush-lib/src/logic/buildCache/OperationBuildCache.ts index b1a8acb973..ef31aa8951 100644 --- a/libraries/rush-lib/src/logic/buildCache/OperationBuildCache.ts +++ b/libraries/rush-lib/src/logic/buildCache/OperationBuildCache.ts @@ -4,7 +4,7 @@ import * as path from 'node:path'; import * as crypto from 'node:crypto'; -import { FileSystem, type FolderItem, InternalError, Async } from '@rushstack/node-core-library'; +import { FileSystem, type FolderItem, InternalError, Async, LockFile } from '@rushstack/node-core-library'; import type { ITerminal } from '@rushstack/terminal'; import type { RushConfigurationProject } from '../../api/RushConfigurationProject'; @@ -15,6 +15,16 @@ import { TarExecutable } from '../../utilities/TarExecutable'; import { EnvironmentVariableNames } from '../../api/EnvironmentConfiguration'; import type { IBaseOperationExecutionResult } from '../operations/IOperationExecutionResult'; +/** + * How long to wait to acquire the per-cache-entry download lock (see + * {@link OperationBuildCache._getDirectFileTransferLockResourceName}) before giving up and + * downloading independently. This is strictly a best-effort optimization to avoid redundant + * downloads when multiple local Rush processes race to restore the same cache entry; it is not + * required for correctness, since downloads always land in a uniquely-named temp file that is + * atomically renamed into place. + */ +const DIRECT_FILE_TRANSFER_LOCK_MAX_WAIT_MS: number = 5 * 60 * 1000; // Five minutes + /** * @internal */ @@ -66,6 +76,13 @@ interface IPathsToCache { outputFilePaths: string[]; } +function _getDirectFileTransferLockResourceName(cacheId: string): string { + // LockFile resource names must match /^[a-zA-Z0-9][a-zA-Z0-9-.]+[a-zA-Z0-9]$/, but cacheId may + // contain other characters (e.g. "/") depending on the configured cacheEntryNamePattern, so hash + // it into a fixed-length, lock-file-safe token. + return crypto.createHash('sha1').update(cacheId).digest('hex'); +} + /** * @internal */ @@ -188,43 +205,79 @@ export class OperationBuildCache { // Use file-based path to avoid loading the entire cache entry into memory. // The provider downloads directly to a temp file that is atomically moved into place. const targetPath: string = this._localBuildCacheProvider.getCacheEntryPath(cacheId); - const tempTargetPath: string = OperationBuildCache._getTempLocalCacheEntryPath(targetPath); + + // If multiple local Rush processes race to restore the same cache entry (e.g. parallel + // "rush build" invocations on the same machine or CI agent), avoid redundant downloads by + // having later processes wait for the first one to finish, mirroring the pattern used for + // installing the package manager (see InstallHelpers.ensureLocalPackageManagerAsync). This + // is strictly a best-effort optimization: if the lock cannot be acquired (for example, + // because it is held by a process on a different machine sharing a network cache folder, + // where the lock's stale-holder detection cannot see across machines) we simply fall back + // to downloading independently below. Correctness never depends on the lock, because + // downloads always land in a uniquely-named temp file that is atomically renamed into + // place. + let downloadLock: LockFile | undefined; try { - const downloadedToTempFile: boolean = - await this._cloudBuildCacheProvider.tryDownloadCacheEntryToFileAsync( - terminal, - cacheId, - tempTargetPath - ); - if (downloadedToTempFile) { - await Async.runWithRetriesAsync({ - action: () => - FileSystem.moveAsync({ - sourcePath: tempTargetPath, - destinationPath: targetPath, - overwrite: true - }), - maxRetries: 2, - retryDelayMs: 500 - }); + downloadLock = await LockFile.acquireAsync( + path.dirname(targetPath), + _getDirectFileTransferLockResourceName(cacheId), + DIRECT_FILE_TRANSFER_LOCK_MAX_WAIT_MS + ); + } catch (e) { + terminal.writeVerboseLine( + `Unable to acquire the local download lock for cache entry "${cacheId}"; downloading independently: ${e}` + ); + } + + try { + // Another process may have finished populating the cache entry while we were waiting + // for the lock (or the lock may not have been acquired at all). + if (await FileSystem.existsAsync(targetPath)) { cloudCacheHit = true; localCacheEntryPath = targetPath; updateLocalCacheSuccess = true; - } - } catch (e) { - terminal.writeVerboseLine(`Failed to download cache entry to local cache: ${e}`); - updateLocalCacheSuccess = false; - } + } else { + const tempTargetPath: string = OperationBuildCache._getTempLocalCacheEntryPath(targetPath); + try { + const downloadedToTempFile: boolean = + await this._cloudBuildCacheProvider.tryDownloadCacheEntryToFileAsync( + terminal, + cacheId, + tempTargetPath + ); + if (downloadedToTempFile) { + await Async.runWithRetriesAsync({ + action: () => + FileSystem.moveAsync({ + sourcePath: tempTargetPath, + destinationPath: targetPath, + overwrite: true + }), + maxRetries: 2, + retryDelayMs: 500 + }); + cloudCacheHit = true; + localCacheEntryPath = targetPath; + updateLocalCacheSuccess = true; + } + } catch (e) { + terminal.writeVerboseLine(`Failed to download cache entry to local cache: ${e}`); + updateLocalCacheSuccess = false; + } - if (!cloudCacheHit) { - // Clean up any partial file left by the failed or missed download so it isn't - // mistaken for a valid cache entry on the next build. Providers may catch errors - // internally and return false instead of throwing, leaving a partially written file. - try { - await FileSystem.deleteFileAsync(tempTargetPath); - } catch { - // Ignore cleanup errors (file may not have been created) + if (!cloudCacheHit) { + // Clean up any partial file left by the failed or missed download so it isn't + // mistaken for a valid cache entry on the next build. Providers may catch errors + // internally and return false instead of throwing, leaving a partially written file. + try { + await FileSystem.deleteFileAsync(tempTargetPath); + } catch { + // Ignore cleanup errors (file may not have been created) + } + } } + } finally { + downloadLock?.release(); } } else { const cacheEntryBuffer: Buffer | undefined = diff --git a/libraries/rush-lib/src/logic/buildCache/test/OperationBuildCache.test.ts b/libraries/rush-lib/src/logic/buildCache/test/OperationBuildCache.test.ts index a35e2d0f14..2120295dcf 100644 --- a/libraries/rush-lib/src/logic/buildCache/test/OperationBuildCache.test.ts +++ b/libraries/rush-lib/src/logic/buildCache/test/OperationBuildCache.test.ts @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license. // See LICENSE in the project root for license information. -import { FileSystem, type FolderItem } from '@rushstack/node-core-library'; +import { FileSystem, type FolderItem, LockFile } from '@rushstack/node-core-library'; import { StringBufferTerminalProvider, Terminal, type ITerminal } from '@rushstack/terminal'; import type { BuildCacheConfiguration } from '../../../api/BuildCacheConfiguration'; @@ -77,6 +77,18 @@ describe(OperationBuildCache.name, () => { }); describe('direct file cloud cache restore', () => { + let fakeLockRelease: jest.Mock; + + beforeEach(() => { + fakeLockRelease = jest.fn(); + // By default, simulate an uncontended lock acquisition and no pre-existing cache entry. + // Individual tests may override these to exercise the lock-contention/fallback paths. + jest + .spyOn(LockFile, 'acquireAsync') + .mockImplementation(async () => ({ release: fakeLockRelease }) as unknown as LockFile); + jest.spyOn(FileSystem, 'existsAsync').mockResolvedValue(false); + }); + afterEach(() => { Reflect.set(OperationBuildCache, '_tarUtilityPromise', undefined); jest.restoreAllMocks(); @@ -151,6 +163,12 @@ describe(OperationBuildCache.name, () => { }) ); expect(deleteFileAsyncSpy).not.toHaveBeenCalled(); + expect(LockFile.acquireAsync).toHaveBeenCalledWith( + '/cache', + expect.stringMatching(/^[0-9a-f]{40}$/), + expect.any(Number) + ); + expect(fakeLockRelease).toHaveBeenCalledTimes(1); }); it('cleans up the temp file when a direct file download misses or fails', async () => { @@ -174,6 +192,59 @@ describe(OperationBuildCache.name, () => { expect(tempPath).toMatch(/^\/cache\/acme-wizard-cache-entry-[0-9a-f]+\.temp$/); expect(deleteFileAsyncSpy).toHaveBeenCalledWith(tempPath); }); + + it('skips downloading when another process already populated the cache entry while waiting for the lock', async () => { + const tryDownloadCacheEntryToFileAsync: jest.Mock< + Promise, + [ITerminal, string, string] + > = jest.fn(); + const subject: OperationBuildCache = prepareDirectTransferSubject({ + tryDownloadCacheEntryToFileAsync + }); + const terminal: Terminal = new Terminal(new StringBufferTerminalProvider()); + const tryUntarAsync: jest.Mock = jest.fn().mockResolvedValue(0); + + jest.spyOn(FileSystem, 'deleteFolderAsync').mockResolvedValue(); + // Simulate the cache entry having been fully populated (e.g. by another local process) + // by the time we acquired the lock. + jest.spyOn(FileSystem, 'existsAsync').mockResolvedValue(true); + Reflect.set(OperationBuildCache, '_tarUtilityPromise', Promise.resolve({ tryUntarAsync })); + + const result: boolean = await subject.tryRestoreFromCacheAsync(terminal); + + expect(result).toBe(true); + expect(tryDownloadCacheEntryToFileAsync).not.toHaveBeenCalled(); + expect(tryUntarAsync).toHaveBeenCalledWith( + expect.objectContaining({ + archivePath: '/cache/acme-wizard-cache-entry' + }) + ); + expect(fakeLockRelease).toHaveBeenCalledTimes(1); + }); + + it('falls back to downloading independently when the download lock cannot be acquired', async () => { + jest.spyOn(LockFile, 'acquireAsync').mockRejectedValue(new Error('Exceeded maximum wait time')); + + const tryDownloadCacheEntryToFileAsync: jest.Mock, [ITerminal, string, string]> = jest + .fn() + .mockResolvedValue(true); + const subject: OperationBuildCache = prepareDirectTransferSubject({ + tryDownloadCacheEntryToFileAsync + }); + const terminal: Terminal = new Terminal(new StringBufferTerminalProvider()); + const tryUntarAsync: jest.Mock = jest.fn().mockResolvedValue(0); + + jest.spyOn(FileSystem, 'deleteFolderAsync').mockResolvedValue(); + jest.spyOn(FileSystem, 'moveAsync').mockResolvedValue(); + Reflect.set(OperationBuildCache, '_tarUtilityPromise', Promise.resolve({ tryUntarAsync })); + + const result: boolean = await subject.tryRestoreFromCacheAsync(terminal); + + expect(result).toBe(true); + expect(tryDownloadCacheEntryToFileAsync).toHaveBeenCalledTimes(1); + // No lock instance was returned, so there is nothing to release. + expect(fakeLockRelease).not.toHaveBeenCalled(); + }); }); describe('AppleDouble file exclusion', () => {