Skip to content

Commit 78bfa3e

Browse files
committed
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).
1 parent 2c52567 commit 78bfa3e

3 files changed

Lines changed: 58 additions & 23 deletions

File tree

‎History.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ faster. Pass `min_timeout_ms`, `max_timeout_ms` and `multiplier` to a
2020

2121
* Uploads are retried on 408, 410, 429, 460, and 5xx except 501, 505 and 511.
2222
* 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`.
23-
* 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.
23+
* 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.
2424
* 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).
2525
* Network errors are retried on the same schedule as failed responses, rather than dropping the batch.
2626
* A pending retry no longer delays shutdown.

‎lib/segment/analytics/retry_budget.rb‎

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,21 @@ def next_backoff_delay
3737

3838
@retries_remaining -= 1
3939

40-
@backoff_start_time ||= monotonic_now
41-
return spent('Max total backoff duration exceeded for batch') if elapsed?(@backoff_start_time, @max_total_backoff_duration)
40+
# One reading for the budget test and the fit check below, as on the
41+
# rate-limit path.
42+
now = monotonic_now
43+
@backoff_start_time ||= now
44+
45+
remaining = @max_total_backoff_duration - (now - @backoff_start_time)
46+
return spent('Max total backoff duration exceeded for batch') if remaining <= 0
47+
48+
# A backoff longer than what is left of the budget is a wait whose attempt
49+
# can never run, so there is nothing to schedule.
50+
delay = @backoff_policy.next_interval.to_f / 1000
51+
return spent('Backoff interval does not fit the remaining budget') if delay > remaining
4252

43-
delay_ms = @backoff_policy.next_interval
44-
@logger.debug("Retrying request, #{@retries_remaining} retries left. Waiting #{delay_ms}ms")
45-
delay_ms.to_f / 1000
53+
@logger.debug("Retrying request, #{@retries_remaining} retries left. Waiting #{delay}s")
54+
delay
4655
end
4756

4857
def next_rate_limit_delay(retry_after, status_code)
@@ -58,10 +67,14 @@ def next_rate_limit_delay(retry_after, status_code)
5867
remaining = @max_rate_limit_duration - (now - @rate_limit_start_time)
5968
return spent('Max rate limit duration exceeded for batch') if remaining <= 0
6069

61-
# Clamped to what is left of the budget as well as to the cap: the check
62-
# above runs before the wait, so without this a check passing just inside
63-
# the budget would sleep a full Retry-After on top and overshoot it.
64-
delay = [retry_after, @rate_limit_retry_after_cap, remaining].min
70+
# Capped, then required to fit. Shortening the wait to fit would send the
71+
# next request inside the window the server asked us to wait out — one it
72+
# has already said it will not serve — and the budget is spent by then, so
73+
# it would be the last attempt either way. Giving up loses the same batch
74+
# and sends one request fewer at something already rate-limiting us.
75+
delay = [retry_after, @rate_limit_retry_after_cap].min
76+
return spent('Retry-After does not fit the remaining rate limit budget') if delay > remaining
77+
6578
@logger.debug("Retry-After: #{delay}s on #{status_code}. Retrying after delay.")
6679
delay
6780
end

‎spec/segment/analytics/retry_budget_spec.rb‎

Lines changed: 35 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,22 +19,30 @@ def budget(retries, intervals = nil)
1919
end
2020

2121
describe '#next_rate_limit_delay' do
22-
it 'clamps the delay to what is left of the budget' do
23-
# The elapsed check runs before the wait, so without clamping a check
24-
# passing just inside the budget sleeps a full Retry-After on top and
25-
# overshoots it. Positioned one second from the end of whatever the budget
26-
# is, rather than at a hardcoded elapsed time, so changing the default
27-
# cannot quietly move this away from the edge it is testing.
22+
it 'gives up rather than retrying inside the window the server asked for' do
23+
# Shortening the wait to fit would send the next request before the time
24+
# the server named, and the budget is spent by then, so it would be the
25+
# last attempt either way. Positioned relative to the constant so changing
26+
# the default cannot move this away from the edge it is testing.
2827
subject = budget(10)
2928
subject.instance_variable_set(
3029
:@rate_limit_start_time,
3130
Process.clock_gettime(Process::CLOCK_MONOTONIC) -
3231
(Defaults::Request::MAX_RATE_LIMIT_DURATION - 1)
3332
)
3433

35-
delay = subject.next_rate_limit_delay(60, 429)
34+
expect(subject.next_rate_limit_delay(60, 429)).to be_nil
35+
end
36+
37+
it 'honours a wait that does fit, in full' do
38+
# "Never shorten" must not become "never wait".
39+
subject = budget(10)
40+
subject.instance_variable_set(
41+
:@rate_limit_start_time,
42+
Process.clock_gettime(Process::CLOCK_MONOTONIC) - 60
43+
)
3644

37-
expect(delay).to be <= 2
45+
expect(subject.next_rate_limit_delay(60, 429)).to eq(60)
3846
end
3947

4048
it 'never returns a negative delay when the budget has just run out' do
@@ -72,18 +80,17 @@ def budget(retries, intervals = nil)
7280
"returned #{delay.inspect}, which sleep would reject"
7381
end
7482

75-
it 'returns a small positive delay at the very edge of the budget' do
83+
it 'gives up at the very edge of the budget rather than returning a sliver' do
84+
# 0.5s left against a 60s Retry-After: the wait cannot fit, so there is
85+
# nothing useful to schedule.
7686
subject = budget(10)
7787
subject.instance_variable_set(
7888
:@rate_limit_start_time,
7989
Process.clock_gettime(Process::CLOCK_MONOTONIC) -
8090
(Defaults::Request::MAX_RATE_LIMIT_DURATION - 0.5)
8191
)
8292

83-
delay = subject.next_rate_limit_delay(60, 429)
84-
85-
expect(delay).to be > 0
86-
expect(delay).to be <= 0.5
93+
expect(subject.next_rate_limit_delay(60, 429)).to be_nil
8794
end
8895

8996
it 'clamps the delay to the Retry-After cap' do
@@ -98,6 +105,21 @@ def budget(retries, intervals = nil)
98105
end
99106
end
100107

108+
describe 'the shipped defaults' do
109+
it 'leaves room for more than one maximal Retry-After' do
110+
# At parity the rate-limit path degenerates: one capped wait spends the
111+
# whole budget, so the episode ends having made a single attempt. The cap
112+
# also stops binding, because whatever is left of the budget is then always
113+
# the smaller of the two.
114+
budget_s = Defaults::Request::MAX_RATE_LIMIT_DURATION
115+
cap_s = Defaults::Request::RATE_LIMIT_RETRY_AFTER_CAP
116+
117+
expect(budget_s).to be > cap_s,
118+
"budget #{budget_s}s against a #{cap_s}s cap leaves no room to retry"
119+
expect(budget_s / cap_s).to be >= 2
120+
end
121+
end
122+
101123
describe '#next_backoff_delay' do
102124
it 'grants exactly as many retries as configured' do
103125
# N means N. Decrementing before the exhaustion check spends one retry on

0 commit comments

Comments
 (0)