-
Notifications
You must be signed in to change notification settings - Fork 429
Add retrying to the upload #13
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
Conversation
c2f4484 to
73f7756
Compare
73f7756 to
1da651c
Compare
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 see we're re-trying not only on 5xx responses but on all. Additionally, the behaviour we had of not setting the job as failed when we get a 500 is changed too. Can you provide more of an insight as to why you've decided to go this way?
src/upload-lib.ts
Outdated
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.
will be retry -> will retry
Yes, thanks for spotting that. I completely forgot to mention that I made that change as part of this. I can easily back that change out if you'd prefer not to make it at this time. I believe that the will of @jhutchings1 and @joshhale was to now fail the action if there's a 500 when uploading. |
|
In fact, I will back this change out and we can make it at another time. |
|
Changed to only retry on 5xx errors, and I've changed the error message to include the status code. I've run some tests again so I'm happy with the testing of this. |
| ' seconds: (' + statusCode + ') ' + await res.readBody()); | ||
| // Sleep for the backoff period | ||
| await new Promise(r => setTimeout(r, backoffPeriods[attempt] * 1000)); | ||
| continue; |
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.
why do we need this continue here?
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.
To get to the next iteration of the loop. Technically it's not necessary as this is the last statement of the loop in this branch, but it doesn't hurt as far as I can see.
cd4c319 to
129ce28
Compare
This should make the upload retry a few times and with a small amount of delay between attempts. Hopefully this will help get over very minor issues, though if there's a larger outage of github lasting more than a few seconds then it won't be able to help.
Merge / deployment checklist
upload_retrywhere everything is normalupload_retry_errorwhere all attempts failupload_retry_partial_errorwhere the first attempt fails