[RateLimiter] CompoundLimiter was accepting requests even when some limiters already consumed all tokens

Author: 10n

src/Symfony/Component/RateLimiter/CompoundLimiter.php

@@ -42,7 +42,11 @@ public function consume(int $tokens = 1): RateLimit
         foreach ($this->limiters as $limiter) {
             $rateLimit = $limiter->consume($tokens);
 
-            if (null === $minimalRateLimit || $rateLimit->getRemainingTokens() < $minimalRateLimit->getRemainingTokens()) {
+            if (
+                null === $minimalRateLimit
+                || $rateLimit->getRemainingTokens() < $minimalRateLimit->getRemainingTokens()
+                || ($minimalRateLimit->isAccepted() && !$rateLimit->isAccepted())
+            ) {
                 $minimalRateLimit = $rateLimit;
             }
         }

src/Symfony/Component/RateLimiter/Tests/CompoundLimiterTest.php

@@ -36,19 +36,49 @@ public function testConsume()
     {
         $limiter1 = $this->createLimiter(4, new \DateInterval('PT1S'));
         $limiter2 = $this->createLimiter(8, new \DateInterval('PT10S'));
-        $limiter3 = $this->createLimiter(12, new \DateInterval('PT30S'));
+        $limiter3 = $this->createLimiter(16, new \DateInterval('PT30S'));
         $limiter = new CompoundLimiter([$limiter1, $limiter2, $limiter3]);
 
-        $this->assertEquals(0, $limiter->consume(4)->getRemainingTokens(), 'Limiter 1 reached the limit');
+        $rateLimit = $limiter->consume(4);
+        $this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 1 reached the limit');
+        $this->assertTrue($rateLimit->isAccepted(), 'All limiters accept (exact limit on limiter 1)');
+
+        $rateLimit = $limiter->consume(1);
+        $this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 1 reached the limit');
+        $this->assertFalse($rateLimit->isAccepted(), 'Limiter 1 did not accept limit');
+
         sleep(1); // reset limiter1's window
-        $this->assertTrue($limiter->consume(3)->isAccepted());
 
-        $this->assertEquals(0, $limiter->consume()->getRemainingTokens(), 'Limiter 2 has no remaining tokens left');
-        sleep(10); // reset limiter2's window
-        $this->assertTrue($limiter->consume(3)->isAccepted());
+        $rateLimit = $limiter->consume(3);
+        $this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 2 consumed exactly the remaining tokens');
+        $this->assertTrue($rateLimit->isAccepted(), 'All accept the request (exact limit on limiter 2)');
+
+        $rateLimit = $limiter->consume(1);
+        $this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 2 had remaining tokens left');
+        $this->assertFalse($rateLimit->isAccepted(), 'Limiter 2 did not accept the request');
+
+        sleep(1); // reset limiter1's window again,  to make sure that the limiter2 overrides limiter1
+
+        // make sure to consume all allowed by limiter1, limiter2 already had 0 remaining
+        $rateLimit = $limiter->consume(4);
+        $this->assertEquals(
+            0,
+            $rateLimit->getRemainingTokens(),
+            'Limiter 1 consumed the remaining tokens (accept), Limiter 2 did not have any remaining (not accept)'
+        );
+        $this->assertFalse($rateLimit->isAccepted(), 'Limiter 2 reached the limit already');
+
+        sleep(10); // reset limiter2's window (also limiter1)
+
+        $rateLimit = $limiter->consume(3);
+        $this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 3 had exactly 3 tokens   (accept)');
+        $this->assertTrue($rateLimit->isAccepted());
+
+        $rateLimit = $limiter->consume(1);
+        $this->assertFalse($rateLimit->isAccepted(), 'Limiter 3 reached the limit previously');
+
+        sleep(30); // reset limiter3's window (also limiter1 and limiter2)
 
-        $this->assertEquals(0, $limiter->consume()->getRemainingTokens(), 'Limiter 3 reached the limit');
-        sleep(20); // reset limiter3's window
         $this->assertTrue($limiter->consume()->isAccepted());
     }