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

Accommodate when a 404 is not a client error #35780

Merged
merged 1 commit into from
Feb 14, 2025
Merged

Conversation

kaapstorm
Copy link
Contributor

Technical Summary

Traefik proxy returns (client error) 404 instead of (server error) 503 when it has not been configured with a catch-all router and is unable to match a request to a router. See https://doc.traefik.io/traefik/getting-started/faq/#404-not-found

Traefik ought to be configured in such a way that it only responds with a client error when a client has made an error. This change is to accommodate when Traefik has not been configured that way.

Almost 100% of the time, a server returns a 404 response when a request refers to a resource that is not found because the request is permanently incorrect. In a vanishingly small number of situations -- really just this one -- that is not true. The implications of this change are that all payloads that cause 404 errors will be retried MAX_ATTEMPTS (3) times, instead of being cancelled immediately. That feels wrong, but this is a pragmatic solution.

Another option would be to make HTTP_STATUS_4XX_RETRY configurable on a per-ConnectionSettings basis. That is significantly more effort, and I'm not sure it's worth it. I'm curious to hear your thoughts.

Safety Assurance

Safety story

Our large number of Celery workers will easily handle the small increase in load.

Automated test coverage

HTTP_STATUS_4XX_RETRY is covered by corehq/motech/repeaters/tests/test_models.py::TestRepeaterHandleResponse::test_handle_4XX_retry_codes.

QA Plan

No QA planned

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@kaapstorm kaapstorm added the product/invisible Change has no end-user visible impact label Feb 14, 2025
@kaapstorm kaapstorm merged commit 7a30f2a into master Feb 14, 2025
14 checks passed
@kaapstorm kaapstorm deleted the nh/404_server_error branch February 14, 2025 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants