fix: transactional rate-limit admission — charge/refund lifecycle + capacity path#234
Conversation
Two capacity-path fixes for the rate limiter (#123, #125). #123 — bounded/sharded capacity eviction. The admission path ran an O(CLEANUP_BATCH_SIZE=1000) eviction scan while holding the limiter's single global write lock, so a key-cardinality flood serialized every rate-limit check behind that scan. Fix on two axes: - Shard the limiter into up to MAX_SHARDS (32) independent RwLock<LruCache> stripes keyed by hash(key) % N, so checks for keys in different stripes never contend on the same lock. Tiny caches (< MIN_ENTRIES_PER_SHARD = 256) stay single-stripe so small-capacity LRU eviction semantics are byte-for-byte identical to before. Per-stripe capacities sum to exactly max_entries. - Use a small admission-path scan bound (ADMISSION_SCAN_LIMIT = 32), distinct from the periodic-cleanup CLEANUP_BATCH_SIZE (1000), so a full stripe costs bounded, small work per check. The #206 reservation identity is preserved: next_hit_id remains a single process-wide AtomicU64 shared across stripes, so reservation ids stay globally unique and a delayed rollback can never remove a newer hit. The #210 admission-eviction reporting is unchanged. cleanup() now sweeps every stripe (up to CLEANUP_BATCH_SIZE each). #125 — live cache-size gauge. check_and_increment now returns a sampled total entry count (RateLimitCheck::sampled_cache_len) on roughly one in GAUGE_SAMPLE_INTERVAL (64) stripe-mutating admissions, summed across stripes to keep the metric's per-cache_type label shape. The caller publishes it so transponder_rate_limit_cache_size tracks growth toward capacity instead of lagging until the 60s cleanup tick, without a metric write per check. Adds tests for single-vs-sharded stripe selection, capacity summing, cross-stripe reservation uniqueness, the bounded admission scan, the sampled gauge, and concurrent cross-stripe admissions. Two large-cache tests that asserted the old 1000-wide global scan were rewritten: one to a single-stripe variant preserving the exact semantic, plus a new sharding-aware sibling. Part of #218. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add PushDispatcher::accepts(platform) and PushDispatcher::token_is_encodable(payload) so the event processor can reject undispatchable tokens BEFORE charging the rate limiters (#177). accepts mirrors dispatch()'s platform-client presence filter; the token-encoding check mirrors its FCM UTF-8 filter. The existing filters inside dispatch() are kept as a defensive second layer (and their comment now says so) — in the normal event path they no longer fire because process_inner pre-filters, but dispatch still never enqueues an undeliverable token if a caller skips the pre-filter. Adds unit tests for both helpers. Part of #218. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Make per-token rate-limit charging transactional in process_inner so no admission path can strand a spent increment (#170, #177), fold the redundant dedup lock on the success path (#197), and wire the live rate-limit cache-size gauge (#125). AdmissionGuard (RAII-style, explicit-consume). After the encrypted-token limiter charges a token, the charge is held by an AdmissionGuard that every exit path must resolve: - commit() — fully admitted; hands the encrypted+device charges to the dispatch-failure rollback ledger. - refund() — dropped after charging; rolls back every charge held. - keep_charge() — dropped but the charge is intentionally retained. The guard's Drop asserts it was resolved. Because the limiter refund is async (the stripe lock is a tokio RwLock), a blocking Drop-time refund isn't viable, so this is an explicit-consume guard rather than fire-and-forget RAII — the trade the tracker calls out as acceptable. It still centralizes the refund decision per exit path. #170 — device-limiter reject now refunds the encrypted charge. Previously a device reject `continue`d without rolling back the encrypted-token increment already spent for that token, so a legitimate transient redelivery could read as a replay. guard.refund() on the device-reject branch fixes this. The decrypt-failure path keeps its charge via guard.keep_charge() (documented replay defense). #177 — dispatch-level drops refund and are counted. Tokens targeting an unconfigured platform, or (FCM) carrying non-UTF-8 device bytes, were dropped silently inside dispatch() AFTER both limiters were charged, with no metric. process_inner now pre-filters via dispatcher.accepts() / token_is_encodable() right after decrypt (platform is only known then) and before the device charge: it refunds the encrypted charge and records record_push_failed(platform, "unconfigured" | "invalid_encoding"). These drops mark the event terminally dropped (not a retryable rate shed). #197 — single dedup write-lock on the success path. mark_seen refreshed the completion timestamp by re-put'ing the entry under a second lock, redundantly re-writing the dedup_cache_size gauge. SeenEventStore gains mark_terminal, which mutates the existing reservation entry in place (refreshing seen_at, flipping terminal, promoting LRU position) and reports whether the set size changed, so the gauge is written only when it actually moved. Completion-timestamp-refresh and release_reservation semantics are preserved. #125 — the sampled cross-stripe cache length from check_and_increment is published to transponder_rate_limit_cache_size on the admission path. Tests: device-reject refund, unconfigured-platform and non-UTF-8 FCM drop (refund + metric), decrypt-failure charge retention, a multi-thread concurrent device-reject interleaving test proving no encrypted charge is stranded, the live gauge updating before cleanup, and the single dedup-gauge update. The create_processor_with_rate_limits helper now wires a configured APNs mock so its device/encrypted rate-limit tests exercise dispatchable tokens (previously they relied on the #177 bug of charging budget for tokens dropped silently by an unconfigured dispatcher). Fixes #170 Fixes #177 Fixes #197 Part of #218. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: Chapters generated by Stage for commit efaf529 on Jul 5, 2026 1:42pm UTC. |
✅ Code Coverage Report
View detailed coverageDownload the Or run locally: cargo llvm-cov --html
open target/llvm-cov/html/index.html |
Reconciliation regression for the merge with the push-provider rework (#233), which added DeliveryHealth streak state feeding /ready. A token dropped by the #177 pre-filter in process_inner never reaches the send path, so it is not a delivery attempt and must not touch a provider's DeliveryHealth streak. This test seeds a real FCM hard-failure streak just below the flagging threshold, processes an unconfigured-APNs drop, and asserts APNs stays delivering (no spurious streak increment) and FCM's seeded streak survives (one more failure flips it), proving the pre-filter neither increments nor resets the streak — no double-count, no missed update. Part of #218. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Makes per-token rate-limit charging transactional and takes the limiter's capacity path off the single global write lock. Builds on the wave-0/point-fix semantics already on master (PR #206 reservation-aware rollback, PR #203 retryable all-shed, PR #210 admission-eviction counters, PR #231 typed
NonZeroUsizeconfig) rather than fighting them.AdmissionGuard(explicit-consume)After the encrypted-token limiter charges a token, an
AdmissionGuardholds that charge, and every exit path in the per-token loop must resolve it:commit()— token fully admitted; hands the encrypted+device charges to the dispatch-failure rollback ledger.refund().await— token dropped after charging; rolls back every charge the guard holds.keep_charge()— token dropped but its charge is intentionally retained.Dropasserts the guard was resolved. Because the limiter refund isasync(the stripe lock is a tokioRwLock), a blockingDrop-time auto-refund is not viable — so this is an explicit-consume guard rather than fire-and-forget RAII, the trade the tracker calls out as acceptable. It still centralizes the refund decision to one place per exit path.#170— device reject refunds the encrypted chargeA device-limiter reject now refunds the encrypted-token increment already spent for that token (was: stranded, so a legit transient redelivery could read as replay). The decrypt-failure path keeps its charge via
keep_charge()(documented replay defense).#177— dispatch-level drops refund and are countedprocess_innerpre-filters undispatchable tokens right after decrypt (platform is only known then) and before the device charge, via newdispatcher.accepts(platform)/PushDispatcher::token_is_encodable(payload). Unconfigured-platform and non-UTF-8-FCM tokens now refund the encrypted charge and recordrecord_push_failed(platform, "unconfigured" | "invalid_encoding")instead of being dropped silently insidedispatch()after both limiters were charged.dispatch()'s own filters stay as a defensive second layer.Capacity path off the global lock
#123— sharding + small admission scan boundThe limiter is sharded into up to 32 independent
RwLock<LruCache>stripes keyed byhash(key) % N; tiny caches (< 256entries) stay single-stripe so small-capacity LRU semantics are byte-for-byte identical to before. Per-stripe capacities sum to exactlymax_entries. The admission-path eviction scan is bounded at 32 (ADMISSION_SCAN_LIMIT), distinct from the periodic-cleanupCLEANUP_BATCH_SIZE(1000), so a full stripe costs bounded, small work per check.Sharding vs scan-bound choice: both, because they address different halves of the DoS — sharding removes the single-lock serialization (checks in different stripes never contend), and the small scan bound removes the per-check O(1000) cost. Either alone leaves the other. The #206 reservation identity is preserved by keeping
next_hit_ida single process-wideAtomicU64shared across all stripes (reservation ids stay globally unique; a delayed rollback can't remove a newer hit). The #210 admission-eviction metric is untouched.#125— live cache-size gaugecheck_and_incrementreturns a sampled cross-stripe entry count (RateLimitCheck::sampled_cache_len) on ~1-in-64 stripe-mutating admissions, summed across stripes to keep the metric's existing per-cache_typelabel shape.process_innerpublishes it, sotransponder_rate_limit_cache_sizetracks growth toward capacity instead of lagging until the 60s cleanup tick — with no metric write per check.#197— single dedup write-lock on the success pathSeenEventStore::mark_terminalrefreshes the existing reservation entry in place (newseen_at,terminal = true, LRU promote) and reports whether the set size changed, somark_seenwrites thededup_cache_sizegauge only when the length actually moved — removing the redundant second gauge write. Completion-timestamp-refresh andrelease_reservationsemantics preserved. (try_reserveandmark_seenremain separate acquisitions — they are separated in time by the entire unwrap/decrypt/dispatch, so they genuinely cannot share one guard; the redundant gauge write was the avoidable cost.)Metric / semantic changes
transponder_push_failed_total{reason="unconfigured"|"invalid_encoding"}series for dispatch-level drops (free-form[platform, reason]counter — no schema change).transponder_rate_limit_cache_sizenow also updates on the admission path (sampled), not only at cleanup.dispatch()(unconfigured platform / non-UTF8 FCM) keeps its rate-limit budget and is recorded by no metric #177 bug (charging budget for tokens silently dropped by an unconfigured dispatcher) was updated to wire a configured APNs mock.Tests
604 → 622 in
--bin transponder. New: device-reject refund, unconfigured + non-UTF-8 drop (refund + metric), decrypt-failure charge retention, a multi-thread concurrent device-reject interleaving test (no stranded encrypted charge), live gauge before cleanup, single dedup-gauge update; and for the limiter: single-vs-sharded selection, capacity summing, cross-stripe reservation uniqueness, bounded admission scan, sampled gauge, concurrent cross-stripe admissions. Two large-cache tests that asserted the old 1000-wide global scan were rewritten (single-stripe variant preserving the semantic + a sharding-aware sibling).Gates:
cargo fmt --checkclean;cargo clippy --all-targets -- -D warningszero;cargo test --bin transponderall pass;cargo check --all-targets --features torclean.Fixes #170
Fixes #177
Fixes #123
Fixes #125
Fixes #197
Part of #218.
🤖 Generated with Claude Code