fix: rejected requests should not modify ratelimit state by chronark · Pull Request #4080 · unkeyed/unkey

chronark · 2025-10-07T13:33:58Z

What does this PR do?

Fixes a critical bug where multiple rate limits cause incorrect counter decrements when one limit is triggered. When a key has multiple rate limits (e.g., 12/minute and 200/month), and the first limit gets checked and incremented, then the second limit is triggered, the first limit's counter remains incremented even though the request was rejected.

This PR implements an atomic batch processing approach for rate limits, ensuring that when any rate limit check fails, none of the counters are incremented. This prevents counter leaks and ensures users aren't rate limited too early.

Fixes #

Type of change

Bug fix (non-breaking change which fixes an issue)

How should this be tested?

Run the new test case in multiple_ratelimits_overcommit_test.go which verifies that monthly counters aren't decremented when minute limits are hit
Test with keys that have multiple rate limits (e.g., per-minute and per-month) and verify that hitting one limit doesn't affect the other limit's counter

Checklist

Required

vercel · 2025-10-07T13:34:02Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
dashboard	Ready	Preview	Comment	Oct 9, 2025 10:54am
engineering	Ready	Preview	Comment	Oct 9, 2025 10:54am

changeset-bot · 2025-10-07T13:34:03Z

⚠️ No Changeset found

Latest commit: ff39a3b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

chronark · 2025-10-07T13:34:18Z

fix: rejected requests should not modify ratelimit state #4080 👈 (View in Graphite)
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

coderabbitai · 2025-10-07T13:34:40Z

📝 Walkthrough

Walkthrough

Adds batch rate-limiting support and integrates it into key validation. Introduces RatelimitMany API, refactors service internals (bucket identity, locking, window calculation), and updates validation to call batched checks. Adds tests verifying multi-limit behavior and non-leaky counters when one limit rejects a request.

Changes

Cohort / File(s)	Summary
API verify-key tests `go/apps/api/routes/v2_keys_verify_key/multiple_ratelimits_overcommit_test.go`, `go/apps/api/routes/v2_keys_verify_key/ratelimit_response_test.go`	New tests asserting correct multi-limit behavior: per-minute vs. monthly counters, remaining values, exceeded flags, and no counter decrement on rejected requests.
Keys validation batching `go/internal/services/keys/validation.go`	Switches from per-limit calls to batched rate limit processing; maps responses back to per-limit results; preserves no-limit early return; writes results to `k.RatelimitResults`.
Ratelimit interface `go/internal/services/ratelimit/interface.go`	Adds `RatelimitMany(ctx, []RatelimitRequest) ([]RatelimitResponse, error)` to the Service interface; minor doc update.
Ratelimit service core `go/internal/services/ratelimit/service.go`	Implements `RatelimitMany`; refactors locking and bucket evaluation; sliding-window calculation; Redis consult/fallback; replay buffer; config struct; tracing/metrics; single-request path delegates to shared logic.
Ratelimit bucket internals `go/internal/services/ratelimit/bucket.go`	Adds bucket identifier, key construction via `bucketKey`, and populates identifier on creation.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client
  participant API as API VerifyKey Route
  participant Keys as Keys Validation
  participant RL as Ratelimit Service

  Client->>API: VerifyKey request
  API->>Keys: Validate(key)
  Keys->>Keys: Collect rate limits to check
  alt 0 limits
    Keys-->>API: Valid (no ratelimits)
  else 1 limit
    Keys->>RL: Ratelimit(req)
    RL->>RL: Evaluate bucket/window
    RL-->>Keys: Response (allowed/exceeded, remaining)
  else >1 limits
    Keys->>RL: RatelimitMany(reqs[])
    RL->>RL: Lock buckets, evaluate all
    RL-->>Keys: Responses[]
  end
  Keys->>Keys: Map responses to per-limit results
  Keys-->>API: Validation result (valid/ratelimited, per-limit states)
  API-->>Client: HTTP response (status, headers)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

wip: redis based ratelimiting #3075 — Adds/changes batched RatelimitMany and core ratelimit window/locking logic, overlapping with this batch implementation.
refactor: deduplicate and make more maintaianable #4088 — Modifies ratelimit internals (identifier, bucket key, locking on buckets) similar to the introduced bucket/key refactors.
fix: region not showing and wrong rl id #3722 — Touches keys rate-limit validation path, related to batching and result mapping in validation.go.

Suggested labels

Bug, Needs Approval

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The pull request description includes the required sections for summary, type of change, testing instructions, and checklist, and it generally follows the repository template. However, the placeholder 'Fixes #' is left blank and does not reference a specific issue number, which is mandatory for tracking and clarity. Without an explicit issue reference, it is unclear which bug or task this change is intended to resolve.	Please update the 'Fixes #' line to include the appropriate issue number (for example, 'Fixes #123'), or create and reference a new issue if one does not exist. Ensuring the issue reference is present will improve traceability and clarity for reviewers and maintainers. After adding the issue number, verify that the description matches the template and no placeholders remain.
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title concisely summarizes the primary change, indicating a fix to prevent rejected requests from modifying rate limit state and directly reflecting the bug being addressed. It is clear, specific, and avoids extraneous details or vague phrasing. Therefore, it effectively communicates the main intent of the pull request.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch 10-07-test_rejected_requests_should_not_modify_ratelimit_state

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a776840 and ff39a3b.

📒 Files selected for processing (5)

go/apps/api/routes/v2_keys_verify_key/ratelimit_response_test.go (1 hunks)
go/internal/services/keys/validation.go (1 hunks)
go/internal/services/ratelimit/bucket.go (3 hunks)
go/internal/services/ratelimit/interface.go (1 hunks)
go/internal/services/ratelimit/service.go (8 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

go/internal/services/ratelimit/interface.go

🧰 Additional context used

🧬 Code graph analysis (3)

go/internal/services/keys/validation.go (2)

go/internal/services/ratelimit/interface.go (2)

RatelimitRequest (56-97)

RatelimitResponse (104-140)

go/internal/services/keys/status.go (1)

StatusRateLimited (21-21)

go/apps/api/routes/v2_keys_verify_key/ratelimit_response_test.go (3)

go/pkg/testutil/seed/seed.go (1)

CreateRatelimitRequest (307-315)

go/apps/api/openapi/gen.go (3)

VerifyKeyRatelimitData (1983-2008)

VALID (43-43)

RATELIMITED (41-41)

go/pkg/testutil/http.go (1)

CallRoute (271-305)

go/internal/services/ratelimit/service.go (5)

go/pkg/clock/interface.go (1)

Clock (12-24)

go/pkg/counter/interface.go (1)

Counter (21-147)

go/internal/services/ratelimit/interface.go (2)

RatelimitRequest (56-97)

RatelimitResponse (104-140)

go/pkg/buffer/buffer.go (1)

Buffer (14-23)

go/pkg/prometheus/metrics/ratelimit.go (1)

RatelimitDecision (112-121)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: Test Go API Local / Test
GitHub Check: Test API / API Test Local

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8379888 and a776840.

📒 Files selected for processing (5)

go/apps/api/routes/v2_keys_verify_key/multiple_ratelimits_overcommit_test.go (1 hunks)
go/apps/api/routes/v2_ratelimit_limit/handler.go (1 hunks)
go/internal/services/keys/validation.go (1 hunks)
go/internal/services/ratelimit/interface.go (1 hunks)
go/internal/services/ratelimit/service.go (5 hunks)

🧰 Additional context used

🧬 Code graph analysis (5)

go/apps/api/routes/v2_keys_verify_key/multiple_ratelimits_overcommit_test.go (5)

go/pkg/testutil/http.go (2)

NewHarness (55-189)

CallRoute (271-305)

go/apps/api/routes/v2_ratelimit_limit/handler.go (3)

Handler (36-45)

Request (32-32)

Response (33-33)

go/pkg/testutil/seed/seed.go (1)

CreateRatelimitRequest (306-314)

go/apps/api/openapi/gen.go (2)

VALID (43-43)

RATELIMITED (41-41)

go/pkg/ptr/deref.go (1)

SafeDeref (35-44)

go/apps/api/routes/v2_ratelimit_limit/handler.go (1)

go/internal/services/ratelimit/interface.go (1)

RatelimitRequest (62-103)

go/internal/services/ratelimit/service.go (5)

go/pkg/clock/interface.go (1)

Clock (12-24)

go/pkg/counter/interface.go (1)

Counter (21-147)

go/internal/services/ratelimit/interface.go (2)

RatelimitRequest (62-103)

RatelimitResponse (110-146)

go/pkg/buffer/buffer.go (1)

Buffer (14-23)

go/pkg/prometheus/metrics/ratelimit.go (1)

RatelimitDecision (112-121)

go/internal/services/ratelimit/interface.go (1)

go/pkg/db/models_generated.go (1)

Ratelimit (736-747)

go/internal/services/keys/validation.go (2)

go/internal/services/ratelimit/interface.go (1)

RatelimitRequest (62-103)

go/internal/services/keys/status.go (1)

StatusRateLimited (21-21)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: Test Go API Local / Test
GitHub Check: Test API / API Test Local
GitHub Check: Build / Build

go/internal/services/ratelimit/service.go

* refactor: deduplicate and make more maintaianable * test: correct remaining counts * fix: span attributes

fix: reject ratelimit changes

github-actions · 2025-10-09T10:51:41Z

Thank you for following the naming conventions for pull request titles! 🙏

…fy_ratelimit_state

chronark added 2 commits October 7, 2025 09:38

test: rejected requests should not modify ratelimit state

1c34964

fix: do not increment ratelimit counters for denied requests

a776840

chronark changed the title ~~test: rejected requests should not modify ratelimit state~~ fix: rejected requests should not modify ratelimit state Oct 7, 2025

chronark marked this pull request as ready for review October 7, 2025 13:34

chronark requested review from Flo4604, imeyer and perkinsjr as code owners October 7, 2025 13:34

vercel bot temporarily deployed to Preview – dashboard October 7, 2025 13:34 Inactive

vercel bot temporarily deployed to Preview – engineering October 7, 2025 13:34 Inactive

coderabbitai bot requested a review from ogzhanolguncu October 7, 2025 13:35

coderabbitai bot reviewed Oct 7, 2025

View reviewed changes

go/internal/services/ratelimit/service.go Outdated Show resolved Hide resolved

chronark assigned Flo4604 Oct 7, 2025

Flo4604 reviewed Oct 8, 2025

View reviewed changes

go/internal/services/ratelimit/service.go Show resolved Hide resolved

Flo4604 and others added 5 commits October 8, 2025 19:30

feat: use seperate path for multi vs normal ratelimits

a2f8d4e

remove tests

c0c4c01

fix wrong remaining

fcd184b

refactor: deduplicate and make more maintaianable (#4088)

f3f8232

* refactor: deduplicate and make more maintaianable * test: correct remaining counts * fix: span attributes

Merge pull request #4087 from unkeyed/flo-reject-changes

7af2b69

fix: reject ratelimit changes

Merge branch 'main' into 10-07-test_rejected_requests_should_not_modi…

ff39a3b

…fy_ratelimit_state

vercel bot temporarily deployed to Preview – dashboard October 9, 2025 10:54 Inactive

vercel bot temporarily deployed to Preview – engineering October 9, 2025 10:54 Inactive

chronark merged commit f59b5c6 into main Oct 9, 2025
17 checks passed

chronark deleted the 10-07-test_rejected_requests_should_not_modify_ratelimit_state branch October 9, 2025 11:12

coderabbitai bot mentioned this pull request Nov 4, 2025

fix: use ratelimit name in bucket #4233

Merged

19 tasks

coderabbitai bot mentioned this pull request Nov 12, 2025

feat: ratelimit.multiLimit endpoint #4325

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: rejected requests should not modify ratelimit state#4080

fix: rejected requests should not modify ratelimit state#4080
chronark merged 8 commits intomainfrom
10-07-test_rejected_requests_should_not_modify_ratelimit_state

chronark commented Oct 7, 2025 •

edited

Loading

Uh oh!

vercel bot commented Oct 7, 2025 •

edited

Loading

Uh oh!

changeset-bot bot commented Oct 7, 2025 •

edited

Loading

Uh oh!

chronark commented Oct 7, 2025

Uh oh!

coderabbitai bot commented Oct 7, 2025 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested labels

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 9, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

chronark commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Type of change

How should this be tested?

Checklist

Required

Uh oh!

vercel bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

chronark commented Oct 7, 2025

Uh oh!

coderabbitai bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested labels

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chronark commented Oct 7, 2025 •

edited

Loading

vercel bot commented Oct 7, 2025 •

edited

Loading

changeset-bot bot commented Oct 7, 2025 •

edited

Loading

coderabbitai bot commented Oct 7, 2025 •

edited

Loading

github-actions bot commented Oct 9, 2025 •

edited

Loading