diff --git a/docs/en/framework/infrastructure/operation-rate-limiting.md b/docs/en/framework/infrastructure/operation-rate-limiting.md index bdc55f1c7e..89e808d121 100644 --- a/docs/en/framework/infrastructure/operation-rate-limiting.md +++ b/docs/en/framework/infrastructure/operation-rate-limiting.md @@ -283,6 +283,7 @@ The exception includes the following data properties: | `CurrentCount` | int | Current usage count | | `RemainingCount` | int | Remaining allowed count | | `RetryAfterSeconds` | int | Seconds until the window resets | +| `RetryAfterMinutes` | int | Minutes until the window resets (rounded down) | | `RetryAfter` | string | Localized retry-after description (e.g., "5 minutes") | | `WindowDurationSeconds` | int | Total window duration in seconds | | `WindowDescription` | string | Localized window description | diff --git a/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Checker/OperationRateLimitingChecker.cs b/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Checker/OperationRateLimitingChecker.cs index 3b3006b248..095fa6cbf6 100644 --- a/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Checker/OperationRateLimitingChecker.cs +++ b/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Checker/OperationRateLimitingChecker.cs @@ -65,16 +65,31 @@ public class OperationRateLimitingChecker : IOperationRateLimitingChecker, ITran ThrowRateLimitException(policy, aggregatedResult, context); } - // Phase 2: All rules pass - now increment all counters. - // Also guard against a concurrent race where another request consumed the last quota + // Phase 2: All rules passed in Phase 1 - now increment counters. + // Guard against concurrent races where another request consumed the last quota // between Phase 1 and Phase 2. + // Once any rule fails during increment, stop incrementing subsequent rules + // to minimize wasted quota. Remaining rules use read-only check instead. var incrementResults = new List(); + var phase2Failed = false; foreach (var rule in rules) { - incrementResults.Add(await rule.AcquireAsync(context)); + if (phase2Failed) + { + incrementResults.Add(await rule.CheckAsync(context)); + } + else + { + var result = await rule.AcquireAsync(context); + incrementResults.Add(result); + if (!result.IsAllowed) + { + phase2Failed = true; + } + } } - if (incrementResults.Any(r => !r.IsAllowed)) + if (phase2Failed) { var aggregatedResult = AggregateResults(incrementResults, policy); ThrowRateLimitException(policy, aggregatedResult, context); @@ -132,6 +147,11 @@ public class OperationRateLimitingChecker : IOperationRateLimitingChecker, ITran public virtual async Task ResetAsync(string policyName, OperationRateLimitingContext? context = null) { + if (!Options.IsEnabled) + { + return; + } + context = EnsureContext(context); var policy = await PolicyProvider.GetAsync(policyName); var rules = CreateRules(policy); @@ -153,12 +173,11 @@ public class OperationRateLimitingChecker : IOperationRateLimitingChecker, ITran { var rules = new List(); - for (var i = 0; i < policy.Rules.Count; i++) + foreach (var ruleDefinition in policy.Rules) { rules.Add(new FixedWindowOperationRateLimitingRule( policy.Name, - i, - policy.Rules[i], + ruleDefinition, Store, CurrentUser, CurrentTenant, @@ -188,7 +207,7 @@ public class OperationRateLimitingChecker : IOperationRateLimitingChecker, ITran IsAllowed = isAllowed, RemainingCount = mostRestrictive.RemainingCount, MaxCount = mostRestrictive.MaxCount, - CurrentCount = mostRestrictive.MaxCount - mostRestrictive.RemainingCount, + CurrentCount = mostRestrictive.CurrentCount, RetryAfter = ruleResults.Any(r => !r.IsAllowed && r.RetryAfter.HasValue) ? ruleResults .Where(r => !r.IsAllowed && r.RetryAfter.HasValue) @@ -233,7 +252,7 @@ public class OperationRateLimitingChecker : IOperationRateLimitingChecker, ITran ["IsAllowed"] = ruleResult.IsAllowed, ["MaxCount"] = ruleResult.MaxCount, ["RemainingCount"] = ruleResult.RemainingCount, - ["CurrentCount"] = ruleResult.MaxCount - ruleResult.RemainingCount, + ["CurrentCount"] = ruleResult.CurrentCount, ["WindowDurationSeconds"] = (int)ruleResult.WindowDuration.TotalSeconds, ["WindowDescription"] = ruleResult.WindowDuration > TimeSpan.Zero ? formatter.Format(ruleResult.WindowDuration) diff --git a/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Checker/OperationRateLimitingRuleResult.cs b/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Checker/OperationRateLimitingRuleResult.cs index e05e6bf4fb..d725b8f7f2 100644 --- a/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Checker/OperationRateLimitingRuleResult.cs +++ b/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Checker/OperationRateLimitingRuleResult.cs @@ -8,6 +8,8 @@ public class OperationRateLimitingRuleResult public bool IsAllowed { get; set; } + public int CurrentCount { get; set; } + public int RemainingCount { get; set; } public int MaxCount { get; set; } diff --git a/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Rules/FixedWindowOperationRateLimitingRule.cs b/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Rules/FixedWindowOperationRateLimitingRule.cs index cd2dd4db32..bd869e2c5b 100644 --- a/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Rules/FixedWindowOperationRateLimitingRule.cs +++ b/framework/src/Volo.Abp.OperationRateLimiting/Volo/Abp/OperationRateLimiting/Rules/FixedWindowOperationRateLimitingRule.cs @@ -10,7 +10,6 @@ public class FixedWindowOperationRateLimitingRule : IOperationRateLimitingRule private const string HostTenantKey = "host"; protected string PolicyName { get; } - protected int RuleIndex { get; } protected OperationRateLimitingRuleDefinition Definition { get; } protected IOperationRateLimitingStore Store { get; } protected ICurrentUser CurrentUser { get; } @@ -19,7 +18,6 @@ public class FixedWindowOperationRateLimitingRule : IOperationRateLimitingRule public FixedWindowOperationRateLimitingRule( string policyName, - int ruleIndex, OperationRateLimitingRuleDefinition definition, IOperationRateLimitingStore store, ICurrentUser currentUser, @@ -27,7 +25,6 @@ public class FixedWindowOperationRateLimitingRule : IOperationRateLimitingRule IWebClientInfoProvider webClientInfoProvider) { PolicyName = policyName; - RuleIndex = ruleIndex; Definition = definition; Store = store; CurrentUser = currentUser; @@ -140,6 +137,7 @@ public class FixedWindowOperationRateLimitingRule : IOperationRateLimitingRule { RuleName = $"{PolicyName}:Rule[{(long)Definition.Duration.TotalSeconds}s,{Definition.MaxCount},{Definition.PartitionType}]", IsAllowed = storeResult.IsAllowed, + CurrentCount = storeResult.CurrentCount, RemainingCount = storeResult.MaxCount - storeResult.CurrentCount, MaxCount = storeResult.MaxCount, RetryAfter = storeResult.RetryAfter, diff --git a/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/AbpOperationRateLimitingPhase2EarlyBreakTestModule.cs b/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/AbpOperationRateLimitingPhase2EarlyBreakTestModule.cs new file mode 100644 index 0000000000..b36b9778cd --- /dev/null +++ b/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/AbpOperationRateLimitingPhase2EarlyBreakTestModule.cs @@ -0,0 +1,102 @@ +using System; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; +using Volo.Abp.Autofac; +using Volo.Abp.ExceptionHandling; +using Volo.Abp.Modularity; + +namespace Volo.Abp.OperationRateLimiting; + +/// +/// A mock store that simulates a multi-rule Phase 2 race condition: +/// - GetAsync always reports quota available (Phase 1 passes for all rules). +/// - IncrementAsync succeeds for the first call, fails on the second call +/// (simulating a concurrent race on Rule2), and tracks total increment calls +/// so tests can verify that Rule3 was never incremented (early break). +/// +internal class MultiRuleRaceConditionSimulatorStore : IOperationRateLimitingStore +{ + private int _incrementCallCount; + + /// + /// Total number of IncrementAsync calls made. + /// + public int IncrementCallCount => _incrementCallCount; + + public Task GetAsync(string key, TimeSpan duration, int maxCount) + { + return Task.FromResult(new OperationRateLimitingStoreResult + { + IsAllowed = true, + CurrentCount = 0, + MaxCount = maxCount + }); + } + + public Task IncrementAsync(string key, TimeSpan duration, int maxCount) + { + var callIndex = Interlocked.Increment(ref _incrementCallCount); + + if (callIndex == 2) + { + // Second rule: simulate concurrent race - another request consumed the last slot. + return Task.FromResult(new OperationRateLimitingStoreResult + { + IsAllowed = false, + CurrentCount = maxCount, + MaxCount = maxCount, + RetryAfter = duration + }); + } + + // First rule (and any others if early break fails): succeed. + return Task.FromResult(new OperationRateLimitingStoreResult + { + IsAllowed = true, + CurrentCount = 1, + MaxCount = maxCount + }); + } + + public Task ResetAsync(string key) + { + return Task.CompletedTask; + } +} + +[DependsOn( + typeof(AbpOperationRateLimitingModule), + typeof(AbpExceptionHandlingModule), + typeof(AbpTestBaseModule), + typeof(AbpAutofacModule) +)] +public class AbpOperationRateLimitingPhase2EarlyBreakTestModule : AbpModule +{ + public override void ConfigureServices(ServiceConfigurationContext context) + { + context.Services.Replace( + ServiceDescriptor.Singleton()); + + Configure(options => + { + // 3-rule composite policy: all PartitionByParameter with different durations + // so they generate unique cache keys and don't trigger duplicate rule validation. + options.AddPolicy("TestMultiRuleRacePolicy", policy => + { + policy.AddRule(rule => rule + .WithFixedWindow(TimeSpan.FromHours(1), maxCount: 5) + .PartitionByParameter()); + + policy.AddRule(rule => rule + .WithFixedWindow(TimeSpan.FromHours(2), maxCount: 5) + .PartitionByParameter()); + + policy.AddRule(rule => rule + .WithFixedWindow(TimeSpan.FromHours(3), maxCount: 5) + .PartitionByParameter()); + }); + }); + } +} diff --git a/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/OperationRateLimitingCheckerFixes_Tests.cs b/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/OperationRateLimitingCheckerFixes_Tests.cs index 8d15d2a2b9..fce15fa466 100644 --- a/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/OperationRateLimitingCheckerFixes_Tests.cs +++ b/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/OperationRateLimitingCheckerFixes_Tests.cs @@ -97,6 +97,59 @@ public class OperationRateLimitingCheckerPhase1_Tests : OperationRateLimitingTes } } +/// +/// Tests for Phase 2 early break: when a multi-rule policy encounters a race condition +/// in Phase 2 (Rule2 fails), Rule3 should NOT be incremented. +/// Uses a mock store where IncrementAsync fails on the 2nd call. +/// +public class OperationRateLimitingCheckerPhase2EarlyBreak_Tests + : AbpIntegratedTest +{ + protected override void SetAbpApplicationCreationOptions(AbpApplicationCreationOptions options) + { + options.UseAutofac(); + } + + [Fact] + public async Task Should_Not_Increment_Remaining_Rules_After_Phase2_Failure() + { + // 3-rule policy. Mock store: Rule1 increment succeeds, Rule2 increment fails (race), + // Rule3 should NOT be incremented due to early break. + var checker = GetRequiredService(); + var store = (MultiRuleRaceConditionSimulatorStore)GetRequiredService(); + var context = new OperationRateLimitingContext { Parameter = "early-break-test" }; + + var exception = await Assert.ThrowsAsync(async () => + { + await checker.CheckAsync("TestMultiRuleRacePolicy", context); + }); + + exception.PolicyName.ShouldBe("TestMultiRuleRacePolicy"); + exception.Result.IsAllowed.ShouldBeFalse(); + + // Key assertion: only 2 IncrementAsync calls were made (Rule1 + Rule2). + // Rule3 was skipped (used CheckAsync instead) due to early break. + store.IncrementCallCount.ShouldBe(2); + } + + [Fact] + public async Task Should_Include_All_Rule_Results_Despite_Early_Break() + { + // Even with early break, the aggregated result should contain all 3 rules + // (Rule3 via CheckAsync instead of AcquireAsync). + var checker = GetRequiredService(); + var context = new OperationRateLimitingContext { Parameter = $"all-results-{Guid.NewGuid()}" }; + + var exception = await Assert.ThrowsAsync(async () => + { + await checker.CheckAsync("TestMultiRuleRacePolicy", context); + }); + + exception.Result.RuleResults.ShouldNotBeNull(); + exception.Result.RuleResults!.Count.ShouldBe(3); + } +} + /// /// Tests for Fix #1: Phase 2 in CheckAsync now checks the result of AcquireAsync. /// Uses a mock store that simulates a concurrent race condition: diff --git a/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/OperationRateLimitingChecker_Tests.cs b/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/OperationRateLimitingChecker_Tests.cs index 8a967077c5..08a605c894 100644 --- a/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/OperationRateLimitingChecker_Tests.cs +++ b/framework/test/Volo.Abp.OperationRateLimiting.Tests/Volo/Abp/OperationRateLimiting/OperationRateLimitingChecker_Tests.cs @@ -731,6 +731,57 @@ public class OperationRateLimitingChecker_Tests : OperationRateLimitingTestBase }); } + + [Fact] + public async Task Should_Return_Correct_CurrentCount_In_RuleResults() + { + var param = $"current-count-{Guid.NewGuid()}"; + var context = new OperationRateLimitingContext { Parameter = param }; + + await _checker.CheckAsync("TestSimple", context); + await _checker.CheckAsync("TestSimple", context); + + var status = await _checker.GetStatusAsync("TestSimple", context); + status.RuleResults.ShouldNotBeNull(); + status.RuleResults!.Count.ShouldBe(1); + status.RuleResults[0].CurrentCount.ShouldBe(2); + status.RuleResults[0].RemainingCount.ShouldBe(1); + status.RuleResults[0].MaxCount.ShouldBe(3); + } + + [Fact] + public async Task ResetAsync_Should_Skip_When_Disabled() + { + var options = GetRequiredService>(); + var originalValue = options.Value.IsEnabled; + + try + { + var param = $"reset-disabled-{Guid.NewGuid()}"; + var context = new OperationRateLimitingContext { Parameter = param }; + + // Exhaust the quota + await _checker.CheckAsync("TestSimple", context); + await _checker.CheckAsync("TestSimple", context); + await _checker.CheckAsync("TestSimple", context); + + // Disable and call ResetAsync — should be a no-op (counter not actually reset) + options.Value.IsEnabled = false; + await _checker.ResetAsync("TestSimple", context); + + // Re-enable: quota should still be exhausted because reset was skipped + options.Value.IsEnabled = true; + await Assert.ThrowsAsync(async () => + { + await _checker.CheckAsync("TestSimple", context); + }); + } + finally + { + options.Value.IsEnabled = originalValue; + } + } + private static ClaimsPrincipal CreateClaimsPrincipal(Guid userId) { return new ClaimsPrincipal(