Bound the rate-limit retry budget at 30 minutes

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. A `Retry-After` that will not fit in what is left of the budget ends the episode rather than being shortened: retrying inside the window the server asked for sends a request it has already declined to serve, and the budget would be spent by then anyway. `max_total_backoff_duration` works the same way. 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
 ==================

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

lib/segment/analytics/retry_budget.rb

@@ -37,19 +37,44 @@ def next_backoff_delay
 
         @retries_remaining -= 1
 
-        @backoff_start_time ||= monotonic_now
-        return spent('Max total backoff duration exceeded for batch') if elapsed?(@backoff_start_time, @max_total_backoff_duration)
+        # One reading for the budget test and the fit check below, as on the
+        # rate-limit path.
+        now = monotonic_now
+        @backoff_start_time ||= now
 
-        delay_ms = @backoff_policy.next_interval
-        @logger.debug("Retrying request, #{@retries_remaining} retries left. Waiting #{delay_ms}ms")
-        delay_ms.to_f / 1000
+        remaining = @max_total_backoff_duration - (now - @backoff_start_time)
+        return spent('Max total backoff duration exceeded for batch') if remaining <= 0
+
+        # A backoff longer than what is left of the budget is a wait whose attempt
+        # can never run, so there is nothing to schedule.
+        delay = @backoff_policy.next_interval.to_f / 1000
+        return spent('Backoff interval does not fit the remaining budget') if delay > remaining
+
+        @logger.debug("Retrying request, #{@retries_remaining} retries left. Waiting #{delay}s")
+        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
 
+        remaining = @max_rate_limit_duration - (now - @rate_limit_start_time)
+        return spent('Max rate limit duration exceeded for batch') if remaining <= 0
+
+        # Capped, then required to fit. Shortening the wait to fit would send the
+        # next request inside the window the server asked us to wait out — one it
+        # has already said it will not serve — and the budget is spent by then, so
+        # it would be the last attempt either way. Giving up loses the same batch
+        # and sends one request fewer at something already rate-limiting us.
         delay = [retry_after, @rate_limit_retry_after_cap].min
+        return spent('Retry-After does not fit the remaining rate limit budget') if delay > remaining
+
         @logger.debug("Retry-After: #{delay}s on #{status_code}. Retrying after delay.")
         delay
       end

spec/segment/analytics/retry_budget_spec.rb

@@ -11,17 +11,119 @@ 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 'gives up rather than retrying inside the window the server asked for' do
+          # Shortening the wait to fit would send the next request before the time
+          # the server named, and the budget is spent by then, so it would be the
+          # last attempt either way. Positioned relative to the constant so changing
+          # the default cannot 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)
+          )
+
+          expect(subject.next_rate_limit_delay(60, 429)).to be_nil
+        end
+
+        it 'honours a wait that does fit, in full' do
+          # "Never shorten" must not become "never wait".
+          subject = budget(10)
+          subject.instance_variable_set(
+            :@rate_limit_start_time,
+            Process.clock_gettime(Process::CLOCK_MONOTONIC) - 60
+          )
+
+          expect(subject.next_rate_limit_delay(60, 429)).to eq(60)
+        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 'gives up at the very edge of the budget rather than returning a sliver' do
+          # 0.5s left against a 60s Retry-After: the wait cannot fit, so there is
+          # nothing useful to schedule.
+          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)
+          )
+
+          expect(subject.next_rate_limit_delay(60, 429)).to be_nil
+        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 'the shipped defaults' do
+        it 'leaves room for more than one maximal Retry-After' do
+          # At parity the rate-limit path degenerates: one capped wait spends the
+          # whole budget, so the episode ends having made a single attempt. The cap
+          # also stops binding, because whatever is left of the budget is then always
+          # the smaller of the two.
+          budget_s = Defaults::Request::MAX_RATE_LIMIT_DURATION
+          cap_s = Defaults::Request::RATE_LIMIT_RETRY_AFTER_CAP
+
+          expect(budget_s).to be > cap_s,
+                              "budget #{budget_s}s against a #{cap_s}s cap leaves no room to retry"
+          expect(budget_s / cap_s).to be >= 2
+        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)

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