diff --git a/History.md b/History.md index 6abeb7a..7a527e8 100644 --- a/History.md +++ b/History.md @@ -1,27 +1,36 @@ 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_limit_retry_after_cap`). -* 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 options `max_total_backoff_duration` and `max_rate_limit_duration` (default 12 hours each) bound the two waits. -* 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 `host` values. -* Network errors are retried on the same backoff schedule as failed responses instead of dropping the batch. -* Backoff waits no longer block shutdown for the full delay. -* Retry timing uses a monotonic clock, so a system clock change cannot stretch or collapse a backoff. -* Backoff intervals are now jittered at the ceiling as well, so clients that back off together do not retry in lockstep. -* **Default backoff pacing changed**: the base wait is 500ms (was 100ms), the ceiling is 60s (was 10s), and the multiplier is 2 (was 1.5). This aligns ruby with the other Segment SDKs, but it does mean a retry schedule that was previously 100ms, 150ms, 225ms… now starts at 500ms and climbs faster. Set `min_timeout_ms`, `max_timeout_ms` and `multiplier` on a `BackoffPolicy` to keep the old pacing. -* A `backoff_policy` supplied by the caller that does not implement `reset!` now logs a warning. One policy instance serves every batch, so without `reset!` its attempt count accumulates and retries get slower the longer the process runs. -* Fix `retries` granting one fewer attempt than configured. A configured 10 performed 9, and `retries: 1` performed none at all. +### Upgrade note: new request header + +This release sends an `X-Retry-Count` request header on retries. If traffic to +Segment passes 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. + +### Upgrade note: backoff pacing + +The default backoff schedule has changed. The base wait is now 500ms rather than +100ms, the ceiling 60s rather than 10s, and the multiplier 2 rather than 1.5. A +schedule that previously ran 100ms, 150ms, 225ms now starts at 500ms and climbs +faster. Pass `min_timeout_ms`, `max_timeout_ms` and `multiplier` to a +`BackoffPolicy` to restore the previous pacing. + +### Retry handling + +* Uploads are retried on 408, 410, 429, 460, and 5xx except 501, 505 and 511. +* A `Retry-After` header is honoured on any retryable response, not only 429. Numeric seconds and the RFC 7231 HTTP-date formats are both accepted, and the value is capped at `rate_limit_retry_after_cap`. +* Responses carrying `Retry-After` are retried for up to `max_rate_limit_duration` and do not consume the retry count. Other failures use exponential backoff limited by `retries` and by `max_total_backoff_duration` as an upper bound. +* New options, all in seconds: `max_rate_limit_duration` (default 1800), `max_total_backoff_duration` (default 43200) and `rate_limit_retry_after_cap` (default 300). +* Network errors are retried on the same schedule as failed responses, rather than dropping the batch. +* A pending retry no longer delays shutdown. +* A `backoff_policy` supplied by the caller that does not implement `reset!` now logs a warning at construction. A single policy instance serves every batch, so without `reset!` its attempt count accumulates and retries grow longer over the life of the process. + +### Other changes + +* `X-Retry-Count` is sent on retries, allowing the server to distinguish a retry from a first attempt. It is omitted on the first attempt. +* Only 2xx responses count as a successful upload. A 3xx is reported as a failed upload rather than treated as delivered, and is not retried: a redirect `Net::HTTP` has already declined to follow will not succeed on one. The Segment endpoint does not redirect, so this affects only custom `host` values. +* `Response#success?` covers the whole 2xx range, so a 201 or 204 is no longer reported through `on_error`. 2.5.0 / 2024-07-17 ================== diff --git a/lib/segment/analytics/defaults.rb b/lib/segment/analytics/defaults.rb index e443caf..106b274 100644 --- a/lib/segment/analytics/defaults.rb +++ b/lib/segment/analytics/defaults.rb @@ -12,9 +12,20 @@ module Request 'Content-Type' => 'application/json', 'User-Agent' => "analytics-ruby/#{Analytics::VERSION}" } RETRIES = 10 - MAX_TOTAL_BACKOFF_DURATION = 43_200 # 12 hours in seconds - MAX_RATE_LIMIT_DURATION = 43_200 # 12 hours in seconds - RATE_LIMIT_RETRY_AFTER_CAP = 300 # seconds + MAX_TOTAL_BACKOFF_DURATION = 43_200 # 12 hours in seconds + + # Rate-limited attempts are deliberately uncounted, so this duration is the + # only thing bounding them. It is deliberately several times + # RATE_LIMIT_RETRY_AFTER_CAP: when the two are equal a single maximal + # Retry-After consumes the whole budget, leaving one attempt and no retry, + # and the cap can never be the smaller of the two so it never binds at all. + MAX_RATE_LIMIT_DURATION = 1800 # seconds + + # A guard against an absurd header, not a second budget. Waiting less than + # the server asked for does not make the next attempt more likely to + # succeed, it just sends more requests at something already rate-limiting + # us; how long we keep trying is MAX_RATE_LIMIT_DURATION's job. + RATE_LIMIT_RETRY_AFTER_CAP = 300 # seconds end module Queue diff --git a/lib/segment/analytics/retry_budget.rb b/lib/segment/analytics/retry_budget.rb index 3032561..54c078c 100644 --- a/lib/segment/analytics/retry_budget.rb +++ b/lib/segment/analytics/retry_budget.rb @@ -46,10 +46,22 @@ def next_backoff_delay end def next_rate_limit_delay(retry_after, status_code) - @rate_limit_start_time ||= monotonic_now - return spent('Max rate limit duration exceeded for batch') if elapsed?(@rate_limit_start_time, @max_rate_limit_duration) + # One reading serves the episode start, the budget test and the delay. A + # second reading lets the budget expire between them, which yields a negative + # remaining and a negative delay — and Kernel#sleep raises ArgumentError on + # one rather than returning immediately. It also leaves remaining a hair under + # the budget on an episode's first response, which is enough to lose an exact + # comparison against the cap. + now = monotonic_now + @rate_limit_start_time ||= now - delay = [retry_after, @rate_limit_retry_after_cap].min + remaining = @max_rate_limit_duration - (now - @rate_limit_start_time) + return spent('Max rate limit duration exceeded for batch') if remaining <= 0 + + # Clamped to what is left of the budget as well as to the cap: the check + # above runs before the wait, so without this a check passing just inside + # the budget would sleep a full Retry-After on top and overshoot it. + delay = [retry_after, @rate_limit_retry_after_cap, remaining].min @logger.debug("Retry-After: #{delay}s on #{status_code}. Retrying after delay.") delay end diff --git a/spec/segment/analytics/retry_budget_spec.rb b/spec/segment/analytics/retry_budget_spec.rb index 6350f60..1c82c51 100644 --- a/spec/segment/analytics/retry_budget_spec.rb +++ b/spec/segment/analytics/retry_budget_spec.rb @@ -11,17 +11,97 @@ def budget(retries, intervals = nil) described_class.new( :retries => retries, :backoff_policy => FakeBackoffPolicy.new(intervals || Array.new(retries, 1000)), - :max_total_backoff_duration => 43_200, - :max_rate_limit_duration => 43_200, - :rate_limit_retry_after_cap => 300, + :max_total_backoff_duration => Defaults::Request::MAX_TOTAL_BACKOFF_DURATION, + :max_rate_limit_duration => Defaults::Request::MAX_RATE_LIMIT_DURATION, + :rate_limit_retry_after_cap => Defaults::Request::RATE_LIMIT_RETRY_AFTER_CAP, :logger => logger ) end + describe '#next_rate_limit_delay' do + it 'clamps the delay to what is left of the budget' do + # The elapsed check runs before the wait, so without clamping a check + # passing just inside the budget sleeps a full Retry-After on top and + # overshoots it. Positioned one second from the end of whatever the budget + # is, rather than at a hardcoded elapsed time, so changing the default + # cannot quietly move this away from the edge it is testing. + subject = budget(10) + subject.instance_variable_set( + :@rate_limit_start_time, + Process.clock_gettime(Process::CLOCK_MONOTONIC) - + (Defaults::Request::MAX_RATE_LIMIT_DURATION - 1) + ) + + delay = subject.next_rate_limit_delay(60, 429) + + expect(delay).to be <= 2 + end + + it 'never returns a negative delay when the budget has just run out' do + subject = budget(10) + subject.instance_variable_set( + :@rate_limit_start_time, + Process.clock_gettime(Process::CLOCK_MONOTONIC) - + Defaults::Request::MAX_RATE_LIMIT_DURATION + ) + + expect(subject.next_rate_limit_delay(60, 429)).to be_nil + end + + it 'reads the clock once, so the budget cannot expire mid-calculation' do + # Kernel#sleep raises ArgumentError on a negative interval rather than + # returning, so a second reading lets the budget expire between the test + # and the delay and crashes the worker. + # + # The count is asserted directly because the returned value alone does not + # discriminate: a second reading past the budget makes the method return + # nil, which any "never negative" assertion accepts. Only the count + # separates the fix from the defect it guards. + budget_s = Defaults::Request::MAX_RATE_LIMIT_DURATION + start = 1000.0 + subject = budget(10) + allow(subject).to receive(:monotonic_now).and_return( + start, # episode start, budget test and delay share this + start + budget_s + 0.001 # any second reading, already past the budget + ) + + delay = subject.next_rate_limit_delay(60, 429) + + expect(subject).to have_received(:monotonic_now).once + expect(delay.nil? || delay >= 0).to be(true), + "returned #{delay.inspect}, which sleep would reject" + end + + it 'returns a small positive delay at the very edge of the budget' do + subject = budget(10) + subject.instance_variable_set( + :@rate_limit_start_time, + Process.clock_gettime(Process::CLOCK_MONOTONIC) - + (Defaults::Request::MAX_RATE_LIMIT_DURATION - 0.5) + ) + + delay = subject.next_rate_limit_delay(60, 429) + + expect(delay).to be > 0 + expect(delay).to be <= 0.5 + end + + it 'clamps the delay to the Retry-After cap' do + expect(budget(10).next_rate_limit_delay(600, 429)) + .to eq(Defaults::Request::RATE_LIMIT_RETRY_AFTER_CAP) + end + + it 'honours a Retry-After that fits inside the cap and the budget' do + # Waiting less than asked sends more requests at a server already + # rate-limiting us, so a value under the cap is used as given. + expect(budget(10).next_rate_limit_delay(120, 429)).to eq(120) + end + end + describe '#next_backoff_delay' do it 'grants exactly as many retries as configured' do - # The count used to be decremented before the exhaustion check, so a - # configured N yielded N-1. go, python and java all grant N. + # N means N. Decrementing before the exhaustion check spends one retry on + # the check itself and silently yields N-1. subject = budget(3) expect(subject.next_backoff_delay).to eq(1.0) diff --git a/spec/segment/analytics/transport_spec.rb b/spec/segment/analytics/transport_spec.rb index 3e2580f..f6fa3a9 100644 --- a/spec/segment/analytics/transport_spec.rb +++ b/spec/segment/analytics/transport_spec.rb @@ -511,9 +511,9 @@ def next_interval subject { described_class.new } it 'abandons the wait when shutdown is requested instead of sleeping it out' do - # The wait used to be a single sleep broken by Thread#wakeup, which only - # interrupts a sleep already in progress and raises ThreadError if the - # thread has finished. Slicing removes the need for it. + # Slicing the wait is what makes this work. Thread#wakeup is not a + # substitute: it only interrupts a sleep already in progress, and raises + # ThreadError if the thread has since finished. elapsed = nil worker = Thread.new do