Skip to content

fix(server): roll back earlier rate-limit layers when a subsequent layer denies (#1002) - #1331

Open
harrymove-ctrl wants to merge 1 commit into
devfrom
fix/issue-1002-rate-limit-layer-rollback
Open

harrymove-ctrl wants to merge 1 commit into
devfrom
fix/issue-1002-rate-limit-layer-rollback

Conversation

@harrymove-ctrl

Copy link
Copy Markdown
Collaborator

Summary

Fixes #1002

rate_limit_middleware checks three sliding-window rate-limit layers in sequence:

  1. Per-delegate-key burst (rate:dk:{public_key})
  2. Per-account burst (rate:{owner})
  3. Per-account sustained (rate:hr:{owner})

Previously, each layer called check_and_record_window independently, which immediately added weight ZSET members ({request_id}:0 .. {request_id}:{weight-1}) before subsequent layers were evaluated. When Layer 2 (account_burst) or Layer 3 (account_sustained) denied a request, the entries already written to earlier layers were never removed (despite the comment in the Layer-2 denial branch stating that the delegate-key entry was rolled back). As a result, account-level denials permanently drained the caller's per-delegate-key quota for the remainder of the 60-second window and caused subsequent requests in the same minute to fail with layer: "delegate_key".

Changes

  • Added check_and_record_window_with_id and rollback_window_entries (ZREM over {request_id}:0 .. {request_id}:{weight-1}) in services/server/src/rate_limit.rs.
  • Extracted check_write_rate_limit_windows for the 3-layer authenticated write path:
    • When Layer 2 (account_burst) denies or errors, rolls back the Layer-1 (rate:dk:{public_key}) entries recorded for that request_id.
    • When Layer 3 (account_sustained) denies or errors, rolls back both Layer-1 (rate:dk:{public_key}) and Layer-2 (rate:{owner}) entries recorded for that request_id.
  • Added unit and Redis integration regression tests (account_burst_and_sustained_denials_roll_back_earlier_layers and account_burst_denial_does_not_spend_delegate_key_budget_redis) verifying ZCARD and layer attribution across single-weight and multi-weight endpoints.

Test Plan

  • cargo test --manifest-path services/server/Cargo.toml --bins rate_limit (50 passed, 0 failed)

@harrymove-ctrl
harrymove-ctrl requested a review from ducnmm October 27, 2026 14:04
@harrymove-ctrl

Copy link
Copy Markdown
Collaborator Author

@ducnmm This PR is ready for review. Summary of changes:

@harrymove-ctrl
harrymove-ctrl force-pushed the fix/issue-1002-rate-limit-layer-rollback branch from 11cf093 to 892304d Compare October 29, 2026 04:32
@harrymove-ctrl

Copy link
Copy Markdown
Collaborator Author

@ducnmm Rebased cleanly onto dev (commit 892304d) and resolved conflict with #1348 owner budget changes by adapting check_write_rate_limit_windows to accept RequestBudgets and adding Into support for RateLimitConfig. All rate limit unit and Redis integration tests pass.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Rate limiter records the per-delegate-key entry before a later layer can deny, and the "roll back" the comment promises does not exist

1 participant