Fix/academy review findings - #20
Merged
Merged
Conversation
Anonymous quotas were being charged to `request.client.host`, which behind a proxy is the proxy. On a platform that fronts the app with several edge pods it is a *different* proxy from one request to the next, so the effects compounded: visitors shared whichever bucket their request landed in, and the daily allowance was not enforced at all because the next request landed elsewhere. Visible from outside — five consecutive anonymous requests to the deployment returned X-AI-Quota-Remaining of 8, 9, 9, 8, 9 rather than counting down. It is also what produced the reported symptom of a 429 on the first click and success on the second: the first landed in a bucket another visitor had filled, the retry landed somewhere else. The caller now comes from X-Forwarded-For, read from the right. That direction is the whole security argument: anything a client writes itself lands at the left, so only the entries our own infrastructure appended are worth reading. TRUSTED_PROXY_HOPS says how many those are — one by default, matching a single load balancer that appends the caller and then itself. Fly's own header still wins where it is present, and outside production nothing is trusted at all. Ten tests cover it, including the two that state the bug: two callers behind one proxy must be told apart, and one caller through two different edges must not. 157 pytest, 58 api, 72 contract pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reproduced against the deployment: a small request returns 200, and a large one — the shape the résumé rewrite sends — returns 429 carrying "The AI provider could not complete this request". That is not our limiter. It is the shared Groq key meeting Groq's own tokens-per-minute ceiling, passed through with the upstream status. The daily allowance was spent before the call and kept whatever came back, so a visitor with ten requests a day could burn the lot on refusals and receive nothing. SharedRateLimiter can now give a hit back, and the AI route does so whenever the outcome is not a 200, correcting X-AI-Quota-Remaining with it. Retrying is still bounded by the burst limiter, so handing one back cannot become a way around the quota. Checked by mutation — removing the release from the route fails the test that two refusals in a row leave the allowance where it started, and the paired test keeps an answered request being charged for. The client wording is widened to match. It said the free allowance was spent, which is only one of the two causes and the wrong one here; it now names both and gives the step that works either way. 161 pytest, 246 unit, 58 api, 72 contract pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Groq's free tier refuses a large request on tokens-per-minute, which is what made the résumé rewrite fail while short prompts succeeded. Gemini is configured on the same deployment and can answer it, so an ordinary request now tries it rather than giving up. The boundaries are the interesting part, and each has a test: - A grounded request has no fallback. Search grounding is the reason it chose that provider, and answering without it answers a different question. - A 4xx below 429 is not retried. That is the request being wrong, and the second provider would only agree, one wasted call later. - A fallback with no key configured leaves the original failure showing rather than replacing it with its own 503, which would hide why the request failed. - The outcome now carries the provider and model that actually answered, and the route labels metrics with those. Otherwise a fallback would be recorded as a Groq success. One thing had to change for this to be honest: Gemini's request builder added the google_search tool unconditionally, which was correct while it only ever served grounded requests. Serving an ordinary one as a fallback would have turned it into a search. Grounding now follows the request, not the provider. That last bug was real and I nearly shipped it — a scripted edit failed to match and the change silently did not apply, because I had asserted on the batch rather than on that replacement. The test caught it. 168 pytest, 58 api, 72 contract pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
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
1 participant
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.
No description provided.