refactor(backend): generalise the vetKey limiter to TieredRateLimiter - #13862
Conversation
`VetKeyRateLimiters` is the canister's only two-tier limiter — a per-caller budget checked before a shared global one — and both its name and its hardcoded constructor made it look like it belonged to the vetKey endpoints. It does not. That shape is what any endpoint wants once a single caller's budget stops being the whole risk, so the type is now `TieredRateLimiter` and `with_tiers` takes all four caps explicitly. Renaming and reshaping it deserves its own review. The type guards the paid vetKD derivations, and the call sites that move here are live personal-notes code — both are easier to check on their own than folded into a feature diff, where a reviewer reads them as noise around the feature. Behaviour is unchanged. `new()` now calls `with_tiers(2, 10, 20, 100)`, which is what its body spelled out before, so the two personal-notes limiters keep exactly the tiers they were sized with: per-caller 2/min and 10/hour, shared global 20/min and 100/hour. One new test pins that. A second covers the new constructor, showing a global tier refuse a flood spread across principals that each stay well inside their own budget — the case a per-caller limit cannot see. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to 236a08a. Security Overview
Detected Code Changes
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the backend rate-limiting utilities by generalising the existing vetKey-specific two-tier limiter into a reusable TieredRateLimiter, while keeping the vetKey defaults via new(), and updates the personal-notes vetKey endpoints to use the renamed type.
Changes:
- Renamed
VetKeyRateLimiters→TieredRateLimiterand kept vetKey defaults viaTieredRateLimiter::new(). - Added
TieredRateLimiter::with_tiers(...)to allow other endpoints to define their own per-caller and global tiers. - Updated personal-notes vetKey endpoints to call
TieredRateLimiter::check_callerand added/updated unit tests around the new API.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/backend/src/utils/rate_limiter.rs |
Renames/generalises the two-tier limiter, adds with_tiers, updates docs, and extends unit tests. |
src/backend/src/api/personal_notes.rs |
Switches vetKey rate-limiter call sites/imports to the new TieredRateLimiter type. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…sing The regression test was named for four tiers and asserted one. with_tiers takes four positional numbers, so the mistake it exists to catch is a pair swapped — which the old assertion would have passed straight through. It now exercises each tier against the numbers personal notes was sized with, and each of the four assertions was checked by perturbing that tier and confirming the test fails. Writing it turned up that the hour cases are easy to get wrong in the direction of passing: 100 calls one per minute never fill a sliding hour, because the window only holds the last 60. They are packed five per minute over twenty minutes instead. The struct doc claimed the global key is 'never a real registered caller on these endpoints'. That was a statement about two specific callers, written as a property of the type — and now the type is general purpose. On an endpoint the anonymous principal can reach, the per-caller and global tiers are the same bucket and consume each other. The doc now says the guards are what make the shared key safe, and points an anonymous endpoint at RateLimiter instead.
Same correction as #13862, applied here verbatim so the merge is trivial. The struct doc called the global bucket's key 'never a real registered caller on these endpoints' — true of the two personal-notes callers it was written for, and now stated as a property of a general-purpose type that this branch adds six more callers to. On an endpoint the anonymous principal can reach, the per-caller and global tiers are the same bucket and consume each other. Every caller here is behind caller_is_registered_user or caller_is_not_anonymous, so it holds — but as a property of those guards, which is what the doc now says.
Makes the stack real. The refactor was extracted to #13862 but both branches sat on main as siblings, so this PR still displayed 203 lines of it — including the personal_notes.rs call sites the reviewer asked to see on their own. Resolution keeps this branch's additions (the six tip limiters, with_caller_burst and its regression test) and takes the refactor's wording and its four-tier default test, which is strictly stronger than the one-tier version here and is what will be on main once #13862 lands.
|
On chunking the tests — I dug into the two failures, and the short version is that chunking is already there, it was active in both runs, and making the chunks smaller would not have helped. Details, because the reason matters for what to do instead. It is already chunked
Why a smaller chunk would not helpBoth failures landed in the same chunk, at different points:
Chunks 1, 3, 4, 5 and 6 finished 40 tests each, on a fresh server, in both runs. That rules out both suspects. It is not a leak — a leak kills the server near the end of a chunk, and at roughly the same point every time. It is not one bad test either, or it would die at the same test. And since the 31 Aug run died 7 tests into a fresh server, a chunk size of 10 would have died in exactly the same place. What chunk 2 actually isTests 41-80 happen to contain all eight Nothing pins parallelism, so One correction to my own first guess: I assumed the images were near the 100 KiB cap and that payload size explained it. They are not — So the lever is concurrency, not chunk sizeChunking bounds cumulative memory across a run. It does nothing about concurrent peak, which is what is dying here. #13273's comment says the suite "no longer needs to run single-threaded — peak memory is one chunk's instances regardless of parallelism", and that last clause is the half that does not hold: peak is live instances, which is threads, not chunk size. Two ways to bound it, and I would like your call since this is shared CI: 1. 2. Shard the I prefer 2. It fixes the mechanism instead of trading time for headroom, and it reuses a pattern already in the repo. 1 is fine as a stopgap if you want something in today. Worth flagging either way: this will get worse. Those eight image tests landed in that chunk recently, and tips adds six endpoints and 15 more integration tests to the same suite. #13273 called it — "the larger the backend wasm grows, the sooner it dies". Happy to write either as its own PR against |
Conflict in `rate_limiter.rs`, from #13862 landing squashed while this branch carried the same refactor as separate commits. Resolved by keeping main's `new() -> with_tiers(2, 10, 20, 100)` and the tips-only additions on top: the two tip vetKD limiters, `with_caller_burst`, and its burst test. Both sides' tests now run and pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Motivation
Split out of #13859 at review request: renaming and generalising a core shared rate limiter deserves its own review rather than riding in on a 32-file feature PR.
VetKeyRateLimiterswas built for the two personal-notes vetKey endpoints and had its four tiers hardcoded in the constructor. Nothing else could use it. Other endpoints want the same shape — a per-caller tier plus a global one, so a flood spread across many principals is still caught — with numbers of their own.The call sites this touches in
api/personal_notes.rsare live code, which is the other reason it should not be buried in a feature branch.Changes
VetKeyRateLimiterstoTieredRateLimiter, since it is no longer about vetKeys.with_tiers(caller_minute, caller_hour, global_minute, global_hour)so an endpoint can state its own limits.new()as the vetKey defaults, now delegating towith_tiers(2, 10, 20, 100).api/personal_notes.rs.Tests
new()produces 2/min and 10/hour per caller and 20/min and 100/hour globally, the same four valuesVetKeyRateLimiters::new()hardcoded, bound to the same windows.the_default_tiers_survive_the_move_into_with_tiersasserts all four, becausewith_tierstakes four positional numbers and a swapped pair is the mistake it exists to catch — an earlier version checked only the first tier and would have passed with the hour and global limits reversed. Each assertion was verified by perturbing that tier alone and confirming the test fails.cargo test -p backend --lib— 265 passed (263 before, plus these two)../scripts/lint.rust.shandcargo fmt --checkclean.Stack
#13859(tips backend) is now based on this branch, so this one lands first. Nothing else depends on it.One deliberate omission:
#13859also adds awith_caller_bursthelper, for a tip endpoint that wants a higher per-minute burst. It has no caller onmain, so it stays in that PR rather than landing here as dead code.