Skip to content

Add user-specific rate limits with admin API management#440

Open
msaroufim wants to merge 2 commits intomainfrom
claude/add-user-rate-limits-AatMo
Open

Add user-specific rate limits with admin API management#440
msaroufim wants to merge 2 commits intomainfrom
claude/add-user-rate-limits-AatMo

Conversation

@msaroufim
Copy link
Member

Introduces per-user submission rate limiting (hourly and daily caps)
configurable via the admin API. Users without an override are unrestricted.

  • New migration: leaderboard.user_rate_limits table
  • DB methods: get/set/delete user rate limits + submission rate checking
  • Admin endpoints: GET/PUT/DELETE /admin/rate-limits/{user_id}
  • Both submission endpoints check user rate limits (return 429 if exceeded)
  • Tests for DB methods and all API endpoints

https://claude.ai/code/session_01LiSWKfcKe4riGZwMYdubiJ

Introduces per-user submission rate limiting (hourly and daily caps)
configurable via the admin API. Users without an override are unrestricted.

- New migration: leaderboard.user_rate_limits table
- DB methods: get/set/delete user rate limits + submission rate checking
- Admin endpoints: GET/PUT/DELETE /admin/rate-limits/{user_id}
- Both submission endpoints check user rate limits (return 429 if exceeded)
- Tests for DB methods and all API endpoints

https://claude.ai/code/session_01LiSWKfcKe4riGZwMYdubiJ
Copilot AI review requested due to automatic review settings February 8, 2026 08:44
@github-actions
Copy link

github-actions bot commented Feb 8, 2026

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  src/libkernelbot
  leaderboard_db.py 1340
  utils.py
Project Total  

This report was generated by python-coverage-comment-action

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds per-user submission rate limiting (hourly/daily) with admin-managed overrides via new DB table + admin API endpoints, and enforces these limits on submission endpoints.

Changes:

  • Added leaderboard.user_rate_limits table + DB CRUD/check helpers for per-user submission caps.
  • Enforced rate-limit checks in both submission endpoints (429 on exceeded limits).
  • Added admin endpoints and tests for listing/getting/setting/deleting user rate limits.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
tests/test_leaderboard_db.py Adds DB-level tests for rate limit CRUD and enforcement behavior.
tests/test_admin_api.py Adds admin API tests for rate limit management endpoints.
tests/conftest.py Ensures new table is truncated between tests.
src/migrations/20260208_01_RkLmN-add-user-rate-limits.py Introduces the user_rate_limits table.
src/libkernelbot/leaderboard_db.py Implements rate limit CRUD plus submission-rate checking logic.
src/kernelbot/api/main.py Enforces per-user rate limits on submission endpoints and adds admin endpoints.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +707 to +710
if max_per_hour is not None and (not isinstance(max_per_hour, int) or max_per_hour < 0):
raise HTTPException(status_code=400, detail="max_submissions_per_hour must be a non-negative integer")

if max_per_day is not None and (not isinstance(max_per_day, int) or max_per_day < 0):
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isinstance(x, int) also accepts booleans (True/False), which would silently become limits of 1/0. Use a stricter check (e.g., type(x) is int) to ensure only real integers are accepted.

Suggested change
if max_per_hour is not None and (not isinstance(max_per_hour, int) or max_per_hour < 0):
raise HTTPException(status_code=400, detail="max_submissions_per_hour must be a non-negative integer")
if max_per_day is not None and (not isinstance(max_per_day, int) or max_per_day < 0):
if max_per_hour is not None and (type(max_per_hour) is not int or max_per_hour < 0):
raise HTTPException(status_code=400, detail="max_submissions_per_hour must be a non-negative integer")
if max_per_day is not None and (type(max_per_day) is not int or max_per_day < 0):

Copilot uses AI. Check for mistakes.
Comment on lines +684 to +689
async def set_user_rate_limit(
user_id: str,
payload: dict,
_: Annotated[None, Depends(require_admin)],
db_context=Depends(get_db),
) -> dict:
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using an untyped payload: dict loses FastAPI schema/validation and makes the API contract ambiguous. Prefer a Pydantic model (optional max_submissions_per_hour, max_submissions_per_day, note) so invalid types are rejected consistently and the OpenAPI docs reflect the request body.

Copilot uses AI. Check for mistakes.
Comment on lines +1256 to +1270
self.cursor.execute(
"""
INSERT INTO leaderboard.user_rate_limits
(user_id, max_submissions_per_hour, max_submissions_per_day, note)
VALUES (%s, %s, %s, %s)
ON CONFLICT (user_id) DO UPDATE SET
max_submissions_per_hour = EXCLUDED.max_submissions_per_hour,
max_submissions_per_day = EXCLUDED.max_submissions_per_day,
note = EXCLUDED.note,
updated_at = NOW()
RETURNING user_id, max_submissions_per_hour, max_submissions_per_day,
note, created_at, updated_at
""",
(user_id, max_submissions_per_hour, max_submissions_per_day, note),
)
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This insert is subject to the FK constraint (user_rate_limits.user_iduser_info.id). If an admin tries to set a limit for a non-existent user, Postgres will raise an integrity error that currently becomes a generic KernelBotError (likely surfacing as a 500). Catch integrity violations and return a domain-specific error so the API can respond with a clear 400/404.

Copilot uses AI. Check for mistakes.
Comment on lines +13 to +20
CREATE TABLE leaderboard.user_rate_limits (
user_id TEXT PRIMARY KEY REFERENCES leaderboard.user_info(id),
max_submissions_per_hour INTEGER,
max_submissions_per_day INTEGER,
note TEXT,
created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(),
updated_at TIMESTAMP WITH TIME ZONE DEFAULT NOW()
);
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The schema allows negative limits, which would invert enforcement semantics and can slip in via non-API callers. Add CHECK constraints to enforce max_submissions_per_hour >= 0 / max_submissions_per_day >= 0 (while still allowing NULL).

Copilot uses AI. Check for mistakes.
Comment on lines +1350 to +1359
self.cursor.execute(
"""
SELECT
COUNT(*) FILTER (WHERE submission_time > NOW() - INTERVAL '1 hour') AS hourly_count,
COUNT(*) FILTER (WHERE submission_time > NOW() - INTERVAL '1 day') AS daily_count
FROM leaderboard.submission
WHERE user_id = %s
""",
(user_id,),
)
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This query runs on every submission for rate-limited users and will become expensive as leaderboard.submission grows unless it can use an index efficiently. Consider adding an index like (user_id, submission_time) (or confirm an existing one) to keep the hourly/daily counts fast.

Copilot uses AI. Check for mistakes.
with db_context as db:
rate_check = db.check_user_submission_rate(user_info["user_id"])
if not rate_check["allowed"]:
raise HTTPException(status_code=429, detail=f"Rate limit exceeded. {rate_check['retry_after']}")
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 429 responses, it’s best practice to include a Retry-After header (seconds or HTTP date). Since retry_after is currently a human string, consider also returning a machine-readable duration and setting HTTPException(..., headers={'Retry-After': ...}).

Suggested change
raise HTTPException(status_code=429, detail=f"Rate limit exceeded. {rate_check['retry_after']}")
retry_after_header = str(rate_check.get("retry_after_seconds", rate_check["retry_after"]))
raise HTTPException(
status_code=429,
detail=f"Rate limit exceeded. {rate_check['retry_after']}",
headers={"Retry-After": retry_after_header},
)

Copilot uses AI. Check for mistakes.
Comment on lines +469 to +475
def test_set_user_rate_limit(self, test_client, mock_backend):
"""PUT /admin/rate-limits/{user_id} sets rate limit."""
self._setup_db_mock(mock_backend)
mock_backend.db.set_user_rate_limit = MagicMock(return_value={
"user_id": "123", "max_submissions_per_hour": 5, "max_submissions_per_day": 20,
"note": "restricted", "created_at": None, "updated_at": None,
})
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the FK constraint in the migration, setting a rate limit for a non-existent user_id is an important error path. Add an admin API test that PUT /admin/rate-limits/{user_id} returns a clear 400/404 when the user doesn’t exist (once the handler is updated to translate the DB integrity error).

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants