Dual-path retry: exponential backoff + rate-limit handling

.gitignore

@@ -2,3 +2,6 @@
 Gemfile.lock
 .ruby-version
 coverage/
+.bundle/
+vendor/
+*-plan.md

.rubocop_todo.yml

@@ -62,7 +62,7 @@ Lint/UnusedBlockArgument:
 # Offense count: 3
 # Configuration parameters: AllowedMethods, AllowedPatterns, IgnoredMethods, CountRepeatedAttributes.
 Metrics/AbcSize:
-  Max: 25
+  Max: 33
 
 # Offense count: 3
 # Configuration parameters: CountComments, CountAsOne, ExcludedMethods, AllowedMethods, AllowedPatterns, IgnoredMethods.
@@ -73,7 +73,7 @@ Metrics/BlockLength:
 # Offense count: 1
 # Configuration parameters: CountComments, CountAsOne.
 Metrics/ClassLength:
-  Max: 115
+  Max: 167
 
 # Offense count: 2
 # Configuration parameters: AllowedMethods, AllowedPatterns, IgnoredMethods.
@@ -83,7 +83,7 @@ Metrics/CyclomaticComplexity:
 # Offense count: 11
 # Configuration parameters: CountComments, CountAsOne, ExcludedMethods, AllowedMethods, AllowedPatterns, IgnoredMethods.
 Metrics/MethodLength:
-  Max: 16
+  Max: 21
 
 # Offense count: 1
 # This cop supports safe autocorrection (--autocorrect).

History.md

@@ -1,3 +1,28 @@
+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.
+
 2.5.0 / 2024-07-17
 ==================
 

e2e-cli/e2e-config.json

@@ -1,7 +1,9 @@
 {
   "sdk": "ruby",
-  "test_suites": "basic",
+  "test_suites": "basic,retry",
   "auto_settings": false,
   "patch": null,
-  "env": {}
+  "env": {
+    "AUTH_HEADER": "true"
+  }
 }

lib/segment/analytics.rb

@@ -6,6 +6,7 @@
 require 'segment/analytics/field_parser'
 require 'segment/analytics/client'
 require 'segment/analytics/worker'
+require 'segment/analytics/retry_budget'
 require 'segment/analytics/transport'
 require 'segment/analytics/response'
 require 'segment/analytics/logging'

lib/segment/analytics/backoff_policy.rb

@@ -26,25 +26,17 @@ def initialize(opts = {})
       # @return [Numeric] the next backoff interval, in milliseconds.
       def next_interval
         interval = @min_timeout_ms * (@multiplier**@attempts)
-        interval = add_jitter(interval, @randomization_factor)
-
         @attempts += 1
 
-        [interval, @max_timeout_ms].min
+        # Clamp first, then jitter. Jittering before the clamp meant every attempt
+        # at the ceiling returned exactly max_timeout_ms, so a fleet that backed off
+        # together stayed in lockstep. Jitter only subtracts, so the ceiling holds.
+        capped = [interval, @max_timeout_ms].min
+        capped - (rand * capped * @randomization_factor)
       end
 
-      private
-
-      def add_jitter(base, randomization_factor)
-        random_number = rand
-        max_deviation = base * randomization_factor
-        deviation = random_number * max_deviation
-
-        if random_number < 0.5
-          base - deviation
-        else
-          base + deviation
-        end
+      def reset!
+        @attempts = 0
       end
     end
   end

lib/segment/analytics/client.rb

@@ -32,7 +32,11 @@ def initialize(opts = {})
 
         check_write_key!
 
-        at_exit { @worker_thread && @worker_thread[:should_exit] = true }
+        # The worker checks this between sleep slices, so a Retry-After or backoff
+        # wait is abandoned within a second rather than holding shutdown for up to
+        # rate_limit_retry_after_cap seconds. Assigning to a dead thread is safe;
+        # Thread#wakeup is not, and raising here would force a non-zero exit status.
+        at_exit { @worker_thread[:should_exit] = true if @worker_thread }
       end
 
       # Synchronously waits until the worker has flushed the queue.

lib/segment/analytics/defaults.rb

@@ -12,6 +12,9 @@ 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
       end
 
       module Queue
@@ -28,9 +31,9 @@ module MessageBatch
       end
 
       module BackoffPolicy
-        MIN_TIMEOUT_MS = 100
-        MAX_TIMEOUT_MS = 10000
-        MULTIPLIER = 1.5
+        MIN_TIMEOUT_MS = 500
+        MAX_TIMEOUT_MS = 60_000
+        MULTIPLIER = 2
         RANDOMIZATION_FACTOR = 0.5
       end
     end

lib/segment/analytics/response.rb

@@ -12,6 +12,10 @@ def initialize(status = 200, error = nil)
         @status = status
         @error  = error
       end
+
+      def success?
+        status >= 200 && status < 300
+      end
     end
   end
 end

lib/segment/analytics/retry_budget.rb

@@ -0,0 +1,78 @@
+# frozen_string_literal: true
+
+module Segment
+  class Analytics
+    # Tracks the two independent budgets one send may spend.
+    #
+    # A retryable status carrying Retry-After spends the rate-limit budget, which
+    # is bounded by wall clock only. Anything else retryable spends the counted
+    # backoff budget, bounded by both a retry count and wall clock. Keeping them
+    # separate is what stops a rate-limited server from exhausting the retries
+    # available to genuine failures.
+    #
+    # The caller performs the wait, so both methods return the delay in seconds,
+    # or nil when the budget is spent and the batch should be abandoned.
+    class RetryBudget
+      attr_reader :retry_count
+
+      # Keyword arguments would be cleaner but need Ruby 2.1; the gemspec still
+      # declares >= 2.0, which is also what rubocop is configured to parse.
+      def initialize(options = {})
+        @retries_remaining = options[:retries]
+        @backoff_policy = options[:backoff_policy]
+        @max_total_backoff_duration = options[:max_total_backoff_duration]
+        @max_rate_limit_duration = options[:max_rate_limit_duration]
+        @rate_limit_retry_after_cap = options[:rate_limit_retry_after_cap]
+        @logger = options[:logger]
+        @retry_count = 0
+        @backoff_start_time = nil
+        @rate_limit_start_time = nil
+      end
+
+      def next_backoff_delay
+        # Checked before the decrement: decrementing first spent one retry on the
+        # exhaustion test itself, so a configured N only ever performed N-1, and
+        # retries: 1 and retries: 0 were indistinguishable.
+        return spent('Retries exhausted for batch') if @retries_remaining <= 0
+
+        @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)
+
+        delay_ms = @backoff_policy.next_interval
+        @logger.debug("Retrying request, #{@retries_remaining} retries left. Waiting #{delay_ms}ms")
+        delay_ms.to_f / 1000
+      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)
+
+        delay = [retry_after, @rate_limit_retry_after_cap].min
+        @logger.debug("Retry-After: #{delay}s on #{status_code}. Retrying after delay.")
+        delay
+      end
+
+      def record_retry
+        @retry_count += 1
+      end
+
+      private
+
+      def elapsed?(start_time, limit)
+        (monotonic_now - start_time) >= limit
+      end
+
+      # Wall-clock time can jump; these budgets must not expire or stretch with it.
+      def monotonic_now
+        Process.clock_gettime(Process::CLOCK_MONOTONIC)
+      end
+
+      def spent(message)
+        @logger.error(message)
+        nil
+      end
+    end
+  end
+end