Adding retry when we received 200

If we received 200, we will attempt to download the stream. If the
stream is gzipped, gzip should throw an error when trying to decompress
the stream; or if the stream is truncated, the received bytes should be
different from the value set in content-length header.

packages/artifact/__tests__/download.test.ts

@@ -124,7 +124,7 @@ describe('Download Tests', () => {
     )
     const targetPath = path.join(root, 'FileA.txt')
 
-    setupDownloadItemResponse(true, 200, false, fileContents)
+    setupDownloadItemResponse(true, 200, false, fileContents, false)
     const downloadHttpClient = new DownloadHttpClient()
 
     const items: DownloadItem[] = []
@@ -147,7 +147,7 @@ describe('Download Tests', () => {
     )
     const targetPath = path.join(root, 'FileB.txt')
 
-    setupDownloadItemResponse(false, 200, false, fileContents)
+    setupDownloadItemResponse(false, 200, false, fileContents, false)
     const downloadHttpClient = new DownloadHttpClient()
 
     const items: DownloadItem[] = []
@@ -171,7 +171,7 @@ describe('Download Tests', () => {
       const fileContents = Buffer.from('try, try again\n', defaultEncoding)
       const targetPath = path.join(root, `FileC-${statusCode}.txt`)
 
-      setupDownloadItemResponse(false, statusCode, false, fileContents)
+      setupDownloadItemResponse(false, statusCode, false, fileContents, true)
       const downloadHttpClient = new DownloadHttpClient()
 
       const items: DownloadItem[] = []
@@ -195,7 +195,8 @@ describe('Download Tests', () => {
     )
     const targetPath = path.join(root, 'FileD.txt')
 
-    setupDownloadItemResponse(true, 200, true, fileContents)
+    setupDownloadItemResponse(true, 200, true, fileContents, true)
+    setupThrowWhenReadingStream()
     const downloadHttpClient = new DownloadHttpClient()
 
     const items: DownloadItem[] = []
@@ -218,7 +219,7 @@ describe('Download Tests', () => {
     )
     const targetPath = path.join(root, 'FileE.txt')
 
-    setupDownloadItemResponse(false, 200, true, fileContents)
+    setupDownloadItemResponse(false, 200, true, fileContents, true)
     const downloadHttpClient = new DownloadHttpClient()
 
     const items: DownloadItem[] = []
@@ -291,6 +292,18 @@ describe('Download Tests', () => {
     })
   }
 
+  function setupThrowWhenReadingStream(): void {
+    const spyInstance = jest
+      .spyOn(DownloadHttpClient.prototype, 'pipeResponseToFile')
+      .mockImplementationOnce(async () => {
+        return new Promise<void>(resolve => {
+          throw new Error(
+            'simulate GZip reading truncated buffer and throw Z_BUF_ERROR'
+          )
+        })
+      })
+  }
+
   /**
    * Setups up HTTP GET response for downloading items
    * @param isGzip is the downloaded item gzip encoded
@@ -300,7 +313,8 @@ describe('Download Tests', () => {
     isGzip: boolean,
     firstHttpResponseCode: number,
     truncateFirstResponse: boolean,
-    fileContents: Buffer
+    fileContents: Buffer,
+    retryExpected: boolean
   ): void {
     const spyInstance = jest
       .spyOn(HttpClient.prototype, 'get')
@@ -334,7 +348,7 @@ describe('Download Tests', () => {
       })
 
     // set up a second mock only if we expect a retry. Otherwise this mock will affect other tests.
-    if (firstHttpResponseCode !== 200) {
+    if (retryExpected) {
       spyInstance.mockImplementationOnce(async () => {
         // chained response, if the HTTP GET function gets called again, return a successful response
         const fullResponse = await constructResponse(isGzip, fileContents)

packages/artifact/src/internal/download-http-client.ts

@@ -9,7 +9,9 @@ import {
   isThrottledStatusCode,
   getExponentialRetryTimeInMilliseconds,
   tryGetRetryAfterValueTimeInMilliseconds,
-  displayHttpDiagnostics
+  displayHttpDiagnostics,
+  getFileSize,
+  rmFile
 } from './utils'
 import {URL} from 'url'
 import {StatusReporter} from './status-reporter'
@@ -151,7 +153,7 @@ export class DownloadHttpClient {
   ): Promise<void> {
     let retryCount = 0
     const retryLimit = getRetryLimit()
-    const destinationStream = fs.createWriteStream(downloadPath)
+    let destinationStream = fs.createWriteStream(downloadPath)
     const headers = getDownloadHeaders('application/json', true, true)
 
     // a single GET request is used to download a file
@@ -201,6 +203,27 @@ export class DownloadHttpClient {
       }
     }
 
+    const isAllBytesReceived = (
+      expected?: string,
+      received?: number
+    ): boolean => {
+      // be lenient, if any input is missing, assume success, i.e. not truncated
+      if (
+        !expected ||
+        !received ||
+        process.env['ACTIONS_ARTIFACT_SKIP_DOWNLOAD_VALIDATION']
+      ) {
+        return true
+      }
+
+      return parseInt(expected) === received
+    }
+
+    const resetDestinationStream = async (downloadPath: string) => {
+      await rmFile(downloadPath)
+      destinationStream = fs.createWriteStream(downloadPath)
+    }
+
     // keep trying to download a file until a retry limit has been reached
     while (retryCount <= retryLimit) {
       let response: IHttpClientResponse
@@ -217,19 +240,37 @@ export class DownloadHttpClient {
         continue
       }
 
+      let forceRetry = false
       if (isSuccessStatusCode(response.message.statusCode)) {
         // The body contains the contents of the file however calling response.readBody() causes all the content to be converted to a string
         // which can cause some gzip encoded data to be lost
         // Instead of using response.readBody(), response.message is a readableStream that can be directly used to get the raw body contents
-        return this.pipeResponseToFile(
+        try {
-          response,
+          const isGzipped = isGzip(response.message.headers)
-          destinationStream,
+          await this.pipeResponseToFile(response, destinationStream, isGzipped)
-          isGzip(response.message.headers)
+
-        )
+          if (
-      } else if (isRetryableStatusCode(response.message.statusCode)) {
+            isGzipped ||
+            isAllBytesReceived(
+              response.message.headers['content-length'],
+              await getFileSize(downloadPath)
+            )
+          ) {
+            return
+          } else {
+            forceRetry = true
+          }
+        } catch (error) {
+          // retry on error, most likely streams were corrupted
+          forceRetry = true
+        }
+      }
+
+      if (forceRetry || isRetryableStatusCode(response.message.statusCode)) {
         core.info(
           `A ${response.message.statusCode} response code has been received while attempting to download an artifact`
         )
+        resetDestinationStream(downloadPath)
         // if a throttled status code is received, try to get the retryAfter header value, else differ to standard exponential backoff
         isThrottledStatusCode(response.message.statusCode)
           ? await backOff(

packages/artifact/src/internal/utils.ts

@@ -303,6 +303,15 @@ export async function createEmptyFilesForArtifact(
   }
 }
 
+export async function getFileSize(filePath: string): Promise<number> {
+  const stats = await fs.stat(filePath)
+  return stats.size
+}
+
+export async function rmFile(filePath: string): Promise<void> {
+  await fs.unlink(filePath)
+}
+
 export function getProperRetention(
   retentionInput: number,
   retentionSetting: string | undefined