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

@@ -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
 	}

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) {