Fix/academy review findings - #18
Merged
Merged
Conversation
… quota Sign-in fails on the live deployment, and so does every AI request. The cause is not in the sign-in code: SharedRateLimiter is failing closed. In production the limiter counts in Postgres, keyed by an HMAC of the caller's identity, and needs RATE_LIMIT_SALT (or METRICS_ID_SALT) to build that key. Without one it returns (False, 0) for every caller — the right call, since a quota that cannot count must not wave paid API calls through — but it did so silently, and the caller saw a bare 429. That is indistinguishable from a real quota, which is why the outage sat there: /api/readyz reported "ready", the logs said nothing, and the site told visitors "Too many sign-in attempts." Diagnosed against the running deployment rather than guessed: /api/readyz reports the database available, and POST /api/ai/generate answers 429 with no X-AI-Quota-* headers — the burst branch, which returns before those headers exist — and still does after a full 60s window has rolled, which an exhausted bucket could not. - rate_limit.py: shared_quota_problem() names why a production quota cannot count, and hit() logs it once per cause rather than per request. - /api/readyz reports it in a new optional `rateLimiting` field. The status code still follows the database alone: Fly health-checks this path, so turning a degraded quota into a 503 would stop the machine and make it worse. Spec, Zod and client regenerated. - ops.py takes the database probe through Depends, so the readiness tests cover both branches instead of skipping when no database is reachable. The runbook is what let this ship: the first-deploy `fly secrets set` block never listed RATE_LIMIT_SALT, and it was mentioned later only as an aside under monitoring. It is in the block now, with what breaks without it, and the post-deploy check says to confirm `rateLimiting` is absent. This does not by itself restore the deployment — the secret still has to be set on the host — but the next deployment that misses it says so. 133 pytest, 242 unit, 58 api, 72 contract pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to 68dde9a, which made the broken quota store visible but left it taking sign-in down. These are the three things that turned one missing secret into a silent, unexplained outage. 1. Failing closed was a single policy for four different quotas. SharedRateLimiter now takes `when_unavailable`. The two AI quotas keep refusing: they guard a key billed per call, and a limiter that cannot count must not wave those through. The login and admin quotas degrade to the in-memory limiter instead, because they bound brute force against credentials that are verified independently — a Google signature, an admin token compared with hmac.compare_digest — so refusing everyone protects nothing and takes authentication down. Degrading keeps a real per-worker bound; it loses only the sharing between workers and across restarts. "refuse" stays the default, so a new bucket opts into leniency. 2. The relay handed visitors control of their own quota identity. It forwarded every inbound header except the country, and the origin keys its quotas on `fly-client-ip`. A visitor who set that through the relay chose a fresh identity per request and never reached a limit. It is now dropped along with x-forwarded-for, x-real-ip, cf-connecting-ip and true-client-ip, for the reason the country header already was: an inbound value is the caller's claim about themselves, not an observation of them. 3. The client threw away what the server said. Every non-2xx became "Sign-in failed. Please try again." — including the 429 this whole outage produced, which told visitors to retry in a loop that could not succeed while implying the fault was theirs. AuthContext now keeps a reason: 429 is busy, 5xx is unavailable, everything else is the original wording, which is what a rejected credential actually is. 143 pytest, 242 unit, 172 component, 58 api, 72 contract, 69 e2e. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The limiter tests cover the policy; these cover what a visitor actually hit. A production deployment with no RATE_LIMIT_SALT answered every credential POST with 429, and the other half of the trade — the billed AI key staying shut under the same conditions — is asserted alongside it, so a future edit cannot recover sign-in by making everything lenient. 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.