diff --git a/internal/resilience/gate_test.go b/internal/resilience/gate_test.go index 43f24bf88..0ad0b4905 100644 --- a/internal/resilience/gate_test.go +++ b/internal/resilience/gate_test.go @@ -329,22 +329,30 @@ func TestRateLimiterWaitReportsALongRetryAfterImmediately(t *testing.T) { assert.Less(t, time.Since(start), 100*time.Millisecond, "did not burn the budget on a block it cannot outlast") } -// The jitter that spreads retries must not stretch a sleep to or past the -// budget: a block that lifts just inside the deadline is waited out and the -// token taken, not overshot or rejected at the wire. Pinned to the maximum -// jitter, which unchecked would overshoot by half. +// The jitter that spreads retries must not stretch a sleep past the budget: a +// block that lifts just inside the deadline is waited out and the token taken, +// not overshot and rejected at the wire. Pinned to the maximum jitter, which +// unchecked would overshoot by half. +// +// What is checked is the sleep the gate asks for, not the wall clock around a +// real one. This test used to run Wait against a 200ms block with a 220ms +// budget and time it: a sleep has no upper bound on a loaded machine, so a +// wake-up 20ms late spent the budget and Wait returned the client-limit error +// it is not about. Waiting out a block end to end is covered, with a budget +// that is not a stopwatch, by TestRateLimiterWaitOutlastsAShortRetryAfter. func TestRateLimiterWaitNeverSleepsPastTheDeadline(t *testing.T) { previous := jitter jitter = func(d time.Duration) time.Duration { return d / 2 } t.Cleanup(func() { jitter = previous }) - rl := NewRateLimiter(NewStore(t.TempDir()), RateLimiterConfig{}) - require.NoError(t, rl.SetRetryAfterDuration(200*time.Millisecond)) - - start := time.Now() - budget := 220 * time.Millisecond - require.NoError(t, rl.Wait(context.Background(), start.Add(budget))) - assert.Less(t, time.Since(start), budget+60*time.Millisecond) + assert.Equal(t, 300*time.Millisecond, sleepWithin(200*time.Millisecond, time.Second), + "room to spare: the jitter is added") + assert.Equal(t, 200*time.Millisecond, sleepWithin(200*time.Millisecond, 220*time.Millisecond), + "no room: the bare wait, not the 300ms that would wake past the deadline") + assert.Equal(t, minRefillWait*3/2, sleepWithin(time.Microsecond, time.Second), + "a deficit of microseconds still waits out minRefillWait, jitter and all") + assert.Equal(t, time.Microsecond, sleepWithin(time.Microsecond, time.Millisecond), + "nor does the floor itself overshoot a budget shorter than it") } func TestRateLimiterWaitRoundsTheRetryAfterUp(t *testing.T) { diff --git a/internal/resilience/rate_limiter.go b/internal/resilience/rate_limiter.go index d525c3767..58aa4debd 100644 --- a/internal/resilience/rate_limiter.go +++ b/internal/resilience/rate_limiter.go @@ -110,6 +110,20 @@ func (rl *RateLimiter) take() (allowed bool, wait time.Duration, blocked bool) { // consumed by whichever process reaches the lock first anyway. const minRefillWait = 5 * time.Millisecond +// sleepWithin is how long to wait before the next attempt at the bucket: +// wait, floored at minRefillWait and jittered so that processes which woke +// together do not retry in lockstep. Jitter never pushes the sleep past the +// remaining budget — a wake there would be rejected unheard after having +// waited out the very refill or block it was waiting for — so when the +// jittered sleep would not fit, the bare wait is used. +func sleepWithin(wait, remaining time.Duration) time.Duration { + sleep := jittered(max(wait, minRefillWait)) + if sleep > remaining { + return wait + } + return sleep +} + // Wait consumes tokens for one request, sleeping for refills or for a // Retry-After block to lift, until deadline. It returns nil when the request // may proceed, a *GateError when the deadline would pass first, or ctx.Err(). @@ -139,14 +153,7 @@ func (rl *RateLimiter) waitSince(ctx context.Context, start, deadline time.Time) if wait > remaining { return rl.gateError(blocked, wait, rl.now().Sub(start)) } - // Jitter never pushes the sleep to the deadline: a wake there would be - // rejected unheard after having waited out the very refill or block - // it was waiting for. - sleep := jittered(max(wait, minRefillWait)) - if sleep > remaining { - sleep = wait - } - if err := pause(ctx, sleep); err != nil { + if err := pause(ctx, sleepWithin(wait, remaining)); err != nil { return err } }