fix #156, negative hits when remaining 0 (#157)

algorithms.go

@@ -176,7 +176,7 @@ func tokenBucket(ctx context.Context, s Store, c Cache, r *RateLimitReq) (resp *
 		}
 
 		// If we are already at the limit.
-		if rl.Remaining == 0 {
+		if rl.Remaining == 0 && r.Hits > 0 {
 			span.AddEvent("Already over the limit")
 			overLimitCounter.Add(1)
 			rl.Status = Status_OVER_LIMIT
@@ -414,7 +414,7 @@ func leakyBucket(ctx context.Context, s Store, c Cache, r *RateLimitReq) (resp *
 		}
 
 		// If we are already at the limit
-		if int64(b.Remaining) == 0 {
+		if int64(b.Remaining) == 0 && r.Hits > 0 {
 			overLimitCounter.Add(1)
 			rl.Status = Status_OVER_LIMIT
 			return rl, nil

functional_test.go

@@ -292,6 +292,78 @@ func TestTokenBucketGregorian(t *testing.T) {
 	}
 }
 
+func TestTokenBucketNegativeHits(t *testing.T) {
+	defer clock.Freeze(clock.Now()).Unfreeze()
+
+	addr := cluster.GetRandomPeer(cluster.DataCenterNone).GRPCAddress
+	client, errs := guber.DialV1Server(addr, nil)
+	require.Nil(t, errs)
+
+	tests := []struct {
+		name      string
+		Remaining int64
+		Status    guber.Status
+		Sleep     clock.Duration
+		Hits      int64
+	}{
+		{
+			name:      "remaining should be three",
+			Remaining: 3,
+			Status:    guber.Status_UNDER_LIMIT,
+			Sleep:     clock.Duration(0),
+			Hits:      -1,
+		},
+		{
+			name:      "remaining should be four and under limit",
+			Remaining: 4,
+			Status:    guber.Status_UNDER_LIMIT,
+			Sleep:     clock.Duration(0),
+			Hits:      -1,
+		},
+		{
+			name:      "remaining should be 0 and under limit",
+			Remaining: 0,
+			Status:    guber.Status_UNDER_LIMIT,
+			Sleep:     clock.Duration(0),
+			Hits:      4,
+		},
+		{
+			name:      "remaining should be 1 and under limit",
+			Remaining: 1,
+			Status:    guber.Status_UNDER_LIMIT,
+			Sleep:     clock.Duration(0),
+			Hits:      -1,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			resp, err := client.GetRateLimits(context.Background(), &guber.GetRateLimitsReq{
+				Requests: []*guber.RateLimitReq{
+					{
+						Name:      "test_token_bucket_negative",
+						UniqueKey: "account:12345",
+						Algorithm: guber.Algorithm_TOKEN_BUCKET,
+						Duration:  guber.Millisecond * 5,
+						Limit:     2,
+						Hits:      tt.Hits,
+					},
+				},
+			})
+			require.Nil(t, err)
+
+			rl := resp.Responses[0]
+
+			assert.Empty(t, rl.Error)
+			assert.Equal(t, tt.Status, rl.Status)
+			assert.Equal(t, tt.Remaining, rl.Remaining)
+			assert.Equal(t, int64(2), rl.Limit)
+			assert.True(t, rl.ResetTime != 0)
+			clock.Advance(tt.Sleep)
+		})
+	}
+}
+
 func TestLeakyBucket(t *testing.T) {
 	defer clock.Freeze(clock.Now()).Unfreeze()
 
@@ -591,6 +663,77 @@ func TestLeakyBucketGregorian(t *testing.T) {
 	}
 }
 
+func TestLeakyBucketNegativeHits(t *testing.T) {
+	defer clock.Freeze(clock.Now()).Unfreeze()
+
+	client, errs := guber.DialV1Server(cluster.PeerAt(0).GRPCAddress, nil)
+	require.Nil(t, errs)
+
+	tests := []struct {
+		Name      string
+		Hits      int64
+		Remaining int64
+		Status    guber.Status
+		Sleep     clock.Duration
+	}{
+		{
+			Name:      "first hit",
+			Hits:      1,
+			Remaining: 9,
+			Status:    guber.Status_UNDER_LIMIT,
+			Sleep:     clock.Duration(0),
+		},
+		{
+			Name:      "can increase remaining",
+			Hits:      -1,
+			Remaining: 10,
+			Status:    guber.Status_UNDER_LIMIT,
+			Sleep:     clock.Duration(0),
+		},
+		{
+			Name:      "remaining should be zero",
+			Hits:      10,
+			Remaining: 0,
+			Status:    guber.Status_UNDER_LIMIT,
+			Sleep:     clock.Duration(0),
+		},
+		{
+			Name:      "can append one to remaining when remaining is zero",
+			Hits:      -1,
+			Remaining: 1,
+			Status:    guber.Status_UNDER_LIMIT,
+			Sleep:     clock.Duration(0),
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.Name, func(t *testing.T) {
+			resp, err := client.GetRateLimits(context.Background(), &guber.GetRateLimitsReq{
+				Requests: []*guber.RateLimitReq{
+					{
+						Name:      "test_leaky_bucket_negative",
+						UniqueKey: "account:12345",
+						Algorithm: guber.Algorithm_LEAKY_BUCKET,
+						Duration:  guber.Second * 30,
+						Hits:      test.Hits,
+						Limit:     10,
+					},
+				},
+			})
+			require.NoError(t, err)
+			require.Len(t, resp.Responses, 1)
+
+			rl := resp.Responses[0]
+
+			assert.Equal(t, test.Status, rl.Status)
+			assert.Equal(t, test.Remaining, rl.Remaining)
+			assert.Equal(t, int64(10), rl.Limit)
+			assert.Equal(t, clock.Now().Unix()+(rl.Limit-rl.Remaining)*3, rl.ResetTime/1000)
+			clock.Advance(test.Sleep)
+		})
+	}
+}
+
 func TestMissingFields(t *testing.T) {
 	client, errs := guber.DialV1Server(cluster.GetRandomPeer(cluster.DataCenterNone).GRPCAddress, nil)
 	require.Nil(t, errs)