Retry-After delay shouldn't exceed MaxRetryDelay (Azure#19118)

* Retry-After delay shouldn't exceed MaxRetryDelay

Don't retry when Retry-After has a delay greater than the configured
MaxRetryDelay value.

* add test for the retry path

Author: jhendrixMSFT

sdk/azcore/CHANGELOG.md

@@ -7,6 +7,7 @@
 ### Breaking Changes
 
 ### Bugs Fixed
+* Don't retry a request if the `Retry-After` delay is greater than the configured `RetryOptions.MaxRetryDelay`.
 
 ### Other Changes
 

sdk/azcore/runtime/policy_retry.go

@@ -168,14 +168,19 @@ func (p *retryPolicy) Do(req *policy.Request) (resp *http.Response, err error) {
 			return
 		}
 
-		// drain before retrying so nothing is leaked
-		Drain(resp)
-
 		// use the delay from retry-after if available
 		delay := shared.RetryAfter(resp)
 		if delay <= 0 {
 			delay = calcDelay(options, try)
+		} else if delay > options.MaxRetryDelay {
+			// the retry-after delay exceeds the the cap so don't retry
+			log.Writef(log.EventRetryPolicy, "Retry-After delay %s exceeds MaxRetryDelay of %s", delay, options.MaxRetryDelay)
+			return
 		}
+
+		// drain before retrying so nothing is leaked
+		Drain(resp)
+
 		log.Writef(log.EventRetryPolicy, "End Try #%d, Delay=%v", try, delay)
 		select {
 		case <-time.After(delay):

sdk/azcore/runtime/policy_retry_test.go

@@ -672,6 +672,38 @@ func TestRetryPolicySuccessWithPerTryTimeoutNoRetryWithBodyDownload(t *testing.T
 	require.Equal(t, largeBody, body)
 }
 
+func TestPipelineNoRetryOn429(t *testing.T) {
+	srv, close := mock.NewServer()
+	defer close()
+	// initial response is throttling with a long retry-after delay, it should not trigger a retry
+	srv.AppendResponse(mock.WithStatusCode(http.StatusTooManyRequests), mock.WithHeader(shared.HeaderRetryAfter, "300"))
+	perRetryPolicy := countingPolicy{}
+	req, err := NewRequest(context.Background(), http.MethodGet, srv.URL())
+	require.NoError(t, err)
+	pl := exported.NewPipeline(srv, NewRetryPolicy(nil), &perRetryPolicy)
+	resp, err := pl.Do(req)
+	require.NoError(t, err)
+	require.Equal(t, http.StatusTooManyRequests, resp.StatusCode)
+	require.Equal(t, 1, perRetryPolicy.count)
+}
+
+func TestPipelineRetryOn429(t *testing.T) {
+	srv, close := mock.NewServer()
+	defer close()
+	srv.AppendResponse(mock.WithStatusCode(http.StatusTooManyRequests), mock.WithHeader(shared.HeaderRetryAfter, "1"))
+	srv.AppendResponse(mock.WithStatusCode(http.StatusTooManyRequests), mock.WithHeader(shared.HeaderRetryAfter, "1"))
+	srv.AppendResponse(mock.WithStatusCode(http.StatusOK))
+	perRetryPolicy := countingPolicy{}
+	req, err := NewRequest(context.Background(), http.MethodGet, srv.URL())
+	require.NoError(t, err)
+	opt := testRetryOptions()
+	pl := exported.NewPipeline(srv, NewRetryPolicy(opt), &perRetryPolicy)
+	resp, err := pl.Do(req)
+	require.NoError(t, err)
+	require.Equal(t, http.StatusOK, resp.StatusCode)
+	require.Equal(t, 3, perRetryPolicy.count)
+}
+
 func newRewindTrackingBody(s string) *rewindTrackingBody {
 	// there are two rewinds that happen before rewinding for a retry
 	// 1. to get the body's size in SetBody()