Address review comments: fix response.resume race, stream retry bug, error logging, stream cleanup, and nits

Agent-Logs-Url: https://github.com/microsoft/rushstack/sessions/d0190bf7-a346-4a71-93bf-d5375d98b552

Co-authored-by: iclanton <5010588+iclanton@users.noreply.github.com>

Author: Copilot

libraries/rush-lib/src/logic/buildCache/OperationBuildCache.ts

@@ -190,6 +190,7 @@ export class OperationBuildCache {
             );
             updateLocalCacheSuccess = true;
           } catch (e) {
+            terminal.writeVerboseLine(`Failed to update local cache: ${e}`);
             updateLocalCacheSuccess = false;
           }
         }
@@ -207,6 +208,7 @@ export class OperationBuildCache {
             );
             updateLocalCacheSuccess = true;
           } catch (e) {
+            terminal.writeVerboseLine(`Failed to update local cache: ${e}`);
             updateLocalCacheSuccess = false;
           }
         }
@@ -353,11 +355,12 @@ export class OperationBuildCache {
       ) {
         // Use streaming upload to avoid loading the entire cache entry into memory
         const entryStream: FileSystemReadStream = FileSystem.createReadStream(localCacheEntryPath);
-        setCloudCacheEntryPromise = this._cloudBuildCacheProvider.trySetCacheEntryStreamAsync(
-          terminal,
-          cacheId,
-          entryStream
-        );
+        setCloudCacheEntryPromise = this._cloudBuildCacheProvider
+          .trySetCacheEntryStreamAsync(terminal, cacheId, entryStream)
+          .catch((e: Error) => {
+            entryStream.destroy();
+            throw e;
+          });
       } else {
         const cacheEntryBuffer: Buffer = await FileSystem.readFileToBufferAsync(localCacheEntryPath);
         setCloudCacheEntryPromise = this._cloudBuildCacheProvider.trySetCacheEntryBufferAsync(

libraries/rush-lib/src/utilities/WebClient.ts

@@ -133,10 +133,10 @@ function _makeRawRequestAsync<TResponse>(
           headers: { location: redirectUrl }
         } = response;
         if (statusCode === 301 || statusCode === 302) {
-          // Drain the redirect response before following
-          response.resume();
           switch (redirect) {
             case 'follow': {
+              // Drain the redirect response since we're discarding it
+              response.resume();
               if (redirectUrl) {
                 requestFnAsync(redirectUrl, options, true).then(resolve).catch(reject);
               } else {
@@ -146,6 +146,7 @@ function _makeRawRequestAsync<TResponse>(
             }
 
             case 'error':
+              response.resume();
               reject(new Error(`Received status code ${statusCode}: ${url}`));
               return;
           }

rush-plugins/rush-azure-storage-build-cache-plugin/src/AzureStorageBuildCacheProvider.ts

@@ -86,10 +86,10 @@ export class AzureStorageBuildCacheProvider
   public async trySetCacheEntryBufferAsync(
     terminal: ITerminal,
     cacheId: string,
-    entryStream: Buffer
+    entryBuffer: Buffer
   ): Promise<boolean> {
     return await this._trySetBlobDataAsync(terminal, cacheId, async (blockBlobClient: BlockBlobClient) => {
-      await blockBlobClient.upload(entryStream, entryStream.length);
+      await blockBlobClient.upload(entryBuffer, entryBuffer.length);
     });
   }
 

rush-plugins/rush-http-build-cache-plugin/src/HttpBuildCacheProvider.ts

@@ -379,7 +379,7 @@ export class HttpBuildCacheProvider implements ICloudBuildCacheProvider {
       headers: headers,
       body: body,
       redirect: 'follow',
-      timeoutMs: 0 // Use the default timeout
+      timeoutMs: 0 // Disable timeout for streaming transfers of large cache entries
     };
 
     const response: IWebClientResponse | IWebClientStreamResponse = stream
@@ -399,15 +399,20 @@ export class HttpBuildCacheProvider implements ICloudBuildCacheProvider {
         typeof credentials !== 'string' &&
         safeCredentialOptions === CredentialsOptions.Optional
       ) {
-        // If we don't already have credentials yet, and we got a response from the server
-        // that is a "normal" failure (4xx), then we assume that credentials are probably
-        // required. Re-attempt the request, requiring credentials this time.
-        //
-        // This counts as part of the "first attempt", so it is not included in the max attempts
-        return await this._makeHttpCoreRequestAsync({
-          ...options,
-          credentialOptions: CredentialsOptions.Required
-        });
+        // Skip credential fallback for stream bodies since the stream has already been consumed
+        // by the first attempt and cannot be replayed.
+        const isStreamBody: boolean = !!body && typeof (body as Readable).pipe === 'function';
+        if (!isStreamBody) {
+          // If we don't already have credentials yet, and we got a response from the server
+          // that is a "normal" failure (4xx), then we assume that credentials are probably
+          // required. Re-attempt the request, requiring credentials this time.
+          //
+          // This counts as part of the "first attempt", so it is not included in the max attempts
+          return await this._makeHttpCoreRequestAsync({
+            ...options,
+            credentialOptions: CredentialsOptions.Required
+          });
+        }
       }
 
       if (options.maxAttempts > 1) {