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.
There was a problem hiding this comment.
Correction: that "fixed" reply above was also inaccurate - the fail-closed change was part of the same stranded commit I mentioned in the other thread on this file, and never actually reached this PR. Apologies for the confusion.
Having looked at this more carefully, I'd like to actually keep fail-open rather than switch to fail-closed, and wanted to explain the reasoning rather than just silently leave it as-is. Rate limiting here is a soft quota guard on LLM usage, not an auth/security control - it doesn't gate access to anything sensitive. If it fails closed, a Redis outage (even a brief one) would take down every agent-triggering endpoint platform-wide (chat, vendor onboarding, invoice processing) since they all depend on this same dependency. That feels like a worse outcome than the actual risk being guarded against, which is a namespace temporarily sending more LLM requests than intended during a rare outage window.
Completely open to being wrong here if there's a stronger reason to prioritize fail-closed for this specific case - happy to discuss further if you still think it should block instead.
|
|
||
| # 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.
There was a problem hiding this comment.
Update: the pipeline approach I described above turned out to have real problems once I dug into it - it also switched EXPIRE to run on every request (not just the first, changing the fixed-window behavior) and it dropped the Retry-After header along the way. It was also accidentally left stranded on a branch and never actually reached this PR, so this thread has been unresolved longer than my earlier reply suggested - sorry about that.
Replaced it with a single atomic Lua script (EVAL) that does the INCR and conditionally sets EXPIRE (only on the first request in the window) in one Redis round trip, in commit ca197e0. This fully removes the race you flagged - there's no window between the two ops where a crash could leave an orphaned key - while keeping the original fixed-window semantics and the Retry-After header intact.
Verified live against real Redis in Docker (10 requests succeed, 11th correctly returns 429, no fail-open errors logged) and via the full test suite (398 passed, 0 regressions).
…n _extract_texts across all 6 detectors
…ed on Redis error
mekaizen flagged a race condition on PR GenAI-Security-Project#532: if the process crashed or lost its Redis connection between the separate INCR and EXPIRE calls, a key could be left permanently incremented with no TTL, silently blocking that namespace forever. A previous attempt to fix this (commit a03481a) used a Redis pipeline and switched the limiter to fail-closed on Redis errors, but that commit was accidentally left stranded on a feature branch and never merged - and it also changed EXPIRE to run on every request instead of only the first, altering the fixed-window semantics, and would have broken the existing test_expire_only_set_on_first_request test and removed the just-added Retry-After header. This commit takes a narrower approach: a single atomic Lua script (EVAL) that performs the INCR and conditionally sets EXPIRE (only on the first request in the window) in one Redis round trip. This fully eliminates the race - there is no window between the two operations where a crash could leave an orphaned key - while preserving the original fixed-window behavior, the Retry-After header, and the deliberate fail-open design (rate limiting is a soft quota guard, not an auth control, so a Redis outage should not take down agent endpoints entirely). Test suite updated to mock eval() instead of incr()/expire() separately, with a new test asserting the operation happens in a single call. Verified live against real Redis in Docker: 10 requests succeed, 11th correctly returns 429, no fail-open errors logged. Full suite: 398 passed, 26 skipped, 6 pre-existing failures unchanged, 0 regressions.
Commit a03481a on this branch switched INCR+EXPIRE to a Redis pipeline and changed the limiter to fail-closed (503) on Redis errors. That approach had three problems: it ran EXPIRE on every request instead of only the first (changing the fixed-window semantics), it dropped the Retry-After header and negative-TTL guard, and fail-closed would mean any Redis outage takes down agent endpoints entirely. Replaced with a single atomic Lua script (EVAL) that performs the INCR and conditionally sets EXPIRE only on the first request in the window - fully eliminating the original race condition without the above side effects. Kept the deliberate fail-open design: rate limiting is a soft quota guard, not an auth control. Verified live against real Redis in Docker (10 requests succeed, 11th correctly returns 429) and via the full test suite (398 passed, 0 regressions).
|
Closing this in favor of a single combined final submission: #567, which includes all the real work from this PR plus the fixes discussed above (see the threads on this PR for details on what changed and why). |
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).