Repository navigation
fix(nginx): pass app 429 responses through - #645
Open
balamurali27 wants to merge 1 commit into
Open
balamurali27 wants to merge 1 commit into
balamurali27 wants to merge 1 commit into
Conversation
error_page 429 replaced every 429 with the "Daily Usage Limit Reached" page and dropped the app's body and headers (Retry-After, X-RateLimit-*), so api clients of custom rate limiters could not back off. sub_filter now swaps only frappe's own plan rate limit response (bare text/plain "Too Many Requests"), like the 500 page. the proxy stops intercepting 429. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTWXzC9v7otLpaxALSTvEN
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
[Medium impact] The change affects 429 responses across sites served by these nginx configs. Fix custom plain-text 429 body replacement before merging. Reviews (1) · Last reviewed commit: "fix(nginx): pass app 429 responses throu..." · Reviewed by Greptile |
|
|
||
| sub_filter '<h1>Internal Server Error</h1>' $internal_server_error_page_{{ bench_name_slug }}; | ||
| sub_filter '<h1><p>Internal Server Error</p></h1>' $gunicorn_internal_server_error_page_{{ bench_name_slug }}; | ||
| sub_filter 'Too Many Requests' $exceeded_page_{{ bench_name_slug }}; |
There was a problem hiding this comment.
Custom limiter messages get replaced
sub_filter matches a phrase, not the bare Frappe response. A custom text/plain 429 such as Too Many Requests. Retry in 30 seconds. gets part of its message replaced with an iframe in both server blocks. Restrict the replacement to the known Frappe response so custom limiters keep their full body.
Prompt To Fix With AI
This is a comment left during a code review.
Path: agent/templates/bench/nginx.conf.jinja2
Line: 185
Comment:
**Custom limiter messages get replaced**
`sub_filter` matches a phrase, not the bare Frappe response. A custom `text/plain` 429 such as `Too Many Requests. Retry in 30 seconds.` gets part of its message replaced with an iframe in both server blocks. Restrict the replacement to the known Frappe response so custom limiters keep their full body.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
error_page 429replaced every 429 with the "Daily Usage Limit Reached" page. nginx then dropped the app's body and headers (Retry-After,X-RateLimit-*), so API clients of custom rate limiters cannot back off.Now only frappe's own plan rate limit response (
rate_limiter.respond(): 429, baretext/plain"Too Many Requests") gets our page. The swap usessub_filter, the same method as the 500 page.Evidence
Rendered bench template on openresty 1.27.1.2, with a fake upstream:
text/plain"Too Many Requests" (frappe limiter)text/html)application/json+Retry-After,X-RateLimit-*text/plain"Too Many Requests"GET /exceeded.htmlMerge Danger
Door: two-way
Revert and regenerate the nginx configs.
Blast Radius: wide
text/plain429 goes out astext/html. A plain-text app 429 keeps its body.🤖 Generated with Claude Code
https://claude.ai/code/session_01LTWXzC9v7otLpaxALSTvEN