Feat rate limit #386

S1ro1 · 2025-12-21T01:28:42Z

implements rate limit (fully vibecoded)

allows per user override (for trusted people/us etc)
gettable/settable with admin commands on discord
if set to i.e. 10submissions/min, it allows a submission at most each 60s/10 -> 6s

Copilot

Pull request overview

This PR implements a rate limiting feature for submissions using a token bucket algorithm. It adds support for configurable default rate limits and per-user overrides to allow trusted users to bypass or have custom limits. The implementation includes database tables for storing rate limit settings and state, enforcement logic in the submission pipeline, and Discord admin commands for configuration.

Database migrations create three new tables for rate limit settings, user overrides, and token bucket state
Token bucket algorithm enforces rate limits with configurable submissions-per-minute and bucket capacity
Discord admin commands allow getting/setting default and per-user rate limits

Reviewed changes

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

Show a summary per file

File	Description
src/migrations/20251221_01_kRtL1-submission-rate-limit.py	Adds database tables for rate limit settings, per-user overrides, and token bucket state tracking
src/libkernelbot/leaderboard_db.py	Implements token bucket rate limiting with methods to get/set rate limits and enforce them during submission
src/libkernelbot/submission.py	Integrates rate limit enforcement into the submission preparation flow
src/kernelbot/cogs/admin_cog.py	Adds Discord admin commands to configure default and per-user rate limits
src/kernelbot/api/main.py	Updates error handling to properly propagate rate limit errors as HTTP 429 responses and includes code formatting improvements
src/kernelbot/api/api_utils.py	Adds KernelBotError exception handling for rate limit errors

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

Copilot · 2025-12-21T01:32:27Z

src/libkernelbot/leaderboard_db.py

+        try:
+            self.cursor.execute(
+                """
+                INSERT INTO leaderboard.submission_rate_limit_state (user_id, tokens, last_refill)
+                VALUES (%s, %s, %s)
+                ON CONFLICT (user_id) DO NOTHING;
+                """,
+                (user_id, float(capacity), now),
+            )
+
+            self.cursor.execute(
+                """
+                SELECT tokens, last_refill
+                FROM leaderboard.submission_rate_limit_state
+                WHERE user_id = %s
+                FOR UPDATE;
+                """,
+                (user_id,),
+            )
+            tokens, last_refill = self.cursor.fetchone()
+            tokens = float(tokens)
+
+            dt_seconds = max(0.0, (now - last_refill).total_seconds())
+            refill = (dt_seconds / 60.0) * rate
+            tokens = min(float(capacity), tokens + refill)
+
+            if tokens < 1.0:
+                self.cursor.execute(
+                    """
+                    UPDATE leaderboard.submission_rate_limit_state
+                    SET tokens = %s, last_refill = %s
+                    WHERE user_id = %s;
+                    """,
+                    (tokens, now, user_id),
+                )
+                self.connection.commit()
+
+                wait_seconds = int(((1.0 - tokens) * 60.0) / rate) + 1
+                if wait_seconds < 0:
+                    wait_seconds = 0
+                raise KernelBotError(
+                    f"Rate limit exceeded. Please wait {wait_seconds}s before submitting again.",
+                    code=429,
+                )
+
+            tokens -= 1.0
+            self.cursor.execute(
+                """
+                UPDATE leaderboard.submission_rate_limit_state
+                SET tokens = %s, last_refill = %s
+                WHERE user_id = %s;
+                """,
+                (tokens, now, user_id),
+            )
+            self.connection.commit()
+        except psycopg2.Error as e:
+            self.connection.rollback()
+            logger.exception("Error enforcing submission rate limit.", exc_info=e)
+            raise KernelBotError("Database error while enforcing submission rate limit") from e


The enforce_submission_rate_limit method can raise KernelBotError but catch psycopg2.Error. However, if the nested call to get_submission_rate_limits (line 385) raises a KernelBotError, it will not be caught by the except block on line 451, which only catches psycopg2.Error. This means the transaction will not be rolled back properly if get_submission_rate_limits fails, potentially leaving the database in an inconsistent state.

Copilot · 2025-12-21T01:32:28Z

src/libkernelbot/leaderboard_db.py

+                wait_seconds = int(((1.0 - tokens) * 60.0) / rate) + 1
+                if wait_seconds < 0:
+                    wait_seconds = 0


The wait time calculation has an edge case issue. When tokens is very close to 1.0 but slightly less (e.g., 0.99), the calculation ((1.0 - tokens) * 60.0) / rate will result in a very small value (e.g., 0.6 seconds for rate=1). After converting to int and adding 1, this becomes 1 second. However, this doesn't account for the fact that by the time the user retries, they may still not have enough tokens if the actual time elapsed is less than the calculated wait time. The calculation should round up or add a small buffer to ensure the user will definitely have enough tokens when they retry.

Copilot · 2025-12-21T01:32:28Z

src/libkernelbot/leaderboard_db.py

+                if wait_seconds < 0:
+                    wait_seconds = 0


The conditional check for wait_seconds < 0 on line 434 is unnecessary because the calculation on line 433 ensures wait_seconds will always be >= 1. The expression ((1.0 - tokens) * 60.0) / rate can only produce a negative value if tokens > 1.0, but this code is only executed when tokens < 1.0 (line 422). Therefore, this condition can never be true and represents dead code.

Suggested change

if wait_seconds < 0:

wait_seconds = 0

Copilot · 2025-12-21T01:32:28Z

src/libkernelbot/leaderboard_db.py

+    def set_default_submission_rate_limit(self, rate_per_minute: Optional[float]) -> None:
+        """Set the default submission rate limit in submissions/minute (None = unlimited)."""
+        try:
+            self.cursor.execute(
+                """
+                INSERT INTO leaderboard.submission_rate_limit_settings AS s
+                    (id, default_rate_per_minute, updated_at)
+                VALUES
+                    (TRUE, %s, NOW())
+                ON CONFLICT (id) DO UPDATE
+                SET
+                    default_rate_per_minute = EXCLUDED.default_rate_per_minute,
+                    updated_at              = NOW();
+                """,
+                (rate_per_minute,),
+            )
+            self.connection.commit()
+        except psycopg2.Error as e:
+            self.connection.rollback()
+            logger.exception("Could not set default submission rate limit.", exc_info=e)
+            raise KernelBotError("Database error while setting submission rate limit") from e
+
+    def get_default_submission_rate_limit(self) -> Tuple[Optional[float], float]:
+        """Return (default_rate_per_minute, capacity)."""
+        try:
+            self.cursor.execute(
+                """
+                SELECT default_rate_per_minute, capacity
+                FROM leaderboard.submission_rate_limit_settings
+                WHERE id = TRUE
+                """,
+            )
+            row = self.cursor.fetchone()
+            if not row:
+                return None, 1.0
+            return row[0], float(row[1])
+        except psycopg2.Error as e:
+            self.connection.rollback()
+            logger.exception("Could not read default submission rate limit.", exc_info=e)
+            raise KernelBotError("Database error while reading submission rate limit") from e
+
+    def set_user_submission_rate_limit(
+        self, user_id: str, rate_per_minute: Optional[float]
+    ) -> None:
+        """Set a per-user submission rate limit in submissions/minute (None = unlimited)."""
+        try:
+            self.cursor.execute(
+                """
+                INSERT INTO leaderboard.submission_rate_limit_user AS u
+                    (user_id, rate_per_minute, updated_at)
+                VALUES
+                    (%s, %s, NOW())
+                ON CONFLICT (user_id) DO UPDATE
+                SET
+                    rate_per_minute = EXCLUDED.rate_per_minute,
+                    updated_at      = NOW();
+                """,
+                (str(user_id), rate_per_minute),
+            )
+            self.connection.commit()
+        except psycopg2.Error as e:
+            self.connection.rollback()
+            logger.exception("Could not set user submission rate limit.", exc_info=e)
+            raise KernelBotError("Database error while setting submission rate limit") from e
+
+    def clear_user_submission_rate_limit(self, user_id: str) -> None:
+        """Remove a per-user override so the default applies again."""
+        try:
+            self.cursor.execute(
+                """
+                DELETE FROM leaderboard.submission_rate_limit_user
+                WHERE user_id = %s;
+                """,
+                (str(user_id),),
+            )
+            self.connection.commit()
+        except psycopg2.Error as e:
+            self.connection.rollback()
+            logger.exception("Could not clear user submission rate limit.", exc_info=e)
+            raise KernelBotError("Database error while clearing submission rate limit") from e
+
+    def get_submission_rate_limits(
+        self, user_id: str
+    ) -> Tuple[Optional[float], bool, Optional[float], Optional[float], float]:
+        """Return (effective_rate, has_override, user_rate, default_rate, capacity)."""
+        default_rate, capacity = self.get_default_submission_rate_limit()
+        user_rate: Optional[float] = None
+        has_override = False
+
+        try:
+            self.cursor.execute(
+                """
+                SELECT rate_per_minute
+                FROM leaderboard.submission_rate_limit_user
+                WHERE user_id = %s
+                """,
+                (str(user_id),),
+            )
+            row = self.cursor.fetchone()
+            if row is not None:
+                has_override = True
+                user_rate = row[0]
+
+            effective = user_rate if has_override else default_rate
+            return effective, has_override, user_rate, default_rate, capacity
+        except psycopg2.Error as e:
+            self.connection.rollback()
+            logger.exception("Could not read submission rate limit config.", exc_info=e)
+            raise KernelBotError("Database error while reading submission rate limit config") from e
+
+    def enforce_submission_rate_limit(self, user_id: str) -> None:
+        """Enforce per-user submission rate limiting (token bucket, submissions/minute)."""
+        user_id = str(user_id)
+        now = datetime.datetime.now(datetime.timezone.utc)
+
+        effective_rate, _, _, _, capacity = self.get_submission_rate_limits(user_id)
+        if effective_rate is None:
+            return
+
+        rate = float(effective_rate)
+        if rate <= 0:
+            raise KernelBotError(
+                "You are currently rate-limited from submitting. Please contact an admin.",
+                code=429,
+            )
+
+        try:
+            self.cursor.execute(
+                """
+                INSERT INTO leaderboard.submission_rate_limit_state (user_id, tokens, last_refill)
+                VALUES (%s, %s, %s)
+                ON CONFLICT (user_id) DO NOTHING;
+                """,
+                (user_id, float(capacity), now),
+            )
+
+            self.cursor.execute(
+                """
+                SELECT tokens, last_refill
+                FROM leaderboard.submission_rate_limit_state
+                WHERE user_id = %s
+                FOR UPDATE;
+                """,
+                (user_id,),
+            )
+            tokens, last_refill = self.cursor.fetchone()
+            tokens = float(tokens)
+
+            dt_seconds = max(0.0, (now - last_refill).total_seconds())
+            refill = (dt_seconds / 60.0) * rate
+            tokens = min(float(capacity), tokens + refill)
+
+            if tokens < 1.0:
+                self.cursor.execute(
+                    """
+                    UPDATE leaderboard.submission_rate_limit_state
+                    SET tokens = %s, last_refill = %s
+                    WHERE user_id = %s;
+                    """,
+                    (tokens, now, user_id),
+                )
+                self.connection.commit()
+
+                wait_seconds = int(((1.0 - tokens) * 60.0) / rate) + 1
+                if wait_seconds < 0:
+                    wait_seconds = 0
+                raise KernelBotError(
+                    f"Rate limit exceeded. Please wait {wait_seconds}s before submitting again.",
+                    code=429,
+                )
+
+            tokens -= 1.0
+            self.cursor.execute(
+                """
+                UPDATE leaderboard.submission_rate_limit_state
+                SET tokens = %s, last_refill = %s
+                WHERE user_id = %s;
+                """,
+                (tokens, now, user_id),
+            )
+            self.connection.commit()
+        except psycopg2.Error as e:
+            self.connection.rollback()
+            logger.exception("Error enforcing submission rate limit.", exc_info=e)
+            raise KernelBotError("Database error while enforcing submission rate limit") from e


The rate limiting implementation lacks test coverage. The repository has comprehensive tests for other database operations in tests/test_leaderboard_db.py, but there are no tests for the new rate limiting functionality including set_default_submission_rate_limit, set_user_submission_rate_limit, get_submission_rate_limits, clear_user_submission_rate_limit, and enforce_submission_rate_limit. Given the complexity of the token bucket algorithm and the critical nature of rate limiting, these functions should have test coverage.

Copilot · 2025-12-21T01:32:28Z

src/migrations/20251221_01_kRtL1-submission-rate-limit.py

+        CREATE TABLE IF NOT EXISTS leaderboard.submission_rate_limit_settings (
+            id                      BOOLEAN PRIMARY KEY DEFAULT TRUE,
+            default_rate_per_minute DOUBLE PRECISION DEFAULT NULL,
+            capacity                DOUBLE PRECISION NOT NULL DEFAULT 1.0,
+            updated_at              TIMESTAMPTZ NOT NULL DEFAULT NOW()
+        );


The migration creates a singleton table using a boolean primary key constraint (id BOOLEAN PRIMARY KEY DEFAULT TRUE), which is a clever pattern to ensure only one row exists. However, the table lacks a constraint to prevent insertion of FALSE values. While the default and the INSERT statement both use TRUE, a malicious or buggy query could insert a row with id=FALSE, defeating the singleton pattern. Consider adding a CHECK constraint like 'CHECK (id = TRUE)' to enforce this at the database level.

@b9r5 mind just quickly skimming this?

I guess at a high level the design I'd initially thought of was just creating some user table with properties of those users instead

Copilot · 2025-12-21T01:32:29Z

src/kernelbot/cogs/admin_cog.py

+                if clear_override:
+                    await send_discord_message(
+                        interaction,
+                        "For default limit, use a number or `none` (not `default`).",


The parsing logic for rate_per_minute has overlapping semantics that could confuse users. Both 'none' and 'default' set parsed_rate to None, but they have different meanings (unlimited vs. clear override). When a user sets the default rate to 'none' (unlimited), and then later tries to set it to 'default', they get an error message. However, the parameter description doesn't clearly explain that 'default' is only valid for per-user overrides, not for the default rate itself. Consider renaming the special value to something more explicit like 'remove' or 'clear' to make it obvious it's for removing user overrides.

Suggested change

"For default limit, use a number or `none` (not `default`).",

"For the default submission limit, use a number or `none` (unlimited). "

"The special value `default` is only valid when setting a per-user limit, "

"where it clears that user's override so the default applies.",

Agree with AI

Copilot · 2025-12-21T01:32:29Z

src/kernelbot/api/main.py

    # All other unexpected errors → 500
    except Exception as e:
        # logger.exception("Unexpected error in run_submission_v2")
        logger.error(f"Unexpected error in api submissoin: {e}")


Typo in log message: 'submissoin' should be 'submission'.

Suggested change

logger.error(f"Unexpected error in api submissoin: {e}")

logger.error(f"Unexpected error in api submission: {e}")

Copilot · 2025-12-21T01:32:29Z

src/libkernelbot/leaderboard_db.py

+            dt_seconds = max(0.0, (now - last_refill).total_seconds())
+            refill = (dt_seconds / 60.0) * rate
+            tokens = min(float(capacity), tokens + refill)
+
+            if tokens < 1.0:
+                self.cursor.execute(
+                    """
+                    UPDATE leaderboard.submission_rate_limit_state
+                    SET tokens = %s, last_refill = %s
+                    WHERE user_id = %s;
+                    """,
+                    (tokens, now, user_id),
+                )
+                self.connection.commit()
+
+                wait_seconds = int(((1.0 - tokens) * 60.0) / rate) + 1
+                if wait_seconds < 0:
+                    wait_seconds = 0
+                raise KernelBotError(
+                    f"Rate limit exceeded. Please wait {wait_seconds}s before submitting again.",
+                    code=429,
+                )
+
+            tokens -= 1.0
+            self.cursor.execute(
+                """
+                UPDATE leaderboard.submission_rate_limit_state
+                SET tokens = %s, last_refill = %s
+                WHERE user_id = %s;
+                """,
+                (tokens, now, user_id),
+            )
+            self.connection.commit()


The token bucket implementation has a logic issue with the last_refill timestamp update. Currently, last_refill is updated to 'now' on every submission attempt (both at line 426 when rate limited and at line 445 when successful). This is incorrect for a token bucket algorithm.

The last_refill should represent the last time tokens were refilled, not the last time they were consumed. By updating it on every access, the next refill calculation will incorrectly assume tokens have been refilling since the last submission attempt, when in reality they should refill continuously based on elapsed time.

The correct approach is to update last_refill to 'now' only after calculating the new token count (after line 420), and use this updated timestamp for both the rate-limited and successful cases. This ensures the refill calculation is based on time elapsed since the last refill, not the last submission attempt.

Copilot · 2025-12-21T01:32:29Z

src/kernelbot/api/main.py

+        except KernelBotError as e:
+            raise HTTPException(status_code=e.http_code, detail=str(e)) from e
        except Exception as e:
-            raise HTTPException(status_code=400, detail=f"failed to prepare submission request: {str(e)}") from e
+            raise HTTPException(
+                status_code=400,
+                detail=f"failed to prepare submission request: {str(e)}",
+            ) from e


There is duplicate error handling for KernelBotError. The inner try-except block (lines 458-471) catches KernelBotError and converts it to HTTPException. However, the outer exception handling also catches KernelBotError (visible in the larger context). Since the inner handler already raises HTTPException for KernelBotError, any outer KernelBotError handler would never be reached and represents dead code.

Yeah agree with AI here

msaroufim · 2025-12-25T22:31:00Z

src/kernelbot/api/api_utils.py

 ):
    try:
        req = prepare_submission(submission, backend)
+    except KernelBotError as e:


why do we need this?

msaroufim · 2025-12-25T22:31:23Z

src/kernelbot/api/main.py

    except Exception as e:
        # Catch unexpected errors during OAuth handling
-        raise HTTPException(status_code=500, detail=f"Error during {auth_provider} OAuth flow: {e}") from e
+        raise HTTPException(


are these actual lint changes or claude gone wild?

msaroufim · 2025-12-25T22:31:57Z

src/kernelbot/api/main.py

+        except KernelBotError as e:
+            raise HTTPException(status_code=e.http_code, detail=str(e)) from e
        except Exception as e:
-            raise HTTPException(status_code=400, detail=f"failed to prepare submission request: {str(e)}") from e
+            raise HTTPException(
+                status_code=400,
+                detail=f"failed to prepare submission request: {str(e)}",
+            ) from e


Yeah agree with AI here

msaroufim · 2025-12-25T22:33:02Z

src/kernelbot/cogs/admin_cog.py

            name="set-forum-ids", description="Sets forum IDs"
        )(self.set_forum_ids)

+        self.set_submission_rate_limit = bot.admin_group.command(


my thinking is we would instead say something like "you can't submit anything new, until your last request goes through or some timeout expires" - please reach out to mods if you'd like to be overriden

msaroufim · 2025-12-25T22:33:28Z

src/kernelbot/cogs/admin_cog.py

+    def _parse_user_id_arg(self, user_id: str) -> str:
+        """Accepts a raw id or a discord mention and returns the id string."""
+        s = (user_id or "").strip()
+        if s.startswith("<@") and s.endswith(">"):


could you have some example tests for this function?

msaroufim · 2025-12-25T22:35:20Z

src/kernelbot/cogs/admin_cog.py

+            )
+            return
+
+        rate_s = (rate_per_minute or "").strip().lower()


this code is too defensive, it's mainly us mods making the change right? in which case we don't need to be this paranaoid

Also now as a mod i'm not sure what value should i set closer to 1 or 0.5, it's partly why I favor unlimited or at most one submission just cause it's easier for me to intuit what to put in

msaroufim · 2025-12-25T22:35:21Z

src/kernelbot/cogs/admin_cog.py

+            )
+            return
+
+        rate_s = (rate_per_minute or "").strip().lower()


this code is too defensive, it's mainly us mods making the change right? in which case we don't need to be this paranaoid

Also now as a mod i'm not sure what value should i set closer to 1 or 0.5, it's partly why I favor unlimited or at most one submission just cause it's easier for me to intuit what to put in

msaroufim · 2025-12-25T22:35:43Z

src/kernelbot/cogs/admin_cog.py

+                if clear_override:
+                    await send_discord_message(
+                        interaction,
+                        "For default limit, use a number or `none` (not `default`).",


Agree with AI

msaroufim · 2025-12-25T22:40:17Z

src/migrations/20251221_01_kRtL1-submission-rate-limit.py

+        CREATE TABLE IF NOT EXISTS leaderboard.submission_rate_limit_settings (
+            id                      BOOLEAN PRIMARY KEY DEFAULT TRUE,
+            default_rate_per_minute DOUBLE PRECISION DEFAULT NULL,
+            capacity                DOUBLE PRECISION NOT NULL DEFAULT 1.0,
+            updated_at              TIMESTAMPTZ NOT NULL DEFAULT NOW()
+        );


@b9r5 mind just quickly skimming this?

I guess at a high level the design I'd initially thought of was just creating some user table with properties of those users instead

yangw-dev · 2025-12-26T23:46:48Z

src/kernelbot/api/api_utils.py

 ):
    try:
        req = prepare_submission(submission, backend)
+    except KernelBotError as e:


yangw-dev · 2025-12-27T00:23:31Z

src/kernelbot/cogs/admin_cog.py

+
+            uid = self._parse_user_id_arg(user_id)
+            effective, has_override, user_rate, default_rate, capacity = (
+                db.get_submission_rate_limits(uid)


just make sure,
it seems there is no creation for user limit if users only has web access
db.set_user_submission_rate_limit seems only happens for discrod.

I think for web/cli, you also need

check if it's admin

if it's not and the user's limit not in db, create it

if exist, check remaining

meanwhile, i will update rate limit on FE side to be 10/minute too

S1ro1 added 2 commits December 21, 2025 02:23

Implement rate limiting

36e39d6

Fix: import

9bb6166

Copilot AI review requested due to automatic review settings December 21, 2025 01:28

Copilot started reviewing on behalf of S1ro1 December 21, 2025 01:29 View session

Copilot AI reviewed Dec 21, 2025

View reviewed changes

msaroufim reviewed Dec 25, 2025

View reviewed changes

yangw-dev reviewed Dec 27, 2025

View reviewed changes

-                        "For default limit, use a number or `none` (not `default`).",
+                        "For the default submission limit, use a number or `none` (unlimited). "
+                        "The special value `default` is only valid when setting a per-user limit, "
+                        "where it clears that user's override so the default applies.",

	logger.error(f"Unexpected error in api submissoin: {e}")
	logger.error(f"Unexpected error in api submission: {e}")

Feat rate limit #386

Are you sure you want to change the base?

Feat rate limit #386

Uh oh!

Conversation

S1ro1 commented Dec 21, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants