Fix connection errors if a retry is encountered #82
Conversation
| @@ -21,7 +21,7 @@ async function run(): Promise<void> { | |||
|
|
|||
| const artifactClient = create() | |||
| const options: UploadOptions = { | |||
| continueOnError: true | |||
| continueOnError: false | |||
There was a problem hiding this comment.
This is a behavior change?
There was a problem hiding this comment.
I don't think we should silently change behaviors. If this is really bothering people, consider exposing this option?
There was a problem hiding this comment.
How did V1 behave? We should keep this consistent and expose an option if users request it
There was a problem hiding this comment.
Yup, currently if an error is hit and a file can't get uploaded, the problematic files doesn't get uploaded but everything else continues. Users also have to dig into their logs to see that something didn't get uploaded. This will make the step with the upload action fail so users will immediately know that something bad happened during artifact upload. See the comments linked in the issue.
See this log for example: https://github.com/zhangyoufu/alpine/runs/629165891#step:6:110
The file upload fails, but there is no indication or error unless users dig into their logs. This change will mark this upload step as a failure.
There was a problem hiding this comment.
Yeah, I understand it's quite annoying that you can't trust the exit code, but this is the behavior since v1 right? v2 is just being consistent with v1?
There was a problem hiding this comment.
I think V1 would still fail the step if an upload failed
There was a problem hiding this comment.
Here is the design decision we came up with ADR
The upload step is failed (which causes the action to fail and turn red)
So this actually makes it consistent with what we planned out earlier. I missed this 😱
The artifact will still be available for download at the end and the reported size will only be that which was uploaded before the failure. V1 would generally fail fast if an error was encountered so this also makes it consistent with that.
There was a problem hiding this comment.
I think V1 would still fail the step if an upload failed
If this is the case, then we are good. I just read some users mentioned this is consistent with v1.
We had the window of introducing breaking changes when we add v2. Now we should be careful about making any breaking changes, even though it was in the design. We missed the window. However, if this change makes us consistent with v1, I feel better about changing the behavior now.
There was a problem hiding this comment.
This behavior change only manifests itself if a really bad error is encountered (like the ECONNRESET users encountered with 4 retries doing nothing). Ideally there shouldn't be any errors like this :)
This should have been set to false for the initial v2 release
I bumped it up to It would be cool if in the future we had an action that bumps up the version numbers, updates the tags, and also creates a release automatically through a chatop or something😀 |
| name || getDefaultArtifactName(), | ||
| searchResult.filesToUpload, | ||
| searchResult.rootDirectory, | ||
| options | ||
| ) | ||
|
|
||
| core.info('Artifact upload has finished successfully!') | ||
| if (uploadResponse.failedItems.length > 0) { | ||
| core.setFailed( |
There was a problem hiding this comment.
I double checked my code in @actions/artifact and it's up to us to call core.setFailed if something doesn't get uploaded.
If an error is encountered with continueOnError set to false (true doesn't make a difference either), any remaining files are not attempted to be uploaded (aborts the upload) and it just calls core.error which doesn't actually fail the step
Overview
Fixes: #71
@actions/artifactnpm package fromactions/toolkitwas updated to0.3.2. With the new release, there was a fix forreadstreamsnot correctly being reset if an error is encountered. This was addressed in the following PR: Correctly reset chunk during artifact upload on retry toolkit#458continueOnErrortofalseso that if there are any more errors, the artifact upload stops completely and users won't have have to click on their runs to see if there was a failureTesting
This is a relatively low risk change, considering the same bug was fixed in
@actions/cache: actions/cache#305Stress test uploading 9000+ small files and 1 large file: https://github.com/konradpabjan/artifact-test/runs/679266178?check_suite_focus=true