From ed287cf3ef481fd5c9a2bbfa9550a3ee277b033a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 19:03:35 +0200 Subject: [PATCH 1/2] sec: replace math/rand v1 with math/rand/v2 for rate-limiter jitter (#419) math/rand/v2 (Go 1.22+) is the project standard for non-crypto jitter (already used in pkg/retry/exponential.go). Swaps the package-level import in providers/aws/recommendations/ratelimiter.go; the call-site (rand.Float64()) is identical in v2. Adds TestWait_JitterRange: 200 iterations verify jitter stays within [0, 20%] of the base delay and spans more than 5% (non-stuck source). --- providers/aws/recommendations/ratelimiter.go | 2 +- .../aws/recommendations/ratelimiter_test.go | 46 +++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/providers/aws/recommendations/ratelimiter.go b/providers/aws/recommendations/ratelimiter.go index 9f90c40dc..5610bddeb 100644 --- a/providers/aws/recommendations/ratelimiter.go +++ b/providers/aws/recommendations/ratelimiter.go @@ -4,7 +4,7 @@ import ( "context" "errors" "math" - "math/rand" + "math/rand/v2" "strings" "time" ) diff --git a/providers/aws/recommendations/ratelimiter_test.go b/providers/aws/recommendations/ratelimiter_test.go index 7353ffa1c..db557dab6 100644 --- a/providers/aws/recommendations/ratelimiter_test.go +++ b/providers/aws/recommendations/ratelimiter_test.go @@ -287,3 +287,49 @@ func TestRateLimiter_ExhaustsRetries(t *testing.T) { assert.Equal(t, 2, limiter.GetRetryCount()) assert.Error(t, lastErr) } + +// TestWait_JitterRange verifies that the jitter added to the backoff delay spans +// the expected range [0, 20% of delay). Over 200 iterations the observed jitter +// must include at least one value above 5% of the base delay (non-zero spread) +// and must never exceed 20% of the base delay (upper bound). +func TestWait_JitterRange(t *testing.T) { + const iterations = 200 + // Use a short base delay so the test runs quickly. + baseDelay := 100 * time.Millisecond + limiter := NewRateLimiterWithOptions(baseDelay, 10*time.Second, 100) + + // retryCount=1 => delay = 2^0 * baseDelay = baseDelay; jitter <= 0.2*baseDelay. + limiter.retryCount = 1 + + maxJitter := time.Duration(float64(baseDelay) * 0.2) // 20 ms + var observedMax time.Duration + + for i := 0; i < iterations; i++ { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + start := time.Now() + err := limiter.Wait(ctx) + elapsed := time.Since(start) + cancel() + require.NoError(t, err) + + jitter := elapsed - baseDelay + if jitter < 0 { + jitter = 0 + } + if jitter > observedMax { + observedMax = jitter + } + // Jitter must never exceed the 20% cap (add 5ms scheduling slack). + assert.LessOrEqual(t, jitter, maxJitter+5*time.Millisecond, + "jitter %s exceeds 20%% of base delay %s", jitter, baseDelay) + + // Reset so the delay is the same each iteration. + limiter.retryCount = 1 + } + + // After 200 draws from [0, 0.2*delay), we must have seen at least one value + // above 5% of baseDelay -- confirming the source is not stuck at zero. + minSpread := time.Duration(float64(baseDelay) * 0.05) // 5 ms + assert.Greater(t, observedMax, minSpread, + "jitter appears stuck near zero (max observed %s over %d iterations)", observedMax, iterations) +} From 840c5e5f29296a5bc210d7f7f19c52f897b33500 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 16:17:19 +0200 Subject: [PATCH 2/2] test(ratelimiter): make jitter test deterministic to remove wall-clock flakiness (refs #419) Extract computeJitter(delay, r) as a pure function and replace the wall-clock-based TestWait_JitterRange with TestComputeJitter, which drives the function with 10,000 evenly-spaced r values in [0,1) and asserts the output is within [0, 0.2*delay]. No sleeping means OS scheduling overhead cannot blow the upper-bound check. The spread assertion (max observed >= 95% of cap) still catches a regressed always-zero jitter source. --- providers/aws/recommendations/ratelimiter.go | 10 ++- .../aws/recommendations/ratelimiter_test.go | 62 +++++++------------ 2 files changed, 32 insertions(+), 40 deletions(-) diff --git a/providers/aws/recommendations/ratelimiter.go b/providers/aws/recommendations/ratelimiter.go index 5610bddeb..d24ab4c06 100644 --- a/providers/aws/recommendations/ratelimiter.go +++ b/providers/aws/recommendations/ratelimiter.go @@ -75,8 +75,7 @@ func (r *RateLimiter) Wait(ctx context.Context) error { // Add jitter (up to 20% of delay). Math/rand is intentional: jitter only // needs uniform distribution, not cryptographic randomness. - jitter := time.Duration(float64(delay) * 0.2 * rand.Float64()) // #nosec G404 - delay += jitter + delay += computeJitter(delay, rand.Float64()) // #nosec G404 select { case <-time.After(delay): @@ -86,6 +85,13 @@ func (r *RateLimiter) Wait(ctx context.Context) error { } } +// computeJitter returns the jitter component to add to a backoff delay. +// r is a uniform random value in [0, 1); jitter is up to 20% of delay. +// Extracted as a pure function so it can be unit-tested without sleeping. +func computeJitter(delay time.Duration, r float64) time.Duration { + return time.Duration(float64(delay) * 0.2 * r) +} + // ShouldRetry checks if we should retry based on error type and retry count. // Only throttling and transient errors are retried. func (r *RateLimiter) ShouldRetry(err error) bool { diff --git a/providers/aws/recommendations/ratelimiter_test.go b/providers/aws/recommendations/ratelimiter_test.go index db557dab6..f8be208f7 100644 --- a/providers/aws/recommendations/ratelimiter_test.go +++ b/providers/aws/recommendations/ratelimiter_test.go @@ -288,48 +288,34 @@ func TestRateLimiter_ExhaustsRetries(t *testing.T) { assert.Error(t, lastErr) } -// TestWait_JitterRange verifies that the jitter added to the backoff delay spans -// the expected range [0, 20% of delay). Over 200 iterations the observed jitter -// must include at least one value above 5% of the base delay (non-zero spread) -// and must never exceed 20% of the base delay (upper bound). -func TestWait_JitterRange(t *testing.T) { - const iterations = 200 - // Use a short base delay so the test runs quickly. - baseDelay := 100 * time.Millisecond - limiter := NewRateLimiterWithOptions(baseDelay, 10*time.Second, 100) - - // retryCount=1 => delay = 2^0 * baseDelay = baseDelay; jitter <= 0.2*baseDelay. - limiter.retryCount = 1 +// TestComputeJitter verifies that computeJitter produces values in [0, 0.2*delay] +// and that the output varies (i.e. is not stuck at zero). The test is deterministic: +// it calls the pure function directly and never sleeps, so OS scheduling cannot cause +// spurious failures (replaces the former wall-clock-based TestWait_JitterRange). +func TestComputeJitter(t *testing.T) { + const iterations = 10_000 + delay := 100 * time.Millisecond + maxJitter := time.Duration(float64(delay) * 0.2) // 20 ms - maxJitter := time.Duration(float64(baseDelay) * 0.2) // 20 ms var observedMax time.Duration - for i := 0; i < iterations; i++ { - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - start := time.Now() - err := limiter.Wait(ctx) - elapsed := time.Since(start) - cancel() - require.NoError(t, err) - - jitter := elapsed - baseDelay - if jitter < 0 { - jitter = 0 - } - if jitter > observedMax { - observedMax = jitter - } - // Jitter must never exceed the 20% cap (add 5ms scheduling slack). - assert.LessOrEqual(t, jitter, maxJitter+5*time.Millisecond, - "jitter %s exceeds 20%% of base delay %s", jitter, baseDelay) + // Use evenly-spaced r values to cover the full [0,1) range deterministically. + r := float64(i) / float64(iterations) + j := computeJitter(delay, r) + + require.GreaterOrEqualf(t, j, time.Duration(0), + "jitter must be non-negative for r=%f", r) + require.LessOrEqualf(t, j, maxJitter, + "jitter %s exceeds 20%% of delay %s for r=%f", j, delay, r) - // Reset so the delay is the same each iteration. - limiter.retryCount = 1 + if j > observedMax { + observedMax = j + } } - // After 200 draws from [0, 0.2*delay), we must have seen at least one value - // above 5% of baseDelay -- confirming the source is not stuck at zero. - minSpread := time.Duration(float64(baseDelay) * 0.05) // 5 ms - assert.Greater(t, observedMax, minSpread, - "jitter appears stuck near zero (max observed %s over %d iterations)", observedMax, iterations) + // The spread check: with r spanning [0,1) the maximum jitter should reach + // close to maxJitter (within 1 step of iterations). We use 95% as the floor. + minSpread := time.Duration(float64(maxJitter) * 0.95) + assert.GreaterOrEqual(t, observedMax, minSpread, + "jitter spread appears stuck: max observed %s, expected >= %s", observedMax, minSpread) }