Skip to content

Produce different keys for inline ratelimiting policies - #68521

Open
Youssef1313 wants to merge 1 commit into
mainfrom
dev/ygerges/ratelimiter-buckets
Open

Produce different keys for inline ratelimiting policies#68521
Youssef1313 wants to merge 1 commit into
mainfrom
dev/ygerges/ratelimiter-buckets

Conversation

@Youssef1313

Copy link
Copy Markdown
Member

Fixes #67326

Copilot AI lite review requested due to automatic review settings August 14, 2026 11:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a rate-limiting correctness issue where inline RequireRateLimiting(IRateLimiterPolicy<T>) policies could unintentionally share cached limiters when they produced equal partition keys, causing cross-policy collisions and incorrect limiter configuration being applied.

Changes:

  • Namespaces inline policy partition keys so distinct inline policy instances no longer collide when producing equal partition keys.
  • Adds tests verifying (1) two different inline policy instances don’t share a limiter for the same key, and (2) the same policy instance reused across endpoints still shares.
  • Introduces a small stateful test RateLimiter/policy to observe limiter sharing vs isolation behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/Middleware/RateLimiting/src/RateLimiterEndpointConventionBuilderExtensions.cs Adds a per-inline-policy namespace token to avoid cross-policy partition collisions.
src/Middleware/RateLimiting/test/RateLimitingMiddlewareTests.cs Adds regression tests and a small counting limiter/policy to validate isolation vs sharing behavior.
Suppressed comments (1)

src/Middleware/RateLimiting/test/RateLimitingMiddlewareTests.cs:740

  • CountingRateLimiter.GetStatistics returns null even though the overridden return type is non-nullable. This can produce nullable warnings and may break the build if warnings are treated as errors. Prefer throwing NotImplementedException (consistent with other test limiters) since the test doesn't use statistics.
        public override RateLimiterStatistics GetStatistics() => null;

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Middleware/RateLimiting/test/RateLimitingMiddlewareTests.cs
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.

RequireRateLimiting(IRateLimiterPolicy<T>) inline policies share a partition-key namespace, causing cross-policy partition collisions

2 participants