FIX: sliding window end time in rate limiter (#11691)

FIX: sliding window end time in rate limiter (#11691)

If the sliding window size is N seconds, then a moment at the Nth second should be considered as the moment outside of the sliding window.

Otherwise, if the sliding window is already full, at the Nth second, a new call wouldn’t be allowed, but a time to wait before the next call would be equal to zero, which is confusing.

In other words, the end of the time range shouldn’t be included in the sliding window.

Let’s say we start at the second 0, and the sliding window size is 10 seconds. In the current version of rate limiter, this sliding window will be considered as a time range [0, 10] (including the end of the range), which actually is 11 seconds in length.

After this fix, the time range will be considered as [0, 10) (excluding the end of the range), which is exactly 10 seconds in length.

diff --git a/lib/rate_limiter.rb b/lib/rate_limiter.rb
index 2622593..83761bf 100644
--- a/lib/rate_limiter.rb
+++ b/lib/rate_limiter.rb
@@ -66,7 +66,7 @@ class RateLimiter
 
 
       if ((tonumber(redis.call("LLEN", key)) < max) or
-          (now - tonumber(redis.call("LRANGE", key, -1, -1)[1])) > secs) then
+          (now - tonumber(redis.call("LRANGE", key, -1, -1)[1])) >= secs) then
         redis.call("LPUSH", key, now)
         redis.call("LTRIM", key, 0, max - 1)
         redis.call("EXPIRE", key, secs * 2)
@@ -91,7 +91,7 @@ class RateLimiter
       local return_val = 0
 
       if ((tonumber(redis.call("LLEN", key)) < max) or
-          (now - tonumber(redis.call("LRANGE", key, -1, -1)[1])) > secs) then
+          (now - tonumber(redis.call("LRANGE", key, -1, -1)[1])) >= secs) then
         return_val = 1
       else
         return_val = 0
@@ -185,8 +185,8 @@ class RateLimiter
   def is_under_limit?
     # number of events in buffer less than max allowed? OR
     (redis.llen(prefixed_key) < @max) ||
-    # age bigger than silding window size?
-    (age_of_oldest > @secs)
+    # age bigger or equal than sliding window size?
+    (age_of_oldest >= @secs)
   end
 
   def rate_unlimited?
diff --git a/spec/components/rate_limiter_spec.rb b/spec/components/rate_limiter_spec.rb
index 506fa13..2b32df7 100644
--- a/spec/components/rate_limiter_spec.rb
+++ b/spec/components/rate_limiter_spec.rb
@@ -72,10 +72,10 @@ describe RateLimiter do
           limiter.performed!
         end.to raise_error(RateLimiter::LimitExceeded)
 
-        freeze_time 31.seconds.from_now
+        freeze_time 30.seconds.from_now
 
-        limiter.performed!
-        limiter.performed!
+        expect { limiter.performed! }.not_to raise_error
+        expect { limiter.performed! }.not_to raise_error
 
       end
     end
@@ -150,6 +150,7 @@ describe RateLimiter do
 
     context "multiple calls" do
       before do
+        freeze_time
         rate_limiter.performed!
         rate_limiter.performed!
       end
@@ -160,7 +161,15 @@ describe RateLimiter do
       end
 
       it "raises an error the third time called" do
-        expect { rate_limiter.performed! }.to raise_error(RateLimiter::LimitExceeded)
+        expect { rate_limiter.performed! }.to raise_error do |error|
+          expect(error).to be_a(RateLimiter::LimitExceeded)
+          expect(error).to having_attributes(available_in: 60)
+        end
+      end
+
+      it 'raises no error when the sliding window ended' do
+        freeze_time 60.seconds.from_now
+        expect { rate_limiter.performed! }.not_to raise_error
       end
 
       context "as an admin/moderator" do

GitHub sha: e25dd41a

This commit appears in #11691 which was approved by eviltrout. It was merged by eviltrout.