From 46231a7da3ee695120e08c7b111173131245fa5e Mon Sep 17 00:00:00 2001 From: Shubham Tiwari <64764738+tiwarishub@users.noreply.github.com> Date: Fri, 24 Jun 2022 10:46:20 +0530 Subject: [PATCH] Graceful handling of error (non-validation one) (#1122) * Initial changes * added info error as well * Format * Unused package * adding message field * removed line * Review comments * review comment to add validation as errors handling --- packages/cache/__tests__/restoreCache.test.ts | 8 ++- packages/cache/__tests__/saveCache.test.ts | 42 ++++++++++---- packages/cache/src/cache.ts | 56 +++++++++++++------ 3 files changed, 75 insertions(+), 31 deletions(-) diff --git a/packages/cache/__tests__/restoreCache.test.ts b/packages/cache/__tests__/restoreCache.test.ts index cb3a06c1..36ec8801 100644 --- a/packages/cache/__tests__/restoreCache.test.ts +++ b/packages/cache/__tests__/restoreCache.test.ts @@ -73,13 +73,17 @@ test('restore with no cache found', async () => { test('restore with server error should fail', async () => { const paths = ['node_modules'] const key = 'node-test' + const logWarningMock = jest.spyOn(core, 'warning') jest.spyOn(cacheHttpClient, 'getCacheEntry').mockImplementation(() => { throw new Error('HTTP Error Occurred') }) - await expect(restoreCache(paths, key)).rejects.toThrowError( - 'HTTP Error Occurred' + const cacheKey = await restoreCache(paths, key) + expect(cacheKey).toBe(undefined) + expect(logWarningMock).toHaveBeenCalledTimes(1) + expect(logWarningMock).toHaveBeenCalledWith( + 'Failed to restore: HTTP Error Occurred' ) }) diff --git a/packages/cache/__tests__/saveCache.test.ts b/packages/cache/__tests__/saveCache.test.ts index 92628f71..945b254f 100644 --- a/packages/cache/__tests__/saveCache.test.ts +++ b/packages/cache/__tests__/saveCache.test.ts @@ -48,6 +48,7 @@ test('save with large cache outputs should fail', async () => { const cachePaths = [path.resolve(filePath)] const createTarMock = jest.spyOn(tar, 'createTar') + const logWarningMock = jest.spyOn(core, 'warning') const cacheSize = 11 * 1024 * 1024 * 1024 //~11GB, over the 10GB limit jest @@ -58,8 +59,11 @@ test('save with large cache outputs should fail', async () => { .spyOn(cacheUtils, 'getCompressionMethod') .mockReturnValueOnce(Promise.resolve(compression)) - await expect(saveCache([filePath], primaryKey)).rejects.toThrowError( - 'Cache size of ~11264 MB (11811160064 B) is over the 10GB limit, not saving cache.' + const cacheId = await saveCache([filePath], primaryKey) + expect(cacheId).toBe(-1) + expect(logWarningMock).toHaveBeenCalledTimes(1) + expect(logWarningMock).toHaveBeenCalledWith( + 'Failed to save: Cache size of ~11264 MB (11811160064 B) is over the 10GB limit, not saving cache.' ) const archiveFolder = '/foo/bar' @@ -79,6 +83,7 @@ test('save with large cache outputs should fail in GHES with error message', asy const cachePaths = [path.resolve(filePath)] const createTarMock = jest.spyOn(tar, 'createTar') + const logWarningMock = jest.spyOn(core, 'warning') const cacheSize = 11 * 1024 * 1024 * 1024 //~11GB, over the 10GB limit jest @@ -106,8 +111,11 @@ test('save with large cache outputs should fail in GHES with error message', asy return response }) - await expect(saveCache([filePath], primaryKey)).rejects.toThrowError( - 'The cache filesize must be between 0 and 1073741824 bytes' + const cacheId = await saveCache([filePath], primaryKey) + expect(cacheId).toBe(-1) + expect(logWarningMock).toHaveBeenCalledTimes(1) + expect(logWarningMock).toHaveBeenCalledWith( + 'Failed to save: The cache filesize must be between 0 and 1073741824 bytes' ) const archiveFolder = '/foo/bar' @@ -127,6 +135,7 @@ test('save with large cache outputs should fail in GHES without error message', const cachePaths = [path.resolve(filePath)] const createTarMock = jest.spyOn(tar, 'createTar') + const logWarningMock = jest.spyOn(core, 'warning') const cacheSize = 11 * 1024 * 1024 * 1024 //~11GB, over the 10GB limit jest @@ -150,8 +159,11 @@ test('save with large cache outputs should fail in GHES without error message', return response }) - await expect(saveCache([filePath], primaryKey)).rejects.toThrowError( - 'Cache size of ~11264 MB (11811160064 B) is over the data cap limit, not saving cache.' + const cacheId = await saveCache([filePath], primaryKey) + expect(cacheId).toBe(-1) + expect(logWarningMock).toHaveBeenCalledTimes(1) + expect(logWarningMock).toHaveBeenCalledWith( + 'Failed to save: Cache size of ~11264 MB (11811160064 B) is over the data cap limit, not saving cache.' ) const archiveFolder = '/foo/bar' @@ -168,6 +180,7 @@ test('save with large cache outputs should fail in GHES without error message', test('save with reserve cache failure should fail', async () => { const paths = ['node_modules'] const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' + const logInfoMock = jest.spyOn(core, 'info') const reserveCacheMock = jest .spyOn(cacheHttpClient, 'reserveCache') @@ -187,9 +200,13 @@ test('save with reserve cache failure should fail', async () => { .spyOn(cacheUtils, 'getCompressionMethod') .mockReturnValueOnce(Promise.resolve(compression)) - await expect(saveCache(paths, primaryKey)).rejects.toThrowError( - `Unable to reserve cache with key ${primaryKey}, another job may be creating this cache.` + const cacheId = await saveCache(paths, primaryKey) + expect(cacheId).toBe(-1) + expect(logInfoMock).toHaveBeenCalledTimes(1) + expect(logInfoMock).toHaveBeenCalledWith( + `Failed to save: Unable to reserve cache with key ${primaryKey}, another job may be creating this cache. More details: undefined` ) + expect(reserveCacheMock).toHaveBeenCalledTimes(1) expect(reserveCacheMock).toHaveBeenCalledWith(primaryKey, paths, { compressionMethod: compression @@ -203,7 +220,7 @@ test('save with server error should fail', async () => { const filePath = 'node_modules' const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' const cachePaths = [path.resolve(filePath)] - + const logWarningMock = jest.spyOn(core, 'warning') const cacheId = 4 const reserveCacheMock = jest .spyOn(cacheHttpClient, 'reserveCache') @@ -228,9 +245,12 @@ test('save with server error should fail', async () => { .spyOn(cacheUtils, 'getCompressionMethod') .mockReturnValueOnce(Promise.resolve(compression)) - await expect(saveCache([filePath], primaryKey)).rejects.toThrowError( - 'HTTP Error Occurred' + await saveCache([filePath], primaryKey) + expect(logWarningMock).toHaveBeenCalledTimes(1) + expect(logWarningMock).toHaveBeenCalledWith( + 'Failed to save: HTTP Error Occurred' ) + expect(reserveCacheMock).toHaveBeenCalledTimes(1) expect(reserveCacheMock).toHaveBeenCalledWith(primaryKey, [filePath], { compressionMethod: compression diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index b11d6310..609c7f94 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -86,23 +86,24 @@ export async function restoreCache( } const compressionMethod = await utils.getCompressionMethod() - - // path are needed to compute version - const cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, { - compressionMethod - }) - if (!cacheEntry?.archiveLocation) { - // Cache not found - return undefined - } - - const archivePath = path.join( - await utils.createTempDirectory(), - utils.getCacheFileName(compressionMethod) - ) - core.debug(`Archive Path: ${archivePath}`) - + let archivePath = '' try { + // path are needed to compute version + const cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, { + compressionMethod + }) + + if (!cacheEntry?.archiveLocation) { + // Cache not found + return undefined + } + + archivePath = path.join( + await utils.createTempDirectory(), + utils.getCacheFileName(compressionMethod) + ) + core.debug(`Archive Path: ${archivePath}`) + // Download the cache from the cache entry await cacheHttpClient.downloadCache( cacheEntry.archiveLocation, @@ -123,6 +124,16 @@ export async function restoreCache( await extractTar(archivePath, compressionMethod) core.info('Cache restored successfully') + + return cacheEntry.cacheKey + } catch (error) { + const typedError = error as Error + if (typedError.name === ValidationError.name) { + throw error + } else { + // Supress all non-validation cache related errors because caching should be optional + core.warning(`Failed to restore: ${(error as Error).message}`) + } } finally { // Try to delete the archive to save space try { @@ -132,7 +143,7 @@ export async function restoreCache( } } - return cacheEntry.cacheKey + return undefined } /** @@ -152,7 +163,7 @@ export async function saveCache( checkKey(key) const compressionMethod = await utils.getCompressionMethod() - let cacheId = null + let cacheId = -1 const cachePaths = await utils.resolvePaths(paths) core.debug('Cache Paths:') @@ -217,6 +228,15 @@ export async function saveCache( core.debug(`Saving Cache (ID: ${cacheId})`) await cacheHttpClient.saveCache(cacheId, archivePath, options) + } catch (error) { + const typedError = error as Error + if (typedError.name === ValidationError.name) { + throw error + } else if (typedError.name === ReserveCacheError.name) { + core.info(`Failed to save: ${typedError.message}`) + } else { + core.warning(`Failed to save: ${typedError.message}`) + } } finally { // Try to delete the archive to save space try {