From 6e0b6538b820b491c67881fc5fc76b06aa5433a9 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 16:42:28 -0400 Subject: [PATCH 1/8] Bound the rate-limit budget at 5 minutes instead of 12 hours MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 12 hour default was a backstop on the assumption a retry count would stop us reaching it. Rate-limited attempts are deliberately uncounted, so it was the operative limit instead. With one worker thread that meant a stuck batch stalled all delivery for half a day, filled the 10,000-message queue, and blocked flush for the same period. Five minutes matches the counted path's ~4 minute worst case. rate_limit_retry_after_cap drops to 60s: at 300s it equalled the whole budget, so one sleep consumed it and the rate-limit path gave a single attempt. The delay is also clamped to the remaining budget, since the elapsed check runs before the wait. The RetryBudget spec helper now takes the shipped defaults from Defaults::Request rather than restating them, so a spec cannot pass against numbers the library no longer uses — which is exactly what happened when the cap moved and the helper kept its own 300. 214 examples, rubocop clean, 61-test e2e suite passes. --- History.md | 3 +++ lib/segment/analytics/defaults.rb | 15 +++++++++--- lib/segment/analytics/retry_budget.rb | 6 ++++- spec/segment/analytics/retry_budget_spec.rb | 27 ++++++++++++++++++--- 4 files changed, 44 insertions(+), 7 deletions(-) diff --git a/History.md b/History.md index 6abeb7a..258c599 100644 --- a/History.md +++ b/History.md @@ -1,6 +1,9 @@ Unreleased ========== +* `max_rate_limit_duration` now defaults to 5 minutes rather than 12 hours, and `rate_limit_retry_after_cap` to 60s rather than 300s. The 12 hour value was a backstop meant to be unreachable, but rate-limited attempts are deliberately uncounted, so it was the only limit on that path — and with a single worker thread a stuck batch stalled all delivery, filled the 10,000-message queue and blocked `flush` for the same period. Five minutes matches the counted path's ~4 minute worst case. +* The rate-limit delay is clamped to the remaining budget. The elapsed check runs before the wait, so a check passing just inside the budget previously slept a full `Retry-After` on top. + ### Upgrade note: new request header and proxy allowlists This release sends an `X-Retry-Count` request header on retries. If your diff --git a/lib/segment/analytics/defaults.rb b/lib/segment/analytics/defaults.rb index e443caf..c5b8b51 100644 --- a/lib/segment/analytics/defaults.rb +++ b/lib/segment/analytics/defaults.rb @@ -12,9 +12,18 @@ 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 + + # Five minutes, in line with the counted-backoff path's ~4 minute worst + # case. This was 12 hours, meant as a backstop a retry count would stop us + # reaching — but rate-limited attempts are deliberately uncounted, so it + # was the only limit on that path. + MAX_RATE_LIMIT_DURATION = 300 # seconds + + # Kept well below MAX_RATE_LIMIT_DURATION so the budget buys several + # attempts rather than one long sleep. At the old 300s a single sleep + # consumed the whole budget. + RATE_LIMIT_RETRY_AFTER_CAP = 60 # seconds end module Queue diff --git a/lib/segment/analytics/retry_budget.rb b/lib/segment/analytics/retry_budget.rb index 3032561..7f1ff89 100644 --- a/lib/segment/analytics/retry_budget.rb +++ b/lib/segment/analytics/retry_budget.rb @@ -49,7 +49,11 @@ 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 + # Clamped to what is left of the budget as well as to the cap: the elapsed + # 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. + remaining = @max_rate_limit_duration - (monotonic_now - @rate_limit_start_time) + 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..8af655d 100644 --- a/spec/segment/analytics/retry_budget_spec.rb +++ b/spec/segment/analytics/retry_budget_spec.rb @@ -11,13 +11,34 @@ 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 — at a + # 5 minute budget that doubles the bound rather than rounding it. + subject = budget(10) + subject.instance_variable_set( + :@rate_limit_start_time, + Process.clock_gettime(Process::CLOCK_MONOTONIC) - 299 + ) + + delay = subject.next_rate_limit_delay(60, 429) + + expect(delay).to be <= 2 + end + + it 'clamps the delay to the Retry-After cap' do + expect(budget(10).next_rate_limit_delay(600, 429)).to eq(60) + 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 From 01acdade884284737ec18650df747abf5e2daf14 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 17:53:09 -0400 Subject: [PATCH 2/8] Rewrite the release notes for a reader seeing them in isolation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three problems. The notes described changes between states that never shipped, so a customer read that a default moved from 12 hours to 5 minutes when only the 5 minutes was ever released. They referred to other SDKs, which means nothing to someone reading one library's notes. And they had accumulated over several passes into contradictions — Retry-After was documented as capped at both 300s and 60s, and the rate-limit budget as both 12 hours and 5 minutes. Rewritten to describe the behaviour this version has, in a consistent structure: upgrade notes that need action first, then retry handling, then everything else. Entries covering fixes to code that has not shipped are dropped, since there is nothing for a reader to compare against. --- History.md | 54 ++++++++++++++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/History.md b/History.md index 258c599..4e36bc2 100644 --- a/History.md +++ b/History.md @@ -1,30 +1,36 @@ Unreleased ========== -* `max_rate_limit_duration` now defaults to 5 minutes rather than 12 hours, and `rate_limit_retry_after_cap` to 60s rather than 300s. The 12 hour value was a backstop meant to be unreachable, but rate-limited attempts are deliberately uncounted, so it was the only limit on that path — and with a single worker thread a stuck batch stalled all delivery, filled the 10,000-message queue and blocked `flush` for the same period. Five minutes matches the counted path's ~4 minute worst case. -* The rate-limit delay is clamped to the remaining budget. The elapsed check runs before the wait, so a check passing just inside the budget previously slept a full `Retry-After` on top. - -### 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 300), `max_total_backoff_duration` (default 43200) and `rate_limit_retry_after_cap` (default 60). +* 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 ================== From be1026e2fd579e43403b9122b6633851011897dc Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 19:17:02 -0400 Subject: [PATCH 3/8] Read the clock once when computing the rate-limit delay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The budget test and the remaining-time calculation each took their own reading, so the budget could expire between them. That yields a negative remaining and a negative delay, and Kernel#sleep raises ArgumentError on a negative interval rather than returning immediately — so the worker thread would die rather than the episode ending. One reading now serves both, and the guard tests the remaining time directly. The condition is equivalent: remaining <= 0 is elapsed >= limit. Found while reviewing my own change. php samples once and is unaffected; python reuses one sample and guards on a positive wait; go and C# express the overshoot as a timer or a timestamp, where a negative is harmless. Two of the three new specs are worth noting as nearly useless: asserting the boundary passes against the two-reading version as well, because the defect is the gap between readings rather than the boundary itself. The third stubs the clock so the later reading falls outside the budget, and that one does fail against the old code, reporting the negative it would have handed to sleep. 217 examples, rubocop clean, 61-test e2e suite passes. --- lib/segment/analytics/retry_budget.rb | 13 ++++-- spec/segment/analytics/retry_budget_spec.rb | 46 +++++++++++++++++++++ 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/lib/segment/analytics/retry_budget.rb b/lib/segment/analytics/retry_budget.rb index 7f1ff89..aa5abaa 100644 --- a/lib/segment/analytics/retry_budget.rb +++ b/lib/segment/analytics/retry_budget.rb @@ -47,12 +47,17 @@ def next_backoff_delay 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) - # Clamped to what is left of the budget as well as to the cap: the elapsed - # 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. + # One clock reading serves both the budget test and the delay below. Reading + # it twice 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. remaining = @max_rate_limit_duration - (monotonic_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 diff --git a/spec/segment/analytics/retry_budget_spec.rb b/spec/segment/analytics/retry_budget_spec.rb index 8af655d..8a0bad1 100644 --- a/spec/segment/analytics/retry_budget_spec.rb +++ b/spec/segment/analytics/retry_budget_spec.rb @@ -34,6 +34,52 @@ def budget(retries, intervals = nil) 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 'cannot return a negative delay if the budget expires mid-calculation' do + # Kernel#sleep raises ArgumentError on a negative interval rather than + # returning, so reading the clock once for the budget test and again for + # the delay lets the budget expire between them and crashes the worker. + # The stubbed clock advances past the budget on the later reading, which + # only a single-reading implementation is immune to. + 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 + start + budget_s - 0.001, # a budget test, just inside + start + budget_s + 0.001 # any later reading, just outside + ) + + delay = subject.next_rate_limit_delay(60, 429) + + 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(60) end From 333586dcdc304d6e57e839298e30076aa506d53d Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Wed, 23 Sep 2026 21:19:03 -0400 Subject: [PATCH 4/8] Cut the comments back to why, not history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applying the team convention to my own work from today. The comments explaining these changes had accumulated into potted histories: why a value had been twelve hours, what a test used to assert, which path used to be unreachable. Six months from now none of that resolves to anything — the diff and the commit messages hold it, and the comment should say why the code is the way it is. What stayed is what a maintainer would undo without it: that Kernel#sleep raises on a negative interval, that Thread#wakeup only interrupts a sleep already in progress, that OkHttp's reads are governed by SO_TIMEOUT so an interrupt does not reach them, and that inverting one assertion would make the duration budget unreachable again. Comments only, no behaviour change. --- lib/segment/analytics/defaults.rb | 7 +++---- spec/segment/analytics/retry_budget_spec.rb | 4 ++-- spec/segment/analytics/transport_spec.rb | 6 +++--- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/lib/segment/analytics/defaults.rb b/lib/segment/analytics/defaults.rb index c5b8b51..4056937 100644 --- a/lib/segment/analytics/defaults.rb +++ b/lib/segment/analytics/defaults.rb @@ -14,10 +14,9 @@ module Request RETRIES = 10 MAX_TOTAL_BACKOFF_DURATION = 43_200 # 12 hours in seconds - # Five minutes, in line with the counted-backoff path's ~4 minute worst - # case. This was 12 hours, meant as a backstop a retry count would stop us - # reaching — but rate-limited attempts are deliberately uncounted, so it - # was the only limit on that path. + # Rate-limited attempts are deliberately uncounted, so this duration is the + # only thing bounding them. Five minutes keeps that in line with the + # counted-backoff path's own worst case. MAX_RATE_LIMIT_DURATION = 300 # seconds # Kept well below MAX_RATE_LIMIT_DURATION so the budget buys several diff --git a/spec/segment/analytics/retry_budget_spec.rb b/spec/segment/analytics/retry_budget_spec.rb index 8a0bad1..338b4c1 100644 --- a/spec/segment/analytics/retry_budget_spec.rb +++ b/spec/segment/analytics/retry_budget_spec.rb @@ -87,8 +87,8 @@ def budget(retries, intervals = nil) 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 From 263bf2bdc760f9d1d51dffdd21e9a6115ed2829c Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Thu, 24 Sep 2026 09:54:10 -0400 Subject: [PATCH 5/8] Honour Retry-After up to 300s rather than capping it at 60 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Capping at 60s meant waiting less than the server asked for, which does not make the next attempt more likely to succeed — it just sends more requests at something already rate-limiting us. Against a Retry-After of 180s inside a 5 minute budget it turns 3 requests into 6; against 300s it turns 2 into 6. The cap is a guard against an absurd header, not a second budget. How long we keep trying is max_rate_limit_duration's job, and the clamp to the remaining budget already stops a single wait running past it, so the cap now rarely binds at all. It also bought nothing for the client this was partly aimed at: with no background thread, a shorter cap turns one long wait into several short ones for the same total blocking time and more requests. Tests that pinned 60 are updated, and each SDK gains one asserting that a Retry-After inside the cap is used as given rather than shortened. --- History.md | 2 +- lib/segment/analytics/defaults.rb | 9 +++++---- spec/segment/analytics/retry_budget_spec.rb | 9 ++++++++- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/History.md b/History.md index 4e36bc2..19b5cc9 100644 --- a/History.md +++ b/History.md @@ -21,7 +21,7 @@ faster. Pass `min_timeout_ms`, `max_timeout_ms` and `multiplier` to a * 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 300), `max_total_backoff_duration` (default 43200) and `rate_limit_retry_after_cap` (default 60). +* New options, all in seconds: `max_rate_limit_duration` (default 300), `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. diff --git a/lib/segment/analytics/defaults.rb b/lib/segment/analytics/defaults.rb index 4056937..bd2d1b4 100644 --- a/lib/segment/analytics/defaults.rb +++ b/lib/segment/analytics/defaults.rb @@ -19,10 +19,11 @@ module Request # counted-backoff path's own worst case. MAX_RATE_LIMIT_DURATION = 300 # seconds - # Kept well below MAX_RATE_LIMIT_DURATION so the budget buys several - # attempts rather than one long sleep. At the old 300s a single sleep - # consumed the whole budget. - RATE_LIMIT_RETRY_AFTER_CAP = 60 # 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/spec/segment/analytics/retry_budget_spec.rb b/spec/segment/analytics/retry_budget_spec.rb index 338b4c1..0d9761d 100644 --- a/spec/segment/analytics/retry_budget_spec.rb +++ b/spec/segment/analytics/retry_budget_spec.rb @@ -81,7 +81,14 @@ def budget(retries, intervals = nil) end it 'clamps the delay to the Retry-After cap' do - expect(budget(10).next_rate_limit_delay(600, 429)).to eq(60) + 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 From 69c363567448f04e532a2bb54d9005a26a256e05 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 25 Sep 2026 14:16:38 -0400 Subject: [PATCH 6/8] Read the clock once in next_rate_limit_delay, for real this time The ||= initialising @rate_limit_start_time took its own reading, so the budget test and the delay were computed from two. On an episode's first response that left remaining a hair under max_rate_limit_duration, and since the cap and the budget are both 300 it is remaining that wins the min -- so a large Retry-After returned 299.99999928 rather than 300. Two exact-equality assertions land on that value, one of them in transport_spec and untouched by this branch. They pass on macOS, whose CLOCK_MONOTONIC resolution is 1us and where consecutive readings usually collide, and fail on Linux CI, where they do not. Measured here: 653 failures in 20000 constructions before, 0 after. The spec written to guard this could not catch it. It stubbed three readings to drive the delay negative, but a second reading past the budget makes the method return nil, which its "never negative" assertion accepts. It now asserts the reading count, which is the property in question and fails deterministically: expected 1 time, received 2. 218 examples, 0 failures. Rubocop's 27 offences are all in e2e-cli/main.rb and predate this branch. --- lib/segment/analytics/retry_budget.rb | 15 +++++++++------ spec/segment/analytics/retry_budget_spec.rb | 19 +++++++++++-------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/lib/segment/analytics/retry_budget.rb b/lib/segment/analytics/retry_budget.rb index aa5abaa..54c078c 100644 --- a/lib/segment/analytics/retry_budget.rb +++ b/lib/segment/analytics/retry_budget.rb @@ -46,13 +46,16 @@ def next_backoff_delay end def next_rate_limit_delay(retry_after, status_code) - @rate_limit_start_time ||= monotonic_now - - # One clock reading serves both the budget test and the delay below. Reading - # it twice lets the budget expire between them, which yields a negative + # 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. - remaining = @max_rate_limit_duration - (monotonic_now - @rate_limit_start_time) + # 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 # Clamped to what is left of the budget as well as to the cap: the check diff --git a/spec/segment/analytics/retry_budget_spec.rb b/spec/segment/analytics/retry_budget_spec.rb index 0d9761d..bc1dfc7 100644 --- a/spec/segment/analytics/retry_budget_spec.rb +++ b/spec/segment/analytics/retry_budget_spec.rb @@ -45,23 +45,26 @@ def budget(retries, intervals = nil) expect(subject.next_rate_limit_delay(60, 429)).to be_nil end - it 'cannot return a negative delay if the budget expires mid-calculation' do + 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 reading the clock once for the budget test and again for - # the delay lets the budget expire between them and crashes the worker. - # The stubbed clock advances past the budget on the later reading, which - # only a single-reading implementation is immune to. + # 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 - start + budget_s - 0.001, # a budget test, just inside - start + budget_s + 0.001 # any later reading, just outside + 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 From 2c525672882cdeb19191f85b29195c9346ba93d5 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 25 Sep 2026 14:33:20 -0400 Subject: [PATCH 7/8] Raise the rate-limit budget to 30 minutes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The budget and the Retry-After cap were both 300s, and at parity the rate-limit path degenerates. A response with no usable Retry-After waits the cap by default, the elapsed check runs before the wait, so that one wait spends the whole budget and the batch is dropped having been tried once. A legitimate Retry-After of 300 does the same. The cap also stops binding: whatever is left of the budget is always the smaller term, so the cap can never be the value that clamps. Thirty minutes restores the relationship the two knobs are meant to have — the cap bounds one wait, the budget bounds the episode — and leaves room for several attempts. It costs nothing in normal operation, since the budget only binds when the server has been rate-limiting us for a long time, and in that case keeping the data is the point. --- History.md | 2 +- lib/segment/analytics/defaults.rb | 8 +++++--- spec/segment/analytics/retry_budget_spec.rb | 9 ++++++--- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/History.md b/History.md index 19b5cc9..7a527e8 100644 --- a/History.md +++ b/History.md @@ -21,7 +21,7 @@ faster. Pass `min_timeout_ms`, `max_timeout_ms` and `multiplier` to a * 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 300), `max_total_backoff_duration` (default 43200) and `rate_limit_retry_after_cap` (default 300). +* 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. diff --git a/lib/segment/analytics/defaults.rb b/lib/segment/analytics/defaults.rb index bd2d1b4..106b274 100644 --- a/lib/segment/analytics/defaults.rb +++ b/lib/segment/analytics/defaults.rb @@ -15,9 +15,11 @@ module Request 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. Five minutes keeps that in line with the - # counted-backoff path's own worst case. - MAX_RATE_LIMIT_DURATION = 300 # seconds + # 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 diff --git a/spec/segment/analytics/retry_budget_spec.rb b/spec/segment/analytics/retry_budget_spec.rb index bc1dfc7..1c82c51 100644 --- a/spec/segment/analytics/retry_budget_spec.rb +++ b/spec/segment/analytics/retry_budget_spec.rb @@ -21,12 +21,15 @@ def budget(retries, intervals = nil) 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 — at a - # 5 minute budget that doubles the bound rather than rounding it. + # 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) - 299 + Process.clock_gettime(Process::CLOCK_MONOTONIC) - + (Defaults::Request::MAX_RATE_LIMIT_DURATION - 1) ) delay = subject.next_rate_limit_delay(60, 429) From 78bfa3e40f233333c90fb9c2044b7d69adeadbb2 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 25 Sep 2026 17:02:23 -0400 Subject: [PATCH 8/8] End an episode rather than retry inside the window the server asked for Review point, and a fair one: shortening a Retry-After to fit the remaining budget sends the next request before the time the server named, which it has already said it will not serve, and the budget is spent by then so it would be the final attempt either way. The clamp bought one guaranteed-refused request per episode at a server already rate-limiting us. Both paths now give up at that point instead. next_backoff_delay gets the same rule, and with it the single clock reading the rate-limit path already had -- elapsed? took its own, which was harmless while nothing was derived from the difference and would not have been once a fit check depends on it. Added the spec pinning budget > cap with room for two waits. go, python and C# each got one; ruby's own comment states the invariant most precisely of the six and nothing tested it, so reverting the default to parity left the suite green. It now fails with "budget 300s against a 300s cap leaves no room to retry". 220 examples, rubocop unchanged (its 27 offences are all in e2e-cli/main.rb and predate this branch). --- History.md | 2 +- lib/segment/analytics/retry_budget.rb | 31 +++++++++---- spec/segment/analytics/retry_budget_spec.rb | 48 +++++++++++++++------ 3 files changed, 58 insertions(+), 23 deletions(-) diff --git a/History.md b/History.md index 7a527e8..1e73cbe 100644 --- a/History.md +++ b/History.md @@ -20,7 +20,7 @@ faster. Pass `min_timeout_ms`, `max_timeout_ms` and `multiplier` to a * 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. +* 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. diff --git a/lib/segment/analytics/retry_budget.rb b/lib/segment/analytics/retry_budget.rb index 54c078c..0af0396 100644 --- a/lib/segment/analytics/retry_budget.rb +++ b/lib/segment/analytics/retry_budget.rb @@ -37,12 +37,21 @@ 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 + + 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 - delay_ms = @backoff_policy.next_interval - @logger.debug("Retrying request, #{@retries_remaining} retries left. Waiting #{delay_ms}ms") - delay_ms.to_f / 1000 + @logger.debug("Retrying request, #{@retries_remaining} retries left. Waiting #{delay}s") + delay end def next_rate_limit_delay(retry_after, status_code) @@ -58,10 +67,14 @@ def next_rate_limit_delay(retry_after, status_code) 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 + # 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 diff --git a/spec/segment/analytics/retry_budget_spec.rb b/spec/segment/analytics/retry_budget_spec.rb index 1c82c51..e75f76e 100644 --- a/spec/segment/analytics/retry_budget_spec.rb +++ b/spec/segment/analytics/retry_budget_spec.rb @@ -19,12 +19,11 @@ def budget(retries, intervals = nil) 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. + 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, @@ -32,9 +31,18 @@ def budget(retries, intervals = nil) (Defaults::Request::MAX_RATE_LIMIT_DURATION - 1) ) - delay = subject.next_rate_limit_delay(60, 429) + 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(delay).to be <= 2 + 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 @@ -72,7 +80,9 @@ def budget(retries, intervals = nil) "returned #{delay.inspect}, which sleep would reject" end - it 'returns a small positive delay at the very edge of the budget' do + 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, @@ -80,10 +90,7 @@ def budget(retries, intervals = nil) (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 + expect(subject.next_rate_limit_delay(60, 429)).to be_nil end it 'clamps the delay to the Retry-After cap' do @@ -98,6 +105,21 @@ def budget(retries, intervals = nil) 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 # N means N. Decrementing before the exhaustion check spends one retry on