From 08127467f7bf776d7f9a16f642dea5991973e684 Mon Sep 17 00:00:00 2001 From: Ian Clanton-Thuon Date: Fri, 17 Jul 2026 20:50:56 -0400 Subject: [PATCH 1/3] Avoid redundant downloads when local processes race for the same build cache entry When useDirectFileTransfersForBuildCache is enabled, multiple local Rush processes (e.g. parallel "rush build" invocations on the same machine or CI agent) restoring the same cache entry would each independently download it from the cloud cache provider. Use LockFile to have later processes wait for the first one to finish downloading, mirroring the pattern already used for installing the package manager (see InstallHelpers.ensureLocalPackageManagerAsync). After acquiring the lock (or failing/timing out), recheck whether the final cache entry path already exists before downloading, so a process that waited can skip the network round-trip entirely if a peer already populated it. This is strictly a best-effort optimization: if the lock cannot be acquired (for example, it's held by a process on a different machine sharing a network cache folder, where LockFile's stale-holder detection cannot see across machines) we fall back to downloading independently, exactly as before. Correctness never depends on the lock, since downloads always land in a uniquely-named temp file that is atomically renamed into place. The lock's resource name is derived by hashing the cache ID (sha1-hex), since cache IDs can contain characters (e.g. "+", "_", "/") that LockFile.getLockFilePath's resource name validation rejects, depending on the configured cacheEntryNamePattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8d807794-b652-4bf8-bc4b-8ee4b2039f92 --- .../logic/buildCache/OperationBuildCache.ts | 117 +++++++++++++----- .../test/OperationBuildCache.test.ts | 73 ++++++++++- 2 files changed, 157 insertions(+), 33 deletions(-) diff --git a/libraries/rush-lib/src/logic/buildCache/OperationBuildCache.ts b/libraries/rush-lib/src/logic/buildCache/OperationBuildCache.ts index b1a8acb973..5570ccf3f4 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; + /** * @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', () => { From 82f027fde507351d6bd8b6865c90958e0d994765 Mon Sep 17 00:00:00 2001 From: Ian Clanton-Thuon Date: Fri, 17 Jul 2026 20:51:47 -0400 Subject: [PATCH 2/3] Add change file Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8d807794-b652-4bf8-bc4b-8ee4b2039f92 --- ...build-cache-download-lock_2026-07-18-00-51-14.json | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 common/changes/@microsoft/rush/iclanton-build-cache-download-lock_2026-07-18-00-51-14.json 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 From 521643670dfa36231489c9cc8a7bd66c49dfcc29 Mon Sep 17 00:00:00 2001 From: Ian Clanton-Thuon Date: Fri, 17 Jul 2026 20:52:54 -0400 Subject: [PATCH 3/3] fixup! Avoid redundant downloads when local processes race for the same build cache entry --- libraries/rush-lib/src/logic/buildCache/OperationBuildCache.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/rush-lib/src/logic/buildCache/OperationBuildCache.ts b/libraries/rush-lib/src/logic/buildCache/OperationBuildCache.ts index 5570ccf3f4..ef31aa8951 100644 --- a/libraries/rush-lib/src/logic/buildCache/OperationBuildCache.ts +++ b/libraries/rush-lib/src/logic/buildCache/OperationBuildCache.ts @@ -23,7 +23,7 @@ import type { IBaseOperationExecutionResult } from '../operations/IOperationExec * 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; +const DIRECT_FILE_TRANSFER_LOCK_MAX_WAIT_MS: number = 5 * 60 * 1000; // Five minutes /** * @internal