From c09b74b90369216105db4b6ef445c79a93739b08 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Fri, 18 Sep 2026 17:49:49 +0200 Subject: [PATCH] Check the clamp, not the clock, in the deadline test TestRateLimiterWaitNeverSleepsPastTheDeadline ran Wait against a real 200ms Retry-After block with a 220ms deadline and timed it. A wake-up more than 20ms late spends the budget, Wait correctly reports that the deadline passed, and the test fails with the client-limit error rather than on the jitter clamp it is named after. Nothing promises a 200ms sleep returns within 20ms of its due time; a wider margin is the same defect with a longer fuse. The clamp is now sleepWithin(wait, remaining), and the test checks that decision directly -- no timers, no wall clock. Waiting a block out end to end stays covered by TestRateLimiterWaitOutlastsAShortRetryAfter. 30 of 30 failures with the test and 256 busy loops pinned to the same two cores, and again on one saturated core; 0 of 30 on both after. --- internal/resilience/gate_test.go | 30 ++++++++++++++++++----------- internal/resilience/rate_limiter.go | 23 ++++++++++++++-------- 2 files changed, 34 insertions(+), 19 deletions(-) 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 } }