Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cache: wrap create failures in ReserveCacheError #1966

Merged
merged 1 commit into from
Feb 25, 2025
Merged

cache: wrap create failures in ReserveCacheError #1966

merged 1 commit into from
Feb 25, 2025

Conversation

robherley
Copy link
Contributor

When we create cache entries, it's normal behavior for multiple jobs to "race" to create the same entry. In the cache v1 implementations, we suppressed these errors with just a normal log statement however in v2 we create an annotation with warning that is confusing folks.

Here's the v1 implementation:

toolkit/packages/cache/src/cache.ts

Lines 402 to 425 in 662b9d9

const reserveCacheResponse = await cacheHttpClient.reserveCache(
key,
paths,
{
compressionMethod,
enableCrossOsArchive,
cacheSize: archiveFileSize
}
)
if (reserveCacheResponse?.result?.cacheId) {
cacheId = reserveCacheResponse?.result?.cacheId
} else if (reserveCacheResponse?.statusCode === 400) {
throw new Error(
reserveCacheResponse?.error?.message ??
`Cache size of ~${Math.round(
archiveFileSize / (1024 * 1024)
)} MB (${archiveFileSize} B) is over the data cap limit, not saving cache.`
)
} else {
throw new ReserveCacheError(
`Unable to reserve cache with key ${key}, another job may be creating this cache. More details: ${reserveCacheResponse?.error?.message}`
)
}

In v2, our client throws an error for a failed request, so we need to wrap that in the ReserveCacheError. That ensures we only emit an info statement, not a warning annotation:

toolkit/packages/cache/src/cache.ts

Lines 564 to 565 in 662b9d9

} else if (typedError.name === ReserveCacheError.name) {
core.info(`Failed to save: ${typedError.message}`)

@robherley robherley requested a review from a team as a code owner February 25, 2025 17:51
@robherley robherley merged commit 1b9063e into main Feb 25, 2025
16 of 17 checks passed
@robherley robherley mentioned this pull request Feb 25, 2025
Merged
@crazy-max crazy-max mentioned this pull request Feb 26, 2025
Merged
@robherley robherley mentioned this pull request Feb 26, 2025
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshmgross joshmgross joshmgross approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@robherley @joshmgross