Repository: xanzy/go-gitlab
Pull request #1993 (2024-08-19 14:16:28)

Add an exponential backoff to the retry function
This PR adds a 200% exponential backoff to the default retry function. This backoff only applies in situations where the Rate Limit header doesn't exist, which should be vanishingly few situations, but unfortunately does exist within the SaaS environment of GitLab: https://gitlab.com/gitlab-org/gitlab/-/issues/365728

Without the backoff, all 5 default retries would be exhausted in < 1-2 seconds, which would likely result in 429 errors being passed upstream since the API limits on SaaS are measured in requests per minute. With the backoff, the 100ms backoff translates to 3.2 seconds on the final wait (which isn't horrible). If a user increased the retry up to 10 requests, the final retry would wait just shy of 2 minutes.

This feels like a reasonable low impact change that would result in a good quality of life improvement for most users of the library :)

I also debated making the exponent configurable, but I'm not sure that's necessary, since you can already control the min and the max.


Patch:
diff --git a/gitlab.go b/gitlab.go
index db60417..7d1a457 100644
--- a/gitlab.go
+++ b/gitlab.go
@@ -24,6 +24,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"math"
 	"math/rand"
 	"mime/multipart"
 	"net/http"
@@ -509,7 +510,7 @@ func (c *Client) retryHTTPBackoff(min, max time.Duration, attemptNum int, resp *
 // min and max are mainly used for bounding the jitter that will be added to
 // the reset time retrieved from the headers. But if the final wait time is
 // less then min, min will be used instead.
-func rateLimitBackoff(min, max time.Duration, _ int, resp *http.Response) time.Duration {
+func rateLimitBackoff(min, max time.Duration, attemptNum int, resp *http.Response) time.Duration {
 	// rnd is used to generate pseudo-random numbers.
 	rnd := rand.New(rand.NewSource(time.Now().UnixNano()))
 
@@ -524,6 +525,11 @@ func rateLimitBackoff(min, max time.Duration, _ int, resp *http.Response) time.D
 					min = wait
 				}
 			}
+		} else {
+			// In case the RateLimit-Reset header is not set, back off an additional
+			// 100% exponentially. With the default milliseconds being set to 100 for
+			// `min`, this makes the 5th retry wait 3.2 seconds (3,200 ms) by default.
+			min = time.Duration(float64(min) * math.Pow(2, float64(attemptNum)))
 		}
 	}
 


Test patch:
diff --git a/gitlab_test.go b/gitlab_test.go
index d6e0273..b07631f 100644
--- a/gitlab_test.go
+++ b/gitlab_test.go
@@ -418,3 +418,39 @@ func TestPaginationPopulatePageValuesKeyset(t *testing.T) {
 		}
 	}
 }
+
+func TestExponentialBackoffLogic(t *testing.T) {
+	// Can't use the default `setup` because it disabled the backoff
+	mux := http.NewServeMux()
+	server := httptest.NewServer(mux)
+	t.Cleanup(server.Close)
+	client, err := NewClient("",
+		WithBaseURL(server.URL),
+	)
+	if err != nil {
+		t.Fatalf("Failed to create client: %v", err)
+	}
+
+	// Create a method that returns 429
+	mux.HandleFunc("/api/v4/projects/1", func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, http.MethodGet)
+		w.WriteHeader(http.StatusTooManyRequests)
+	})
+
+	// Measure the time at the start of the test
+	start := time.Now()
+
+	// Send a request (which will get a bunch of 429s)
+	// None of the responses matter, so ignore them all
+	_, resp, _ := client.Projects.GetProject(1, nil)
+	end := time.Now()
+
+	// The test should run for _at least_ 3,200 milliseconds
+	duration := float64(end.Sub(start))
+	if duration < float64(3200*time.Millisecond) {
+		t.Fatal("Wait was shorter than expected. Expected a minimum of 5 retries taking 3200 milliseconds, got:", duration)
+	}
+	if resp.StatusCode != 429 {
+		t.Fatal("Expected to get a 429 code given the server is hard-coded to return this. Received instead:", resp.StatusCode)
+	}
+}