Fix atomic cloud cache download writes

Copilot · web-flow · commit 7254544859e1 · 2026-07-17T20:54:46.000Z
diff --git a/libraries/rush-lib/src/logic/buildCache/OperationBuildCache.ts b/libraries/rush-lib/src/logic/buildCache/OperationBuildCache.ts
@@ -106,6 +106,14 @@ export class OperationBuildCache {
     this._useDirectFileTransfersForBuildCache = useDirectFileTransfersForBuildCache;
   }
 
+  private static _getTempLocalCacheEntryPath(finalLocalCacheEntryPath: string): string {
+    // Derive the temp file from the destination path to ensure they are on the same volume.
+    // In the case of a shared network drive containing the build cache, we also need to make
+    // sure the temp path won't be shared by two parallel rush builds.
+    const randomSuffix: string = crypto.randomBytes(8).toString('hex');
+    return `${finalLocalCacheEntryPath}-${randomSuffix}.temp`;
+  }
+
   private static _tryGetTarUtility(terminal: ITerminal): Promise<TarExecutable | undefined> {
     if (!OperationBuildCache._tarUtilityPromise) {
       OperationBuildCache._tarUtilityPromise = TarExecutable.tryInitializeAsync(terminal);
@@ -178,15 +186,28 @@ export class OperationBuildCache {
         this._cloudBuildCacheProvider.tryDownloadCacheEntryToFileAsync
       ) {
         // Use file-based path to avoid loading the entire cache entry into memory.
-        // The provider downloads directly to the local cache file.
+        // 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);
         try {
-          cloudCacheHit = await this._cloudBuildCacheProvider.tryDownloadCacheEntryToFileAsync(
-            terminal,
-            cacheId,
-            targetPath
-          );
-          if (cloudCacheHit) {
+          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;
           }
@@ -200,7 +221,7 @@ export class OperationBuildCache {
           // 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(targetPath);
+            await FileSystem.deleteFileAsync(tempTargetPath);
           } catch {
             // Ignore cleanup errors (file may not have been created)
           }
@@ -296,12 +317,8 @@ export class OperationBuildCache {
     const tarUtility: TarExecutable | undefined = await OperationBuildCache._tryGetTarUtility(terminal);
     if (tarUtility) {
       const finalLocalCacheEntryPath: string = this._localBuildCacheProvider.getCacheEntryPath(cacheId);
-
-      // Derive the temp file from the destination path to ensure they are on the same volume
-      // In the case of a shared network drive containing the build cache, we also need to make
-      // sure the the temp path won't be shared by two parallel rush builds.
-      const randomSuffix: string = crypto.randomBytes(8).toString('hex');
-      const tempLocalCacheEntryPath: string = `${finalLocalCacheEntryPath}-${randomSuffix}.temp`;
+      const tempLocalCacheEntryPath: string =
+        OperationBuildCache._getTempLocalCacheEntryPath(finalLocalCacheEntryPath);
 
       const logFilePath: string = this._getTarLogFilePath(cacheId, 'tar');
       const tarExitCode: number = await tarUtility.tryCreateArchiveFromProjectPathsAsync({
diff --git a/libraries/rush-lib/src/logic/buildCache/test/OperationBuildCache.test.ts b/libraries/rush-lib/src/logic/buildCache/test/OperationBuildCache.test.ts
@@ -2,7 +2,7 @@
 // See LICENSE in the project root for license information.
 
 import { FileSystem, type FolderItem } from '@rushstack/node-core-library';
-import { StringBufferTerminalProvider, Terminal } from '@rushstack/terminal';
+import { StringBufferTerminalProvider, Terminal, type ITerminal } from '@rushstack/terminal';
 
 import type { BuildCacheConfiguration } from '../../../api/BuildCacheConfiguration';
 import type { RushConfigurationProject } from '../../../api/RushConfigurationProject';
@@ -52,6 +52,7 @@ describe(OperationBuildCache.name, () => {
         packageName: 'acme-wizard',
         projectRelativeFolder: 'apps/acme-wizard',
         projectFolder: '/repo/apps/acme-wizard',
+        projectRushTempFolder: '/repo/common/temp/project',
         dependencyProjects: []
       } as unknown as RushConfigurationProject,
       // Value from past tests, for consistency.
@@ -75,6 +76,106 @@ describe(OperationBuildCache.name, () => {
     });
   });
 
+  describe('direct file cloud cache restore', () => {
+    afterEach(() => {
+      Reflect.set(OperationBuildCache, '_tarUtilityPromise', undefined);
+      jest.restoreAllMocks();
+    });
+
+    function prepareDirectTransferSubject(cloudBuildCacheProvider: {
+      tryDownloadCacheEntryToFileAsync: jest.Mock<Promise<boolean>, [ITerminal, string, string]>;
+    }): OperationBuildCache {
+      const terminal: Terminal = new Terminal(new StringBufferTerminalProvider());
+
+      return OperationBuildCache.getOperationBuildCache({
+        buildCacheConfiguration: {
+          buildCacheEnabled: true,
+          getCacheEntryId: (opts: IGenerateCacheEntryIdOptions) =>
+            `${opts.projectName}/${opts.projectStateHash}`,
+          localCacheProvider: {
+            getCacheEntryPath: jest.fn().mockReturnValue('/cache/acme-wizard-cache-entry'),
+            tryGetCacheEntryPathByIdAsync: jest.fn().mockResolvedValue(undefined)
+          },
+          cloudCacheProvider: {
+            isCacheWriteAllowed: false,
+            ...cloudBuildCacheProvider
+          }
+        } as unknown as BuildCacheConfiguration,
+        projectOutputFolderNames: ['dist'],
+        project: {
+          packageName: 'acme-wizard',
+          projectRelativeFolder: 'apps/acme-wizard',
+          projectFolder: '/repo/apps/acme-wizard',
+          projectRushTempFolder: '/repo/common/temp/project',
+          dependencyProjects: []
+        } as unknown as RushConfigurationProject,
+        operationStateHash: '1926f30e8ed24cb47be89aea39e7efd70fcda075',
+        terminal,
+        phaseName: 'build',
+        excludeAppleDoubleFiles: false,
+        useDirectFileTransfersForBuildCache: true
+      });
+    }
+
+    it('downloads cloud cache entries to a temp file before atomically moving them into place', async () => {
+      const tryDownloadCacheEntryToFileAsync: jest.Mock<Promise<boolean>, [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();
+      const moveAsyncSpy: jest.SpyInstance = jest.spyOn(FileSystem, 'moveAsync').mockResolvedValue();
+      const deleteFileAsyncSpy: jest.SpyInstance = jest
+        .spyOn(FileSystem, 'deleteFileAsync')
+        .mockResolvedValue();
+      Reflect.set(OperationBuildCache, '_tarUtilityPromise', Promise.resolve({ tryUntarAsync }));
+
+      const result: boolean = await subject.tryRestoreFromCacheAsync(terminal);
+
+      expect(result).toBe(true);
+      expect(tryDownloadCacheEntryToFileAsync).toHaveBeenCalledTimes(1);
+      const [, , tempPath]: [ITerminal, string, string] = tryDownloadCacheEntryToFileAsync.mock.calls[0];
+      expect(tempPath).toMatch(/^\/cache\/acme-wizard-cache-entry-[0-9a-f]+\.temp$/);
+      expect(moveAsyncSpy).toHaveBeenCalledWith({
+        sourcePath: tempPath,
+        destinationPath: '/cache/acme-wizard-cache-entry',
+        overwrite: true
+      });
+      expect(tryUntarAsync).toHaveBeenCalledWith(
+        expect.objectContaining({
+          archivePath: '/cache/acme-wizard-cache-entry'
+        })
+      );
+      expect(deleteFileAsyncSpy).not.toHaveBeenCalled();
+    });
+
+    it('cleans up the temp file when a direct file download misses or fails', async () => {
+      const tryDownloadCacheEntryToFileAsync: jest.Mock<Promise<boolean>, [ITerminal, string, string]> = jest
+        .fn()
+        .mockResolvedValue(false);
+      const subject: OperationBuildCache = prepareDirectTransferSubject({
+        tryDownloadCacheEntryToFileAsync
+      });
+      const terminal: Terminal = new Terminal(new StringBufferTerminalProvider());
+
+      const deleteFileAsyncSpy: jest.SpyInstance = jest
+        .spyOn(FileSystem, 'deleteFileAsync')
+        .mockResolvedValue();
+
+      const result: boolean = await subject.tryRestoreFromCacheAsync(terminal);
+
+      expect(result).toBe(false);
+      expect(tryDownloadCacheEntryToFileAsync).toHaveBeenCalledTimes(1);
+      const [, , tempPath]: [ITerminal, string, string] = tryDownloadCacheEntryToFileAsync.mock.calls[0];
+      expect(tempPath).toMatch(/^\/cache\/acme-wizard-cache-entry-[0-9a-f]+\.temp$/);
+      expect(deleteFileAsyncSpy).toHaveBeenCalledWith(tempPath);
+    });
+  });
+
   describe('AppleDouble file exclusion', () => {
     const originalPlatform: NodeJS.Platform = process.platform;
 
