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 7 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,11 @@ export async function restoreCache( | |
|
||
await extractTar(archivePath, compressionMethod) | ||
core.info('Cache restored successfully') | ||
|
||
return cacheEntry.cacheKey | ||
} catch (error) { | ||
// Supress all 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 +138,7 @@ export async function restoreCache( | |
} | ||
} | ||
|
||
return cacheEntry.cacheKey | ||
return undefined | ||
} | ||
|
||
/** | ||
|
@@ -152,7 +158,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 +223,13 @@ 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 === ReserveCacheError.name) { | ||
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. Should this check happen in action itself? I see that we do some checks in action itself. Should we also not do this check there? 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. the condition which you mentioned in actions will be removed after this change and basically idea here is we will also need to add any checks like this in other actions setup-* or wherever this toolkit cache package is getting used 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. @tiwarishub but I don't see you catching the ValidationError and handling it. Does that still happen in action while rest happens in toolkit? 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. no, in the given try block we are not throwing ValidationError exception https://github.com/actions/toolkit/pull/1122/files#diff-34e72845d1658a8a9b9fdfe6b5b9261952e7f0496a0a3fcec87dae722718a64aL151-L152. So ValidationError will be still thrown as a error 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. Got it. Though I feel we should also add the ValidationError check here as well. We do not have a specific Validate method and that means the validation can potentially be added in many places in future and without a check all those validations will get suppressed with this global suppression and will be difficult to understand why that is happening. 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. added |
||
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.