Conversation
…kends Adds ASGI middleware that rate-limits MCP requests per Jira/Confluence username. Token-to-user mapping is cached after first auth validation, so multiple tokens for the same user share one rate limit bucket. Config via env vars: - RATE_LIMIT_ENABLED (default: false) - RATE_LIMIT_RPM (default: 60) - RATE_LIMIT_BURST (default: 20) - RATE_LIMIT_REDIS_URL (optional, enables Redis sliding-window backend)
- Refactor RedisBackend to use pipeline commands instead of Lua scripts for compatibility with fakeredis in tests - Add Redis tests using fakeredis (6 tests, all passing) - Add redis service to e2e docker-compose - Add redis as dev dependency
…e limiting Cover RateLimitMiddleware ASGI behavior (passthrough, 429 response), _register_rate_limit_user edge cases, and Redis fail-open on connection errors.
- TTLCache takes 2 type args, not 3 - Apply ruff-format to test file
Tests against a real Redis instance: sliding window, burst, TTL, key independence, usage tracking, and token-to-user mapping. Skipped automatically when Redis is unavailable or --integration flag is not passed.
Add port mapping 6379:6379 so host-side integration tests can reach Redis. Update README with docker-compose instructions.
AmirF194
left a comment
There was a problem hiding this comment.
RedisBackend.is_allowed() (src/mcp_atlassian/utils/rate_limiter.py:111-129) checks the count and adds the new entry in two separate pipelines, with no lock or atomic conditional between them:
count = self._prune_and_count(redis_key) # pipeline 1: prune + zcard
if count >= limit:
return False
...
pipe.zadd(redis_key, {member: now}) # pipeline 2: add
pipe.expire(redis_key, 61)
pipe.execute()
Under concurrent requests this is a check-then-act race: several requests can all read a count under the limit before any of them adds their entry, so the effective limit is not enforced. I reproduced it in Docker against a real Redis (head 871bf2fa, python:3.12-slim, redis-server in-container): 30 concurrent calls to is_allowed(key, rpm=5, burst=0) let 24-30 of them through across three runs, against a limit of 5. InMemoryBackend.is_allowed() (same file, holds a single Lock across the whole check-and-append) stayed correctly capped at 5 in the same test.
[redis] limit=5 concurrent_requests=30 allowed=30 (expected <= 5)
[in-memory] limit=5 concurrent_requests=30 allowed=5 (expected <= 5)
Blocking: this is the Redis backend's whole purpose (rate limiting under concurrent/multi-instance load), and none of the added tests exercise it concurrently, they all call is_allowed sequentially in a loop, so the race doesn't show up there. Making the check-and-add atomic server-side, for example a small Lua script via EVAL that does the prune, count check, and conditional ZADD in one round trip, would close this. Happy to share the repro script if useful.
|
Thanks @AmirF194, added a failing concurrent test case, and fixed by switching from sorted-set sliding window to INCR fixed-window counters. This way we ensure atomic operation while avoiding EVAL. The tradeoff is fixed-window can allow up to 2x burst at minute boundaries, but this matches what most production rate limiters do (Stripe, GitHub) and is negligible at 60s windows. |
InMemoryBackend passes (lock-protected). RedisBackend fails — 30/30 requests allowed through a limit of 5 due to check-then-act race in is_allowed(). Github-Issue:sooperset#1554
Replace sorted-set sliding window with atomic INCR fixed-window counters. INCR is a single atomic Redis operation — no gap between check and set, so concurrent requests can no longer bypass the limit. Github-Issue:sooperset#1554
- Fix RedisBackend docstring: "sliding window" → "fixed-window INCR" - Add redis as optional extra in pyproject.toml for production use - Compute actual retry_after from window boundary instead of hardcoded 60s - Fix misleading get_usage log: remove inflated counter from 429 warning - Evict inactive InMemoryBackend entries via TTLCache (prevents slow leak) Github-Issue:sooperset#1554
InMemoryBackend passes (lock-protected). RedisBackend fails — 30/30 requests allowed through a limit of 5 due to check-then-act race in is_allowed(). Github-Issue:sooperset#1554
Replace sorted-set sliding window with atomic INCR fixed-window counters. INCR is a single atomic Redis operation — no gap between check and set, so concurrent requests can no longer bypass the limit. Github-Issue:sooperset#1554
- Fix RedisBackend docstring: "sliding window" → "fixed-window INCR" - Add redis as optional extra in pyproject.toml for production use - Compute actual retry_after from window boundary instead of hardcoded 60s - Fix misleading get_usage log: remove inflated counter from 429 warning - Evict inactive InMemoryBackend entries via TTLCache (prevents slow leak) Github-Issue:sooperset#1554
Description
Add optional per-user rate limiting for the Streamable HTTP transport. When multiple users share a single MCP server, one runaway client can monopolize the Atlassian API. This adds a configurable sliding-window rate limiter with both in-memory (default) and Redis backends.
Changes
rate_limiter.pywithInMemoryBackend(deque-based sliding window) andRedisBackend(sorted sets via pipeline), plusRateLimiterwith token-to-user cacheRateLimitMiddlewareASGI middleware inservers/main.py— returns 429 withRetry-Afterheader when limit is exceededservers/dependencies.pyso rate limits track by username, not just token hashdocker-compose.ymlwith port mapping for integration testsredis>=7.2.0as dev dependencyConfiguration
RATE_LIMIT_ENABLEDfalseRATE_LIMIT_RPM60RATE_LIMIT_BURST20RATE_LIMIT_REDIS_URLTesting
tests/unit/utils/test_rate_limiter.py)tests/integration/test_rate_limiter_redis.pyagainst real Redis)Checklist