Tidy retry delay jitter calculations

Jitter calc for trying secondary was wrong.  For primary it was correct, so just tidied the formula up for consistency.

Author: JohnRusk

azblob/zc_policy_retry.go

@@ -120,7 +120,8 @@ func (o RetryOptions) calcDelay(try int32) time.Duration { // try is >=1; never
 	}
 
 	// Introduce some jitter:  [0.0, 1.0) / 2 = [0.0, 0.5) + 0.8 = [0.8, 1.3)
-	delay = time.Duration(delay.Seconds() * (rand.Float64()/2 + 0.8) * float64(time.Second)) // NOTE: We want math/rand; not crypto/rand
+	// For casts and rounding - be careful, as per https://github.com/golang/go/issues/20757
+	delay = time.Duration(float32(delay) * (rand.Float32()/2 + 0.8)) // NOTE: We want math/rand; not crypto/rand
 	if delay > o.MaxRetryDelay {
 		delay = o.MaxRetryDelay
 	}
@@ -157,7 +158,8 @@ func NewRetryPolicyFactory(o RetryOptions) pipeline.Factory {
 					logf("Primary try=%d, Delay=%v\n", primaryTry, delay)
 					time.Sleep(delay) // The 1st try returns 0 delay
 				} else {
-					delay := time.Second * time.Duration(rand.Float32()/2+0.8)
+					// For casts and rounding - be careful, as per https://github.com/golang/go/issues/20757
+					delay := time.Duration(float32(time.Second) * (rand.Float32()/2 + 0.8))
 					logf("Secondary try=%d, Delay=%v\n", try-primaryTry, delay)
 					time.Sleep(delay) // Delay with some jitter before trying secondary
 				}

azblob/zt_policy_retry_test.go

@@ -155,6 +155,7 @@ func testRetryTestScenario(c *chk.C, scenario retryTestScenario) {
 		MaxRetryDelay:               4 * time.Second,
 		RetryReadsFromSecondaryHost: "SecondaryDC",
 	}
+	minExpectedTimeToMaxRetries := (retryOptions.MaxRetryDelay * time.Duration(retryOptions.MaxTries-3)) / 2 // a very rough approximation, of a lower bound, given assumption that we hit the cap early in the retry count, and pessimistically assuming that all get halved by random jitter calcs
 	ctx := context.Background()
 	ctx, cancel := context.WithTimeout(ctx, 64 /*2^MaxTries(6)*/ *retryOptions.TryTimeout)
 	retrytestPolicyFactory := newRetryTestPolicyFactory(c, scenario, retryOptions.MaxTries, cancel)
@@ -164,16 +165,18 @@ func testRetryTestScenario(c *chk.C, scenario retryTestScenario) {
 	}
 	p := pipeline.NewPipeline(factories[:], pipeline.Options{})
 	request, err := pipeline.NewRequest(http.MethodGet, *u, strings.NewReader("TestData"))
+	start := time.Now()
 	response, err := p.Do(ctx, nil, request)
 	switch scenario {
 	case retryTestScenarioRetryUntilSuccess:
 		if err != nil || response == nil || response.Response() == nil || response.Response().StatusCode != http.StatusOK {
 			c.Fail() // Operation didn't run to success
 		}
 	case retryTestScenarioRetryUntilMaxRetries:
-		c.Assert(err, chk.NotNil)                                               // Ensure we ended with an error
-		c.Assert(response, chk.IsNil)                                           // Ensure we ended without a valid response
-		c.Assert(retrytestPolicyFactory.try, chk.Equals, retryOptions.MaxTries) // Ensure the operation ends with the exact right number of tries
+		c.Assert(err, chk.NotNil)                                                   // Ensure we ended with an error
+		c.Assert(response, chk.IsNil)                                               // Ensure we ended without a valid response
+		c.Assert(retrytestPolicyFactory.try, chk.Equals, retryOptions.MaxTries)     // Ensure the operation ends with the exact right number of tries
+		c.Assert(time.Since(start) > minExpectedTimeToMaxRetries, chk.Equals, true) // Ensure it took about as long to get here as we expect (bearing in mind randomness in the jitter), as a basic sanity check of our delay duration calculations
 	case retryTestScenarioRetryUntilOperationCancel:
 		c.Assert(err, chk.Equals, context.Canceled)                                     // Ensure we ended due to cancellation
 		c.Assert(response, chk.IsNil)                                                   // Ensure we ended without a valid response