Skip to content

Populate RetryAfter on SlidingWindowRateLimiter#131225

Open
nwoolls wants to merge 7 commits into
dotnet:mainfrom
nwoolls:131175-slidingwindow-retryafter
Open

Populate RetryAfter on SlidingWindowRateLimiter#131225
nwoolls wants to merge 7 commits into
dotnet:mainfrom
nwoolls:131175-slidingwindow-retryafter

Conversation

@nwoolls

@nwoolls nwoolls commented Jul 22, 2026

Copy link
Copy Markdown

SlidingWindowLease.FailedLease was a static singleton constructed once with retryAfter: null, so TryGetMetadata(RetryAfter) always returned false on a rejected lease regardless of limiter state. This mirrors the fix already in place for FixedWindowRateLimiter.

Unlike the fixed-window case, a sliding window frees permits one segment at a time rather than resetting all at once, so the estimate walks forward through _requestsPerSegment from the next segment to free until enough permits are accounted for, rather than assuming the very next replenishment tick is sufficient.

CreateFailedSlidingWindowLease reads _requestsPerSegment and _currentSegmentIndex, both of which are otherwise only mutated under the limiter's lock (in ReplenishInternal). Callers on the permitCount == 0 fast path previously read state outside the lock; that path now acquires the lock before computing the failed lease to avoid a torn read.

Fix #131175

SlidingWindowLease.FailedLease was a static singleton constructed once
with retryAfter: null, so TryGetMetadata(RetryAfter) always returned
false on a rejected lease regardless of limiter state. This mirrors
the fix already in place for FixedWindowRateLimiter.

Unlike the fixed-window case, a sliding window frees permits one
segment at a time rather than resetting all at once, so the estimate
walks forward through _requestsPerSegment from the next segment to
free until enough permits are accounted for, rather than assuming the
very next replenishment tick is sufficient.

CreateFailedSlidingWindowLease reads _requestsPerSegment and
_currentSegmentIndex, both of which are otherwise only mutated under
the limiter's lock (in ReplenishInternal). Callers on the
permitCount == 0 fast path previously read state outside the lock;
that path now acquires the lock before computing the failed lease to
avoid a torn read.

Fix dotnet#131175
Copilot AI review requested due to automatic review settings July 22, 2026 17:39
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Jul 22, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @VSadov
See info in area-owners.md if you want to be subscribed.

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 SlidingWindowRateLimiter failed leases always reporting no RetryAfter metadata by computing an estimated retry delay based on the sliding window’s per-segment state, aligning behavior with other rate limiter implementations.

Changes:

  • Replace the static singleton failed lease path with per-failure SlidingWindowLease instances that include an estimated RetryAfter.
  • Add CreateFailedSlidingWindowLease (lock-required) to compute retry estimates by walking _requestsPerSegment across segments.
  • Expand tests to validate RetryAfter metadata behavior across common and edge-case scenarios (queued items, partial segment elapsed, window expired).

Reviewed changes

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

File Description
src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/SlidingWindowRateLimiter.cs Compute and return failed leases with populated RetryAfter metadata, with locking to avoid torn reads.
src/libraries/System.Threading.RateLimiting/tests/SlidingWindowRateLimiterTests.cs Add/adjust tests to validate RetryAfter metadata for failed leases and related timing scenarios.

Copilot AI review requested due to automatic review settings July 22, 2026 18:02

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

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

nwoolls added 4 commits July 22, 2026 14:49
…mits

This update ensures the correct lease type is returned when permits become available while waiting for a lock. Added a new test to validate the fix and prevent regression.
Copilot AI review requested due to automatic review settings July 22, 2026 19:07

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

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

@nwoolls

nwoolls commented Jul 23, 2026

Copy link
Copy Markdown
Author

Not sure on the failing builds — I tried running them locally (if I read the YAML correctly) and they succeeded for me on macOS.

./build.sh -s clr+clr.runtime+libs+packs+libs.tests -c Release /p:ArchiveTests=true /p:DevTeamProvisioning=- /p:UseMonoRuntime=false /p:UseNativeAOTRuntime=false /p:RunSmokeTestsOnly=true /p:BuildTestsOnHelix=true /p:EnableAdditionalTimezoneChecks=true /p:UsePortableRuntimePack=false /p:EnableAggressiveTrimming=true
...
Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:02:15.23

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

Labels

area-System.Threading community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RETRY_AFTER is null in Lease object in case of SlidingWindow rate limiter

2 participants