Dual-path retry: exponential backoff + rate-limit handling - #206
Open
MichaelGHSeg wants to merge 7 commits into
Open
Dual-path retry: exponential backoff + rate-limit handling#206MichaelGHSeg wants to merge 7 commits into
MichaelGHSeg wants to merge 7 commits into
Conversation
…dling 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
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).
abueide
reviewed
Jun 3, 2026
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.
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.
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.
This reverts commit eddf6e4.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the fixed 10-attempt retry loop with a structured dual-path retry system that separates rate-limiting (429 + Retry-After) from general transient failures.
MaxRateLimitDuration(default 12h).MaxRetries(default 10) andMaxTotalBackoffDuration(default 12h).select { case <-c.quit }from retry sleep paths —Close()now waits for in-flight retries to finish naturally viawg.Wait()instead of killing them mid-flight.X-Retry-Countheader on retry attempts (omitted on first attempt).httpErrorpreserving the original status code's retryability (fixes a bug where a non-retryable 4xx with a broken response body would be incorrectly retried).MaxRetries,MaxTotalBackoffDuration,MaxRateLimitDuration.ErrBackoffBudgetExceeded,ErrRateLimitBudgetExceeded.retrytest suite; wiresmaxRetriesfrom e2e-cli input config.Test plan
go test -race ./...passesbasic,retrysuites pass (48/48)