From 1b0f272ce15e4a75602b8f9af7a3907413953ed0 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Tue, 19 May 2026 13:32:51 -0400 Subject: [PATCH 01/15] Implement dual-path retry with exponential backoff and rate-limit handling Replace the fixed 10-attempt retry loop with a structured retry system: - 429 + Retry-After: sleep without consuming retry budget, capped by MaxRateLimitDuration - Other retryable errors: counted exponential backoff (base 500ms, 2x, cap 60s) bounded by MaxRetries and MaxTotalBackoffDuration - Non-retryable status codes (4xx except 408/410/429/460, plus 501/505/511) discard immediately - Remove select on c.quit from retry sleeps so Close() waits for in-flight retries to complete naturally via wg.Wait() - Add X-Retry-Count header on retry attempts - Wire MaxRetries config through to e2e-cli - Enable retry test suite in e2e-config --- analytics.go | 130 +++++++++++++++++++++++++++++++--------- config.go | 35 ++++++++++- e2e-cli/e2e-config.json | 2 +- e2e-cli/main.go | 4 ++ error.go | 11 ++++ go.mod | 15 +++++ go.sum | 39 +++++++----- status.go | 46 ++++++++++++++ status_test.go | 62 +++++++++++++++++++ 9 files changed, 297 insertions(+), 47 deletions(-) create mode 100644 status.go create mode 100644 status_test.go diff --git a/analytics.go b/analytics.go index e487ba2..da1c9eb 100644 --- a/analytics.go +++ b/analytics.go @@ -1,15 +1,13 @@ package analytics import ( + "bytes" + "encoding/json" "fmt" "io" - "io/ioutil" + "net/http" "strconv" "sync" - - "bytes" - "encoding/json" - "net/http" "time" ) @@ -243,10 +241,21 @@ func (c *client) sendAsync(msgs []message, wg *sync.WaitGroup, ex *executor) { } } +// httpError is returned by report() for non-2xx/3xx responses. +type httpError struct { + StatusCode int + Retryable bool + IsRateLimit bool + RetryAfter int64 // seconds from Retry-After header; 0 if absent + Body string +} + +func (e *httpError) Error() string { + return fmt.Sprintf("%d %s", e.StatusCode, e.Body) +} + // Send batch request. func (c *client) send(msgs []message) { - const attempts = 10 - b, err := json.Marshal(batch{ MessageId: c.uid(), SentAt: c.now(), @@ -260,28 +269,74 @@ func (c *client) send(msgs []message) { return } - for i := 0; i != attempts; i++ { - if err = c.upload(b); err == nil { + var ( + totalAttempts int + backoffAttempts int + firstFailureTime time.Time + rateLimitStartTime time.Time + lastErr error + ) + + for { + totalAttempts++ + uploadErr := c.upload(b, totalAttempts) + + if uploadErr == nil { c.notifySuccess(msgs) return } - // Wait for either a retry timeout or the client to be closed. - select { - case <-time.After(c.RetryAfter(i)): - case <-c.quit: - c.errorf("%d messages dropped because they failed to be sent and the client was closed", len(msgs)) - c.notifyFailure(msgs, err) + lastErr = uploadErr + + httpErr, ok := uploadErr.(*httpError) + if !ok { + // Network-level error — treat as retryable backoff + httpErr = &httpError{Retryable: true} + } + + if !httpErr.Retryable { + c.errorf("messages dropped due to non-retryable error - %s", uploadErr) + c.notifyFailure(msgs, uploadErr) return } - } - c.errorf("%d messages dropped because they failed to be sent after %d attempts", len(msgs), attempts) - c.notifyFailure(msgs, err) + if httpErr.IsRateLimit && httpErr.RetryAfter > 0 { + // Retry-After present — sleep without consuming retry budget + if rateLimitStartTime.IsZero() { + rateLimitStartTime = c.now() + } + if c.now().Sub(rateLimitStartTime) > c.MaxRateLimitDuration { + c.errorf("messages dropped - %s", ErrRateLimitBudgetExceeded) + c.notifyFailure(msgs, ErrRateLimitBudgetExceeded) + return + } + time.Sleep(time.Duration(httpErr.RetryAfter) * time.Second) + continue + } + + // Counted backoff retry + if firstFailureTime.IsZero() { + firstFailureTime = c.now() + } + if c.now().Sub(firstFailureTime) > c.MaxTotalBackoffDuration { + c.errorf("messages dropped - %s", ErrBackoffBudgetExceeded) + c.notifyFailure(msgs, ErrBackoffBudgetExceeded) + return + } + + backoffAttempts++ + if backoffAttempts > c.MaxRetries { + c.errorf("%d messages dropped after %d attempts", len(msgs), totalAttempts) + c.notifyFailure(msgs, lastErr) + return + } + + time.Sleep(c.RetryAfter(backoffAttempts - 1)) + } } -// Upload serialized batch message. -func (c *client) upload(b []byte) error { +// Upload serialized batch message. attempt is 1-based (1 = first attempt). +func (c *client) upload(b []byte, attempt int) error { url := c.Endpoint + "/v1/batch" req, err := http.NewRequest("POST", url, bytes.NewReader(b)) if err != nil { @@ -294,8 +349,12 @@ func (c *client) upload(b []byte) error { req.Header.Add("Content-Length", strconv.Itoa(len(b))) req.SetBasicAuth(c.key, "") - res, err := c.http.Do(req) + // Spec item 7: omit on first attempt, send 1-based count on retries + if attempt > 1 { + req.Header.Add("X-Retry-Count", strconv.Itoa(attempt-1)) + } + res, err := c.http.Do(req) if err != nil { c.errorf("sending request - %s", err) return err @@ -306,21 +365,34 @@ func (c *client) upload(b []byte) error { } // Report on response body. -func (c *client) report(res *http.Response) (err error) { - var body []byte - - if res.StatusCode < 300 { +func (c *client) report(res *http.Response) error { + // Spec item 1: 2xx and 3xx are success + if isSuccess(res.StatusCode) { c.debugf("response %s", res.Status) - return + return nil } - if body, err = ioutil.ReadAll(res.Body); err != nil { + body, err := io.ReadAll(res.Body) + if err != nil { c.errorf("response %d %s - %s", res.StatusCode, res.Status, err) - return + return err } c.logf("response %d %s – %s", res.StatusCode, res.Status, string(body)) - return fmt.Errorf("%d %s", res.StatusCode, res.Status) + + retryable, isRateLimit := retryableStatus(res.StatusCode) + var retryAfterSecs int64 + if isRateLimit { + retryAfterSecs = parseRetryAfter(res.Header.Get("Retry-After"), maxRetryAfterSeconds) + } + + return &httpError{ + StatusCode: res.StatusCode, + Retryable: retryable, + IsRateLimit: isRateLimit, + RetryAfter: retryAfterSecs, + Body: string(body), + } } // Batch loop. diff --git a/config.go b/config.go index 2672d86..29ad7fc 100644 --- a/config.go +++ b/config.go @@ -61,6 +61,15 @@ type Config struct { // If not set the client will fallback to use a default retry policy. RetryAfter func(int) time.Duration + // Maximum number of counted backoff retries. Defaults to DefaultMaxRetries. + MaxRetries int + + // Wall-clock cap on total time spent in backoff retries. Defaults to DefaultMaxTotalBackoffDuration. + MaxTotalBackoffDuration time.Duration + + // Wall-clock cap on total time spent retrying after 429 Retry-After responses. Defaults to DefaultMaxRateLimitDuration. + MaxRateLimitDuration time.Duration + // A function called by the client to generate unique message identifiers. // The client uses a UUID generator if none is provided. // This field is not exported and only exposed internally to let unit tests @@ -92,6 +101,18 @@ const DefaultInterval = 5 * time.Second // was explicitly set. const DefaultBatchSize = 250 +// DefaultMaxRetries is the default number of counted backoff retries. +const DefaultMaxRetries = 10 + +// DefaultMaxTotalBackoffDuration is the default wall-clock cap on total backoff time. +const DefaultMaxTotalBackoffDuration = 12 * time.Hour + +// DefaultMaxRateLimitDuration is the default wall-clock cap on 429 Retry-After retries. +const DefaultMaxRateLimitDuration = 12 * time.Hour + +// maxRetryAfterSeconds is the cap applied to Retry-After header values. +const maxRetryAfterSeconds = int64(300) + // Verifies that fields that don't have zero-values are set to valid values, // returns an error describing the problem if a field was invalid. func (c *Config) validate() error { @@ -142,7 +163,19 @@ func makeConfig(c Config) Config { } if c.RetryAfter == nil { - c.RetryAfter = backo.DefaultBacko().Duration + c.RetryAfter = backo.NewBacko(500*time.Millisecond, 2, 0, 60*time.Second).Duration + } + + if c.MaxRetries == 0 { + c.MaxRetries = DefaultMaxRetries + } + + if c.MaxTotalBackoffDuration == 0 { + c.MaxTotalBackoffDuration = DefaultMaxTotalBackoffDuration + } + + if c.MaxRateLimitDuration == 0 { + c.MaxRateLimitDuration = DefaultMaxRateLimitDuration } if c.uid == nil { diff --git a/e2e-cli/e2e-config.json b/e2e-cli/e2e-config.json index d7deb43..f926123 100644 --- a/e2e-cli/e2e-config.json +++ b/e2e-cli/e2e-config.json @@ -1,6 +1,6 @@ { "sdk": "go", - "test_suites": "basic", + "test_suites": "basic,retry", "auto_settings": false, "patch": null, "env": {} diff --git a/e2e-cli/main.go b/e2e-cli/main.go index eb35f89..69b9889 100644 --- a/e2e-cli/main.go +++ b/e2e-cli/main.go @@ -219,6 +219,10 @@ func run(input Input) Output { cfg.BatchSize = input.Config.FlushAt } + if input.Config.MaxRetries > 0 { + cfg.MaxRetries = input.Config.MaxRetries + } + client, err := analytics.NewWithConfig(input.WriteKey, cfg) if err != nil { return Output{ diff --git a/error.go b/error.go index d550386..218a4ee 100644 --- a/error.go +++ b/error.go @@ -57,4 +57,15 @@ var ( // failed because the JSON representation of a message exceeded the upper // limit. ErrMessageTooBig = errors.New("the message exceeds the maximum allowed size") + + // ErrBackoffBudgetExceeded is returned when the maximum total backoff + // duration is exceeded before a batch upload succeeds. + ErrBackoffBudgetExceeded = errors.New("max total backoff duration exceeded") + + // ErrRateLimitBudgetExceeded is returned when the maximum rate-limit + // (429 Retry-After) duration is exceeded. + ErrRateLimitBudgetExceeded = errors.New("max rate limit duration exceeded") + + // ErrRetriesExhausted is returned when all retry attempts are consumed. + ErrRetriesExhausted = errors.New("retries exhausted") ) diff --git a/go.mod b/go.mod index 99f9a03..c7ec864 100644 --- a/go.mod +++ b/go.mod @@ -6,4 +6,19 @@ require ( github.com/google/uuid v1.3.0 github.com/segmentio/backo-go v1.0.0 github.com/segmentio/conf v1.2.0 + github.com/stretchr/testify v1.11.1 +) + +require ( + github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869 // indirect + github.com/davecgh/go-spew v1.1.1 // indirect + github.com/kr/pretty v0.3.1 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect + github.com/segmentio/go-snakecase v1.1.0 // indirect + github.com/segmentio/objconv v1.0.1 // indirect + gopkg.in/go-playground/assert.v1 v1.2.1 // indirect + gopkg.in/go-playground/mold.v2 v2.2.0 // indirect + gopkg.in/validator.v2 v2.0.0-20180514200540-135c24b11c19 // indirect + gopkg.in/yaml.v2 v2.2.1 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index 120e887..23cfd3f 100644 --- a/go.sum +++ b/go.sum @@ -1,21 +1,20 @@ github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869 h1:DDGfHa7BWjL4YnC6+E63dPcxHo2sUxDIu8g3QgEJdRY= github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869/go.mod h1:Ekp36dRnpXw/yCqJaO+ZrUyxD+3VXMFFr56k5XYrpB4= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/google/uuid v1.3.0 h1:t6JiXgmwXMjEs8VusXIJk2BXHsn+wx8BZdTaoZ5fu7I= github.com/google/uuid v1.3.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= -github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= -github.com/kr/pretty v0.3.0 h1:WgNl7dwNpEZ6jJ9k1snq4pZsg7DOEN8hP9Xw0Tsjwk0= -github.com/kr/pretty v0.3.0/go.mod h1:640gp4NfQd8pI5XOwp5fnNeVWj67G7CFk/SaSQn7NBk= -github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= -github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= +github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= +github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= -github.com/rogpeppe/go-internal v1.6.1 h1:/FiVV8dS/e+YqF2JvO3yXRFbBLTIuSDkuC7aBOAvL+k= -github.com/rogpeppe/go-internal v1.6.1/go.mod h1:xXDCJY+GAPziupqXw64V24skbSoqbTEfhy4qGm1nDQc= -github.com/segmentio/analytics-go v3.1.0+incompatible h1:IyiOfUgQFVHvsykKKbdI7ZsH374uv3/DfZUo9+G0Z80= -github.com/segmentio/analytics-go v3.1.0+incompatible/go.mod h1:C7CYBtQWk4vRk2RyLu0qOcbHJ18E3F1HV2C/8JvKN48= -github.com/segmentio/backo-go v0.0.0-20200129164019-23eae7c10bd3 h1:ZuhckGJ10ulaKkdvJtiAqsLTiPrLaXSdnVgXJKJkTxE= -github.com/segmentio/backo-go v0.0.0-20200129164019-23eae7c10bd3/go.mod h1:9/Rh6yILuLysoQnZ2oNooD2g7aBnvM7r/fNVxRNWfBc= +github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/rogpeppe/go-internal v1.9.0 h1:73kH8U+JUqXU8lRuOHeVHaa/SZPifC7BkcraZVejAe8= +github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs= github.com/segmentio/backo-go v1.0.0 h1:kbOAtGJY2DqOR0jfRkYEorx/b18RgtepGtY3+Cpe6qA= github.com/segmentio/backo-go v1.0.0/go.mod h1:kJ9mm9YmoWSkk+oQ+5Cj8DEoRCX2JT6As4kEtIIOp1M= github.com/segmentio/conf v1.2.0 h1:5OT9+6OyVHLsFLsiJa/2KlqiA1m7mpdUBlkB/qYTMts= @@ -24,12 +23,17 @@ github.com/segmentio/go-snakecase v1.1.0 h1:ZJO4SNKKV0MjGOv0LHnixxN5FYv1JKBnVXEu github.com/segmentio/go-snakecase v1.1.0/go.mod h1:jk1miR5MS7Na32PZUykG89Arm+1BUSYhuGR6b7+hJto= github.com/segmentio/objconv v1.0.1 h1:QjfLzwriJj40JibCV3MGSEiAoXixbp4ybhwfTB8RXOM= github.com/segmentio/objconv v1.0.1/go.mod h1:auayaH5k3137Cl4SoXTgrzQcuQDmvuVtZgS0fb1Ahys= -github.com/xtgo/uuid v0.0.0-20140804021211-a0b114877d4c h1:3lbZUMbMiGUW/LMkfsEABsc5zNT9+b1CvsJx47JzJ8g= -github.com/xtgo/uuid v0.0.0-20140804021211-a0b114877d4c/go.mod h1:UrdRz5enIKZ63MEE3IF9l2/ebyx59GyGgPi+tICQdmM= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= +github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= +github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA= +github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= +github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= +github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= +github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= -gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 h1:qIbj1fsPNlZgppZ+VLlY7N33q108Sa+fhmuc+sWQYwY= -gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= -gopkg.in/errgo.v2 v2.1.0/go.mod h1:hNsd1EY+bozCKY1Ytp96fpM3vjJbqLJn88ws8XvfDNI= gopkg.in/go-playground/assert.v1 v1.2.1 h1:xoYuJVE7KT85PYWrN730RguIQO0ePzVRfFMXadIrXTM= gopkg.in/go-playground/assert.v1 v1.2.1/go.mod h1:9RXL0bg/zibRAgZUYszZSwO/z8Y/a8bDuhia5mkpMnE= gopkg.in/go-playground/mold.v2 v2.2.0 h1:Y4IYB4/HYQfuq43zaKh6vs9cVelLE9qbqe2fkyfCTWQ= @@ -38,3 +42,6 @@ gopkg.in/validator.v2 v2.0.0-20180514200540-135c24b11c19 h1:WB265cn5OpO+hK3pikC9 gopkg.in/validator.v2 v2.0.0-20180514200540-135c24b11c19/go.mod h1:o4V0GXN9/CAmCsvJ0oXYZvrZOe7syiDZSN1GWGZTGzc= gopkg.in/yaml.v2 v2.2.1 h1:mUhvW9EsL+naU5Q3cakzfE91YhliOondGd6ZrsDBHQE= gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/status.go b/status.go new file mode 100644 index 0000000..c9f7b83 --- /dev/null +++ b/status.go @@ -0,0 +1,46 @@ +package analytics + +import ( + "strconv" + "strings" +) + +// isSuccess returns true for 2xx and 3xx responses (spec item 1). +func isSuccess(status int) bool { + return status >= 200 && status < 400 +} + +// retryableStatus returns the retry strategy for a given HTTP status code. +// Returns (retryable bool, isRateLimit bool). +func retryableStatus(status int) (retryable bool, isRateLimit bool) { + switch status { + case 429: + return true, true + case 408, 410, 460: + return true, false + case 501, 505, 511: + return false, false + default: + if status >= 500 && status < 600 { + return true, false + } + return false, false + } +} + +// parseRetryAfter parses the Retry-After header value (integer seconds only). +// Returns 0 if the value is absent, invalid, zero, or an HTTP-date. +// Caps the value at cap. +func parseRetryAfter(header string, cap int64) int64 { + if header == "" { + return 0 + } + n, err := strconv.ParseInt(strings.TrimSpace(header), 10, 64) + if err != nil || n <= 0 { + return 0 + } + if n > cap { + return cap + } + return n +} diff --git a/status_test.go b/status_test.go new file mode 100644 index 0000000..4eb32cf --- /dev/null +++ b/status_test.go @@ -0,0 +1,62 @@ +package analytics + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestIsSuccess(t *testing.T) { + cases := []struct { + status int + want bool + }{ + {200, true}, {201, true}, {204, true}, {301, true}, {302, true}, + {400, false}, {429, false}, {500, false}, {0, false}, {199, false}, + } + for _, tc := range cases { + assert.Equal(t, tc.want, isSuccess(tc.status), "status %d", tc.status) + } +} + +func TestRetryableStatus(t *testing.T) { + cases := []struct { + status int + retryable bool + rateLimit bool + }{ + {408, true, false}, + {410, true, false}, + {429, true, true}, + {460, true, false}, + {500, true, false}, + {502, true, false}, + {503, true, false}, + {504, true, false}, + {508, true, false}, + {501, false, false}, + {505, false, false}, + {511, false, false}, + {400, false, false}, + {401, false, false}, + {403, false, false}, + {413, false, false}, + {200, false, false}, + } + for _, tc := range cases { + r, rl := retryableStatus(tc.status) + assert.Equal(t, tc.retryable, r, "retryable for status %d", tc.status) + assert.Equal(t, tc.rateLimit, rl, "isRateLimit for status %d", tc.status) + } +} + +func TestParseRetryAfter(t *testing.T) { + assert.Equal(t, int64(60), parseRetryAfter("60", 300)) + assert.Equal(t, int64(300), parseRetryAfter("9999", 300)) // capped + assert.Equal(t, int64(0), parseRetryAfter("0", 300)) + assert.Equal(t, int64(0), parseRetryAfter("-1", 300)) + assert.Equal(t, int64(0), parseRetryAfter("", 300)) + assert.Equal(t, int64(0), parseRetryAfter("Wed, 07 May 2026 12:00:00 GMT", 300)) + assert.Equal(t, int64(1), parseRetryAfter("1", 300)) + assert.Equal(t, int64(300), parseRetryAfter("300", 300)) +} From bbf12a19ee8346a928711594eb9007707b8325d7 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 21 May 2026 10:57:39 -0400 Subject: [PATCH 02/15] Fix review finding: preserve status code retryability on body-read error When io.ReadAll fails on a non-2xx response, wrap the error in httpError with the original status code's retryability instead of returning a raw error (which would be treated as a network error and always retried). --- analytics.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/analytics.go b/analytics.go index da1c9eb..f2ab845 100644 --- a/analytics.go +++ b/analytics.go @@ -375,7 +375,13 @@ func (c *client) report(res *http.Response) error { body, err := io.ReadAll(res.Body) if err != nil { c.errorf("response %d %s - %s", res.StatusCode, res.Status, err) - return err + retryable, isRL := retryableStatus(res.StatusCode) + return &httpError{ + StatusCode: res.StatusCode, + Retryable: retryable, + IsRateLimit: isRL, + Body: err.Error(), + } } c.logf("response %d %s – %s", res.StatusCode, res.Status, string(body)) From 1f2a42f32dd5dbd1ff6e19665f1792fabd18bdc7 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 4 Jun 2026 20:08:11 -0400 Subject: [PATCH 03/15] Refactor send into small, single-responsibility helpers Extract retry loop body into retryState.classify/handleRateLimit/handleBackoff so that the send function reads as a simple attempt-classify-act switch. The control flow (return, sleep-and-continue) is now visible at a glance. Also fix TestClientResponseBodyError to match the httpError wrapping introduced in bbf12a1. --- analytics.go | 137 +++++++++++++++++++++++++++++----------------- analytics_test.go | 6 +- 2 files changed, 91 insertions(+), 52 deletions(-) diff --git a/analytics.go b/analytics.go index f2ab845..0be4720 100644 --- a/analytics.go +++ b/analytics.go @@ -269,70 +269,107 @@ func (c *client) send(msgs []message) { return } - var ( - totalAttempts int - backoffAttempts int - firstFailureTime time.Time - rateLimitStartTime time.Time - lastErr error - ) - + retry := retryState{client: c, msgs: msgs} for { - totalAttempts++ - uploadErr := c.upload(b, totalAttempts) + retry.totalAttempts++ + uploadErr := c.upload(b, retry.totalAttempts) if uploadErr == nil { c.notifySuccess(msgs) return } - lastErr = uploadErr - - httpErr, ok := uploadErr.(*httpError) - if !ok { - // Network-level error — treat as retryable backoff - httpErr = &httpError{Retryable: true} - } - - if !httpErr.Retryable { - c.errorf("messages dropped due to non-retryable error - %s", uploadErr) - c.notifyFailure(msgs, uploadErr) + action := retry.classify(uploadErr) + switch action { + case retryActionDrop: return + case retryActionRateLimit: + time.Sleep(retry.rateLimitDelay) + case retryActionBackoff: + time.Sleep(c.RetryAfter(retry.backoffAttempts - 1)) } + } +} - if httpErr.IsRateLimit && httpErr.RetryAfter > 0 { - // Retry-After present — sleep without consuming retry budget - if rateLimitStartTime.IsZero() { - rateLimitStartTime = c.now() - } - if c.now().Sub(rateLimitStartTime) > c.MaxRateLimitDuration { - c.errorf("messages dropped - %s", ErrRateLimitBudgetExceeded) - c.notifyFailure(msgs, ErrRateLimitBudgetExceeded) - return - } - time.Sleep(time.Duration(httpErr.RetryAfter) * time.Second) - continue - } +type retryAction int - // Counted backoff retry - if firstFailureTime.IsZero() { - firstFailureTime = c.now() - } - if c.now().Sub(firstFailureTime) > c.MaxTotalBackoffDuration { - c.errorf("messages dropped - %s", ErrBackoffBudgetExceeded) - c.notifyFailure(msgs, ErrBackoffBudgetExceeded) - return - } +const ( + retryActionBackoff retryAction = iota + retryActionRateLimit retryAction = iota + retryActionDrop retryAction = iota +) - backoffAttempts++ - if backoffAttempts > c.MaxRetries { - c.errorf("%d messages dropped after %d attempts", len(msgs), totalAttempts) - c.notifyFailure(msgs, lastErr) - return - } +// retryState tracks state across attempts within a single send call. +type retryState struct { + client *client + msgs []message + totalAttempts int + backoffAttempts int + firstFailureTime time.Time + rateLimitStartTime time.Time + rateLimitDelay time.Duration +} + +// classify determines what to do after a failed upload. It updates internal +// counters, logs/notifies on terminal failures, and returns the action the +// caller should take. +func (r *retryState) classify(uploadErr error) retryAction { + c := r.client + + httpErr, ok := uploadErr.(*httpError) + if !ok { + httpErr = &httpError{Retryable: true} + } + + if !httpErr.Retryable { + c.errorf("messages dropped due to non-retryable error - %s", uploadErr) + c.notifyFailure(r.msgs, uploadErr) + return retryActionDrop + } + + if httpErr.IsRateLimit && httpErr.RetryAfter > 0 { + return r.handleRateLimit(httpErr) + } + + return r.handleBackoff(uploadErr) +} + +func (r *retryState) handleRateLimit(httpErr *httpError) retryAction { + c := r.client + + if r.rateLimitStartTime.IsZero() { + r.rateLimitStartTime = c.now() + } + if c.now().Sub(r.rateLimitStartTime) > c.MaxRateLimitDuration { + c.errorf("messages dropped - %s", ErrRateLimitBudgetExceeded) + c.notifyFailure(r.msgs, ErrRateLimitBudgetExceeded) + return retryActionDrop + } + + r.rateLimitDelay = time.Duration(httpErr.RetryAfter) * time.Second + return retryActionRateLimit +} + +func (r *retryState) handleBackoff(lastErr error) retryAction { + c := r.client - time.Sleep(c.RetryAfter(backoffAttempts - 1)) + if r.firstFailureTime.IsZero() { + r.firstFailureTime = c.now() } + if c.now().Sub(r.firstFailureTime) > c.MaxTotalBackoffDuration { + c.errorf("messages dropped - %s", ErrBackoffBudgetExceeded) + c.notifyFailure(r.msgs, ErrBackoffBudgetExceeded) + return retryActionDrop + } + + r.backoffAttempts++ + if r.backoffAttempts > c.MaxRetries { + c.errorf("%d messages dropped after %d attempts", len(r.msgs), r.totalAttempts) + c.notifyFailure(r.msgs, lastErr) + return retryActionDrop + } + + return retryActionBackoff } // Upload serialized batch message. attempt is 1-based (1 = first attempt). diff --git a/analytics_test.go b/analytics_test.go index 50efa11..a895aff 100644 --- a/analytics_test.go +++ b/analytics_test.go @@ -781,8 +781,10 @@ func TestClientResponseBodyError(t *testing.T) { if err := <-errchan; err == nil { t.Error("failure callback not triggered for a 400 response") - } else if err != testError { - t.Errorf("invalid error returned by erroring response body: %T: %s", err, err) + } else if httpErr, ok := err.(*httpError); !ok { + t.Errorf("expected *httpError, got %T: %s", err, err) + } else if httpErr.StatusCode != 400 { + t.Errorf("expected status 400, got %d", httpErr.StatusCode) } } From c603cb64552b964fff4024a9f37adf5cd93ab33c Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 2 Sep 2026 19:38:14 -0400 Subject: [PATCH 04/15] Handle Retry-After on every retryable status, including 529 Route any retryable response carrying a valid Retry-After header through the rate-limit path (no retry-budget cost) instead of special-casing 429. Retryable statuses without Retry-After continue to use counted exponential backoff. Adds 529 to the retryable set and covers both paths with tests. Matches the behaviour already shipped in analytics-java 3.5.5 and the generic-retry-after conformance suite in sdk-e2e-tests. --- analytics.go | 32 +++++------- analytics_test.go | 128 ++++++++++++++++++++++++++++++++++++++++++++++ status.go | 53 ++++++++++++------- status_test.go | 56 ++++++++++++-------- 4 files changed, 211 insertions(+), 58 deletions(-) diff --git a/analytics.go b/analytics.go index 0be4720..46d72b1 100644 --- a/analytics.go +++ b/analytics.go @@ -243,11 +243,10 @@ func (c *client) sendAsync(msgs []message, wg *sync.WaitGroup, ex *executor) { // httpError is returned by report() for non-2xx/3xx responses. type httpError struct { - StatusCode int - Retryable bool - IsRateLimit bool - RetryAfter int64 // seconds from Retry-After header; 0 if absent - Body string + StatusCode int + Retryable bool + RetryAfter int64 // seconds from Retry-After header; 0 if absent + Body string } func (e *httpError) Error() string { @@ -327,7 +326,7 @@ func (r *retryState) classify(uploadErr error) retryAction { return retryActionDrop } - if httpErr.IsRateLimit && httpErr.RetryAfter > 0 { + if httpErr.RetryAfter > 0 { return r.handleRateLimit(httpErr) } @@ -412,29 +411,26 @@ func (c *client) report(res *http.Response) error { body, err := io.ReadAll(res.Body) if err != nil { c.errorf("response %d %s - %s", res.StatusCode, res.Status, err) - retryable, isRL := retryableStatus(res.StatusCode) return &httpError{ - StatusCode: res.StatusCode, - Retryable: retryable, - IsRateLimit: isRL, - Body: err.Error(), + StatusCode: res.StatusCode, + Retryable: retryableStatus(res.StatusCode), + Body: err.Error(), } } c.logf("response %d %s – %s", res.StatusCode, res.Status, string(body)) - retryable, isRateLimit := retryableStatus(res.StatusCode) + retryable := retryableStatus(res.StatusCode) var retryAfterSecs int64 - if isRateLimit { + if retryable { retryAfterSecs = parseRetryAfter(res.Header.Get("Retry-After"), maxRetryAfterSeconds) } return &httpError{ - StatusCode: res.StatusCode, - Retryable: retryable, - IsRateLimit: isRateLimit, - RetryAfter: retryAfterSecs, - Body: string(body), + StatusCode: res.StatusCode, + Retryable: retryable, + RetryAfter: retryAfterSecs, + Body: string(body), } } diff --git a/analytics_test.go b/analytics_test.go index a895aff..c1dcd0f 100644 --- a/analytics_test.go +++ b/analytics_test.go @@ -820,3 +820,131 @@ func TestClientMaxConcurrentRequests(t *testing.T) { t.Errorf("invalid error returned by erroring response body: %T: %s", err, err) } } + +// makeRetryStateForTest constructs a retryState backed by a minimal client +// with the supplied config defaults filled in. +func makeRetryStateForTest(cfg Config) *retryState { + c := &client{Config: makeConfig(cfg)} + return &retryState{client: c} +} + +// TestRetry503WithRetryAfterUsesRateLimitPath verifies that a 503 response +// with a Retry-After header routes through handleRateLimit (not backoff). +func TestRetry503WithRetryAfterUsesRateLimitPath(t *testing.T) { + r := makeRetryStateForTest(Config{}) + + err := &httpError{ + StatusCode: 503, + Retryable: true, + RetryAfter: 2, + } + + action := r.classify(err) + if action != retryActionRateLimit { + t.Fatalf("expected retryActionRateLimit, got %v", action) + } + if r.rateLimitDelay != 2*time.Second { + t.Errorf("expected rateLimitDelay=2s, got %v", r.rateLimitDelay) + } +} + +// TestRetry529WithRetryAfterUsesRateLimitPath verifies the same for 529. +func TestRetry529WithRetryAfterUsesRateLimitPath(t *testing.T) { + r := makeRetryStateForTest(Config{}) + + err := &httpError{ + StatusCode: 529, + Retryable: true, + RetryAfter: 1, + } + + action := r.classify(err) + if action != retryActionRateLimit { + t.Fatalf("expected retryActionRateLimit, got %v", action) + } + if r.rateLimitDelay != 1*time.Second { + t.Errorf("expected rateLimitDelay=1s, got %v", r.rateLimitDelay) + } +} + +// TestRetry529WithoutRetryAfterUsesExponentialBackoff verifies that a 529 +// without a Retry-After header goes through the backoff path. +func TestRetry529WithoutRetryAfterUsesExponentialBackoff(t *testing.T) { + r := makeRetryStateForTest(Config{}) + + err := &httpError{ + StatusCode: 529, + Retryable: true, + RetryAfter: 0, + } + + action := r.classify(err) + if action != retryActionBackoff { + t.Fatalf("expected retryActionBackoff, got %v", action) + } +} + +// TestRetryAfterOnRetryableStatusUsesRateLimitSleep verifies that a retryable +// status with Retry-After uses the rate-limit path and eventually succeeds. +func TestRetryAfterOnRetryableStatusUsesRateLimitSleep(t *testing.T) { + sleeps := make([]time.Duration, 0, 4) + + // Transport: fail twice with 503+Retry-After:2, then succeed. + attempt := 0 + transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) { + attempt++ + if attempt <= 2 { + hdr := http.Header{} + hdr.Set("Retry-After", "2") + return &http.Response{ + Status: "503 Service Unavailable", + StatusCode: 503, + Proto: r.Proto, + ProtoMajor: r.ProtoMajor, + ProtoMinor: r.ProtoMinor, + Header: hdr, + Body: ioutil.NopCloser(strings.NewReader("overloaded")), + Request: r, + }, nil + } + return &http.Response{ + Status: "200 OK", + StatusCode: 200, + Proto: r.Proto, + ProtoMajor: r.ProtoMajor, + ProtoMinor: r.ProtoMinor, + Body: ioutil.NopCloser(strings.NewReader("")), + Request: r, + }, nil + }) + + reschan := make(chan bool, 1) + + client, _ := NewWithConfig("0123456789", Config{ + Logger: testLogger{t.Logf, t.Logf}, + Callback: testCallback{ + func(m Message) { reschan <- true }, + nil, + }, + Transport: transport, + BatchSize: 1, + // Use a stub sleep tracker via RetryAfter to catch any exponential call. + // The real sleep is in the send loop — we just verify success here. + RetryAfter: func(i int) time.Duration { + d := time.Millisecond // fast for test + sleeps = append(sleeps, d) + return d + }, + }) + + client.Enqueue(Track{UserId: "A", Event: "B"}) + client.Close() + + select { + case <-reschan: + // success — the message eventually went through + case <-time.After(5 * time.Second): + t.Fatal("timed out waiting for success callback") + } + _ = sleeps // collected but not asserted; we just care that it succeeded +} diff --git a/status.go b/status.go index c9f7b83..d85df48 100644 --- a/status.go +++ b/status.go @@ -3,6 +3,7 @@ package analytics import ( "strconv" "strings" + "time" ) // isSuccess returns true for 2xx and 3xx responses (spec item 1). @@ -10,37 +11,53 @@ func isSuccess(status int) bool { return status >= 200 && status < 400 } -// retryableStatus returns the retry strategy for a given HTTP status code. -// Returns (retryable bool, isRateLimit bool). -func retryableStatus(status int) (retryable bool, isRateLimit bool) { +// retryableStatus returns whether the given HTTP status code is retryable. +func retryableStatus(status int) bool { switch status { - case 429: - return true, true - case 408, 410, 460: - return true, false + case 408, 410, 429, 460: + return true case 501, 505, 511: - return false, false + return false default: - if status >= 500 && status < 600 { - return true, false - } - return false, false + return status >= 500 && status < 600 } } -// parseRetryAfter parses the Retry-After header value (integer seconds only). -// Returns 0 if the value is absent, invalid, zero, or an HTTP-date. +// parseRetryAfter parses the Retry-After header value. +// Supports integer seconds and HTTP-date format (RFC 7231 §7.1.1.1). +// Returns 0 if the value is absent, invalid, zero, or in the past. // Caps the value at cap. func parseRetryAfter(header string, cap int64) int64 { if header == "" { return 0 } - n, err := strconv.ParseInt(strings.TrimSpace(header), 10, 64) - if err != nil || n <= 0 { + header = strings.TrimSpace(header) + // Try integer seconds first + n, err := strconv.ParseInt(header, 10, 64) + if err == nil { + if n <= 0 { + return 0 + } + if n > cap { + return cap + } + return n + } + // Try HTTP-date format (RFC 7231 §7.1.1.1) + t, err := time.Parse(time.RFC1123, header) + if err != nil { + // Also try RFC1123Z (with numeric timezone) + t, err = time.Parse(time.RFC1123Z, header) + if err != nil { + return 0 + } + } + seconds := int64(time.Until(t).Seconds()) + if seconds <= 0 { return 0 } - if n > cap { + if seconds > cap { return cap } - return n + return seconds } diff --git a/status_test.go b/status_test.go index 4eb32cf..4420e46 100644 --- a/status_test.go +++ b/status_test.go @@ -2,6 +2,7 @@ package analytics import ( "testing" + "time" "github.com/stretchr/testify/assert" ) @@ -23,30 +24,28 @@ func TestRetryableStatus(t *testing.T) { cases := []struct { status int retryable bool - rateLimit bool }{ - {408, true, false}, - {410, true, false}, - {429, true, true}, - {460, true, false}, - {500, true, false}, - {502, true, false}, - {503, true, false}, - {504, true, false}, - {508, true, false}, - {501, false, false}, - {505, false, false}, - {511, false, false}, - {400, false, false}, - {401, false, false}, - {403, false, false}, - {413, false, false}, - {200, false, false}, + {408, true}, + {410, true}, + {429, true}, + {460, true}, + {500, true}, + {502, true}, + {503, true}, + {504, true}, + {508, true}, + {529, true}, + {501, false}, + {505, false}, + {511, false}, + {400, false}, + {401, false}, + {403, false}, + {413, false}, + {200, false}, } for _, tc := range cases { - r, rl := retryableStatus(tc.status) - assert.Equal(t, tc.retryable, r, "retryable for status %d", tc.status) - assert.Equal(t, tc.rateLimit, rl, "isRateLimit for status %d", tc.status) + assert.Equal(t, tc.retryable, retryableStatus(tc.status), "retryable for status %d", tc.status) } } @@ -56,7 +55,20 @@ func TestParseRetryAfter(t *testing.T) { assert.Equal(t, int64(0), parseRetryAfter("0", 300)) assert.Equal(t, int64(0), parseRetryAfter("-1", 300)) assert.Equal(t, int64(0), parseRetryAfter("", 300)) - assert.Equal(t, int64(0), parseRetryAfter("Wed, 07 May 2026 12:00:00 GMT", 300)) assert.Equal(t, int64(1), parseRetryAfter("1", 300)) assert.Equal(t, int64(300), parseRetryAfter("300", 300)) } + +func TestParseRetryAfterHTTPDate(t *testing.T) { + // Date ~2 seconds in the future should return ~2 + future := time.Now().Add(2 * time.Second).UTC().Format(time.RFC1123) + result := parseRetryAfter(future, 300) + assert.True(t, result >= 1 && result <= 3, "expected ~2 seconds, got %d", result) + + // Date in the past should return 0 + past := time.Now().Add(-10 * time.Second).UTC().Format(time.RFC1123) + assert.Equal(t, int64(0), parseRetryAfter(past, 300)) + + // Garbage string should return 0 + assert.Equal(t, int64(0), parseRetryAfter("not-a-date-or-number", 300)) +} From eddf6e4d88edb551ae0d1aba496ba69710732b38 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 3 Sep 2026 14:51:15 -0400 Subject: [PATCH 05/15] Let Close() interrupt retries again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Splitting send() into helpers replaced the v3.0 select on time.After/c.quit with a bare time.Sleep, so nothing in the retry path observed c.quit any more. Close() waits on loop()'s wg.Wait(), which meant shutdown blocked until every in-flight retry schedule finished: about 4 minutes against a dead endpoint, and up to MaxRateLimitDuration — 12 hours by default — against a server that keeps returning Retry-After. The v3.0 "messages dropped because the client was closed" failure notification had gone with it. The wait is a select on time.After and c.quit again, for both the backoff and rate-limit paths, and notifies failure on close as it used to. This was measurable: TestClientNewRequestError alone took 243.9s on this branch and 0.57s after the fix; the package suite went from 492s to 2.6s. CI runs go test -race, which was at roughly 8m13s against Go's 10m default timeout. TestRetryAfterOnRetryableStatusUsesRateLimitSleep depended on the bug — it called Close() immediately and relied on retries continuing anyway — and asserted nothing about Retry-After: it stubbed Config.RetryAfter, which the rate-limit path never calls, then discarded the result. It now waits for delivery before closing and asserts the backoff function was never called, which is what actually distinguishes the rate-limit path from backoff. Also ran gofmt on analytics.go, which this branch had left unformatted. --- analytics.go | 22 +++++++++++++++++----- analytics_test.go | 45 +++++++++++++++++++++++++++++++-------------- 2 files changed, 48 insertions(+), 19 deletions(-) diff --git a/analytics.go b/analytics.go index 46d72b1..ee1a783 100644 --- a/analytics.go +++ b/analytics.go @@ -279,13 +279,25 @@ func (c *client) send(msgs []message) { } action := retry.classify(uploadErr) + var delay time.Duration switch action { case retryActionDrop: return case retryActionRateLimit: - time.Sleep(retry.rateLimitDelay) + delay = retry.rateLimitDelay case retryActionBackoff: - time.Sleep(c.RetryAfter(retry.backoffAttempts - 1)) + delay = c.RetryAfter(retry.backoffAttempts - 1) + } + + // Wait for either the retry delay or the client to be closed. Without the + // second case Close() cannot interrupt an in-flight retry schedule, so + // shutdown blocks for up to MaxRateLimitDuration (12h by default). + select { + case <-time.After(delay): + case <-c.quit: + c.errorf("%d messages dropped because they failed to be sent and the client was closed", len(msgs)) + c.notifyFailure(msgs, uploadErr) + return } } } @@ -293,9 +305,9 @@ func (c *client) send(msgs []message) { type retryAction int const ( - retryActionBackoff retryAction = iota - retryActionRateLimit retryAction = iota - retryActionDrop retryAction = iota + retryActionBackoff retryAction = iota + retryActionRateLimit retryAction = iota + retryActionDrop retryAction = iota ) // retryState tracks state across attempts within a single send call. diff --git a/analytics_test.go b/analytics_test.go index c1dcd0f..38f602d 100644 --- a/analytics_test.go +++ b/analytics_test.go @@ -13,6 +13,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" "time" ) @@ -887,15 +888,20 @@ func TestRetry529WithoutRetryAfterUsesExponentialBackoff(t *testing.T) { // TestRetryAfterOnRetryableStatusUsesRateLimitSleep verifies that a retryable // status with Retry-After uses the rate-limit path and eventually succeeds. func TestRetryAfterOnRetryableStatusUsesRateLimitSleep(t *testing.T) { - sleeps := make([]time.Duration, 0, 4) - - // Transport: fail twice with 503+Retry-After:2, then succeed. + var mu sync.Mutex + backoffCalls := 0 attempt := 0 + + // Fail twice with 503 + Retry-After: 1, then succeed. transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) { + mu.Lock() attempt++ - if attempt <= 2 { + n := attempt + mu.Unlock() + + if n <= 2 { hdr := http.Header{} - hdr.Set("Retry-After", "2") + hdr.Set("Retry-After", "1") return &http.Response{ Status: "503 Service Unavailable", StatusCode: 503, @@ -928,23 +934,34 @@ func TestRetryAfterOnRetryableStatusUsesRateLimitSleep(t *testing.T) { }, Transport: transport, BatchSize: 1, - // Use a stub sleep tracker via RetryAfter to catch any exponential call. - // The real sleep is in the send loop — we just verify success here. + // Counting call sites proves which path ran: the rate-limit path sleeps + // retry.rateLimitDelay and must never consult the backoff function. RetryAfter: func(i int) time.Duration { - d := time.Millisecond // fast for test - sleeps = append(sleeps, d) - return d + mu.Lock() + backoffCalls++ + mu.Unlock() + return time.Millisecond }, }) client.Enqueue(Track{UserId: "A", Event: "B"}) - client.Close() + // Wait for delivery before closing — Close() now interrupts retries. select { case <-reschan: - // success — the message eventually went through - case <-time.After(5 * time.Second): + case <-time.After(20 * time.Second): t.Fatal("timed out waiting for success callback") } - _ = sleeps // collected but not asserted; we just care that it succeeded + client.Close() + + mu.Lock() + gotBackoff, gotAttempts := backoffCalls, attempt + mu.Unlock() + + if gotBackoff != 0 { + t.Errorf("Retry-After responses must use the rate-limit path, but the backoff function was called %d time(s)", gotBackoff) + } + if gotAttempts != 3 { + t.Errorf("expected 3 upload attempts (2 rate-limited retries then success), got %d", gotAttempts) + } } From 70cd6062967f3a9815b788775df34c35bcc7228c Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 3 Sep 2026 14:53:06 -0400 Subject: [PATCH 06/15] Revert "Let Close() interrupt retries again" This reverts commit eddf6e4d88edb551ae0d1aba496ba69710732b38. --- analytics.go | 22 +++++----------------- analytics_test.go | 45 ++++++++++++++------------------------------- 2 files changed, 19 insertions(+), 48 deletions(-) diff --git a/analytics.go b/analytics.go index ee1a783..46d72b1 100644 --- a/analytics.go +++ b/analytics.go @@ -279,25 +279,13 @@ func (c *client) send(msgs []message) { } action := retry.classify(uploadErr) - var delay time.Duration switch action { case retryActionDrop: return case retryActionRateLimit: - delay = retry.rateLimitDelay + time.Sleep(retry.rateLimitDelay) case retryActionBackoff: - delay = c.RetryAfter(retry.backoffAttempts - 1) - } - - // Wait for either the retry delay or the client to be closed. Without the - // second case Close() cannot interrupt an in-flight retry schedule, so - // shutdown blocks for up to MaxRateLimitDuration (12h by default). - select { - case <-time.After(delay): - case <-c.quit: - c.errorf("%d messages dropped because they failed to be sent and the client was closed", len(msgs)) - c.notifyFailure(msgs, uploadErr) - return + time.Sleep(c.RetryAfter(retry.backoffAttempts - 1)) } } } @@ -305,9 +293,9 @@ func (c *client) send(msgs []message) { type retryAction int const ( - retryActionBackoff retryAction = iota - retryActionRateLimit retryAction = iota - retryActionDrop retryAction = iota + retryActionBackoff retryAction = iota + retryActionRateLimit retryAction = iota + retryActionDrop retryAction = iota ) // retryState tracks state across attempts within a single send call. diff --git a/analytics_test.go b/analytics_test.go index 38f602d..c1dcd0f 100644 --- a/analytics_test.go +++ b/analytics_test.go @@ -13,7 +13,6 @@ import ( "os" "path/filepath" "strings" - "sync" "testing" "time" ) @@ -888,20 +887,15 @@ func TestRetry529WithoutRetryAfterUsesExponentialBackoff(t *testing.T) { // TestRetryAfterOnRetryableStatusUsesRateLimitSleep verifies that a retryable // status with Retry-After uses the rate-limit path and eventually succeeds. func TestRetryAfterOnRetryableStatusUsesRateLimitSleep(t *testing.T) { - var mu sync.Mutex - backoffCalls := 0 - attempt := 0 + sleeps := make([]time.Duration, 0, 4) - // Fail twice with 503 + Retry-After: 1, then succeed. + // Transport: fail twice with 503+Retry-After:2, then succeed. + attempt := 0 transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) { - mu.Lock() attempt++ - n := attempt - mu.Unlock() - - if n <= 2 { + if attempt <= 2 { hdr := http.Header{} - hdr.Set("Retry-After", "1") + hdr.Set("Retry-After", "2") return &http.Response{ Status: "503 Service Unavailable", StatusCode: 503, @@ -934,34 +928,23 @@ func TestRetryAfterOnRetryableStatusUsesRateLimitSleep(t *testing.T) { }, Transport: transport, BatchSize: 1, - // Counting call sites proves which path ran: the rate-limit path sleeps - // retry.rateLimitDelay and must never consult the backoff function. + // Use a stub sleep tracker via RetryAfter to catch any exponential call. + // The real sleep is in the send loop — we just verify success here. RetryAfter: func(i int) time.Duration { - mu.Lock() - backoffCalls++ - mu.Unlock() - return time.Millisecond + d := time.Millisecond // fast for test + sleeps = append(sleeps, d) + return d }, }) client.Enqueue(Track{UserId: "A", Event: "B"}) + client.Close() - // Wait for delivery before closing — Close() now interrupts retries. select { case <-reschan: - case <-time.After(20 * time.Second): + // success — the message eventually went through + case <-time.After(5 * time.Second): t.Fatal("timed out waiting for success callback") } - client.Close() - - mu.Lock() - gotBackoff, gotAttempts := backoffCalls, attempt - mu.Unlock() - - if gotBackoff != 0 { - t.Errorf("Retry-After responses must use the rate-limit path, but the backoff function was called %d time(s)", gotBackoff) - } - if gotAttempts != 3 { - t.Errorf("expected 3 upload attempts (2 rate-limited retries then success), got %d", gotAttempts) - } + _ = sleeps // collected but not asserted; we just care that it succeeded } From b000f80206fa5722a8a609341b5ce403dc333ead Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 3 Sep 2026 16:48:56 -0400 Subject: [PATCH 07/15] Bound how long Close waits for in-flight retries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Splitting send() into helpers replaced v3.0's select on time.After/c.quit with a bare time.Sleep, so nothing in the retry path observed c.quit. Close() waits on loop()'s wg.Wait(), so shutdown blocked until every retry schedule finished: about 4 minutes against a dead endpoint, and up to MaxRateLimitDuration — 12 hours by default — against a server that keeps sending Retry-After. Simply restoring the v3.0 behaviour is wrong now: sdk-e2e-tests drives the CLI by enqueueing, calling Close(), and asserting what the server received, so aborting retries on close fails every retry test. Shutdown has to let in-flight retries finish; it just must not wait forever. analytics-java already solved this — shutdownAndWait() bounds its network executor with NETWORK_TERMINATION_TIMEOUT_S (75s) — so go now has the same thing: Config.ShutdownTimeout, defaulting to 75s to match. On close, retries continue until the deadline, then the batch is dropped with the failure notification v3.0 used to send. Test suite goes from 491s to 4s. TestClientNewRequestError and TestClientRoundTripperError assert only that the failure callback fires, so they set a short ShutdownTimeout rather than sitting through the grace period. Also rewrote TestRetryAfterOnRetryableStatusUsesRateLimitSleep, which asserted nothing about Retry-After: it stubbed Config.RetryAfter, which the rate-limit path never calls, then discarded the result. It now waits for delivery before closing and asserts the backoff function was never called, which is what actually distinguishes the two paths. Added TestCloseIsBoundedByShutdownTimeout, and ran gofmt on analytics.go, which this branch had left unformatted. Unit suite and all 58 e2e tests pass. --- analytics.go | 34 ++++++++++++--- analytics_test.go | 103 ++++++++++++++++++++++++++++++++++++++-------- config.go | 14 +++++++ 3 files changed, 129 insertions(+), 22 deletions(-) diff --git a/analytics.go b/analytics.go index 46d72b1..569f8df 100644 --- a/analytics.go +++ b/analytics.go @@ -269,6 +269,7 @@ func (c *client) send(msgs []message) { } retry := retryState{client: c, msgs: msgs} + var shutdownDeadline time.Time for { retry.totalAttempts++ @@ -279,13 +280,36 @@ func (c *client) send(msgs []message) { } action := retry.classify(uploadErr) + var delay time.Duration switch action { case retryActionDrop: return case retryActionRateLimit: - time.Sleep(retry.rateLimitDelay) + delay = retry.rateLimitDelay case retryActionBackoff: - time.Sleep(c.RetryAfter(retry.backoffAttempts - 1)) + delay = c.RetryAfter(retry.backoffAttempts - 1) + } + + select { + case <-time.After(delay): + case <-c.quit: + // Closing. Keep retrying so a shutdown does not discard a batch the + // server asked us to resend, but bound the wait: without this, a + // server that keeps returning Retry-After holds Close open for up to + // MaxRateLimitDuration (12h by default). + if shutdownDeadline.IsZero() { + shutdownDeadline = time.Now().Add(c.ShutdownTimeout) + } + remaining := time.Until(shutdownDeadline) + if remaining <= 0 { + c.errorf("%d messages dropped because they failed to be sent and the client was closed", len(msgs)) + c.notifyFailure(msgs, uploadErr) + return + } + if delay > remaining { + delay = remaining + } + time.Sleep(delay) } } } @@ -293,9 +317,9 @@ func (c *client) send(msgs []message) { type retryAction int const ( - retryActionBackoff retryAction = iota - retryActionRateLimit retryAction = iota - retryActionDrop retryAction = iota + retryActionBackoff retryAction = iota + retryActionRateLimit retryAction = iota + retryActionDrop retryAction = iota ) // retryState tracks state across attempts within a single send call. diff --git a/analytics_test.go b/analytics_test.go index c1dcd0f..34c2edf 100644 --- a/analytics_test.go +++ b/analytics_test.go @@ -13,6 +13,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" "time" ) @@ -663,8 +664,11 @@ func TestClientNewRequestError(t *testing.T) { errchan := make(chan error, 1) client, _ := NewWithConfig("0123456789", Config{ - Endpoint: "://localhost:80", // Malformed endpoint URL. - Logger: testLogger{t.Logf, t.Logf}, + // These assert only that the failure callback fires; no need to sit + // through the shutdown grace period. + ShutdownTimeout: 100 * time.Millisecond, + Endpoint: "://localhost:80", // Malformed endpoint URL. + Logger: testLogger{t.Logf, t.Logf}, Callback: testCallback{ nil, func(m Message, e error) { errchan <- e }, @@ -684,7 +688,10 @@ func TestClientRoundTripperError(t *testing.T) { errchan := make(chan error, 1) client, _ := NewWithConfig("0123456789", Config{ - Logger: testLogger{t.Logf, t.Logf}, + // These assert only that the failure callback fires; no need to sit + // through the shutdown grace period. + ShutdownTimeout: 100 * time.Millisecond, + Logger: testLogger{t.Logf, t.Logf}, Callback: testCallback{ nil, func(m Message, e error) { errchan <- e }, @@ -887,15 +894,20 @@ func TestRetry529WithoutRetryAfterUsesExponentialBackoff(t *testing.T) { // TestRetryAfterOnRetryableStatusUsesRateLimitSleep verifies that a retryable // status with Retry-After uses the rate-limit path and eventually succeeds. func TestRetryAfterOnRetryableStatusUsesRateLimitSleep(t *testing.T) { - sleeps := make([]time.Duration, 0, 4) - - // Transport: fail twice with 503+Retry-After:2, then succeed. + var mu sync.Mutex + backoffCalls := 0 attempt := 0 + + // Fail twice with 503 + Retry-After: 1, then succeed. transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) { + mu.Lock() attempt++ - if attempt <= 2 { + n := attempt + mu.Unlock() + + if n <= 2 { hdr := http.Header{} - hdr.Set("Retry-After", "2") + hdr.Set("Retry-After", "1") return &http.Response{ Status: "503 Service Unavailable", StatusCode: 503, @@ -928,23 +940,80 @@ func TestRetryAfterOnRetryableStatusUsesRateLimitSleep(t *testing.T) { }, Transport: transport, BatchSize: 1, - // Use a stub sleep tracker via RetryAfter to catch any exponential call. - // The real sleep is in the send loop — we just verify success here. + // Counting call sites proves which path ran: the rate-limit path sleeps + // retry.rateLimitDelay and must never consult the backoff function. RetryAfter: func(i int) time.Duration { - d := time.Millisecond // fast for test - sleeps = append(sleeps, d) - return d + mu.Lock() + backoffCalls++ + mu.Unlock() + return time.Millisecond }, }) client.Enqueue(Track{UserId: "A", Event: "B"}) - client.Close() select { case <-reschan: - // success — the message eventually went through - case <-time.After(5 * time.Second): + case <-time.After(20 * time.Second): t.Fatal("timed out waiting for success callback") } - _ = sleeps // collected but not asserted; we just care that it succeeded + client.Close() + + mu.Lock() + gotBackoff, gotAttempts := backoffCalls, attempt + mu.Unlock() + + if gotBackoff != 0 { + t.Errorf("Retry-After responses must use the rate-limit path, but the backoff function was called %d time(s)", gotBackoff) + } + if gotAttempts != 3 { + t.Errorf("expected 3 upload attempts (2 rate-limited retries then success), got %d", gotAttempts) + } +} + +func TestCloseIsBoundedByShutdownTimeout(t *testing.T) { + // Server never stops rate-limiting, so without a bound Close would wait for + // MaxRateLimitDuration. + transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) { + hdr := http.Header{} + hdr.Set("Retry-After", "5") + return &http.Response{ + Status: "429 Too Many Requests", + StatusCode: 429, + Proto: r.Proto, + ProtoMajor: r.ProtoMajor, + ProtoMinor: r.ProtoMinor, + Header: hdr, + Body: ioutil.NopCloser(strings.NewReader("slow down")), + Request: r, + }, nil + }) + + failures := make(chan bool, 1) + + client, _ := NewWithConfig("0123456789", Config{ + Logger: testLogger{t.Logf, t.Logf}, + Callback: testCallback{nil, func(m Message, e error) { failures <- true }}, + Transport: transport, + BatchSize: 1, + ShutdownTimeout: 1 * time.Second, + MaxRateLimitDuration: 12 * time.Hour, + }) + + client.Enqueue(Track{UserId: "A", Event: "B"}) + time.Sleep(50 * time.Millisecond) // let the first upload start + + start := time.Now() + client.Close() + elapsed := time.Since(start) + + if elapsed > 8*time.Second { + t.Errorf("Close() took %s; ShutdownTimeout of 1s should have bounded it", elapsed) + } + + select { + case <-failures: + case <-time.After(2 * time.Second): + t.Error("expected a failure callback for the batch dropped at shutdown") + } } diff --git a/config.go b/config.go index 29ad7fc..8400976 100644 --- a/config.go +++ b/config.go @@ -70,6 +70,12 @@ type Config struct { // Wall-clock cap on total time spent retrying after 429 Retry-After responses. Defaults to DefaultMaxRateLimitDuration. MaxRateLimitDuration time.Duration + // ShutdownTimeout bounds how long Close will wait for in-flight retries to + // finish before dropping their batches. Without it a client closing while a + // server keeps returning Retry-After blocks for up to MaxRateLimitDuration. + // Mirrors analytics-java's NETWORK_TERMINATION_TIMEOUT_S. + ShutdownTimeout time.Duration + // A function called by the client to generate unique message identifiers. // The client uses a UUID generator if none is provided. // This field is not exported and only exposed internally to let unit tests @@ -95,6 +101,10 @@ const DefaultEndpoint = "https://api.segment.io" // This constant sets the default flush interval used by client instances if // none was explicitly set. +// DefaultShutdownTimeout is how long Close waits for in-flight retries by +// default, matching analytics-java's 75s network-executor termination timeout. +const DefaultShutdownTimeout = 75 * time.Second + const DefaultInterval = 5 * time.Second // This constant sets the default batch size used by client instances if none @@ -142,6 +152,10 @@ func makeConfig(c Config) Config { c.Endpoint = DefaultEndpoint } + if c.ShutdownTimeout == 0 { + c.ShutdownTimeout = DefaultShutdownTimeout + } + if c.Interval == 0 { c.Interval = DefaultInterval } From 1ae5e6b5f16b2be2017cdf914a8d9ff7d7e665f4 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 11 Sep 2026 11:11:11 -0400 Subject: [PATCH 08/15] Tighten retry comments Cut the before/after narration from the comments added with the Retry-After work. The quit branch now says what it does and what bounds it; ShutdownTimeout keeps the analytics-java parity note, which is the part that cannot be reconstructed later. --- analytics.go | 7 +++---- config.go | 11 +++++------ 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/analytics.go b/analytics.go index 569f8df..03a9658 100644 --- a/analytics.go +++ b/analytics.go @@ -293,10 +293,9 @@ func (c *client) send(msgs []message) { select { case <-time.After(delay): case <-c.quit: - // Closing. Keep retrying so a shutdown does not discard a batch the - // server asked us to resend, but bound the wait: without this, a - // server that keeps returning Retry-After holds Close open for up to - // MaxRateLimitDuration (12h by default). + // Closing: finish the retry schedule so shutdown does not discard a + // batch the server asked us to resend, bounded by ShutdownTimeout + // rather than the much longer MaxRateLimitDuration. if shutdownDeadline.IsZero() { shutdownDeadline = time.Now().Add(c.ShutdownTimeout) } diff --git a/config.go b/config.go index 8400976..d29836a 100644 --- a/config.go +++ b/config.go @@ -70,10 +70,9 @@ type Config struct { // Wall-clock cap on total time spent retrying after 429 Retry-After responses. Defaults to DefaultMaxRateLimitDuration. MaxRateLimitDuration time.Duration - // ShutdownTimeout bounds how long Close will wait for in-flight retries to - // finish before dropping their batches. Without it a client closing while a - // server keeps returning Retry-After blocks for up to MaxRateLimitDuration. - // Mirrors analytics-java's NETWORK_TERMINATION_TIMEOUT_S. + // ShutdownTimeout bounds how long Close waits for in-flight retries before + // dropping their batches. Mirrors analytics-java's + // NETWORK_TERMINATION_TIMEOUT_S. ShutdownTimeout time.Duration // A function called by the client to generate unique message identifiers. @@ -101,8 +100,8 @@ const DefaultEndpoint = "https://api.segment.io" // This constant sets the default flush interval used by client instances if // none was explicitly set. -// DefaultShutdownTimeout is how long Close waits for in-flight retries by -// default, matching analytics-java's 75s network-executor termination timeout. +// DefaultShutdownTimeout matches analytics-java's 75s network-executor +// termination timeout. const DefaultShutdownTimeout = 75 * time.Second const DefaultInterval = 5 * time.Second From 1e78afbdded00c49a5016dd9223083883e51968a Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 17 Sep 2026 15:21:59 -0400 Subject: [PATCH 09/15] Opt in to the e2e Authorization check The header assertion in sdk-e2e-tests is opt-in per SDK, since analytics-kotlin and analytics-swift do not send it yet. This SDK does, so it runs the check. --- e2e-cli/e2e-config.json | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/e2e-cli/e2e-config.json b/e2e-cli/e2e-config.json index f926123..9753d0b 100644 --- a/e2e-cli/e2e-config.json +++ b/e2e-cli/e2e-config.json @@ -3,5 +3,7 @@ "test_suites": "basic,retry", "auto_settings": false, "patch": null, - "env": {} + "env": { + "AUTH_HEADER": "true" + } } From 8a7c1d078074ded5b99cf1e26d89f3053fde83a9 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Tue, 22 Sep 2026 12:04:15 -0400 Subject: [PATCH 10/15] Treat only 2xx as a successful upload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every one of these SDKs treated a 3xx as a failure before this work, and the change to 200-399 came from the design doc's "Spec item 1: 2xx and 3xx are success". That line is wrong, and the doc is what needs correcting. Measured against a local server, with the same HTTP clients these SDKs use: 307/308 + Location -> followed as POST with the body, arrives as 200 301/302/303 + Loc. -> followed as GET with no body, arrives as 200 302 without Location-> surfaces raw as 302 300 Multiple Choices-> surfaces raw as 300 304 Not Modified -> surfaces raw as 304 So a raw 3xx only reaches the classifier when the client has already declined to follow it, meaning nothing was uploaded. The one redirect that genuinely works, 307/308, never produces a 3xx here at all — it produces 200 — so narrowing the bound cannot break it. Nothing was gained by the wider range; a 300, 304, or Location-less 302 from a proxy was being logged as a delivered batch and dropped with no error callback. The narrower bound also needs no new branches: a 3xx is neither 5xx nor in the retryable 4xx set, so it already falls through to the non-retryable path and reports a failure. TAPI does not emit 3xx and has no plans to. This matters because host is customer-configurable and proxies in front of it are common. httpError already reports the status, so no extra logging is needed. TestIsSuccess now expects false for 300/301/302/304. --- status.go | 8 ++++++-- status_test.go | 5 ++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/status.go b/status.go index d85df48..7f118c0 100644 --- a/status.go +++ b/status.go @@ -6,9 +6,13 @@ import ( "time" ) -// isSuccess returns true for 2xx and 3xx responses (spec item 1). +// isSuccess reports whether the upload was accepted. Only 2xx counts: net/http +// follows any redirect it can, so a 3xx reaching us means it declined to (no +// Location, a 300, or a 304) and nothing was uploaded. Treating those as success +// would drop the batch silently. The TAPI endpoint does not emit 3xx at all; +// this matters when host points at a customer's proxy or redirector. func isSuccess(status int) bool { - return status >= 200 && status < 400 + return status >= 200 && status < 300 } // retryableStatus returns whether the given HTTP status code is retryable. diff --git a/status_test.go b/status_test.go index 4420e46..a96a4ed 100644 --- a/status_test.go +++ b/status_test.go @@ -12,7 +12,10 @@ func TestIsSuccess(t *testing.T) { status int want bool }{ - {200, true}, {201, true}, {204, true}, {301, true}, {302, true}, + {200, true}, {201, true}, {204, true}, + // Only 2xx is success: a 3xx means net/http declined to follow it, so + // nothing was uploaded. + {300, false}, {301, false}, {302, false}, {304, false}, {400, false}, {429, false}, {500, false}, {0, false}, {199, false}, } for _, tc := range cases { From 132d31395da1813a16c1d87e78d3b4f514c30744 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Tue, 22 Sep 2026 18:57:10 -0400 Subject: [PATCH 11/15] Add release notes for the HTTP response and retry work Records the retry/Retry-After work and, for the SDKs where a header is newly on the wire, an upgrade note: customers whose proxies allowlist request headers had uploads rejected by the already-released analytics-next change, and the same trap applies here. --- History.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/History.md b/History.md index 4d86749..bc6ae82 100644 --- a/History.md +++ b/History.md @@ -1,3 +1,22 @@ +Unreleased +========== + +### Upgrade note: new request header and proxy allowlists + +This release sends an `X-Retry-Count` request header on retries. If your +traffic to Segment goes through a proxy, gateway or WAF that allowlists +request headers, add it before upgrading or retried uploads will be +rejected. The `Authorization` header is unchanged: this client has always +sent the write key as HTTP Basic credentials. + +* Send `X-Retry-Count` on retries, so the server can distinguish a retry from a first attempt. Omitted on the first attempt. +* Unified retry handling: 429, 408, 410, 460 and 5xx (except 501, 505 and 511) are retried. `Retry-After` is honoured on all of them, not just 429, which brings 529 in through the generic 5xx rule. +* `Retry-After` accepts numeric seconds and the RFC 7231 HTTP-date formats, capped at 300s. +* Rate-limited retries are bounded by elapsed time rather than counted against the retry limit, so a long `Retry-After` no longer exhausts the budget. +* New `Config.MaxTotalBackoffDuration` and `Config.MaxRateLimitDuration` (default 12 hours each) bound the two waits, reported as `ErrBackoffBudgetExceeded` and `ErrRateLimitBudgetExceeded`. +* New `Config.ShutdownTimeout` (default 75s) bounds how long `Close` waits for in-flight retries, so shutdown neither discards a batch the server asked us to resend nor blocks for the full rate-limit budget. +* Only 2xx responses count as a successful upload. A 3xx is now logged and retried rather than silently treated as delivered; the Segment endpoint does not redirect, so this only affects custom `Endpoint` values. + v3.3.0 / 2023-10-31 =================== From 27c92caec4f9d4105d9f2adcc0685eab71d80c05 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 07:06:19 -0400 Subject: [PATCH 12/15] Correct the release notes on 3xx handling No SDK retries a 3xx: every one classifies it as non-retryable and reports a failed upload. The notes claimed it was retried, which is wrong, and would have sent anyone debugging a proxy redirect looking for retries that never happen. Also scopes python's 511 line to the OAuth case, which is the one place the spec does allow a 511 retry, and php's new budget options to the LibCurl consumer, since Socket ignores them. --- History.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/History.md b/History.md index bc6ae82..25fc62a 100644 --- a/History.md +++ b/History.md @@ -15,7 +15,7 @@ sent the write key as HTTP Basic credentials. * Rate-limited retries are bounded by elapsed time rather than counted against the retry limit, so a long `Retry-After` no longer exhausts the budget. * New `Config.MaxTotalBackoffDuration` and `Config.MaxRateLimitDuration` (default 12 hours each) bound the two waits, reported as `ErrBackoffBudgetExceeded` and `ErrRateLimitBudgetExceeded`. * New `Config.ShutdownTimeout` (default 75s) bounds how long `Close` waits for in-flight retries, so shutdown neither discards a batch the server asked us to resend nor blocks for the full rate-limit budget. -* Only 2xx responses count as a successful upload. A 3xx is now logged and retried rather than silently treated as delivered; the Segment endpoint does not redirect, so this only affects custom `Endpoint` values. +* Only 2xx responses count as a successful upload. A 3xx is now reported as a failed upload rather than silently treated as delivered. It is not retried: a redirect `net/http` already declined to follow will not succeed on a retry. The Segment endpoint does not redirect, so this only affects custom `Endpoint` values. v3.3.0 / 2023-10-31 =================== From 9b1863815937b5b0386bd3ca1d2e2d2af5097f47 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 08:55:21 -0400 Subject: [PATCH 13/15] Jitter the counted backoff so clients do not retry in lockstep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The default backoff this branch introduced passed 0 as backo's jitter argument, so every client backing off from the same incident retried at exactly 500ms, 1s, 2s, 4s together. That is a thundering herd aimed at the endpoint this initiative exists to protect. Raising backo's jitter would not have fixed it: backo jitters before applying its cap, so once the exponential passes the ceiling the jittered value is clamped to exactly the cap and the fleet is back in step. This clamps first and then subtracts up to 50%, so the ceiling stays a hard ceiling and the spread survives at the point it matters most. Same shape as the ruby fix on status-response-update. The jitter draws from its own seeded source because math/rand's global source is seeded deterministically before Go 1.20 — every process would otherwise draw an identical sequence and stay in step regardless. backo-go was its only remaining use, so it and its test-only transitive dependencies drop out of go.mod. Also removes a comment claiming "2xx and 3xx are success" from above the 2xx-only check. status.go documents the actual rule, including why a 3xx reaching us means nothing was uploaded. --- analytics.go | 1 - config.go | 43 +++++++++++++++++++++++++++++++++++++++++-- config_test.go | 31 +++++++++++++++++++++++++++++++ go.mod | 3 --- go.sum | 12 ------------ 5 files changed, 72 insertions(+), 18 deletions(-) diff --git a/analytics.go b/analytics.go index 03a9658..44a9b75 100644 --- a/analytics.go +++ b/analytics.go @@ -425,7 +425,6 @@ func (c *client) upload(b []byte, attempt int) error { // Report on response body. func (c *client) report(res *http.Response) error { - // Spec item 1: 2xx and 3xx are success if isSuccess(res.StatusCode) { c.debugf("response %s", res.Status) return nil diff --git a/config.go b/config.go index d29836a..9c26dd7 100644 --- a/config.go +++ b/config.go @@ -1,11 +1,13 @@ package analytics import ( + "math" + "math/rand" "net/http" + "sync" "time" "github.com/google/uuid" - "github.com/segmentio/backo-go" ) // Instances of this type carry the different configuration options that may @@ -176,7 +178,7 @@ func makeConfig(c Config) Config { } if c.RetryAfter == nil { - c.RetryAfter = backo.NewBacko(500*time.Millisecond, 2, 0, 60*time.Second).Duration + c.RetryAfter = defaultRetryAfter } if c.MaxRetries == 0 { @@ -217,3 +219,40 @@ func makeConfig(c Config) Config { func uid() string { return uuid.NewString() } + +// Jitter needs its own source: math/rand's global source is seeded +// deterministically before Go 1.20, so every process would draw the same +// sequence and stay in step with every other process anyway. +var ( + jitterMu sync.Mutex + jitterRand = rand.New(rand.NewSource(time.Now().UnixNano())) +) + +func jitterFraction() float64 { + jitterMu.Lock() + defer jitterMu.Unlock() + return jitterRand.Float64() +} + +// defaultRetryAfter returns how long to wait before counted backoff attempt n: +// 500ms doubling to a 60s ceiling, then reduced by up to 50% at random. +// +// The jitter is applied after the clamp and only ever subtracts, so the ceiling +// holds and clients that started backing off together spread out instead of +// retrying in lockstep. Applying it before the clamp — which is what +// backo-go does, and why it is no longer used here — collapses back to exactly +// the cap once the exponential passes it, putting the whole fleet back in step +// at the point the endpoint can least afford it. +func defaultRetryAfter(attempt int) time.Duration { + const ( + base = float64(500 * time.Millisecond) + ceiling = float64(60 * time.Second) + jitter = 0.5 + ) + + delay := base * math.Pow(2, float64(attempt)) + if delay > ceiling { + delay = ceiling + } + return time.Duration(delay - jitterFraction()*delay*jitter) +} diff --git a/config_test.go b/config_test.go index 4c4cb52..902efc5 100644 --- a/config_test.go +++ b/config_test.go @@ -44,3 +44,34 @@ func TestConfigInvalidBatchSize(t *testing.T) { t.Error("invalid field error reported:", e) } } + +func TestDefaultRetryAfterNeverExceedsTheCeiling(t *testing.T) { + for attempt := 0; attempt < 30; attempt++ { + if d := defaultRetryAfter(attempt); d > 60*time.Second { + t.Fatalf("attempt %d returned %s, above the 60s ceiling", attempt, d) + } + } +} + +func TestDefaultRetryAfterJittersAtTheCeiling(t *testing.T) { + // backo-go jittered before clamping, so every attempt past the ceiling + // returned exactly the cap and a fleet stayed in lockstep. Guard against + // regressing to that. + seen := make(map[time.Duration]struct{}) + var min time.Duration = 60 * time.Second + + for i := 0; i < 50; i++ { + d := defaultRetryAfter(20) // well past the ceiling + seen[d] = struct{}{} + if d < min { + min = d + } + } + + if len(seen) < 2 { + t.Errorf("expected jittered values at the ceiling, got the same value %d times", len(seen)) + } + if min < 30*time.Second { + t.Errorf("jitter should subtract at most 50%%, but saw %s", min) + } +} diff --git a/go.mod b/go.mod index c7ec864..26bfec7 100644 --- a/go.mod +++ b/go.mod @@ -4,15 +4,12 @@ go 1.17 require ( github.com/google/uuid v1.3.0 - github.com/segmentio/backo-go v1.0.0 github.com/segmentio/conf v1.2.0 github.com/stretchr/testify v1.11.1 ) require ( - github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869 // indirect github.com/davecgh/go-spew v1.1.1 // indirect - github.com/kr/pretty v0.3.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/segmentio/go-snakecase v1.1.0 // indirect github.com/segmentio/objconv v1.0.1 // indirect diff --git a/go.sum b/go.sum index 23cfd3f..5a8201c 100644 --- a/go.sum +++ b/go.sum @@ -1,22 +1,10 @@ -github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869 h1:DDGfHa7BWjL4YnC6+E63dPcxHo2sUxDIu8g3QgEJdRY= -github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869/go.mod h1:Ekp36dRnpXw/yCqJaO+ZrUyxD+3VXMFFr56k5XYrpB4= -github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/google/uuid v1.3.0 h1:t6JiXgmwXMjEs8VusXIJk2BXHsn+wx8BZdTaoZ5fu7I= github.com/google/uuid v1.3.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= -github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= -github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= -github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= -github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= -github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= -github.com/rogpeppe/go-internal v1.9.0 h1:73kH8U+JUqXU8lRuOHeVHaa/SZPifC7BkcraZVejAe8= -github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs= -github.com/segmentio/backo-go v1.0.0 h1:kbOAtGJY2DqOR0jfRkYEorx/b18RgtepGtY3+Cpe6qA= -github.com/segmentio/backo-go v1.0.0/go.mod h1:kJ9mm9YmoWSkk+oQ+5Cj8DEoRCX2JT6As4kEtIIOp1M= github.com/segmentio/conf v1.2.0 h1:5OT9+6OyVHLsFLsiJa/2KlqiA1m7mpdUBlkB/qYTMts= github.com/segmentio/conf v1.2.0/go.mod h1:Y3B9O/PqqWqjyxyWWseyj/quPEtMu1zDp/kVbSWWaB0= github.com/segmentio/go-snakecase v1.1.0 h1:ZJO4SNKKV0MjGOv0LHnixxN5FYv1JKBnVXEuBpwcbQI= From 9deac3afd03373aa161fd7b0b495de3aa0f63a47 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 11:11:46 -0400 Subject: [PATCH 14/15] Make ShutdownTimeout a real bound, reject negative retry config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four small fixes from review. ShutdownTimeout did not bound what it claimed. After the clamped sleep the loop issued another upload with nothing tying that request to the remaining budget, so Close could overrun by a whole HTTP round trip — bounded only by the unrelated 10s client timeout. The final attempt now carries the shutdown deadline as a request context deadline. The existing test asserted `elapsed <= 8s` against a 1s timeout, a tolerance that only made sense because the bound was soft; it is now 3s and passes in about 1s. Retry-After is parsed before the response body is read. A mid-read I/O error previously discarded it, routing that attempt onto the counted backoff budget instead of the rate-limit one. ErrRetriesExhausted is deleted. It was introduced by this work, documented as returned when retries are consumed, and never returned by anything — a callback checking for it could not have fired. Negative MaxRetries, MaxTotalBackoffDuration, MaxRateLimitDuration and ShutdownTimeout are rejected in validate(), alongside the existing Interval and BatchSize checks. A negative MaxRetries previously survived into the retry loop and dropped every batch after its first failure. On MaxRetries: 0 cannot mean "no retries" here. Config documents that every field's zero value is either meaningful or means "use the default", and every other field follows the latter, so making this one field different would give a caller who never touched it zero retries. Zero keeps meaning the default, which is now stated on the field, and the silent-negative case that was the actual bug is an error instead. Full suite, vet and the 61-test e2e suite pass. --- History.md | 4 +++- analytics.go | 32 +++++++++++++++++++++++--------- analytics_test.go | 6 +++++- config.go | 44 +++++++++++++++++++++++++++++++++++++++++--- config_test.go | 35 +++++++++++++++++++++++++++++++++++ error.go | 3 --- 6 files changed, 107 insertions(+), 17 deletions(-) diff --git a/History.md b/History.md index 25fc62a..677c15b 100644 --- a/History.md +++ b/History.md @@ -14,7 +14,9 @@ sent the write key as HTTP Basic credentials. * `Retry-After` accepts numeric seconds and the RFC 7231 HTTP-date formats, capped at 300s. * Rate-limited retries are bounded by elapsed time rather than counted against the retry limit, so a long `Retry-After` no longer exhausts the budget. * New `Config.MaxTotalBackoffDuration` and `Config.MaxRateLimitDuration` (default 12 hours each) bound the two waits, reported as `ErrBackoffBudgetExceeded` and `ErrRateLimitBudgetExceeded`. -* New `Config.ShutdownTimeout` (default 75s) bounds how long `Close` waits for in-flight retries, so shutdown neither discards a batch the server asked us to resend nor blocks for the full rate-limit budget. +* New `Config.ShutdownTimeout` (default 75s) bounds how long `Close` waits for in-flight retries, so shutdown neither discards a batch the server asked us to resend nor blocks for the full rate-limit budget. The final attempt carries it as a request deadline, so the bound covers the in-flight request too. +* Negative `MaxRetries`, `MaxTotalBackoffDuration`, `MaxRateLimitDuration` and `ShutdownTimeout` are rejected at construction. A negative retry count previously dropped every batch after its first failure. Zero still means "use the default", per the zero-value convention on `Config`. +* `Retry-After` is read before the response body, so a mid-read I/O error no longer loses it and push the attempt onto the counted-backoff budget. * Only 2xx responses count as a successful upload. A 3xx is now reported as a failed upload rather than silently treated as delivered. It is not retried: a redirect `net/http` already declined to follow will not succeed on a retry. The Segment endpoint does not redirect, so this only affects custom `Endpoint` values. v3.3.0 / 2023-10-31 diff --git a/analytics.go b/analytics.go index 44a9b75..d20bcd4 100644 --- a/analytics.go +++ b/analytics.go @@ -2,6 +2,7 @@ package analytics import ( "bytes" + "context" "encoding/json" "fmt" "io" @@ -273,7 +274,7 @@ func (c *client) send(msgs []message) { for { retry.totalAttempts++ - uploadErr := c.upload(b, retry.totalAttempts) + uploadErr := c.upload(b, retry.totalAttempts, shutdownDeadline) if uploadErr == nil { c.notifySuccess(msgs) return @@ -395,7 +396,10 @@ func (r *retryState) handleBackoff(lastErr error) retryAction { } // Upload serialized batch message. attempt is 1-based (1 = first attempt). -func (c *client) upload(b []byte, attempt int) error { +// upload sends one attempt. A non-zero deadline bounds the request itself, so +// Close cannot overrun ShutdownTimeout by a whole HTTP round trip while waiting +// on a final attempt. +func (c *client) upload(b []byte, attempt int, deadline time.Time) error { url := c.Endpoint + "/v1/batch" req, err := http.NewRequest("POST", url, bytes.NewReader(b)) if err != nil { @@ -413,6 +417,12 @@ func (c *client) upload(b []byte, attempt int) error { req.Header.Add("X-Retry-Count", strconv.Itoa(attempt-1)) } + if !deadline.IsZero() { + ctx, cancel := context.WithDeadline(req.Context(), deadline) + defer cancel() + req = req.WithContext(ctx) + } + res, err := c.http.Do(req) if err != nil { c.errorf("sending request - %s", err) @@ -430,24 +440,28 @@ func (c *client) report(res *http.Response) error { return nil } + // Read before the body: a mid-read I/O error must not lose the server's + // Retry-After and route this attempt onto the counted-backoff budget instead + // of the rate-limit one. + retryable := retryableStatus(res.StatusCode) + var retryAfterSecs int64 + if retryable { + retryAfterSecs = parseRetryAfter(res.Header.Get("Retry-After"), maxRetryAfterSeconds) + } + body, err := io.ReadAll(res.Body) if err != nil { c.errorf("response %d %s - %s", res.StatusCode, res.Status, err) return &httpError{ StatusCode: res.StatusCode, - Retryable: retryableStatus(res.StatusCode), + Retryable: retryable, + RetryAfter: retryAfterSecs, Body: err.Error(), } } c.logf("response %d %s – %s", res.StatusCode, res.Status, string(body)) - retryable := retryableStatus(res.StatusCode) - var retryAfterSecs int64 - if retryable { - retryAfterSecs = parseRetryAfter(res.Header.Get("Retry-After"), maxRetryAfterSeconds) - } - return &httpError{ StatusCode: res.StatusCode, Retryable: retryable, diff --git a/analytics_test.go b/analytics_test.go index 34c2edf..fb9f300 100644 --- a/analytics_test.go +++ b/analytics_test.go @@ -1007,7 +1007,11 @@ func TestCloseIsBoundedByShutdownTimeout(t *testing.T) { client.Close() elapsed := time.Since(start) - if elapsed > 8*time.Second { + // The tolerance used to be 8s because the bound was not real: after the + // clamped sleep the loop issued another upload with nothing tying that request + // to the remaining budget, so Close could overrun by a whole HTTP round trip. + // The final attempt now carries the deadline, so this can be tight. + if elapsed > 3*time.Second { t.Errorf("Close() took %s; ShutdownTimeout of 1s should have bounded it", elapsed) } diff --git a/config.go b/config.go index 9c26dd7..2c955e0 100644 --- a/config.go +++ b/config.go @@ -63,7 +63,9 @@ type Config struct { // If not set the client will fallback to use a default retry policy. RetryAfter func(int) time.Duration - // Maximum number of counted backoff retries. Defaults to DefaultMaxRetries. + // Maximum number of counted backoff retries. Zero means use + // DefaultMaxRetries, per the zero-value convention above; there is no way to + // ask for no retries at all. Negative values are rejected. MaxRetries int // Wall-clock cap on total time spent in backoff retries. Defaults to DefaultMaxTotalBackoffDuration. @@ -73,8 +75,9 @@ type Config struct { MaxRateLimitDuration time.Duration // ShutdownTimeout bounds how long Close waits for in-flight retries before - // dropping their batches. Mirrors analytics-java's - // NETWORK_TERMINATION_TIMEOUT_S. + // dropping their batches. It covers the whole remaining retry schedule, + // including the final request, which is issued with this as its deadline. + // Mirrors analytics-java's NETWORK_TERMINATION_TIMEOUT_S. ShutdownTimeout time.Duration // A function called by the client to generate unique message identifiers. @@ -143,6 +146,41 @@ func (c *Config) validate() error { } } + // Zero means "use the default" for these, per the zero-value convention above. + // Negatives used to survive into the retry loop, where they dropped every batch + // after its first failure instead of failing here. + if c.MaxRetries < 0 { + return ConfigError{ + Reason: "negative retry counts are not supported", + Field: "MaxRetries", + Value: c.MaxRetries, + } + } + + if c.MaxTotalBackoffDuration < 0 { + return ConfigError{ + Reason: "negative backoff durations are not supported", + Field: "MaxTotalBackoffDuration", + Value: c.MaxTotalBackoffDuration, + } + } + + if c.MaxRateLimitDuration < 0 { + return ConfigError{ + Reason: "negative rate limit durations are not supported", + Field: "MaxRateLimitDuration", + Value: c.MaxRateLimitDuration, + } + } + + if c.ShutdownTimeout < 0 { + return ConfigError{ + Reason: "negative shutdown timeouts are not supported", + Field: "ShutdownTimeout", + Value: c.ShutdownTimeout, + } + } + return nil } diff --git a/config_test.go b/config_test.go index 902efc5..2c1a317 100644 --- a/config_test.go +++ b/config_test.go @@ -75,3 +75,38 @@ func TestDefaultRetryAfterJittersAtTheCeiling(t *testing.T) { t.Errorf("jitter should subtract at most 50%%, but saw %s", min) } } + +func TestConfigRejectsNegativeRetryFields(t *testing.T) { + // These used to survive validate() and reach the retry loop, where a negative + // MaxRetries dropped every batch after its first failure instead of erroring here. + for _, test := range []struct { + field string + config Config + }{ + {"MaxRetries", Config{MaxRetries: -1}}, + {"MaxTotalBackoffDuration", Config{MaxTotalBackoffDuration: -1 * time.Second}}, + {"MaxRateLimitDuration", Config{MaxRateLimitDuration: -1 * time.Second}}, + {"ShutdownTimeout", Config{ShutdownTimeout: -1 * time.Second}}, + } { + if err := test.config.validate(); err == nil { + t.Errorf("negative %s should be rejected", test.field) + } + } +} + +func TestConfigZeroRetryFieldsTakeDefaults(t *testing.T) { + // Zero means "use the default" for these, per Config's zero-value convention. + // There is deliberately no way to ask for no retries at all. + c := Config{} + if err := c.validate(); err != nil { + t.Fatalf("zero values should be valid: %s", err) + } + + c = makeConfig(c) + if c.MaxRetries != DefaultMaxRetries { + t.Errorf("MaxRetries = %d, want the default %d", c.MaxRetries, DefaultMaxRetries) + } + if c.ShutdownTimeout != DefaultShutdownTimeout { + t.Errorf("ShutdownTimeout = %s, want the default %s", c.ShutdownTimeout, DefaultShutdownTimeout) + } +} diff --git a/error.go b/error.go index 218a4ee..ead7abd 100644 --- a/error.go +++ b/error.go @@ -65,7 +65,4 @@ var ( // ErrRateLimitBudgetExceeded is returned when the maximum rate-limit // (429 Retry-After) duration is exceeded. ErrRateLimitBudgetExceeded = errors.New("max rate limit duration exceeded") - - // ErrRetriesExhausted is returned when all retry attempts are consumed. - ErrRetriesExhausted = errors.New("retries exhausted") ) From 025a9f28c09fc368a2f9187a8e69aec2aba88e97 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 12:17:59 -0400 Subject: [PATCH 15/15] Stop the retry timer on shutdown, drop an unresolvable spec reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit time.After leaves its timer live until it fires, and the c.quit branch of this select can win instead. The loop runs for as long as the retry budget allows — up to 12h by default — with delays up to the 300s Retry-After cap, so abandoning timers there is not free. Replaced with an explicit time.NewTimer plus Stop. Also replaces "// Spec item 7: omit on first attempt, send 1-based count on retries" with what it actually means. Citing the spec is the right instinct and our own comment convention asks for it, but a bare item index is only a citation if the reader can reach the document — and there is no link to that doc in any repo, commit message or note. The behaviour it describes is worth a line; the index number is not. Full suite, vet, -race on the retry tests, and the 61-test e2e suite pass. --- analytics.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/analytics.go b/analytics.go index d20bcd4..8306b8b 100644 --- a/analytics.go +++ b/analytics.go @@ -291,9 +291,14 @@ func (c *client) send(msgs []message) { delay = c.RetryAfter(retry.backoffAttempts - 1) } + timer := time.NewTimer(delay) select { - case <-time.After(delay): + case <-timer.C: case <-c.quit: + // Stopped explicitly: time.After would leave the timer live until it + // fired, and this loop can run for hours with delays up to the + // Retry-After cap. + timer.Stop() // Closing: finish the retry schedule so shutdown does not discard a // batch the server asked us to resend, bounded by ShutdownTimeout // rather than the much longer MaxRateLimitDuration. @@ -412,7 +417,7 @@ func (c *client) upload(b []byte, attempt int, deadline time.Time) error { req.Header.Add("Content-Length", strconv.Itoa(len(b))) req.SetBasicAuth(c.key, "") - // Spec item 7: omit on first attempt, send 1-based count on retries + // Omitted on the first attempt so the server can tell a retry from a first try. if attempt > 1 { req.Header.Add("X-Retry-Count", strconv.Itoa(attempt-1)) }