docs(suggest): the in-app global ceiling would not have worked
Reading slowapi rather than assuming: default_limits and application_limits are both evaluated with the same key_func, so they are per-client across routes, not global. And application_limits only apply 'if in_middleware' — this app installs no SlowAPIMiddleware, so they would never have fired at all. A genuine global cap would need a second Limiter with a constant key plus that middleware. Cloudflare is already in the path on both environments and does this at the right layer, so the ceiling is named as a follow-up there rather than built badly here. The risk that leaves is stated plainly in the risks section instead of being papered over with a mechanism that does not do the job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
This commit is contained in:
1 parent
22c113fc29
commit
e651dd0d65
1 file changed
+30
-16
@@ -70,20 +70,30 @@ single-process uvicorn backend that filters a 25,000-row DataFrame in-process.
|
||||
Correct per-user keying removes that throttle: the origin becomes reachable at
|
||||
60/min *per user* rather than 60/min in total.
|
||||
|
||||
So the keying fix ships **with a global ceiling**, not instead of one:
|
||||
slowapi's application-wide `default_limits`, set above any plausible real load
|
||||
but below what would flood a single worker. **3000/minute**, against a
|
||||
per-user suggest limit of 120/minute — so twenty-five simultaneously active
|
||||
searchers are unaffected, while a runaway client or a scraper still meets a
|
||||
wall.
|
||||
Per-user fairness and origin protection are different jobs. Conflating them
|
||||
is what produced the current behaviour, and the fix must not quietly do it
|
||||
again in the other direction.
|
||||
|
||||
Per-user fairness and origin protection are different jobs and need different
|
||||
limits. Conflating them is what produced the current behaviour.
|
||||
**The global ceiling does not go in this app.** An earlier draft of this
|
||||
section specified one via slowapi's `default_limits`. Reading the library
|
||||
shows that would not have worked, twice over: `default_limits` and
|
||||
`application_limits` are both evaluated with the same `key_func`, so they are
|
||||
per-client across all routes rather than global; and `application_limits` are
|
||||
only applied `if in_middleware`, while this app installs no `SlowAPIMiddleware`
|
||||
at all. Expressing a genuine global cap would take a second `Limiter` with a
|
||||
constant key plus that middleware — two mechanisms to keep correct, for a
|
||||
protection this layer is the wrong place for.
|
||||
|
||||
Both numbers are estimates, not measurements. They are a starting point to
|
||||
revisit against real traffic once the keying is correct enough for real
|
||||
traffic to be visible — which it is not today, because everyone shares one
|
||||
bucket.
|
||||
Cloudflare is already in the request path on both environments and does
|
||||
edge-level rate limiting properly, before traffic reaches a single-process
|
||||
origin at all. That is where a global ceiling belongs, and it is a dashboard
|
||||
change rather than code. Flagged as a follow-up, deliberately not built here.
|
||||
|
||||
What ships instead is conservative per-user limits: the existing 60/minute
|
||||
default is unchanged, and `/api/suggest` gets 120/minute. Both are estimates
|
||||
rather than measurements, and they are a starting point to revisit once the
|
||||
keying is correct enough for real per-user traffic to be visible — which it
|
||||
is not today, because everyone shares one bucket.
|
||||
|
||||
## 2. `GET /api/suggest`
|
||||
|
||||
@@ -115,7 +125,7 @@ GET /api/suggest?q=<query>&limit=8
|
||||
- **Rate limit `120/minute`** per client, not the default 60. A 200 ms
|
||||
debounce tops out near 5 requests/second while someone is actively typing,
|
||||
but averages far below that across a real search; 120 leaves headroom for
|
||||
bursts without letting one client alone consume the global ceiling.
|
||||
bursts while still bounding one client.
|
||||
- **Local authority is part of the payload, not decoration.** There are many
|
||||
schools called "St Mary's"; a suggestion list without the authority is
|
||||
unusable for exactly the queries autosuggest is meant to serve.
|
||||
@@ -210,9 +220,13 @@ effect, since `/api/flags` is denied to the public.
|
||||
|
||||
## 7. Risks
|
||||
|
||||
**Removing the accidental throttle.** Covered in §1. The global ceiling is the
|
||||
mitigation, and 1200/minute is a guess that should be revisited against real
|
||||
traffic rather than treated as a considered number.
|
||||
**Removing the accidental throttle.** Covered in §1. Correct per-user keying
|
||||
means the origin is reachable at 60/minute *per user* where it was 60/minute
|
||||
in total, and no in-app global cap replaces it — that job goes to Cloudflare,
|
||||
which is not done as part of this change. Until it is, a determined caller
|
||||
with many source addresses can put more load on a single-process origin than
|
||||
they can today. Against this site's traffic that is a theoretical risk rather
|
||||
than a live one, but it is a real one and it is the price of the fix.
|
||||
|
||||
**Cloudflare bypass.** If the origin is reachable without passing through
|
||||
Cloudflare, `CF-Connecting-IP` is absent and the `X-Forwarded-For` fallback is
|
||||
|
||||
Reference in new issue
Block a user