feat: add Redis-backed rate limiting with fallback#387
feat: add Redis-backed rate limiting with fallback#387manikanta-tamminana wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthrough
ChangesRedis-backed Rate Limiting
Campaigns Merge Migration
Sequence Diagram(s)sequenceDiagram
participant Client
participant RateLimitMiddleware
participant Redis
participant InMemoryFallback
Client->>RateLimitMiddleware: HTTP request
RateLimitMiddleware->>Redis: INCR path:ip_key
alt first request in window
RateLimitMiddleware->>Redis: EXPIRE key, window_seconds
end
alt count exceeds limit
RateLimitMiddleware-->>Client: 429 {"error": "Too many requests"}
else Redis unavailable
RateLimitMiddleware->>InMemoryFallback: clean timestamps older than window
InMemoryFallback-->>RateLimitMiddleware: current request count
alt count exceeds limit
RateLimitMiddleware-->>Client: 429 {"error": "Too many requests"}
else
RateLimitMiddleware-->>Client: None (request proceeds)
end
else
RateLimitMiddleware-->>Client: None (request proceeds)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
backend/tenants/security.py (1)
28-29: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove unused class attributes.
MAX_REQUESTSandWINDOW_SECONDSare no longer used after the refactor toRATE_LIMITS/DEFAULT_LIMIT.🧹 Proposed fix
DEFAULT_LIMIT = (100, 60) # { ip_address: [timestamp1, timestamp2, ...] } _requests = {} - MAX_REQUESTS = 100 # per window - WINDOW_SECONDS = 60🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tenants/security.py` around lines 28 - 29, Remove the unused class attributes MAX_REQUESTS and WINDOW_SECONDS from the security class. These attributes are no longer referenced in the codebase after the refactoring that introduced RATE_LIMITS and DEFAULT_LIMIT configuration. Delete both the MAX_REQUESTS and WINDOW_SECONDS attribute definitions to clean up the code and remove dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/tenants/security.py`:
- Around line 65-71: The in-memory fallback rate limiting uses only the IP
address as the key in the self._requests dictionary, but Redis uses a composite
key format of path and IP. Update the code to use a composite key (combining
path and ip) instead of just ip when accessing and storing entries in
self._requests to ensure consistent per-path rate limiting behavior between the
Redis implementation and the in-memory fallback during outages.
- Around line 76-77: The indentation on line 76 with the `status=429` parameter
has extra leading space that does not align with the surrounding code. Remove
the extra leading space from the `status=429` line to match the proper
indentation level of the other parameters in the same function call or response
object definition.
- Around line 18-19: The `_get_redis_client` method in the security.py module
creates a new Redis client on every request by calling `redis.from_url()`
repeatedly, which is inefficient. Cache the Redis client at the module level by
creating a single client instance when the module is imported rather than
recreating it on each request. Convert `_get_redis_client` from an instance
method to a module-level function that returns the cached client, and update all
calls to `self._get_redis_client()` (such as in the `process_request` method) to
call the module-level `_get_redis_client()` function directly without the self
reference.
- Around line 46-49: The race condition exists because client.incr(key) and
client.expire(key, window) are separate non-atomic operations. When two requests
arrive simultaneously for a new key, both may call incr before either calls
expire, causing the second request to skip the expire call. Fix this by checking
the key's TTL after the increment operation regardless of the count value, and
set the expiry if the TTL is not already set (indicating it was never
initialized). Alternatively, use Redis commands that combine initialization and
expiration atomically (like SET with NX and EX options) instead of the separate
incr and expire calls on the client object.
In `@backend/tenants/tests/test_rate_limit.py`:
- Around line 5-31: The current RateLimitTests class relies on Redis being
unavailable to test the fallback code path, making tests brittle when Redis is
running. Add explicit mocking of the _get_redis_client method in the
RateLimitMiddleware class by patching it in the setUp method or using mock
decorators. Create separate test methods in RateLimitTests: one that mocks
_get_redis_client to return a valid Redis client and verifies rate limiting
blocks after the threshold with process_request, another that mocks
_get_redis_client to raise an exception to test the fallback path is properly
invoked, and additional tests that verify the per-path limits work correctly for
paths like /api/v1/auth/login/ and /api/v1/auth/register/. This ensures both the
primary Redis path and the fallback _requests dictionary path are explicitly
validated regardless of actual Redis availability.
---
Nitpick comments:
In `@backend/tenants/security.py`:
- Around line 28-29: Remove the unused class attributes MAX_REQUESTS and
WINDOW_SECONDS from the security class. These attributes are no longer
referenced in the codebase after the refactoring that introduced RATE_LIMITS and
DEFAULT_LIMIT configuration. Delete both the MAX_REQUESTS and WINDOW_SECONDS
attribute definitions to clean up the code and remove dead code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ec088f2-33b3-4e0e-aaee-ffc49b1753ee
📒 Files selected for processing (5)
backend/backend/settings.pybackend/campaigns/migrations/0010_merge_20260621_1723.pybackend/tenants/security.pybackend/tenants/tests/__init__.pybackend/tenants/tests/test_rate_limit.py
| count = client.incr(key) | ||
|
|
||
| if count == 1: | ||
| client.expire(key, window) |
There was a problem hiding this comment.
Race condition: incr + expire is not atomic.
If two requests arrive simultaneously for a new key, both may call incr before either calls expire. The second request sees count > 1 and skips the expire, but if the first request's expire hasn't executed yet, or if the first request crashes, the key may persist without a TTL indefinitely.
Use SET NX EX for initialization or check TTL after increment:
🐛 Proposed fix using TTL check
try:
client = self._get_redis_client()
count = client.incr(key)
- if count == 1:
+ if client.ttl(key) == -1:
client.expire(key, window)
if count > limit:This ensures TTL is set even if the count == 1 check was missed due to a race.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| count = client.incr(key) | |
| if count == 1: | |
| client.expire(key, window) | |
| count = client.incr(key) | |
| if client.ttl(key) == -1: | |
| client.expire(key, window) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tenants/security.py` around lines 46 - 49, The race condition exists
because client.incr(key) and client.expire(key, window) are separate non-atomic
operations. When two requests arrive simultaneously for a new key, both may call
incr before either calls expire, causing the second request to skip the expire
call. Fix this by checking the key's TTL after the increment operation
regardless of the count value, and set the expiry if the TTL is not already set
(indicating it was never initialized). Alternatively, use Redis commands that
combine initialization and expiration atomically (like SET with NX and EX
options) instead of the separate incr and expire calls on the client object.
| if ip in self._requests: | ||
| self._requests[ip] = [ | ||
| t for t in self._requests[ip] | ||
| if now - t < window | ||
| ] | ||
| else: | ||
| self._requests[ip] = [] |
There was a problem hiding this comment.
Fallback uses IP-only key, inconsistent with Redis per-path keying.
Redis keys are ratelimit:{path}:{ip}, but the in-memory fallback uses only ip as the key. During Redis outages, all paths share one rate limit per IP instead of having separate per-path limits.
🐛 Proposed fix
+ fallback_key = (request.path, ip)
+
- if ip in self._requests:
- self._requests[ip] = [
- t for t in self._requests[ip]
+ if fallback_key in self._requests:
+ self._requests[fallback_key] = [
+ t for t in self._requests[fallback_key]
if now - t < window
]
else:
- self._requests[ip] = []
+ self._requests[fallback_key] = []
- if len(self._requests[ip]) >= limit:
+ if len(self._requests[fallback_key]) >= limit:
return JsonResponse(
{"error": "Too many requests"},
status=429
)
- self._requests[ip].append(now)
+ self._requests[fallback_key].append(now)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tenants/security.py` around lines 65 - 71, The in-memory fallback
rate limiting uses only the IP address as the key in the self._requests
dictionary, but Redis uses a composite key format of path and IP. Update the
code to use a composite key (combining path and ip) instead of just ip when
accessing and storing entries in self._requests to ensure consistent per-path
rate limiting behavior between the Redis implementation and the in-memory
fallback during outages.
| status=429 | ||
| ) |
There was a problem hiding this comment.
Minor: Fix indentation alignment.
Line 76 has inconsistent indentation with extra leading space before status=429.
if len(self._requests[ip]) >= limit:
return JsonResponse(
{"error": "Too many requests"},
- status=429
+ status=429
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| status=429 | |
| ) | |
| if len(self._requests[ip]) >= limit: | |
| return JsonResponse( | |
| {"error": "Too many requests"}, | |
| status=429 | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tenants/security.py` around lines 76 - 77, The indentation on line 76
with the `status=429` parameter has extra leading space that does not align with
the surrounding code. Remove the extra leading space from the `status=429` line
to match the proper indentation level of the other parameters in the same
function call or response object definition.
| class RateLimitTests(TestCase): | ||
| def setUp(self): | ||
| RateLimitMiddleware._requests = {} | ||
| self.factory = RequestFactory() | ||
| self.middleware = RateLimitMiddleware(lambda request: None) | ||
|
|
||
| def test_api_request_allowed(self): | ||
| request = self.factory.get("/api/test/") | ||
| response = self.middleware.process_request(request) | ||
|
|
||
| self.assertIsNone(response) | ||
|
|
||
| def test_rate_limit_blocks_after_threshold(self): | ||
| # Use a very small limit for testing | ||
| self.middleware.DEFAULT_LIMIT = (2, 60) | ||
|
|
||
| request = self.factory.get("/api/test/") | ||
|
|
||
| # First two requests should pass | ||
| self.assertIsNone(self.middleware.process_request(request)) | ||
| self.assertIsNone(self.middleware.process_request(request)) | ||
|
|
||
| # Third request should be blocked | ||
| response = self.middleware.process_request(request) | ||
|
|
||
| self.assertIsNotNone(response) | ||
| self.assertEqual(response.status_code, 429) No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any existing Redis mocks or test utilities in the codebase
rg -l "mock.*redis|patch.*redis" --type=pyRepository: Kuldeeep18/LeadOrbit
Length of output: 46
🏁 Script executed:
# Find the RateLimitMiddleware implementation
find . -name "*.py" -type f | xargs grep -l "class RateLimitMiddleware" | head -5Repository: Kuldeeep18/LeadOrbit
Length of output: 94
🏁 Script executed:
# Check the test file mentioned in the review
cat -n backend/tenants/tests/test_rate_limit.py | head -50Repository: Kuldeeep18/LeadOrbit
Length of output: 1342
🏁 Script executed:
# Search for _get_redis_client method in the codebase
rg "_get_redis_client" --type=py -A 5Repository: Kuldeeep18/LeadOrbit
Length of output: 742
🏁 Script executed:
# Look for any Redis usage in the middleware or rate limiting code
rg "redis" -i --type=py -l | head -10Repository: Kuldeeep18/LeadOrbit
Length of output: 120
🏁 Script executed:
# Get the full RateLimitMiddleware implementation
cat -n backend/tenants/security.pyRepository: Kuldeeep18/LeadOrbit
Length of output: 3637
🏁 Script executed:
# Check if there's any try-except or error handling around Redis in the process_request
rg "process_request" -A 30 backend/tenants/security.pyRepository: Kuldeeep18/LeadOrbit
Length of output: 872
Add explicit Redis mocking to test both primary and fallback paths.
The tests currently rely on Redis being unavailable to exercise the fallback code path. When Redis is running, the primary path (lines 45–55) executes instead, making the _requests reset ineffective and tests brittle.
Mock _get_redis_client to test the Redis success path explicitly. Add separate tests that mock it to raise an exception, ensuring the fallback path is validated:
- Test that Redis rate limiting blocks after threshold
- Test that fallback engages when Redis fails
- Test per-path limits (
/api/v1/auth/login/,/api/v1/auth/register/)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tenants/tests/test_rate_limit.py` around lines 5 - 31, The current
RateLimitTests class relies on Redis being unavailable to test the fallback code
path, making tests brittle when Redis is running. Add explicit mocking of the
_get_redis_client method in the RateLimitMiddleware class by patching it in the
setUp method or using mock decorators. Create separate test methods in
RateLimitTests: one that mocks _get_redis_client to return a valid Redis client
and verifies rate limiting blocks after the threshold with process_request,
another that mocks _get_redis_client to raise an exception to test the fallback
path is properly invoked, and additional tests that verify the per-path limits
work correctly for paths like /api/v1/auth/login/ and /api/v1/auth/register/.
This ensures both the primary Redis path and the fallback _requests dictionary
path are explicitly validated regardless of actual Redis availability.
|
@manikanta-tamminana ### Review: Request Changes ❌ Thank you for the implementation! The Redis integration and the fallback logic work nicely. However, there are a few issues that need to be addressed before merging:
|
|
Thanks for the review! I've addressed the requested changes:
Please take another look when you have a chance. |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
backend/tenants/security.py (3)
75-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix indentation alignment.
This issue was previously flagged. Line 75 has inconsistent indentation with extra leading space before
status=429.return JsonResponse( {"error": "Too many requests"}, - status=429 + status=429 )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tenants/security.py` at line 75, The line containing status=429 in the security.py file has extra leading whitespace causing misalignment with the surrounding code. Remove the extra spaces before status=429 to align it properly with other parameters at the same indentation level.
64-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback uses IP-only key, inconsistent with Redis per-path keying.
This issue was previously flagged. Redis keys are
ratelimit:{path}:{ip}, but the in-memory fallback uses onlyipas the key. During Redis outages, all paths share one rate limit per IP instead of having separate per-path limits.As suggested in the previous review, use a composite key:
fallback_key = (request.path, ip) if fallback_key in self._requests: self._requests[fallback_key] = [ t for t in self._requests[fallback_key] if now - t < window ] else: self._requests[fallback_key] = [] if len(self._requests[fallback_key]) >= limit: return JsonResponse( {"error": "Too many requests"}, status=429 ) self._requests[fallback_key].append(now)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tenants/security.py` around lines 64 - 72, The fallback rate limiting uses only the IP address as the key in the self._requests dictionary, while Redis uses per-path keys. This causes all paths to share the same rate limit during Redis outages. Replace all uses of ip as the dictionary key with a composite key using both request.path and ip (such as a tuple). Update every reference including the conditional check for membership, all assignments to self._requests, the length comparison, and any append operations to consistently use this composite key throughout the method.
45-48:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffRace condition:
incr+expireis not atomic.This issue was previously flagged. If two requests arrive simultaneously for a new key, both may call
incrbefore either callsexpire. The second request seescount > 1and skips theexpire, but if the first request'sexpirehasn't executed yet, the key may persist without a TTL indefinitely.As suggested in the previous review, use a TTL check after increment to ensure expiry is set:
if client.ttl(key) == -1: client.expire(key, window)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tenants/security.py` around lines 45 - 48, The race condition exists because the check for `if count == 1:` does not guarantee atomicity when setting the TTL. Replace the condition that checks `if count == 1:` with `if client.ttl(key) == -1:` before calling `client.expire(key, window)`. This ensures that the expire is set whenever the key does not have a TTL already configured, preventing the key from persisting indefinitely due to race conditions between the incr and expire operations.
🧹 Nitpick comments (2)
backend/tenants/security.py (2)
27-28: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove unused legacy class attributes.
MAX_REQUESTSandWINDOW_SECONDSare no longer used. The new implementation usesRATE_LIMITSandDEFAULT_LIMITinstead (lines 20-24).🧹 Proposed cleanup
DEFAULT_LIMIT = (100, 60) # { ip_address: [timestamp1, timestamp2, ...] } _requests = {} - MAX_REQUESTS = 100 # per window - WINDOW_SECONDS = 60🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tenants/security.py` around lines 27 - 28, Remove the unused class attributes MAX_REQUESTS and WINDOW_SECONDS from the class definition, as they have been replaced by the new implementation using RATE_LIMITS and DEFAULT_LIMIT. These legacy attributes are no longer referenced anywhere in the codebase and can be safely deleted to clean up technical debt.
64-78: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffConsider bounded fallback dict to prevent memory growth during extended outages.
While per-IP timestamp cleanup prevents unbounded growth per IP (lines 65-68), the
_requestsdict itself grows as new IPs are added during Redis outages. Over an extended outage, this could consume significant memory.Consider adding periodic cleanup of IPs that haven't made requests recently, or implement an LRU cache with a maximum size.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tenants/security.py` around lines 64 - 78, The `_requests` dictionary can grow unbounded as new IP addresses are added during extended Redis outages, even though timestamps per IP are cleaned up. To fix this, implement a bounded storage mechanism for the `_requests` dict by either adding periodic cleanup logic that removes IPs that haven't made requests recently (track the last request time per IP and remove entries older than a threshold), or replace the plain dict with an LRU cache data structure that automatically evicts the least recently used IPs when reaching a maximum size limit. This ensures the total memory footprint remains bounded regardless of how many unique IPs attempt to make requests during an outage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@backend/tenants/security.py`:
- Line 75: The line containing status=429 in the security.py file has extra
leading whitespace causing misalignment with the surrounding code. Remove the
extra spaces before status=429 to align it properly with other parameters at the
same indentation level.
- Around line 64-72: The fallback rate limiting uses only the IP address as the
key in the self._requests dictionary, while Redis uses per-path keys. This
causes all paths to share the same rate limit during Redis outages. Replace all
uses of ip as the dictionary key with a composite key using both request.path
and ip (such as a tuple). Update every reference including the conditional check
for membership, all assignments to self._requests, the length comparison, and
any append operations to consistently use this composite key throughout the
method.
- Around line 45-48: The race condition exists because the check for `if count
== 1:` does not guarantee atomicity when setting the TTL. Replace the condition
that checks `if count == 1:` with `if client.ttl(key) == -1:` before calling
`client.expire(key, window)`. This ensures that the expire is set whenever the
key does not have a TTL already configured, preventing the key from persisting
indefinitely due to race conditions between the incr and expire operations.
---
Nitpick comments:
In `@backend/tenants/security.py`:
- Around line 27-28: Remove the unused class attributes MAX_REQUESTS and
WINDOW_SECONDS from the class definition, as they have been replaced by the new
implementation using RATE_LIMITS and DEFAULT_LIMIT. These legacy attributes are
no longer referenced anywhere in the codebase and can be safely deleted to clean
up technical debt.
- Around line 64-78: The `_requests` dictionary can grow unbounded as new IP
addresses are added during extended Redis outages, even though timestamps per IP
are cleaned up. To fix this, implement a bounded storage mechanism for the
`_requests` dict by either adding periodic cleanup logic that removes IPs that
haven't made requests recently (track the last request time per IP and remove
entries older than a threshold), or replace the plain dict with an LRU cache
data structure that automatically evicts the least recently used IPs when
reaching a maximum size limit. This ensures the total memory footprint remains
bounded regardless of how many unique IPs attempt to make requests during an
outage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ad9a15e3-f28d-40c9-9a50-dbc44c8d22f4
📒 Files selected for processing (1)
backend/tenants/security.py
2 participants
Pull Request
🔗 Related Issue
Closes #336
📝 Summary of Changes
Implemented Redis-backed rate limiting with graceful fallback support.
Changes Made
REDIS_URLsetting.🏷️ Type of Change
🧪 Testing
Verified functionality using Django system checks and automated tests.
Steps to test:
python backend/manage.py checkpython backend/manage.py test tenants.tests.test_rate_limit📸 Screenshots (if applicable)
N/A
✅ Checklist
Summary by CodeRabbit
Infrastructure & Reliability
REDIS_URL(with a local default).Tests