feat: apply API rate limits org-wide via an atomic fixed-window counter - #1019
Open
rohan-chaturvedi wants to merge 1 commit into
Open
feat: apply API rate limits org-wide via an atomic fixed-window counter#1019rohan-chaturvedi wants to merge 1 commit into
rohan-chaturvedi wants to merge 1 commit into
Conversation
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.
🔍 Overview
API rate limits are currently enforced per principal: each User (PAT), Service Account, and Service Token gets its own throttle bucket (
user_<id>/sa_<id>/st_<id>). Since plan rate limits are conceptually org-scoped, this means an organisation's effective API capacity multiplies with every principal it creates, and the plan-based limit doesn't actually bound what an org can send.This PR unifies rate limiting to a single shared bucket per organisation: all members, service accounts, and service tokens of an org draw from one limit, resolved from the org's plan.
💡 Proposed Changes
PlanBasedRateThrottlenow keys the cache onorg_<id>. A newget_organisation()resolves the org fromrequest.auth(organisation→environment→app→ principal fallbacks), covering every auth shapePhaseTokenAuthenticationemits. Unauthenticated requests keep the per-IPanon_bucket.SimpleRateThrottle's cached timestamp list (a non-atomic read-modify-write that loses updates under concurrency) withcache.add+cache.incron a window-suffixed key — atomicSET NX/INCRBYon Redis. This matters now that one hot key is shared by all of an org's workers. It also drops the per-request cost from shipping an O(limit) pickled list to O(1) integer ops.wait()/retry-afternow reports the seconds until the current window resets.RATE_LIMIT_PRO) now falls back toRATE_LIMIT_DEFAULTinstead of silently disabling throttling for that plan tier.🖼️ Screenshots or Demo
N/A — backend-only change. Throttled responses are unchanged in shape:
📝 Release Notes
429responses continue to include aretry-afterheader.RATE_LIMIT_DEFAULTsets the (now org-wide) limit; if unset, no rate limits are applied.phasehq/docsbranchunified-rate-limits.🧪 Testing
tests/api/test_throttling.pyrewritten: 19 tests, all passing in the dev container.request.authshapesPhaseTokenAuthentication.authenticate()emits (env / app-only / org-only modes × User / Service Token / Service Account), instead of synthetic dicts.wait()remainder, fallback warning log, and rate-Nonedisabling throttling.tests/api/views/identities/**can 429 when run back-to-back in a dev container withRATE_LIMIT_DEFAULT=5/min(they hit real throttled views against live Redis). Pre-existing onmain, not introduced here.🎯 Reviewer Focus
backend/api/throttling.py—allow_request(): the add/incr atomicity, window keying, and the plan-rate resolution path.get_organisation()— confirm the resolution order against the auth dict shapes built inbackend/api/auth.py(authenticate()), which always sets one oforganisation/environment/app; the principal fallbacks are defensive only.➕ Additional Context
auth.pyalready resolves the caller org (_resolve_caller_org) but only attaches it torequest.authin org-only/bootstrap modes. A follow-up could attach it on every request and collapseget_organisation()to a single dict read.RATE_LIMIT_DEFAULTsemantics on the self-hosting env vars page) ship separately viaphasehq/docs#unified-rate-limits.✨ How to Test the Changes Locally
docker compose -f dev-docker-compose.yml up -d(setRATE_LIMIT_DEFAULT=5/min).https://localhost/service/public/v1/secrets/?app_id=<id>&env=development, alternating between the two tokens.429with aretry-afterheader, regardless of which token sent it — both principals share the org bucket.docker compose -f dev-docker-compose.yml exec backend pytest tests/api/test_throttling.py -v💚 Did You...