Dual-path retry: exponential backoff + rate-limit handling - #280
Open
MichaelGHSeg wants to merge 12 commits into
Open
MichaelGHSeg wants to merge 12 commits into
MichaelGHSeg wants to merge 12 commits into
Conversation
- Transport: dual-path retry loop (429+Retry-After vs counted exponential backoff), X-Retry-Count header on retries, retryable status classification (5xx except 501/505/511; 4xx only 408/410/429/460) - BackoffPolicy: update constants (base 500ms, cap 60s, multiplier 2); add reset! method - Response: add success? method (2xx+3xx) - Defaults: add MAX_TOTAL_BACKOFF_DURATION, MAX_RATE_LIMIT_DURATION, RATE_LIMIT_RETRY_AFTER_CAP constants - Worker: use response.success? instead of status == 200 - Tests: cover new retry paths, X-Retry-Count, parse_retry_after, retryable/non-retryable status codes
…pdate - Add reset! to FakeBackoffPolicy so transport specs don't crash - Narrow success? and success_status? to 2xx only (Net::HTTP doesn't follow redirects, so 3xx would silently lose batches) - Update malformed-JSON-on-200 test to match new semantics: a 200 means the server accepted the batch regardless of body parseability
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.
The suite was red on this branch: 199 examples, 2 failures. Nothing caught it because analytics-ruby's PR runs no unit-test job, only Wiz, semgrep and opa. Four fixes. 1. Spec item 1 says 2xx and 3xx are success, and response_spec asserted exactly that while Response#success? and success_status? were 2xx-only — so 3xx also fell through to logger.error and on_error, which master did not do. Both now accept 3xx, and the contradictory #success_status? assertion (301 => false) is corrected. That resolves both failures. 2. Transient network errors were no longer retried. The rescue moved from inside the retry helper, where master set should_retry, to a method-level rescue on send, so one ECONNRESET, DNS blip or read timeout unwound the whole loop and dropped the batch on first occurrence. The spec covering this was deleted in the same change. send_request is wrapped again and network failures go through the counted backoff budget; two specs cover retry and eventual give-up. The counted-backoff branch was extracted into a lambda so both callers share one budget implementation. 3. backoff_policy.reset! was called unconditionally, but reset! is new here and backoff_policy is a documented public option. A user duck type that worked on master raised NoMethodError, which the method-level rescue turned into an ordinary Response(-1): every batch failed, no request ever sent. Now guarded with respond_to?. 4. Retry-After and backoff waits blocked shutdown. Ruby's worker is a single thread and the only consumer of the queue, so a sleep of up to rate_limit_retry_after_cap (300s) stalls the whole client, and Thread.current[:should_exit] is only checked at the top of the run loop. at_exit now wakes the worker, which returns the sleep early, and both wait sites check for shutdown immediately afterwards. 202 examples, 0 failures, and all 58 e2e tests pass.
Cut the before/after narration from the comments added with the Retry-After work. The network-error branch now says that those failures use the counted backoff budget rather than recounting how the rescue used to be placed.
rubocop runs as part of rake's default task, over lib/ and spec/, and this branch had pushed Transport#send to 66 lines with an ABC size of 75.77 against a limit of 25 — eleven new offences, so CI would have gone red. The bulk of it was the counted-backoff budget sitting inline as a lambda. That, and the rate-limit budget beside it, are now a RetryBudget class of their own. It reports the delay to wait and nil once a budget is spent; Transport still performs the sleep, which keeps the seam the specs stub. send is down to 21 lines and ABC 32.39, and its cyclomatic and perceived-complexity offences are gone. Option parsing moved out of initialize the same way, clearing all four of its offences, and the response classification and delay choice are now named methods rather than inline branches. Three Max values in .rubocop_todo.yml are raised for what is left, which is a class 146 lines long and two methods a handful of lines over. Regenerating the file wholesale was the alternative and a bad one: it was last generated by rubocop 1.44 in 2023, and 1.90 rewrites the entire baseline. RetryBudget takes an options hash rather than keyword arguments: the gemspec declares required_ruby_version >= 2.0 and rubocop parses as 2.0, where required keyword arguments are a syntax error. rubocop reports no offences over lib/ and spec/. 202 examples, 0 failures, and all 58 e2e tests pass. Line coverage 97.92% -> 98.19%.
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.
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. Net::HTTP never follows redirects, so ruby is the most likely to see a raw 3xx, and logging the body was useless for one. It now logs the status and points at the host. Specs and the misnamed "3xx is treated as success" context corrected. ClassLength raised 150 -> 155 for the added branch; comments do not count toward it.
Two correctness fixes from review: - RetryBudget measured its duration budgets with Time.now, so a wall-clock adjustment could expire a budget early or stretch it indefinitely. Both budgets now read CLOCK_MONOTONIC. HTTP-date parsing and sentAt in transport.rb stay on wall clock, which is what they need. - BackoffPolicy jittered before clamping, so every attempt at the ceiling returned exactly max_timeout_ms and a fleet that backed off together stayed in lockstep. Clamping first and jittering after fixes that; the jitter now only subtracts, so max_timeout_ms remains a hard ceiling rather than becoming a nominal one that ±50% could overshoot. The spec asserting `next_interval == 10000` at the cap encoded exactly the lockstep behaviour being fixed; it is replaced by one spec for the ceiling and one for the spread.
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.
This branch has not been deployed
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
1 participant
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 original retry loop with a structured dual-path retry system.
rate_limit_retry_after_cap), does NOT consume the retry budget. Bounded bymax_rate_limit_duration(default 12h).retriescount andmax_total_backoff_duration(default 12h).X-Retry-Countheader on retry attempts.success?/success_status?to 2xx only —Net::HTTPdoesn't follow redirects, so treating 3xx as success would silently drop batches.FakeBackoffPolicymissingreset!method (caused all transport specs using custom backoff policies to crash).retrytest suite.Test plan
bundle exec rake specpassesbasic,retrysuites pass (48/48)