ratelimit: redis should retry if allowed requests exceeded

Implements retry on Take() for NewRedisLimiter, and now correctly logs debug
message when throttling occurs.

When configured to use redis, ratelimiting has always been least effort. While
Take() may sleep for the requested RetryAfter time, NewRedisLimiter never
re-checks to see if we're actually allowed to make further requests after
the RetryAfter time expires. By re-checking Allow(), this actually enforces
the desired ratelimit when multiple routines are scheduled simultaneously.

This is not an issue with NewLocalLimiter since Limiter.Wait() will block until
an event is allowed (or error with log.Fatal(), terminating the application).

Additionally, the "throttled GitLab requests" message was never useful under its
previous conditions. Both NewRedisLimiter and NewLocalLimiter are calculating
`start.Sub(time.Now())` from the end of the function, which always returns a
negative Duration, meaning the condition was never satisfied.

Since NewLocalLimiter is already throttling correctly via Wait(), and since any
non-successful return from Limiter.Wait() terminates the application via
log.Fatal(); this means this "throttled" log message is likely only useful
in NewRedisLimiter (when it retries).

pkg/ratelimit/ratelimit.go

@@ -3,8 +3,6 @@ package ratelimit
 import (
 	"context"
 	"time"
-
-	log "github.com/sirupsen/logrus"
 )
 
 // Limiter ..
@@ -14,13 +12,5 @@ type Limiter interface {
 
 // Take ..
 func Take(ctx context.Context, l Limiter) {
-	throttled := l.Take(ctx)
-
-	if throttled.Milliseconds() > 10 {
-		log.WithFields(
-			log.Fields{
-				"for": throttled.String(),
-			},
-		).Debug("throttled GitLab requests")
-	}
+	l.Take(ctx)
 }

pkg/ratelimit/redis.go

@@ -29,14 +29,25 @@ func NewRedisLimiter(redisClient *redis.Client, maxRPS int) Limiter {
 func (r Redis) Take(ctx context.Context) time.Duration {
 	start := time.Now()
 
-	res, err := r.Allow(ctx, redisKey, redis_rate.PerSecond(r.MaxRPS))
-	if err != nil {
-		log.WithContext(ctx).
-			WithError(err).
-			Fatal()
+	for {
+		res, err := r.Allow(ctx, redisKey, redis_rate.PerSecond(r.MaxRPS))
+		if err != nil {
+			log.WithContext(ctx).
+				WithError(err).
+				Fatal()
+		}
+
+		if res.Allowed > 0 {
+			break;
+		} else {
+			log.WithFields(
+				log.Fields{
+					"for": res.RetryAfter.String(),
+				},
+			).Debug("throttled GitLab requests")
+			time.Sleep(res.RetryAfter)
+		}
 	}
 
-	time.Sleep(res.RetryAfter)
-
 	return start.Sub(time.Now())
 }