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

Use HTTP code 503 when health check is down #47061

Closed
wants to merge 1 commit into from

Conversation

Spone
Copy link

@Spone Spone commented Jan 19, 2023

Motivation / Background

Following up on this https://github.com/rails/rails/pull/46936/files#r1081774747, 503 seems to be a more appropriate HTTP response code than 500.

See https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/503

Detail

This Pull Request changes 500 to 503 (or :service_unavailable in the controller to be more descriptive).

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the railties label Jan 19, 2023
@Spone Spone marked this pull request as ready for review January 19, 2023 20:51
@skipkayhil
Copy link
Member

Can you update the guides and HealthController documentation as well? Both reference 500 being the status returned

@rails-bot rails-bot bot added the docs label Jan 19, 2023
@Spone
Copy link
Author

Spone commented Jan 19, 2023

Can you update the guides and HealthController documentation as well? Both reference 500 being the status returned

Done in 1bdfb1b

@skipkayhil
Copy link
Member

Done in 1bdfb1b

Perfect. Can you squash your commits? After that I can I mark this ready for review from a committer

@Spone Spone force-pushed the health-controller-http-code branch from 1bdfb1b to 31a2191 Compare January 20, 2023 08:08
@Spone
Copy link
Author

Spone commented Jan 20, 2023

Can you squash your commits?

Done!

@dhh
Copy link
Member

dhh commented Jan 20, 2023

Hmmm. I don't actually agree with this. 503 is for "overloaded" or "service is not yet ready". That's not why we're failing here. We're failing because there's an internal server error that's causing an exception to be raised at the basic level. This might be a wrong master key, a bad configuration of other sorts, or something third. But it's not planned and it's not due to load.

I think 500 Internal Server Error remains correct.

@dhh dhh closed this Jan 20, 2023
@Spone Spone deleted the health-controller-http-code branch January 20, 2023 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants