Repository navigation
Add project request rate limits - #35
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds configurable, Redis-backed two-window request rate limiting for selected inference routes, including project-specific policies, startup verification, structured rejection responses, dashboard policy visibility, documentation, and extensive unit/integration coverage. ChangesRequest rate limiting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant proxy_request
participant RedisRequestRateLimiter
participant Redis
Client->>proxy_request: POST inference request
proxy_request->>RedisRequestRateLimiter: check effective policy
RedisRequestRateLimiter->>Redis: atomic Lua admission
Redis-->>RedisRequestRateLimiter: decision
RedisRequestRateLimiter-->>proxy_request: allow or rate-limit response
proxy_request-->>Client: upstream response, 429, or 503
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Redis-backed, two-window request rate limiter for SmolRouter, applying limits to inference POST routes for both anonymous and project-identified traffic. The implementation includes atomic Lua scripting for concurrency safety, configuration parsing, and UI updates to display effective policies. The review feedback highlights several improvement opportunities: expanding the production environment check to match both "prod" and "production", simplifying the policy resolution logic in _apply_request_rate_limit, preventing exception masking in the rate limiter's verification cleanup, adding traceback logging for backend failures, and safely handling potential None values in the Jinja templates to avoid rendering crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| config = _request_rate_limit_config(container) | ||
| if not config.enabled: | ||
| return | ||
| if os.getenv("APP_ENV", "dev").lower() == "prod" and is_fake_redis(): |
There was a problem hiding this comment.
The check for production environment only matches "prod". If the environment is configured as "production", this critical safety check (which prevents using FakeRedis in production) will be bypassed. It is safer to check for both "prod" and "production".
| if os.getenv("APP_ENV", "dev").lower() == "prod" and is_fake_redis(): | |
| if os.getenv("APP_ENV", "dev").lower() in ("prod", "production") and is_fake_redis(): |
| policy = config.anonymous | ||
| if context.active_container is not None: | ||
| effective_policy = context.active_container.get_effective_request_rate_limit_policy(context.identity) | ||
| if isinstance(effective_policy, RequestRateLimitPolicy): | ||
| policy = effective_policy | ||
| elif context.identity is not None: | ||
| policy = context.identity.rate_limit_policy or config.project_default |
There was a problem hiding this comment.
If context.active_container is None (e.g., during startup/initialization failure or legacy proxy routing), policy defaults to config.anonymous even for identified project requests. Initializing policy to config.project_default for identified projects ensures correct fallback behavior. Additionally, since get_effective_request_rate_limit_policy always returns a RequestRateLimitPolicy, we can simplify the resolution logic and eliminate the dead code block.
| policy = config.anonymous | |
| if context.active_container is not None: | |
| effective_policy = context.active_container.get_effective_request_rate_limit_policy(context.identity) | |
| if isinstance(effective_policy, RequestRateLimitPolicy): | |
| policy = effective_policy | |
| elif context.identity is not None: | |
| policy = context.identity.rate_limit_policy or config.project_default | |
| policy = config.project_default if context.identity else config.anonymous | |
| if context.active_container is not None: | |
| policy = context.active_container.get_effective_request_rate_limit_policy(context.identity) |
| finally: | ||
| await self._client.delete(verification_key) |
There was a problem hiding this comment.
If self._client.eval raises an exception (e.g., due to a Lua script compilation error or Redis permission issue), the finally block will attempt to delete the verification key. If Redis is down or has connection issues, delete will also raise an exception, masking the original primary exception and making troubleshooting extremely difficult. Wrapping the cleanup in a try-except block prevents masking the primary error.
finally:
try:
await self._client.delete(verification_key)
except Exception:
passReferences
- When updating shared data structures in Redis under concurrent conditions, prefer using atomic Lua scripts for the primary path to ensure concurrency safety, or native atomic commands as a fallback, rather than non-atomic read-modify-write patterns.
| ) | ||
| except Exception: | ||
| self._backend_error_count += 1 | ||
| logger.warning("Request rate-limit backend check failed") |
There was a problem hiding this comment.
Logging the rate-limit backend failure without the exception traceback makes diagnosing Redis connection timeouts, authentication failures, or other issues extremely difficult in production. Adding exc_info=True will include the traceback in the warning log.
| logger.warning("Request rate-limit backend check failed") | |
| logger.warning("Request rate-limit backend check failed", exc_info=True) |
| {% for window in project.request_rate_limit.policy.windows %} | ||
| <div class="quota-value">{{ window.requests }} request{{ "s" if window.requests != 1 else "" }} / {{ window.seconds }}s</div> | ||
| {% endfor %} | ||
| <div class="quota-value">{{ project.request_rate_limit.source | replace('_', ' ') }}</div> |
There was a problem hiding this comment.
If project.request_rate_limit.source is None (which can happen if a project is unconfigured or in future edge cases), calling replace directly on it will raise a Jinja template rendering error and crash the entire dashboard page. Using a default filter or logical OR ensures it is always treated as a string before calling replace.
<div class="quota-value">{{ (project.request_rate_limit.source or "") | replace('_', ' ') }}</div>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92bf706a34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if active_container is None: | ||
| return _default_request_rate_limit_config() |
There was a problem hiding this comment.
When container initialization has fallen back to legacy mode, callers reach this helper with active_container is None both during startup verification and request handling, so a routes file that explicitly sets request_rate_limits.enabled: false is replaced with the default enabled policy. In that fallback scenario the proxy will still verify Redis and enforce the 1/s, 3/10s anonymous limit, causing unexpected 429/503 responses even though the operator disabled request limiting.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
smolrouter/app.py (1)
1939-1955: 🩺 Stability & Availability | 🔵 TrivialFail-closed on limiter errors makes Redis a hard dependency for all proxy traffic. Any
check(...)failure returns 503, so a Redis outage rejects every proxied request, not just those that would have been throttled. This is a deliberate posture per the PR, but consider making the degradation strategy explicit/configurable (e.g. fail-open for anonymous/low-risk traffic, or a short-lived circuit breaker) so a transient backend blip doesn't translate into a full proxy outage. Ensure this behavior and its blast radius are covered by alerting.🤖 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 `@smolrouter/app.py` around lines 1939 - 1955, The rate-limiter exception path currently fails closed for every proxied request, making Redis a hard dependency. Update the handling around the limiter check and its `except Exception` branch to make the degradation strategy explicitly configurable, supporting an appropriate fail-open path for anonymous or low-risk traffic while preserving strict rejection where required; also add alerting/metrics that identify limiter failures and their affected traffic scope.smolrouter/request_rate_limits.py (1)
359-370: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse a registered Redis script here
_CHECK_RATE_LIMIT_LUAis executed on the per-request path, soregister_script/EVALSHAwould avoid sending and parsing the full Lua body on every admission. Reuse the same registered script inverify()too.🤖 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 `@smolrouter/request_rate_limits.py` around lines 359 - 370, Update the rate-limit admission flow around the eval call in the relevant verification method to reuse a script registered once with Redis, invoking it through the registered-script callable rather than sending _CHECK_RATE_LIMIT_LUA on every request. Apply the same registered script reuse in verify(), while preserving the existing keys and arguments.
🤖 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 `@smolrouter/app.py`:
- Around line 1828-1849: The 429 payload from _rate_limit_response_payload uses
the project-specific error code for anonymous callers. Make the code
bucket-specific (using an anonymous rate-limit code for the anonymous bucket
while preserving the project code), and update the corresponding assertion in
tests/unit/test_app.py to match.
---
Nitpick comments:
In `@smolrouter/app.py`:
- Around line 1939-1955: The rate-limiter exception path currently fails closed
for every proxied request, making Redis a hard dependency. Update the handling
around the limiter check and its `except Exception` branch to make the
degradation strategy explicitly configurable, supporting an appropriate
fail-open path for anonymous or low-risk traffic while preserving strict
rejection where required; also add alerting/metrics that identify limiter
failures and their affected traffic scope.
In `@smolrouter/request_rate_limits.py`:
- Around line 359-370: Update the rate-limit admission flow around the eval call
in the relevant verification method to reuse a script registered once with
Redis, invoking it through the registered-script callable rather than sending
_CHECK_RATE_LIMIT_LUA on every request. Apply the same registered script reuse
in verify(), while preserving the existing keys and arguments.
🪄 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
Run ID: 4361790a-abb6-46c4-8a83-bfa6f228620a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
README.mdconfig/routes.yaml.examplepyproject.tomlrequirements.txtsmolrouter/app.pysmolrouter/container.pysmolrouter/facade_keys.pysmolrouter/request_rate_limits.pysmolrouter/templates/project_detail.htmlsmolrouter/templates/projects.htmltests/unit/test_app.pytests/unit/test_container.pytests/unit/test_facade_keys.pytests/unit/test_request_rate_limits.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #35 +/- ##
==========================================
+ Coverage 74.88% 75.43% +0.55%
==========================================
Files 36 37 +1
Lines 10008 10275 +267
Branches 1557 1599 +42
==========================================
+ Hits 7494 7751 +257
- Misses 1937 1939 +2
- Partials 577 585 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Summary
Safety and behavior
Validation
Summary by CodeRabbit
New Features
Documentation
request_rate_limits), route coverage/exclusions, and rate-limit response semantics; updated the example routes config.Bug Fixes
Tests