Clamp replenishment timer period to 1ms in rate limiters

[su-senka]

Fixes #109027

Problem

System.Threading.Timer truncates its period to whole milliseconds. So a sub-millisecond ReplenishmentPeriod (TokenBucket) or Window (FixedWindow / SlidingWindow) ends up as a timer period of 0. A period of 0 fires once and then never again, so auto-replenishment silently stops. No exception, the limiter just stalls. The repro in the issue used ReplenishmentPeriod = 500us.

Fix

Clamp the timer period to a 1ms floor at timer creation, in all three replenishing limiters (TokenBucketRateLimiter, FixedWindowRateLimiter, SlidingWindowRateLimiter).

This is the clamp-not-throw direction settled on in the issue. Existing limiters keep working, and a config that would have produced a sub-1ms interval ends up slightly more restrictive than asked for, which is safer than slightly less restrictive.

Notes

The clamp is on the timer only, not on the options. The public ReplenishmentPeriod property still returns whatever the user configured. The tests check this.

SlidingWindow is the easy one to miss here. Its effective period is Window / SegmentsPerWindow, so it can be sub-millisecond even when Window itself is >= 1ms (e.g. Window = 5ms, SegmentsPerWindow = 10 gives 500us). There's a test case for that.

No new exception, so no new resource string. The doc comments on the three options types now mention the 1ms floor.

One thing I left alone: FixedWindowRateLimiter.CreateFailedWindowLease and TokenBucketRateLimiter.CreateFailedTokenLease still compute RetryAfter from the original un-clamped value, so with a sub-1ms config the reported RetryAfter can be a touch under the real timer cadence. Didn't want to change user-visible lease metadata as part of this.

Testing

One regression test per limiter: a sub-millisecond period constructs fine with AutoReplenishment = true, and the observable ReplenishmentPeriod still reflects the configured value. The SlidingWindow test also covers the derived-period case above.

Built and ran the RateLimiting test suite locally on macOS/arm64. Relying on CI for the .NET Framework leg.

[dotnet-policy-service]

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

[su-senka]

@dotnet-policy-service agree

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "fe9a2eaa923b9e61df2934df01c5ee8e513315bc",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "1654543c0a1527e5176b82867e1bb0c43126e2f7",
  "last_reviewed_commit": "fe9a2eaa923b9e61df2934df01c5ee8e513315bc",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "1654543c0a1527e5176b82867e1bb0c43126e2f7",
  "last_recorded_worker_run_id": "29679138661",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "fe9a2eaa923b9e61df2934df01c5ee8e513315bc",
      "review_id": 4730521891
    }
  ]
}

[github-actions]

Holistic Review

Motivation: System.Threading.Timer truncates its period to whole milliseconds, so a sub-millisecond ReplenishmentPeriod (TokenBucket) or Window (FixedWindow / SlidingWindow) collapses to a timer period of 0. A 0 period fires once and then never again, so auto-replenishment silently stalls with no exception. This is a real, reproducible defect (issue #109027).

Approach: At timer construction in each of the three replenishing limiters, the timer period is clamped to a 1ms floor via a shared s_minTimerPeriod constant, before passing it to the Timer constructor. The clamp is applied only to the internal timer; the public ReplenishmentPeriod / Window properties still return the user-configured value. Doc comments on the three options types describe the new floor behavior. The SlidingWindow case correctly clamps the derived Window / SegmentsPerWindow period rather than Window itself.

Summary: The fix is minimal, correct, and consistent across TokenBucketRateLimiter, FixedWindowRateLimiter, and SlidingWindowRateLimiter. The clamp-not-throw direction matches the resolution reached in the issue and is safe: because auto-replenishment adds a fixed amount per tick (TokensPerPeriod / a full window refresh), a clamped shorter-than-1ms configuration becomes at worst slightly more restrictive, never less. The behavior of the non-auto TryReplenish path and the elapsed-time-based (!AutoReplenishment) replenishment math is untouched, which is appropriate. The author's noted omission — RetryAfter in the failed-lease helpers still uses the un-clamped period — is a reasonable scope boundary since it only marginally under-reports cadence for sub-1ms configs and avoids changing user-visible lease metadata.

Detailed Findings

No blocking issues.

Non-actionable observations (not requiring changes):

• The three new regression tests assert only that construction succeeds (IsAutoReplenishing is true) and that the observable ReplenishmentPeriod still reflects the configured value. They do not directly observe that the timer keeps firing over time (which is what actually regressed). That is a reasonable trade-off given the difficulty of deterministically asserting timer cadence in a unit test, and the tests do lock in the "clamp affects only the internal timer, not the public property" contract, which is the part most likely to regress in future refactors.
• The s_minTimerPeriod constant and its clamp logic are duplicated verbatim in all three limiters. This matches the existing per-limiter code structure in this assembly (each limiter is self-contained), so introducing a shared helper is optional and not warranted by this change alone.

Verdict: LGTM.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 49.4 AIC · ⌖ 10.6 AIC · ⊞ 10K