From f9cf2ec9ab545ac05ff62abe5e9f235720a4d0fa Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@hashicorp.com>
Date: Wed, 7 Apr 2021 18:33:11 -0400
Subject: [PATCH] lib/retry: allow jitter to exceed max wait.

I changed this in https://github.com/hashicorp/consul/pull/8802#pullrequestreview-500779357 because
exceeding the MaxWait seemed wrong, but as other have pointed out, that behaviour is probably correct.

When multiple waiters hit the max value, we don't want them to converge, so restore the behaviour of
allowing jitter to exceed max, and document it.
---
 lib/retry/retry.go      | 14 +++++++++-----
 lib/retry/retry_test.go | 12 ++++++++++++
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/lib/retry/retry.go b/lib/retry/retry.go
index 17bbeaf83f..e843b2f4f0 100644
--- a/lib/retry/retry.go
+++ b/lib/retry/retry.go
@@ -42,9 +42,11 @@ type Waiter struct {
 	MinFailures uint
 	// MinWait time. Returned after the first failure.
 	MinWait time.Duration
-	// MaxWait time.
+	// MaxWait time applied before Jitter. Note that the actual maximum wait time
+	// is MaxWait + MaxWait * Jitter.
 	MaxWait time.Duration
-	// Jitter to add to each wait time.
+	// Jitter to add to each wait time. The Jitter is applied after MaxWait, which
+	// may cause the actual wait time to exceed MaxWait.
 	Jitter Jitter
 	// Factor is the multiplier to use when calculating the delay. Defaults to
 	// 1 second.
@@ -67,12 +69,14 @@ func (w *Waiter) delay() time.Duration {
 	if shift < 31 {
 		waitTime = (1 << shift) * factor
 	}
+	// apply MaxWait before jitter so that multiple waiters with the same MaxWait
+	// do not converge when they hit their max.
+	if w.MaxWait != 0 && waitTime > w.MaxWait {
+		waitTime = w.MaxWait
+	}
 	if w.Jitter != nil {
 		waitTime = w.Jitter(waitTime)
 	}
-	if w.MaxWait != 0 && waitTime > w.MaxWait {
-		return w.MaxWait
-	}
 	if waitTime < w.MinWait {
 		return w.MinWait
 	}
diff --git a/lib/retry/retry_test.go b/lib/retry/retry_test.go
index 3b6e8f2585..8c4f664f69 100644
--- a/lib/retry/retry_test.go
+++ b/lib/retry/retry_test.go
@@ -105,6 +105,18 @@ func TestWaiter_Delay(t *testing.T) {
 			require.Equal(t, expected*time.Millisecond, w.delay(), "failure count: %d", i)
 		}
 	})
+
+	t.Run("jitter can exceed MaxWait", func(t *testing.T) {
+		w := &Waiter{
+			MaxWait: 20 * time.Second,
+			Jitter:  NewJitter(300),
+
+			failures: 30,
+		}
+
+		delay := w.delay()
+		require.True(t, delay > 20*time.Second, "expected delay %v to be greater than MaxWait %v", delay, w.MaxWait)
+	})
 }
 
 func TestWaiter_Wait(t *testing.T) {