-
Notifications
You must be signed in to change notification settings - Fork 18
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
GHA cache: handle io.EOF when uploading a part #788
Conversation
And pass-through other errors that Actions Toolkit deems as retryable.
status := http.StatusInternalServerError | ||
|
||
// Cause the cache-related code in the Actions Toolkit to make a re-try[1]. | ||
// | ||
// [1]: https://github.com/actions/toolkit/blob/6dd369c0e648ed58d0ead326cf2426906ea86401/packages/cache/src/internal/requestUtils.ts#L24-L34 | ||
if uploadPartResponse.StatusCode == http.StatusBadGateway || | ||
uploadPartResponse.StatusCode == http.StatusServiceUnavailable || | ||
uploadPartResponse.StatusCode == http.StatusGatewayTimeout { | ||
status = uploadPartResponse.StatusCode | ||
} | ||
|
||
fail(writer, request, status, "GHA cache failed to upload part "+ |
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.
Maybe just propagate the response code?
status := http.StatusInternalServerError | |
// Cause the cache-related code in the Actions Toolkit to make a re-try[1]. | |
// | |
// [1]: https://github.com/actions/toolkit/blob/6dd369c0e648ed58d0ead326cf2426906ea86401/packages/cache/src/internal/requestUtils.ts#L24-L34 | |
if uploadPartResponse.StatusCode == http.StatusBadGateway || | |
uploadPartResponse.StatusCode == http.StatusServiceUnavailable || | |
uploadPartResponse.StatusCode == http.StatusGatewayTimeout { | |
status = uploadPartResponse.StatusCode | |
} | |
fail(writer, request, status, "GHA cache failed to upload part "+ | |
fail(writer, request, uploadPartResponse.StatusCode, "GHA cache failed to upload part "+ |
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.
Maybe just log it to Sentry? To avoid leaking the abstraction details too early.
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 don't see a problem of leaking an abstraction here. Only the status code will be "leaked".
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.
OK, implemented in a467d01.
status := http.StatusInternalServerError | ||
|
||
// Cause the cache-related code in the Actions Toolkit to make a re-try[1]. | ||
// | ||
// [1]: https://github.com/actions/toolkit/blob/6dd369c0e648ed58d0ead326cf2426906ea86401/packages/cache/src/internal/requestUtils.ts#L24-L34 | ||
if errors.Is(err, io.EOF) { | ||
status = http.StatusBadGateway | ||
} | ||
|
||
fail(writer, request, status, "GHA cache failed to upload part "+ |
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.
Maybe always return 503?
status := http.StatusInternalServerError | |
// Cause the cache-related code in the Actions Toolkit to make a re-try[1]. | |
// | |
// [1]: https://github.com/actions/toolkit/blob/6dd369c0e648ed58d0ead326cf2426906ea86401/packages/cache/src/internal/requestUtils.ts#L24-L34 | |
if errors.Is(err, io.EOF) { | |
status = http.StatusBadGateway | |
} | |
fail(writer, request, status, "GHA cache failed to upload part "+ | |
fail(writer, request, http.StatusServiceUnavailable, "GHA cache failed to upload part "+ |
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.
See #789 (comment).
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.
Now this always returns HTTP 502, which seems to be more generic than HTTP 503, which is important since we don't differentiate between different errors at all with this approach.
Let me know if you specifically want HTTP 503.
And pass-through other errors that Actions Toolkit deems as retryable.