New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Graceful handling of error (non-validation one) #1122
Changes from all commits
e051715
639c79e
8c47ec2
5008429
0bde6ed
babb9fb
7b1bc1f
0170075
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,23 +86,24 @@ export async function restoreCache( | |
} | ||
|
||
const compressionMethod = await utils.getCompressionMethod() | ||
let archivePath = '' | ||
try { | ||
// path are needed to compute version | ||
const cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can the try block be just around the http client to handle errors from the server? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the idea here is to ensure that any failure is suppressed so that user does not get blocked due to cache action failures. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, we can even whitelist error based on Response code (by creating exception for those codes) but i want to keep the end user expectation same and based on current implementation of Github Actions actions/cache https://github.com/actions/cache/blob/c3f1317a9e7b1ef106c153ac8c0f00fed3ddbc0d/src/restore.ts#L55-L62 and https://github.com/actions/cache/blob/main/src/save.ts#L51-L59, it is suppressing all the error apart from ValidationError. And this conditions will be removed from actions side. |
||
compressionMethod | ||
}) | ||
|
||
if (!cacheEntry?.archiveLocation) { | ||
// Cache not found | ||
return undefined | ||
} | ||
|
||
// 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}`) | ||
archivePath = path.join( | ||
await utils.createTempDirectory(), | ||
utils.getCacheFileName(compressionMethod) | ||
) | ||
core.debug(`Archive Path: ${archivePath}`) | ||
|
||
try { | ||
// 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this change? IIRC this was made There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, let me check with @t-dedah because this function return type There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me explain more why this change is required, earlier even though Function return type is number and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, that seems to be the case. |
||
|
||
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 { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this will essentially suppress all failures including temp directory creation and tar extraction failures. Essentially we are trading accuracy for continuity here. We definitely need a monitor which will tell us how many users are hitting such failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suppression is already there https://github.com/actions/cache/blob/c3f1317a9e7b1ef106c153ac8c0f00fed3ddbc0d/src/restore.ts#L55-L62 and https://github.com/actions/cache/blob/main/src/save.ts#L51-L59 , the only thing is now it is moved to toolkit instead of Github Actions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check how we can monitors for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is same problem as what @anuragc617 would be looking into w.r.t getting warnings from cache action so that we can have monitor on those.