feat: add per-namespace agent rate limiting - #532
Conversation
ashike24
changed the title
- New finbot/mcp_server/ package with submit_tool_call and get_scoring_results tools, wrapping VendorChatAssistant and UserChallengeProgressRepository respectively - Auth via existing SessionManager (session_id -> SessionContext), no new auth mechanism needed - Runs as separate Docker service (mcp_server) on port 8100, sharing sqlite_data volume + Redis with the main app - Fixed pre-existing .env bug: DATABASE_URL pointed outside the mounted sqlite_data volume, causing data loss on container rebuild for both services - Verified end-to-end: get_scoring_results fully working; submit_tool_call reaches the LLM call correctly, blocked only by a missing OPENAI_API_KEY in local dev env (pre-existing gap, unrelated to this change) - start_challenge_session from original proposal wording dropped: no such concept exists in the codebase; sessions already exist via login and challenges progress implicitly
| try: | ||
| # Fix 2: Explicitly guard Redis initialization | ||
| redis = getattr(event_bus, "redis", None) | ||
| if redis is None: |
There was a problem hiding this comment.
Fail-open rate limiting
If Redis is unavailable or not initialized, the middleware allows every request through, effectively disabling rate limiting. For LLM/agent endpoints, this creates a potential abuse vector during Redis outages.
Consider failing closed (HTTP 503/429) or using an in-memory fallback rate limiter instead of bypassing rate limiting completely.
There was a problem hiding this comment.
Fixed. The limiter now fails closed - if Redis is unavailable or uninitialized, the request is blocked with HTTP 503 rather than allowed through.
|
|
||
| # On the first request in a window, set the expiry | ||
| if count == 1: | ||
| await redis.expire(key, window_seconds) |
There was a problem hiding this comment.
can you check this -
Race condition between INCR and EXPIRE
INCR and EXPIRE are executed as separate Redis commands. If the process crashes or is interrupted after INCR succeeds but before EXPIRE executes, the key is created without a TTL. Since subsequent requests will observe count > 1, the expiration is never set, leaving a permanent rate-limit key.
Consider making these operations atomic using a Redis transaction/pipeline
There was a problem hiding this comment.
Fixed. INCR and EXPIRE are now executed atomically using a Redis pipeline with transaction=True, eliminating the race condition where a crash between the two commands could leave a permanent key with no TTL.
…n _extract_texts across all 6 detectors
…ed on Redis error
2 participants
Summary
Adds per-namespace rate limiting to all agent-triggering endpoints in the FinBot vendor portal, protecting the shared LLM quota from exhaustion and ensuring fair access across namespaces.
Motivation
Without rate limiting, a single namespace generating a burst of requests could consume the entire available LLM quota, degrading the experience for every other user of the platform. This change caps the number of agent-invoking requests per namespace within a fixed time window.
Changes
finbot/config.pyAGENT_RATE_LIMIT_MAX(default: 10) andAGENT_RATE_LIMIT_WINDOW_SECONDS(default: 60) to theSettingsclass with full env var override support.finbot/core/ratelimit/limiter.py(new)finbot:ratelimit:{namespace}:agent.event_bus.redisasync client — no second Redis connection.HTTP 429with a descriptive message including current count, max, and TTL remaining.finbot/core/ratelimit/__init__.py(new)finbot/apps/vendor/routes/api.pyDepends(check_agent_rate_limit)to 5 agent-triggering routes:POST /vendors/register,POST /vendors/{vendor_id}/request-review,POST /invoices,POST /invoices/{invoice_id}/reprocess, andPOST /chat.Test Results
10 integration tests added in
tests/integration/test_rate_limiting.py, all passing. No regressions in the existing suite (382 passed, 6 pre-existing failures unchanged).