diff --git a/packages/artifact/__tests__/download.test.ts b/packages/artifact/__tests__/download.test.ts index 81759d46..518cfc59 100644 --- a/packages/artifact/__tests__/download.test.ts +++ b/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(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) diff --git a/packages/artifact/src/internal/download-http-client.ts b/packages/artifact/src/internal/download-http-client.ts index 64631c5c..2110b6a6 100644 --- a/packages/artifact/src/internal/download-http-client.ts +++ b/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 { 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( - response, - destinationStream, - isGzip(response.message.headers) - ) - } else if (isRetryableStatusCode(response.message.statusCode)) { + try { + const isGzipped = isGzip(response.message.headers) + await this.pipeResponseToFile(response, destinationStream, isGzipped) + + if ( + 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( diff --git a/packages/artifact/src/internal/utils.ts b/packages/artifact/src/internal/utils.ts index a9f76b24..ea203644 100644 --- a/packages/artifact/src/internal/utils.ts +++ b/packages/artifact/src/internal/utils.ts @@ -303,6 +303,15 @@ export async function createEmptyFilesForArtifact( } } +export async function getFileSize(filePath: string): Promise { + const stats = await fs.stat(filePath) + return stats.size +} + +export async function rmFile(filePath: string): Promise { + await fs.unlink(filePath) +} + export function getProperRetention( retentionInput: number, retentionSetting: string | undefined