Repository navigation
Conversation
Confetti3
left a comment
There was a problem hiding this comment.
Thanks, this fixes a real problem: cleanup now uses each bucket's own window, so a short-window caller no longer drops hourly and ten-minute quotas, the store has a firm size, and active quotas are kept rather than evicted. The overflow handling and tests are good. Reviewed at e17b977. Two changes before merging:
1. The bucket limit is shared by every policy. Limiter is one store for the whole bridge, and once it holds MAX_BUCKETS active buckets, any key it does not already have gets Err(1) (server/ember/bridge/src/rate.rs:52), whatever its policy. Session challenges create their bucket at the top of issue (server/ember/bridge/src/routes/sessions.rs:48), before the route and digest checks and before any session is required, so a large number of challenge keys inside a minute uses up room that link-claim:, discord-start: and rooms-ticket: buckets also need. Players who already have one of those buckets keep their quota, but players who do not get 429 until the challenge buckets expire. Please give each policy its own admission budget so challenge buckets cannot take capacity from the signed-in routes. One way is a limiter per policy, built with its fixed limit, window and capacity, instead of one string-keyed store that is passed the policy on every call; that also makes the window conflict check at rate.rs:63 unnecessary. Separately, moving the challenge rate check below route_matches and the digest decode (sessions.rs:57-62) means malformed requests no longer create buckets. A test that fills one policy and shows another still admits new keys would cover it.
2. Times after a backward clock step. state.now() is wall-clock time, and when a stored use is later than now, now.saturating_sub(first) is 0, so the returned retry time is the full window rather than the time left (rate.rs:77). For example, check("k", 1, 60, 100) then check("k", 1, 60, 90) returns 60, but the use only expires at 160, so a retry at 150 is refused again; the old code returned 70 here. When first > now, use window.saturating_add(first - now), and keep the current form otherwise. The same applies to last_cleanup (rate.rs:42): after a backward step no cleanup runs until the clock passes the old value, which at capacity keeps new keys refused for that long. Resetting last_cleanup when now is earlier than it would fix that. Please add a test for each case alongside the existing overflow test.
Extract this focused change from the locally tested development build. Keep other features out of the upstream review; the pull request records implementation details, prior integration evidence, and outstanding isolated validation.
e17b977 to
5728585
Compare
This fixes sliding-window quota cleanup without allowing one policy's traffic to exhaust another policy's admission capacity.
Behavior and implementation
Each of the four existing policies now owns a limiter with a fixed quota, window and a bounded store of 100,000 keys. Challenge, link-claim, Discord sign-in and room-ticket traffic keep their existing quotas and key semantics. Cleanup operates only on the owning policy, preserves active quotas, and scans at most once per second when a store is full.
Challenge routes validate the action/path, digest and any required session before allocating a rate-limit bucket. Malformed requests therefore do not consume valid-request quota.
After a backward wall-clock step, Retry-After includes the time until the recorded use plus its window expires (100 -> 90 with a 60-second window reports 70 seconds). Cleanup resets its epoch immediately. Timestamp arithmetic saturates and recorded uses remain ordered.
Review requests addressed
Validation
On Windows with pinned Rust 1.98.0:
cargo +1.98.0 test --manifest-path server/ember/Cargo.toml --locked -p ember-bridge --lib --test auth_links: 41 passed, 0 failed, 0 ignored (29 unit tests and 12 HTTP integration tests).cargo fmt --all -- --checkandgit diff --checkpass.This validation covers the bridge code and loopback HTTP routes; it does not claim production service deployment testing.