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

Retry createBuild based on error messages #477

Merged
merged 2 commits into from
Dec 13, 2021
Merged

Retry createBuild based on error messages #477

merged 2 commits into from
Dec 13, 2021

Conversation

codykaup
Copy link
Contributor

@codykaup codykaup commented Dec 10, 2021 •
edited
Loading

Connections to the Chromatic backend can get a 503 return error from Heroku after 30 seconds which are then retried by our HTTPClient. We want to lower our error rates to reduce noise in our monitoring by throwing errors before that occurs. Doing this returns a 200 from the fetch then we throw an error in our GraphQLClient. This allows known errors to retry again when creating a build!

@codykaup codykaup self-assigned this Dec 10, 2021
@linear
Copy link

linear bot commented Dec 10, 2021

CH-1139 Handle createBuild errors

Previously Heroku was returning a 503 when an H12 fired, which is a retryable condition and the idempotency stuff would come into play in long builds.  Now we are returning a 500, which is not meant to be retried.

Error in CLI:

{"body":"<!DOCTYPE html>\n\t<html>\n\t  <head>\n\t\t<meta name=\"viewport\" content=\"width=device-width, initial-scale=1\">\n\t\t<meta charset=\"utf-8\">\n\t\t<title>Application Error</title>\n\t\t<style media=\"screen\">\n\t\t  html,body,iframe {\n\t\t\tmargin: 0;\n\t\t\tpadding: 0;\n\t\t  }\n\t\t  html,body {\n\t\t\theight: 100%;\n\t\t\toverflow: hidden;\n\t\t  }\n\t\t  iframe {\n\t\t\twidth: 100%;\n\t\t\theight: 100%;\n\t\t\tborder: 0;\n\t\t  }\n\t\t</style>\n\t  </head>\n\t  <body>\n\t\t<iframe src=\"//www.herokucdn.com/error-pages/application-error.html\"></iframe>\n\t  </body>\n\t</html>"} HTTPClient failed to fetch https://index.staging-chromatic.com/graphql, got 503/Service Unavailable
{"url":"https://index.staging-chromatic.com/graphql","err":{"response":{"size":0},"message":"HTTPClient failed to fetch https://index.staging-chromatic.com/graphql, got 503/Service Unavailable"}} Fetch failed; retrying 1/3

Revert PR: https://github.com/chromaui/chromatic/pull/5679

Original PR: https://github.com/chromaui/chromatic/pull/5678

---

Also, use an upsert to avoid this error:

✖ Failed to verify your Storybook
→ E11000 duplicate key error collection: chromatic-index.idempotencies index: sessionId_1 dup key: { sessionId: "7627d9cd-077a-4394-a93e-869c83a48179" }

@codykaup codykaup force-pushed the cody/ch-1139-handle-createbuild-errors branch 2 times, most recently from 2bcf21f to 1151b66 Compare December 10, 2021 22:25
@codykaup codykaup force-pushed the cody/ch-1139-handle-createbuild-errors branch from 1151b66 to 7c63f42 Compare December 10, 2021 23:31
@codykaup codykaup marked this pull request as ready for review December 10, 2021 23:42
tmeasday
tmeasday approved these changes Dec 11, 2021
View reviewed changes
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

This LGTM but I do wonder about the previous behaviour. It did retry before (on 5xx) right? Was that automatic behaviour of the HTTPClient?

bin-src/tasks/verify.js Outdated
@@ -1,3 +1,4 @@
import retry from 'async-retry';
Copy link
Member

Choose a reason for hiding this comment

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

Should this be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not, good eye!

@codykaup
Copy link
Contributor Author

This LGTM but I do wonder about the previous behaviour. It did retry before (on 5xx) right? Was that automatic behaviour of the HTTPClient?

Yep! It would retry because this fetch would throw an error. Since we're throwing the error ourselves, it returns a 200 so the fetch succeeds.

@codykaup codykaup merged commit 9e13c80 into main Dec 13, 2021
@codykaup codykaup deleted the cody/ch-1139-handle-createbuild-errors branch December 13, 2021 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmeasday tmeasday tmeasday approved these changes

Assignees

@codykaup codykaup

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@codykaup @tmeasday