Wire up API rate limiting in shadow mode - #205
Merged
Merged
Conversation
ORTHOCAL_API_RATELIMIT / BURST_RATE (calendarium/api.py) was defined but never actually passed to NinjaAPI's throttle= parameter -- the rate string format (django-ninja's SimpleRateThrottle expects exactly "N/period") makes clear this was meant to be wired up, just never finished. Rather than flip straight to enforcing (returning real 429s), this adds ShadowAnonRateThrottle: runs the same per-IP check ninja's built-in AnonRateThrottle would, logs a warning for every request that would have been rejected (ip, path, user-agent), but always lets the request through. No client can be broken by this. Once the logs show it's safe -- no legitimate client would trip it -- swap ShadowAnonRateThrottle for AnonRateThrottle directly; same rate, same per-IP behavior, only the enforcement changes. Investigated ninja 1.6.3's actual installed throttling source (SimpleRateThrottle.get_ident, NINJA_NUM_PROXIES) to confirm client identification behind Cloud Run's proxy layer works correctly by default without needing NINJA_NUM_PROXIES configured, since Cloud Run reliably sets X-Forwarded-For. Verified the shadow logic directly with a real cache backend (local dev's local_settings.py swaps CACHES to DummyCache, which silently no-ops set()/get() and would have hidden any real bug here) -- confirmed history accumulates correctly and the warning fires exactly on the requests that exceed the rate, while every request still returns 200. Full test suite (162 tests) passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hf6j2xXQXywHVh3HAVRxB3
3 tasks
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
ORTHOCAL_API_RATELIMIT/BURST_RATE(calendarium/api.py:26) was defined but never actually wired to anything -- the rate-string format ("5/s", matching django-ninja'sSimpleRateThrottleexpectations exactly) makes clear it was meant to drive throttling; that wiring was just never finished.ShadowAnonRateThrottle: runs the exact same per-IP checkninja's built-inAnonRateThrottlewould, logs a warning for every request that would have been rejected (ip,path,user-agent), but always lets the request through. No client can be broken by this PR.ShadowAnonRateThrottleforAnonRateThrottledirectly inthrottle=[...]-- same rate, same per-IP identity behavior, only the enforcement changes.Context / investigation
rtdx-calendar-labeled client (self-described as an "offline Orthodox calendar" build tool, referencing a GitHub repo that returns 404) accounts for ~9.5% of recent traffic, spread across 127 distinct IPs -- worth noting that per-IP throttling won't meaningfully curb that specific pattern (each IP gets its own allowance), but it's still valuable protection against any single misbehaving client.ninja1.6.3's actual installed throttling source (not docs, to avoid version drift) to confirm: on throttle, it returns a bare429with{"detail": "Too many requests."}and noRetry-Afterheader by default -- worth adding a custom exception handler for that later if/when this moves to enforcing.get_ident()uses the fullX-Forwarded-Forvalue whenNINJA_NUM_PROXIESisn't set, which Cloud Run populates reliably) -- no additional settings needed.Verification
local_settings.pyswapsCACHEStoDummyCache(a deliberate, unrelated dev convenience -- keeps local testing always-fresh), which silently no-opsset()/get()and would have hidden any real bug in the throttle logic. Re-verified with a real cache backend (LocMemCache) swapped in for testing only: history correctly accumulates, and the shadow warning fires exactly on the requests exceeding the configured rate (5/s) -- confirmed 5 allowed, then 5 consecutive "would rate-limit" warnings, all 10 requests still returning success.docker compose run --rm tests-- 162 tests, all passing.Test plan
Would rate-limitwarnings for a while to confirm no legitimate client would be broken by enforcingShadowAnonRateThrottle→AnonRateThrottle(and ideally aRetry-Afterheader on the real 429)🤖 Generated with Claude Code
https://claude.ai/code/session_01Hf6j2xXQXywHVh3HAVRxB3