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.

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

Fix retrying uploads by using Octokit retry plugin. #226

Merged
merged 2 commits into from
Sep 22, 2020
Merged

Conversation

chrisgavin
Copy link
Contributor

This removes a whole bunch of error handling logic from the upload library that it turns out never actually gets called. It all works under the assumption that Octokit will return a response even if a request fails with a bad status code but this is actually not true. Instead Octokit rejects the promise so the upload immediately fails anyway.

I've replaced all this with the Octokit Retry plugin. This has the advantage that we're retrying on all the status codes that GitHub officially wants us to (including some 4xx codes), and that we're retrying for all types of requests to the GitHub API, not just SARIF uploads.

By default it retries 3 times with an exponential backoff (I think of 1, 4 and 8 seconds) but we can tweak that if we decide we want more retries or a longer backoff.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

@chrisgavin chrisgavin marked this pull request as ready for review September 21, 2020 18:30
@robertbrignull robertbrignull self-assigned this Sep 22, 2020
Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Have you tried testing it? Perhaps deliberately upload to a non-existent URL or with some invalid sarif. What does the output look like?

@chrisgavin
Copy link
Contributor Author

I've tested it locally with a dotcom API endpoint that always returns 500. The easiest way of testing on Actions is to set the do-not-retry codes to an empty list and use a non-existent SARIF submission URL. You can see the delay due to retrying in the output as well as the number of retries used reflected in the request metadata.

2020-09-22T11:15:46.0237787Z Uploading results
2020-09-22T11:16:00.5356842Z ##[error]Not Found
2020-09-22T11:16:00.5396875Z RequestError [HttpError]: Not Found
2020-09-22T11:16:00.5398641Z     at /home/runner/work/_actions/github/codeql-action/octokit-retry-force/node_modules/@octokit/request/dist-node/index.js:66:23
2020-09-22T11:16:00.5399640Z     at processTicksAndRejections (internal/process/task_queues.js:93:5)
2020-09-22T11:16:00.5401022Z     at async Job.doExecute (/home/runner/work/_actions/github/codeql-action/octokit-retry-force/node_modules/bottleneck/light.js:405:18) {
2020-09-22T11:16:00.5401951Z   name: 'HttpError',
2020-09-22T11:16:00.5402244Z   status: 404,
...
2020-09-22T11:16:00.5672133Z     request: {
2020-09-22T11:16:00.5672415Z       agent: [Agent],
2020-09-22T11:16:00.5673279Z       hook: [Function: bound bound register],
2020-09-22T11:16:00.5673682Z       retryCount: 3,
2020-09-22T11:16:00.5674104Z       retries: 3,
2020-09-22T11:16:00.5674496Z       retryAfter: 16
2020-09-22T11:16:00.5674755Z     }
2020-09-22T11:16:00.5674953Z   },
2020-09-22T11:16:00.5675963Z   documentation_url: 'https://docs.github.com/rest'
2020-09-22T11:16:00.5676420Z }
2020-09-22T11:16:00.7967103Z Post job cleanup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants