From ef7a575d01e07e79a03ebb37e341116f41183c6a Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Wed, 16 Dec 2020 17:28:10 -0500 Subject: [PATCH 01/13] Retry all http calls for artifact upload and download --- packages/artifact/__tests__/download.test.ts | 4 +- packages/artifact/__tests__/retry.test.ts | 116 ++++++++++++++++++ packages/artifact/__tests__/upload.test.ts | 8 +- .../src/internal/download-http-client.ts | 33 +++-- .../artifact/src/internal/requestUtils.ts | 74 +++++++++++ .../src/internal/upload-http-client.ts | 83 ++++++------- packages/artifact/src/internal/utils.ts | 8 +- 7 files changed, 260 insertions(+), 66 deletions(-) create mode 100644 packages/artifact/__tests__/retry.test.ts create mode 100644 packages/artifact/src/internal/requestUtils.ts diff --git a/packages/artifact/__tests__/download.test.ts b/packages/artifact/__tests__/download.test.ts index b77e5a2bc3..05e5707108 100644 --- a/packages/artifact/__tests__/download.test.ts +++ b/packages/artifact/__tests__/download.test.ts @@ -71,7 +71,7 @@ describe('Download Tests', () => { setupFailedResponse() const downloadHttpClient = new DownloadHttpClient() expect(downloadHttpClient.listArtifacts()).rejects.toThrow( - 'Unable to list artifacts for the run' + 'List Artifacts failed: Artifact service responded with 500' ) }) @@ -113,7 +113,7 @@ describe('Download Tests', () => { configVariables.getRuntimeUrl() ) ).rejects.toThrow( - `Unable to get ContainersItems from ${configVariables.getRuntimeUrl()}` + `Get Container Items failed: Artifact service responded with 500` ) }) diff --git a/packages/artifact/__tests__/retry.test.ts b/packages/artifact/__tests__/retry.test.ts new file mode 100644 index 0000000000..1f16700f12 --- /dev/null +++ b/packages/artifact/__tests__/retry.test.ts @@ -0,0 +1,116 @@ +import {retry} from '../src/internal/requestUtils' +import * as core from '@actions/core' + +interface ITestResponse { + statusCode: number + result: string | null + error: Error | null +} + +function TestResponse( + action: number | Error, + result: string | null = null +): ITestResponse { + if (action instanceof Error) { + return { + statusCode: -1, + result, + error: action + } + } else { + return { + statusCode: action, + result, + error: null + } + } +} + +async function handleResponse( + response: ITestResponse | undefined +): Promise { + if (!response) { + // eslint-disable-next-line no-undef + fail('Retry method called too many times') + } + + if (response.error) { + throw response.error + } else { + return Promise.resolve(response) + } +} + +async function testRetryExpectingResult( + responses: ITestResponse[], + expectedResult: string | null +): Promise { + responses = responses.reverse() // Reverse responses since we pop from end + + const actualResult = await retry( + 'test', + async () => handleResponse(responses.pop()), + (response: ITestResponse) => response.statusCode, + new Map(), // extra error message for any particular http codes + 2, // maxAttempts + 0 // delay + ) + + expect(actualResult.result).toEqual(expectedResult) +} + +async function testRetryExpectingError( + responses: ITestResponse[] +): Promise { + responses = responses.reverse() // Reverse responses since we pop from end + + expect( + retry( + 'test', + async () => handleResponse(responses.pop()), + (response: ITestResponse) => response.statusCode, + new Map(), // extra error message for any particular http codes + 2, // maxAttempts, + 0 // delay + ) + ).rejects.toBeInstanceOf(Error) +} + +beforeAll(async () => { + // mock all output so that there is less noise when running tests + jest.spyOn(console, 'log').mockImplementation(() => {}) + jest.spyOn(core, 'debug').mockImplementation(() => {}) + jest.spyOn(core, 'info').mockImplementation(() => {}) + jest.spyOn(core, 'warning').mockImplementation(() => {}) + jest.spyOn(core, 'error').mockImplementation(() => {}) +}) + +test('retry works on successful response', async () => { + await testRetryExpectingResult([TestResponse(200, 'Ok')], 'Ok') +}) + +test('retry works after retryable status code', async () => { + await testRetryExpectingResult( + [TestResponse(503), TestResponse(200, 'Ok')], + 'Ok' + ) +}) + +test('retry fails after exhausting retries', async () => { + await testRetryExpectingError([ + TestResponse(503), + TestResponse(503), + TestResponse(200, 'Ok') + ]) +}) + +test('retry fails after non-retryable status code', async () => { + await testRetryExpectingError([TestResponse(500), TestResponse(200, 'Ok')]) +}) + +test('retry works after error', async () => { + await testRetryExpectingResult( + [TestResponse(new Error('Test error')), TestResponse(200, 'Ok')], + 'Ok' + ) +}) diff --git a/packages/artifact/__tests__/upload.test.ts b/packages/artifact/__tests__/upload.test.ts index f473b2e0f2..c70eec1af6 100644 --- a/packages/artifact/__tests__/upload.test.ts +++ b/packages/artifact/__tests__/upload.test.ts @@ -102,7 +102,7 @@ describe('Upload Tests', () => { uploadHttpClient.createArtifactInFileContainer(artifactName) ).rejects.toEqual( new Error( - `Unable to create a container for the artifact invalid-artifact-name at ${getArtifactUrl()}` + `Create Artifact Container failed: Artifact service responded with 400 : The artifact name invalid-artifact-name is not valid. Request URL ${getArtifactUrl()}` ) ) }) @@ -125,7 +125,7 @@ describe('Upload Tests', () => { uploadHttpClient.createArtifactInFileContainer(artifactName) ).rejects.toEqual( new Error( - 'Artifact storage quota has been hit. Unable to upload any new artifacts' + 'Create Artifact Container failed: Artifact service responded with 403 : Artifact storage quota has been hit. Unable to upload any new artifacts' ) ) }) @@ -362,7 +362,9 @@ describe('Upload Tests', () => { const uploadHttpClient = new UploadHttpClient() expect( uploadHttpClient.patchArtifactSize(-2, 'my-artifact') - ).rejects.toThrow('Unable to finish uploading artifact my-artifact') + ).rejects.toThrow( + 'Patch Artifact Size failed: Artifact service responded with 400' + ) }) /** diff --git a/packages/artifact/src/internal/download-http-client.ts b/packages/artifact/src/internal/download-http-client.ts index e090c54732..20d648b0c0 100644 --- a/packages/artifact/src/internal/download-http-client.ts +++ b/packages/artifact/src/internal/download-http-client.ts @@ -11,7 +11,8 @@ import { tryGetRetryAfterValueTimeInMilliseconds, displayHttpDiagnostics, getFileSize, - rmFile + rmFile, + sleep } from './utils' import {URL} from 'url' import {StatusReporter} from './status-reporter' @@ -22,6 +23,7 @@ import {HttpManager} from './http-manager' import {DownloadItem} from './download-specification' import {getDownloadFileConcurrency, getRetryLimit} from './config-variables' import {IncomingHttpHeaders} from 'http' +import {retryHttpClientResponse} from './requestUtils' export class DownloadHttpClient { // http manager is used for concurrent connections when downloading multiple files at once @@ -46,16 +48,11 @@ export class DownloadHttpClient { // use the first client from the httpManager, `keep-alive` is not used so the connection will close immediately const client = this.downloadHttpManager.getClient(0) const headers = getDownloadHeaders('application/json') - const response = await client.get(artifactUrl, headers) - const body: string = await response.readBody() - - if (isSuccessStatusCode(response.message.statusCode) && body) { - return JSON.parse(body) - } - displayHttpDiagnostics(response) - throw new Error( - `Unable to list artifacts for the run. Resource Url ${artifactUrl}` + const response = await retryHttpClientResponse('List Artifacts', async () => + client.get(artifactUrl, headers) ) + const body: string = await response.readBody() + return JSON.parse(body) } /** @@ -74,14 +71,12 @@ export class DownloadHttpClient { // use the first client from the httpManager, `keep-alive` is not used so the connection will close immediately const client = this.downloadHttpManager.getClient(0) const headers = getDownloadHeaders('application/json') - const response = await client.get(resourceUrl.toString(), headers) + const response = await retryHttpClientResponse( + 'Get Container Items', + async () => client.get(resourceUrl.toString(), headers) + ) const body: string = await response.readBody() - - if (isSuccessStatusCode(response.message.statusCode) && body) { - return JSON.parse(body) - } - displayHttpDiagnostics(response) - throw new Error(`Unable to get ContainersItems from ${resourceUrl}`) + return JSON.parse(body) } /** @@ -188,14 +183,14 @@ export class DownloadHttpClient { core.info( `Backoff due to too many requests, retry #${retryCount}. Waiting for ${retryAfterValue} milliseconds before continuing the download` ) - await new Promise(resolve => setTimeout(resolve, retryAfterValue)) + await sleep(retryAfterValue) } else { // Back off using an exponential value that depends on the retry count const backoffTime = getExponentialRetryTimeInMilliseconds(retryCount) core.info( `Exponential backoff for retry #${retryCount}. Waiting for ${backoffTime} milliseconds before continuing the download` ) - await new Promise(resolve => setTimeout(resolve, backoffTime)) + await sleep(backoffTime) } core.info( `Finished backoff for retry #${retryCount}, continuing with download` diff --git a/packages/artifact/src/internal/requestUtils.ts b/packages/artifact/src/internal/requestUtils.ts new file mode 100644 index 0000000000..5096f7d893 --- /dev/null +++ b/packages/artifact/src/internal/requestUtils.ts @@ -0,0 +1,74 @@ +import {IHttpClientResponse} from '@actions/http-client/interfaces' +import {isRetryableStatusCode, isSuccessStatusCode, sleep} from './utils' +import * as core from '@actions/core' + +export async function retry( + name: string, + method: () => Promise, + getStatusCode: (response: T) => number | undefined, + errorMessages: Map, + maxAttempts: number, + delay: number +): Promise { + let response: T | undefined = undefined + let statusCode: number | undefined = undefined + let isRetryable = false + let errorMessage = '' + let extraErrorInformation: string | undefined = undefined + let attempt = 1 + + while (attempt <= maxAttempts) { + try { + response = await method() + statusCode = getStatusCode(response) + + if (isSuccessStatusCode(statusCode)) { + return response + } + + // Extra error information that we want to display if a particular response code is hit + if (statusCode) { + extraErrorInformation = errorMessages.get(statusCode) + } + + isRetryable = isRetryableStatusCode(statusCode) + errorMessage = `Artifact service responded with ${statusCode}` + } catch (error) { + isRetryable = true + errorMessage = error.message + } + + if (!isRetryable) { + core.info(`${name} - Error is not retryable`) + break + } + + core.info( + `${name} - Attempt ${attempt} of ${maxAttempts} failed with error: ${errorMessage}` + ) + + await sleep(delay) + attempt++ + } + + if (extraErrorInformation) { + throw Error(`${name} failed: ${errorMessage} : ${extraErrorInformation}`) + } + throw Error(`${name} failed: ${errorMessage}`) +} + +export async function retryHttpClientResponse( + name: string, + method: () => Promise, + errorMessages: Map = new Map(), + maxAttempts = 3 +): Promise { + return await retry( + name, + method, + (response: IHttpClientResponse) => response.message.statusCode, + errorMessages, + maxAttempts, + 5000 + ) +} diff --git a/packages/artifact/src/internal/upload-http-client.ts b/packages/artifact/src/internal/upload-http-client.ts index b77e5fd0a2..9cddf63fd9 100644 --- a/packages/artifact/src/internal/upload-http-client.ts +++ b/packages/artifact/src/internal/upload-http-client.ts @@ -15,11 +15,11 @@ import { isRetryableStatusCode, isSuccessStatusCode, isThrottledStatusCode, - isForbiddenStatusCode, displayHttpDiagnostics, getExponentialRetryTimeInMilliseconds, tryGetRetryAfterValueTimeInMilliseconds, - getProperRetention + getProperRetention, + sleep } from './utils' import { getUploadChunkSize, @@ -31,12 +31,13 @@ import {promisify} from 'util' import {URL} from 'url' import {performance} from 'perf_hooks' import {StatusReporter} from './status-reporter' -import {HttpClientResponse} from '@actions/http-client/index' +import {HttpCodes} from '@actions/http-client' import {IHttpClientResponse} from '@actions/http-client/interfaces' import {HttpManager} from './http-manager' import {UploadSpecification} from './upload-specification' import {UploadOptions} from './upload-options' import {createGZipFileOnDisk, createGZipFileInBuffer} from './upload-gzip' +import {retryHttpClientResponse} from './requestUtils' const stat = promisify(fs.stat) export class UploadHttpClient { @@ -80,23 +81,28 @@ export class UploadHttpClient { // use the first client from the httpManager, `keep-alive` is not used so the connection will close immediately const client = this.uploadHttpManager.getClient(0) const headers = getUploadHeaders('application/json', false) - const rawResponse = await client.post(artifactUrl, data, headers) - const body: string = await rawResponse.readBody() - - if (isSuccessStatusCode(rawResponse.message.statusCode) && body) { - return JSON.parse(body) - } else if (isForbiddenStatusCode(rawResponse.message.statusCode)) { - // if a 403 is returned when trying to create a file container, the customer has exceeded - // their storage quota so no new artifact containers can be created - throw new Error( - `Artifact storage quota has been hit. Unable to upload any new artifacts` - ) - } else { - displayHttpDiagnostics(rawResponse) - throw new Error( - `Unable to create a container for the artifact ${artifactName} at ${artifactUrl}` - ) - } + + // Extra information to display when a particular HTTP code is returned + // If a 403 is returned when trying to create a file container, the customer has exceeded + // their storage quota so no new artifact containers can be created + const errorMessages: Map = new Map([ + [ + HttpCodes.Forbidden, + 'Artifact storage quota has been hit. Unable to upload any new artifacts' + ], + [ + HttpCodes.BadRequest, + `The artifact name ${artifactName} is not valid. Request URL ${artifactUrl}` + ] + ]) + + const response = await retryHttpClientResponse( + 'Create Artifact Container', + async () => client.post(artifactUrl, data, headers), + errorMessages + ) + const body: string = await response.readBody() + return JSON.parse(body) } /** @@ -417,13 +423,13 @@ export class UploadHttpClient { core.info( `Backoff due to too many requests, retry #${retryCount}. Waiting for ${retryAfterValue} milliseconds before continuing the upload` ) - await new Promise(resolve => setTimeout(resolve, retryAfterValue)) + await sleep(retryAfterValue) } else { const backoffTime = getExponentialRetryTimeInMilliseconds(retryCount) core.info( `Exponential backoff for retry #${retryCount}. Waiting for ${backoffTime} milliseconds before continuing the upload at offset ${start}` ) - await new Promise(resolve => setTimeout(resolve, backoffTime)) + await sleep(backoffTime) } core.info( `Finished backoff for retry #${retryCount}, continuing with upload` @@ -486,7 +492,6 @@ export class UploadHttpClient { * Updating the size indicates that we are done uploading all the contents of the artifact */ async patchArtifactSize(size: number, artifactName: string): Promise { - const headers = getUploadHeaders('application/json', false) const resourceUrl = new URL(getArtifactUrl()) resourceUrl.searchParams.append('artifactName', artifactName) @@ -496,25 +501,21 @@ export class UploadHttpClient { // use the first client from the httpManager, `keep-alive` is not used so the connection will close immediately const client = this.uploadHttpManager.getClient(0) - const response: HttpClientResponse = await client.patch( - resourceUrl.toString(), - data, - headers + const headers = getUploadHeaders('application/json', false) + + // Extra information to display when a particular HTTP code is returned + const errorMessages: Map = new Map([ + [ + HttpCodes.NotFound, + `An Artifact with the name ${artifactName} was not found` + ] + ]) + + await retryHttpClientResponse( + 'Patch Artifact Size', + async () => client.patch(resourceUrl.toString(), data, headers), + errorMessages ) - const body: string = await response.readBody() - if (isSuccessStatusCode(response.message.statusCode)) { - core.debug( - `Artifact ${artifactName} has been successfully uploaded, total size in bytes: ${size}` - ) - } else if (response.message.statusCode === 404) { - throw new Error(`An Artifact with the name ${artifactName} was not found`) - } else { - displayHttpDiagnostics(response) - core.info(body) - throw new Error( - `Unable to finish uploading artifact ${artifactName} to ${resourceUrl}` - ) - } } } diff --git a/packages/artifact/src/internal/utils.ts b/packages/artifact/src/internal/utils.ts index 2f4f2f470a..59a092cab2 100644 --- a/packages/artifact/src/internal/utils.ts +++ b/packages/artifact/src/internal/utils.ts @@ -65,7 +65,9 @@ export function isForbiddenStatusCode(statusCode?: number): boolean { return statusCode === HttpCodes.Forbidden } -export function isRetryableStatusCode(statusCode?: number): boolean { +export function isRetryableStatusCode( + statusCode?: number | undefined +): boolean { if (!statusCode) { return false } @@ -335,3 +337,7 @@ export function getProperRetention( } return retention } + +export async function sleep(milliseconds: number): Promise { + return new Promise(resolve => setTimeout(resolve, milliseconds)) +} \ No newline at end of file From 175b04e8c70929a915ff091dadb189fbee53d96d Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Wed, 16 Dec 2020 17:32:31 -0500 Subject: [PATCH 02/13] Extra debug information --- packages/artifact/src/internal/upload-http-client.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/artifact/src/internal/upload-http-client.ts b/packages/artifact/src/internal/upload-http-client.ts index 9cddf63fd9..3d85565ed7 100644 --- a/packages/artifact/src/internal/upload-http-client.ts +++ b/packages/artifact/src/internal/upload-http-client.ts @@ -516,6 +516,9 @@ export class UploadHttpClient { async () => client.patch(resourceUrl.toString(), data, headers), errorMessages ) + core.debug( + `Artifact ${artifactName} has been successfully uploaded, total size in bytes: ${size}` + ) } } From edbdbf91ba2db1fd54fc8173adacdc73d8dc0b99 Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Wed, 16 Dec 2020 17:35:09 -0500 Subject: [PATCH 03/13] Fix lint --- packages/artifact/src/internal/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/artifact/src/internal/utils.ts b/packages/artifact/src/internal/utils.ts index 59a092cab2..ee83d0bc07 100644 --- a/packages/artifact/src/internal/utils.ts +++ b/packages/artifact/src/internal/utils.ts @@ -340,4 +340,4 @@ export function getProperRetention( export async function sleep(milliseconds: number): Promise { return new Promise(resolve => setTimeout(resolve, milliseconds)) -} \ No newline at end of file +} From 5b7b7aa294d2a3fd68df112d6cf50dec97fa71ca Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Thu, 17 Dec 2020 10:24:57 -0500 Subject: [PATCH 04/13] Always read response body --- packages/artifact/src/internal/upload-http-client.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/artifact/src/internal/upload-http-client.ts b/packages/artifact/src/internal/upload-http-client.ts index 3d85565ed7..7a9204a91a 100644 --- a/packages/artifact/src/internal/upload-http-client.ts +++ b/packages/artifact/src/internal/upload-http-client.ts @@ -511,11 +511,12 @@ export class UploadHttpClient { ] ]) - await retryHttpClientResponse( + const response = await retryHttpClientResponse( 'Patch Artifact Size', async () => client.patch(resourceUrl.toString(), data, headers), errorMessages ) + await response.readBody() core.debug( `Artifact ${artifactName} has been successfully uploaded, total size in bytes: ${size}` ) From f880c0e59eaa945746519b2aebe203e1c2f324c0 Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Thu, 17 Dec 2020 13:14:02 -0500 Subject: [PATCH 05/13] PR Feedback --- packages/artifact/src/internal/download-http-client.ts | 6 +++--- packages/artifact/src/internal/requestUtils.ts | 10 +++++----- packages/artifact/src/internal/upload-http-client.ts | 8 ++++---- packages/artifact/src/internal/utils.ts | 4 +--- 4 files changed, 13 insertions(+), 15 deletions(-) diff --git a/packages/artifact/src/internal/download-http-client.ts b/packages/artifact/src/internal/download-http-client.ts index 20d648b0c0..5f679d2d5d 100644 --- a/packages/artifact/src/internal/download-http-client.ts +++ b/packages/artifact/src/internal/download-http-client.ts @@ -23,7 +23,7 @@ import {HttpManager} from './http-manager' import {DownloadItem} from './download-specification' import {getDownloadFileConcurrency, getRetryLimit} from './config-variables' import {IncomingHttpHeaders} from 'http' -import {retryHttpClientResponse} from './requestUtils' +import {retryHttpClientRequest} from './requestUtils' export class DownloadHttpClient { // http manager is used for concurrent connections when downloading multiple files at once @@ -48,7 +48,7 @@ export class DownloadHttpClient { // use the first client from the httpManager, `keep-alive` is not used so the connection will close immediately const client = this.downloadHttpManager.getClient(0) const headers = getDownloadHeaders('application/json') - const response = await retryHttpClientResponse('List Artifacts', async () => + const response = await retryHttpClientRequest('List Artifacts', async () => client.get(artifactUrl, headers) ) const body: string = await response.readBody() @@ -71,7 +71,7 @@ export class DownloadHttpClient { // use the first client from the httpManager, `keep-alive` is not used so the connection will close immediately const client = this.downloadHttpManager.getClient(0) const headers = getDownloadHeaders('application/json') - const response = await retryHttpClientResponse( + const response = await retryHttpClientRequest( 'Get Container Items', async () => client.get(resourceUrl.toString(), headers) ) diff --git a/packages/artifact/src/internal/requestUtils.ts b/packages/artifact/src/internal/requestUtils.ts index 5096f7d893..a2592b615b 100644 --- a/packages/artifact/src/internal/requestUtils.ts +++ b/packages/artifact/src/internal/requestUtils.ts @@ -4,11 +4,11 @@ import * as core from '@actions/core' export async function retry( name: string, - method: () => Promise, + operation: () => Promise, getStatusCode: (response: T) => number | undefined, errorMessages: Map, maxAttempts: number, - delay: number + delayMilliseconds: number ): Promise { let response: T | undefined = undefined let statusCode: number | undefined = undefined @@ -19,7 +19,7 @@ export async function retry( while (attempt <= maxAttempts) { try { - response = await method() + response = await operation() statusCode = getStatusCode(response) if (isSuccessStatusCode(statusCode)) { @@ -47,7 +47,7 @@ export async function retry( `${name} - Attempt ${attempt} of ${maxAttempts} failed with error: ${errorMessage}` ) - await sleep(delay) + await sleep(delayMilliseconds) attempt++ } @@ -57,7 +57,7 @@ export async function retry( throw Error(`${name} failed: ${errorMessage}`) } -export async function retryHttpClientResponse( +export async function retryHttpClientRequest( name: string, method: () => Promise, errorMessages: Map = new Map(), diff --git a/packages/artifact/src/internal/upload-http-client.ts b/packages/artifact/src/internal/upload-http-client.ts index 7a9204a91a..54a54b7557 100644 --- a/packages/artifact/src/internal/upload-http-client.ts +++ b/packages/artifact/src/internal/upload-http-client.ts @@ -37,7 +37,7 @@ import {HttpManager} from './http-manager' import {UploadSpecification} from './upload-specification' import {UploadOptions} from './upload-options' import {createGZipFileOnDisk, createGZipFileInBuffer} from './upload-gzip' -import {retryHttpClientResponse} from './requestUtils' +import {retryHttpClientRequest} from './requestUtils' const stat = promisify(fs.stat) export class UploadHttpClient { @@ -96,7 +96,7 @@ export class UploadHttpClient { ] ]) - const response = await retryHttpClientResponse( + const response = await retryHttpClientRequest( 'Create Artifact Container', async () => client.post(artifactUrl, data, headers), errorMessages @@ -511,8 +511,8 @@ export class UploadHttpClient { ] ]) - const response = await retryHttpClientResponse( - 'Patch Artifact Size', + const response = await retryHttpClientRequest( + 'Finalize Artifact Upload', async () => client.patch(resourceUrl.toString(), data, headers), errorMessages ) diff --git a/packages/artifact/src/internal/utils.ts b/packages/artifact/src/internal/utils.ts index ee83d0bc07..9e1b0123b6 100644 --- a/packages/artifact/src/internal/utils.ts +++ b/packages/artifact/src/internal/utils.ts @@ -65,9 +65,7 @@ export function isForbiddenStatusCode(statusCode?: number): boolean { return statusCode === HttpCodes.Forbidden } -export function isRetryableStatusCode( - statusCode?: number | undefined -): boolean { +export function isRetryableStatusCode(statusCode: number | undefined): boolean { if (!statusCode) { return false } From b20b44f66884b2803dda2221c400006c0f45757f Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Thu, 17 Dec 2020 13:51:45 -0500 Subject: [PATCH 06/13] Change error message if patch call fails --- packages/artifact/__tests__/upload.test.ts | 2 +- packages/artifact/src/internal/upload-http-client.ts | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/artifact/__tests__/upload.test.ts b/packages/artifact/__tests__/upload.test.ts index c70eec1af6..e36818abe5 100644 --- a/packages/artifact/__tests__/upload.test.ts +++ b/packages/artifact/__tests__/upload.test.ts @@ -363,7 +363,7 @@ describe('Upload Tests', () => { expect( uploadHttpClient.patchArtifactSize(-2, 'my-artifact') ).rejects.toThrow( - 'Patch Artifact Size failed: Artifact service responded with 400' + 'Finalize artifact upload failed: Artifact service responded with 400' ) }) diff --git a/packages/artifact/src/internal/upload-http-client.ts b/packages/artifact/src/internal/upload-http-client.ts index 54a54b7557..26533b78f6 100644 --- a/packages/artifact/src/internal/upload-http-client.ts +++ b/packages/artifact/src/internal/upload-http-client.ts @@ -511,10 +511,12 @@ export class UploadHttpClient { ] ]) + // TODO retry for all possible response codes, the artifact upload is pretty much complete so it at all costs we should try to finish this const response = await retryHttpClientRequest( - 'Finalize Artifact Upload', + 'Finalize artifact upload', async () => client.patch(resourceUrl.toString(), data, headers), - errorMessages + errorMessages, + 5 ) await response.readBody() core.debug( From 1bcfb42ea312c2640cde7d04a81b6c03c19025b5 Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Thu, 17 Dec 2020 15:37:26 -0500 Subject: [PATCH 07/13] Add exponential backoff when retrying --- packages/artifact/__tests__/retry.test.ts | 8 ++++---- packages/artifact/src/internal/requestUtils.ts | 18 +++++++++++------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/packages/artifact/__tests__/retry.test.ts b/packages/artifact/__tests__/retry.test.ts index 1f16700f12..c804623f57 100644 --- a/packages/artifact/__tests__/retry.test.ts +++ b/packages/artifact/__tests__/retry.test.ts @@ -1,6 +1,8 @@ import {retry} from '../src/internal/requestUtils' import * as core from '@actions/core' +jest.mock('../src/internal/config-variables') + interface ITestResponse { statusCode: number result: string | null @@ -52,8 +54,7 @@ async function testRetryExpectingResult( async () => handleResponse(responses.pop()), (response: ITestResponse) => response.statusCode, new Map(), // extra error message for any particular http codes - 2, // maxAttempts - 0 // delay + 2 // maxAttempts ) expect(actualResult.result).toEqual(expectedResult) @@ -70,8 +71,7 @@ async function testRetryExpectingError( async () => handleResponse(responses.pop()), (response: ITestResponse) => response.statusCode, new Map(), // extra error message for any particular http codes - 2, // maxAttempts, - 0 // delay + 2 // maxAttempts, ) ).rejects.toBeInstanceOf(Error) } diff --git a/packages/artifact/src/internal/requestUtils.ts b/packages/artifact/src/internal/requestUtils.ts index a2592b615b..071a655e07 100644 --- a/packages/artifact/src/internal/requestUtils.ts +++ b/packages/artifact/src/internal/requestUtils.ts @@ -1,14 +1,19 @@ import {IHttpClientResponse} from '@actions/http-client/interfaces' -import {isRetryableStatusCode, isSuccessStatusCode, sleep} from './utils' +import { + isRetryableStatusCode, + isSuccessStatusCode, + sleep, + getExponentialRetryTimeInMilliseconds +} from './utils' import * as core from '@actions/core' +import {getRetryLimit} from './config-variables' export async function retry( name: string, operation: () => Promise, getStatusCode: (response: T) => number | undefined, errorMessages: Map, - maxAttempts: number, - delayMilliseconds: number + maxAttempts: number ): Promise { let response: T | undefined = undefined let statusCode: number | undefined = undefined @@ -47,7 +52,7 @@ export async function retry( `${name} - Attempt ${attempt} of ${maxAttempts} failed with error: ${errorMessage}` ) - await sleep(delayMilliseconds) + await sleep(getExponentialRetryTimeInMilliseconds(attempt)) attempt++ } @@ -61,14 +66,13 @@ export async function retryHttpClientRequest( name: string, method: () => Promise, errorMessages: Map = new Map(), - maxAttempts = 3 + maxAttempts = getRetryLimit() ): Promise { return await retry( name, method, (response: IHttpClientResponse) => response.message.statusCode, errorMessages, - maxAttempts, - 5000 + maxAttempts ) } From f98d4c4d9b6ca7dc2ff0683610ff4373bcd56cd0 Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Fri, 18 Dec 2020 14:49:03 -0500 Subject: [PATCH 08/13] Rework tests and add diagnostic info if exception thrown --- packages/artifact/__tests__/retry.test.ts | 160 +++++++++--------- .../artifact/src/internal/requestUtils.ts | 26 ++- 2 files changed, 91 insertions(+), 95 deletions(-) diff --git a/packages/artifact/__tests__/retry.test.ts b/packages/artifact/__tests__/retry.test.ts index c804623f57..baea719175 100644 --- a/packages/artifact/__tests__/retry.test.ts +++ b/packages/artifact/__tests__/retry.test.ts @@ -1,79 +1,55 @@ -import {retry} from '../src/internal/requestUtils' +import * as http from 'http' +import * as net from 'net' import * as core from '@actions/core' +import * as configVariables from '../src/internal/config-variables' +import {retry} from '../src/internal/requestUtils' +import {IHttpClientResponse} from '@actions/http-client/interfaces' +import {HttpClientResponse} from '@actions/http-client' jest.mock('../src/internal/config-variables') -interface ITestResponse { - statusCode: number - result: string | null - error: Error | null +interface ITestResult { + responseCode: number + errorMessage: string | null } -function TestResponse( - action: number | Error, - result: string | null = null -): ITestResponse { - if (action instanceof Error) { - return { - statusCode: -1, - result, - error: action - } +async function testRetry( + responseCodes: number[], + expectedResult: ITestResult +): Promise { + const reverse = responseCodes.reverse() // Reverse responses since we pop from end + if (expectedResult.errorMessage) { + // we expect some exception to be thrown + expect( + retry( + 'test', + async () => handleResponse(reverse.pop()), + new Map(), // extra error message for any particular http codes + configVariables.getRetryLimit() + ) + ).rejects.toThrow(expectedResult.errorMessage) } else { - return { - statusCode: action, - result, - error: null - } + // we expect a correct status code to be returned + const actualResult = await retry( + 'test', + async () => handleResponse(reverse.pop()), + new Map(), // extra error message for any particular http codes + configVariables.getRetryLimit() + ) + expect(actualResult.message.statusCode).toEqual(expectedResult.responseCode) } } async function handleResponse( - response: ITestResponse | undefined -): Promise { - if (!response) { - // eslint-disable-next-line no-undef - fail('Retry method called too many times') - } - - if (response.error) { - throw response.error - } else { - return Promise.resolve(response) + testResponseCode: number | undefined +): Promise { + if (!testResponseCode) { + fail( + 'Test incorrectly set up. reverse.pop() was called too many times so not enough test response codes were supplied' + ) } -} - -async function testRetryExpectingResult( - responses: ITestResponse[], - expectedResult: string | null -): Promise { - responses = responses.reverse() // Reverse responses since we pop from end - - const actualResult = await retry( - 'test', - async () => handleResponse(responses.pop()), - (response: ITestResponse) => response.statusCode, - new Map(), // extra error message for any particular http codes - 2 // maxAttempts - ) - expect(actualResult.result).toEqual(expectedResult) -} - -async function testRetryExpectingError( - responses: ITestResponse[] -): Promise { - responses = responses.reverse() // Reverse responses since we pop from end - - expect( - retry( - 'test', - async () => handleResponse(responses.pop()), - (response: ITestResponse) => response.statusCode, - new Map(), // extra error message for any particular http codes - 2 // maxAttempts, - ) - ).rejects.toBeInstanceOf(Error) + return setupSingleMockResponse(testResponseCode) } beforeAll(async () => { @@ -85,32 +61,54 @@ beforeAll(async () => { jest.spyOn(core, 'error').mockImplementation(() => {}) }) +/** + * Helpers used to setup mocking for the HttpClient + */ +async function emptyMockReadBody(): Promise { + return new Promise(resolve => { + resolve() + }) +} + +function setupSingleMockResponse( + statusCode: number +): Promise { + const mockMessage = new http.IncomingMessage(new net.Socket()) + const mockReadBody = emptyMockReadBody + mockMessage.statusCode = statusCode + return new Promise(resolve => { + resolve({ + message: mockMessage, + readBody: mockReadBody + }) + }) +} + test('retry works on successful response', async () => { - await testRetryExpectingResult([TestResponse(200, 'Ok')], 'Ok') + await testRetry([200], { + responseCode: 200, + errorMessage: null + }) }) test('retry works after retryable status code', async () => { - await testRetryExpectingResult( - [TestResponse(503), TestResponse(200, 'Ok')], - 'Ok' - ) + await testRetry([503, 200], { + responseCode: 200, + errorMessage: null + }) }) test('retry fails after exhausting retries', async () => { - await testRetryExpectingError([ - TestResponse(503), - TestResponse(503), - TestResponse(200, 'Ok') - ]) + // __mocks__/config-variables caps the max retry count in tests to 2 + await testRetry([503, 503, 200], { + responseCode: 200, + errorMessage: 'test failed: Artifact service responded with 503' + }) }) test('retry fails after non-retryable status code', async () => { - await testRetryExpectingError([TestResponse(500), TestResponse(200, 'Ok')]) -}) - -test('retry works after error', async () => { - await testRetryExpectingResult( - [TestResponse(new Error('Test error')), TestResponse(200, 'Ok')], - 'Ok' - ) + await testRetry([500, 200], { + responseCode: 500, + errorMessage: 'test failed: Artifact service responded with 500' + }) }) diff --git a/packages/artifact/src/internal/requestUtils.ts b/packages/artifact/src/internal/requestUtils.ts index 071a655e07..d3cec50d93 100644 --- a/packages/artifact/src/internal/requestUtils.ts +++ b/packages/artifact/src/internal/requestUtils.ts @@ -3,19 +3,19 @@ import { isRetryableStatusCode, isSuccessStatusCode, sleep, - getExponentialRetryTimeInMilliseconds + getExponentialRetryTimeInMilliseconds, + displayHttpDiagnostics } from './utils' import * as core from '@actions/core' import {getRetryLimit} from './config-variables' -export async function retry( +export async function retry( name: string, - operation: () => Promise, - getStatusCode: (response: T) => number | undefined, + operation: () => Promise, errorMessages: Map, maxAttempts: number -): Promise { - let response: T | undefined = undefined +): Promise { + let response: IHttpClientResponse | undefined = undefined let statusCode: number | undefined = undefined let isRetryable = false let errorMessage = '' @@ -25,7 +25,7 @@ export async function retry( while (attempt <= maxAttempts) { try { response = await operation() - statusCode = getStatusCode(response) + statusCode = response.message.statusCode if (isSuccessStatusCode(statusCode)) { return response @@ -56,6 +56,10 @@ export async function retry( attempt++ } + if (response) { + displayHttpDiagnostics(response) + } + if (extraErrorInformation) { throw Error(`${name} failed: ${errorMessage} : ${extraErrorInformation}`) } @@ -68,11 +72,5 @@ export async function retryHttpClientRequest( errorMessages: Map = new Map(), maxAttempts = getRetryLimit() ): Promise { - return await retry( - name, - method, - (response: IHttpClientResponse) => response.message.statusCode, - errorMessages, - maxAttempts - ) + return await retry(name, method, errorMessages, maxAttempts) } From 00a8824e5f0873d0a44827588ce787aa9c0b4ab1 Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Fri, 18 Dec 2020 14:55:04 -0500 Subject: [PATCH 09/13] Fix lint --- packages/artifact/__tests__/retry.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/artifact/__tests__/retry.test.ts b/packages/artifact/__tests__/retry.test.ts index baea719175..0548cca19e 100644 --- a/packages/artifact/__tests__/retry.test.ts +++ b/packages/artifact/__tests__/retry.test.ts @@ -44,7 +44,7 @@ async function handleResponse( testResponseCode: number | undefined ): Promise { if (!testResponseCode) { - fail( + throw new Error( 'Test incorrectly set up. reverse.pop() was called too many times so not enough test response codes were supplied' ) } From 1796402731dd72b1093ec89b38bc6312c36389a0 Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Fri, 18 Dec 2020 14:59:17 -0500 Subject: [PATCH 10/13] fix lint error for real this time --- packages/artifact/__tests__/retry.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/artifact/__tests__/retry.test.ts b/packages/artifact/__tests__/retry.test.ts index 0548cca19e..102342a9fd 100644 --- a/packages/artifact/__tests__/retry.test.ts +++ b/packages/artifact/__tests__/retry.test.ts @@ -70,7 +70,7 @@ async function emptyMockReadBody(): Promise { }) } -function setupSingleMockResponse( +async function setupSingleMockResponse( statusCode: number ): Promise { const mockMessage = new http.IncomingMessage(new net.Socket()) From ff3709cc45c38bd726474fa4a667df44706a924c Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Fri, 18 Dec 2020 15:10:23 -0500 Subject: [PATCH 11/13] PR cleanup --- packages/artifact/__tests__/upload.test.ts | 4 ++-- packages/artifact/src/internal/requestUtils.ts | 14 +++++++------- .../artifact/src/internal/upload-http-client.ts | 9 ++++----- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/packages/artifact/__tests__/upload.test.ts b/packages/artifact/__tests__/upload.test.ts index e36818abe5..d21c78abdf 100644 --- a/packages/artifact/__tests__/upload.test.ts +++ b/packages/artifact/__tests__/upload.test.ts @@ -102,7 +102,7 @@ describe('Upload Tests', () => { uploadHttpClient.createArtifactInFileContainer(artifactName) ).rejects.toEqual( new Error( - `Create Artifact Container failed: Artifact service responded with 400 : The artifact name invalid-artifact-name is not valid. Request URL ${getArtifactUrl()}` + `Create Artifact Container failed: The artifact name invalid-artifact-name is not valid. Request URL ${getArtifactUrl()}` ) ) }) @@ -125,7 +125,7 @@ describe('Upload Tests', () => { uploadHttpClient.createArtifactInFileContainer(artifactName) ).rejects.toEqual( new Error( - 'Create Artifact Container failed: Artifact service responded with 403 : Artifact storage quota has been hit. Unable to upload any new artifacts' + 'Create Artifact Container failed: Artifact storage quota has been hit. Unable to upload any new artifacts' ) ) }) diff --git a/packages/artifact/src/internal/requestUtils.ts b/packages/artifact/src/internal/requestUtils.ts index d3cec50d93..444925e125 100644 --- a/packages/artifact/src/internal/requestUtils.ts +++ b/packages/artifact/src/internal/requestUtils.ts @@ -12,14 +12,14 @@ import {getRetryLimit} from './config-variables' export async function retry( name: string, operation: () => Promise, - errorMessages: Map, + customErrorMessages: Map, maxAttempts: number ): Promise { let response: IHttpClientResponse | undefined = undefined let statusCode: number | undefined = undefined let isRetryable = false let errorMessage = '' - let extraErrorInformation: string | undefined = undefined + let customErrorInformation: string | undefined = undefined let attempt = 1 while (attempt <= maxAttempts) { @@ -33,7 +33,7 @@ export async function retry( // Extra error information that we want to display if a particular response code is hit if (statusCode) { - extraErrorInformation = errorMessages.get(statusCode) + customErrorInformation = customErrorMessages.get(statusCode) } isRetryable = isRetryableStatusCode(statusCode) @@ -60,8 +60,8 @@ export async function retry( displayHttpDiagnostics(response) } - if (extraErrorInformation) { - throw Error(`${name} failed: ${errorMessage} : ${extraErrorInformation}`) + if (customErrorInformation) { + throw Error(`${name} failed: ${customErrorInformation}`) } throw Error(`${name} failed: ${errorMessage}`) } @@ -69,8 +69,8 @@ export async function retry( export async function retryHttpClientRequest( name: string, method: () => Promise, - errorMessages: Map = new Map(), + customErrorMessages: Map = new Map(), maxAttempts = getRetryLimit() ): Promise { - return await retry(name, method, errorMessages, maxAttempts) + return await retry(name, method, customErrorMessages, maxAttempts) } diff --git a/packages/artifact/src/internal/upload-http-client.ts b/packages/artifact/src/internal/upload-http-client.ts index 26533b78f6..e632c8706c 100644 --- a/packages/artifact/src/internal/upload-http-client.ts +++ b/packages/artifact/src/internal/upload-http-client.ts @@ -85,7 +85,7 @@ export class UploadHttpClient { // Extra information to display when a particular HTTP code is returned // If a 403 is returned when trying to create a file container, the customer has exceeded // their storage quota so no new artifact containers can be created - const errorMessages: Map = new Map([ + const customErrorMessages: Map = new Map([ [ HttpCodes.Forbidden, 'Artifact storage quota has been hit. Unable to upload any new artifacts' @@ -99,7 +99,7 @@ export class UploadHttpClient { const response = await retryHttpClientRequest( 'Create Artifact Container', async () => client.post(artifactUrl, data, headers), - errorMessages + customErrorMessages ) const body: string = await response.readBody() return JSON.parse(body) @@ -504,7 +504,7 @@ export class UploadHttpClient { const headers = getUploadHeaders('application/json', false) // Extra information to display when a particular HTTP code is returned - const errorMessages: Map = new Map([ + const customErrorMessages: Map = new Map([ [ HttpCodes.NotFound, `An Artifact with the name ${artifactName} was not found` @@ -515,8 +515,7 @@ export class UploadHttpClient { const response = await retryHttpClientRequest( 'Finalize artifact upload', async () => client.patch(resourceUrl.toString(), data, headers), - errorMessages, - 5 + customErrorMessages ) await response.readBody() core.debug( From 632e3cf087e0c801861dac00e7c45942ff5b9b1f Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Fri, 18 Dec 2020 15:16:04 -0500 Subject: [PATCH 12/13] 0.5.0 @actions/artifact release --- packages/artifact/RELEASES.md | 4 ++++ packages/artifact/package-lock.json | 2 +- packages/artifact/package.json | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/artifact/RELEASES.md b/packages/artifact/RELEASES.md index 43e3a5650f..0c41df2071 100644 --- a/packages/artifact/RELEASES.md +++ b/packages/artifact/RELEASES.md @@ -50,3 +50,7 @@ - Improved retry-ability when a partial artifact download is encountered +### 0.5.0 + +- Improved retry-ability for all http calls during artifact upload and download if an error is encountered + diff --git a/packages/artifact/package-lock.json b/packages/artifact/package-lock.json index 707c0d64b6..f75bda69a4 100644 --- a/packages/artifact/package-lock.json +++ b/packages/artifact/package-lock.json @@ -1,6 +1,6 @@ { "name": "@actions/artifact", - "version": "0.4.2", + "version": "0.5.0", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/packages/artifact/package.json b/packages/artifact/package.json index db8aeca0fb..c76d4b1d3b 100644 --- a/packages/artifact/package.json +++ b/packages/artifact/package.json @@ -1,6 +1,6 @@ { "name": "@actions/artifact", - "version": "0.4.2", + "version": "0.5.0", "preview": true, "description": "Actions artifact lib", "keywords": [ From 3706baf99d4adb065cce746d08a645cfd0d910b8 Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Fri, 18 Dec 2020 15:28:28 -0500 Subject: [PATCH 13/13] Display diagnostic info if non-retryable code is hit --- packages/artifact/src/internal/requestUtils.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/artifact/src/internal/requestUtils.ts b/packages/artifact/src/internal/requestUtils.ts index 444925e125..d11db03dbb 100644 --- a/packages/artifact/src/internal/requestUtils.ts +++ b/packages/artifact/src/internal/requestUtils.ts @@ -45,6 +45,9 @@ export async function retry( if (!isRetryable) { core.info(`${name} - Error is not retryable`) + if (response) { + displayHttpDiagnostics(response) + } break }