[core] Retry on 503 using the Retry-After header (Azure#15906)

Based on the Retry-After specification, 503 should also be supported when considering the Retry-After header.

This also aligns with upcoming Identity plans.

Author: sadasant

sdk/core/core-http/CHANGELOG.md

@@ -4,7 +4,9 @@
 
 ### Features Added
 
-- Changed TS compilation target to ES2017 in order to produce smaller bundles and use more native platform features
+- Changed TS compilation target to ES2017 in order to produce smaller bundles and use more native platform features.
+- Added support for the `Retry-After` header on responses with status code 503, Service Unavailable.
+- Added support for multiple retries on the `ThrottlingRetryPolicy` (up to 3 by default).
 
 ### Breaking Changes
 

sdk/core/core-http/review/core-http.api.md

@@ -145,6 +145,7 @@ export const Constants: {
         };
         StatusCodes: {
             TooManyRequests: number;
+            ServiceUnavailable: number;
         };
     };
     HeaderConstants: {

sdk/core/core-http/src/policies/exponentialRetryPolicy.ts

@@ -21,6 +21,7 @@ import {
 } from "../util/exponentialBackoffStrategy";
 import { RestError } from "../restError";
 import { logger } from "../log";
+import { Constants } from "../util/constants";
 import { delay } from "../util/delay";
 
 export function exponentialRetryPolicy(
@@ -139,6 +140,10 @@ async function retry(
 ): Promise<HttpOperationResponse> {
   function shouldPolicyRetry(responseParam?: HttpOperationResponse): boolean {
     const statusCode = responseParam?.status;
+    if (statusCode === 503 && response?.headers.get(Constants.HeaderConstants.RETRY_AFTER)) {
+      return false;
+    }
+
     if (
       statusCode === undefined ||
       (statusCode < 500 && statusCode !== 408) ||

sdk/core/core-http/src/policies/throttlingRetryPolicy.ts

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
+import { AbortError } from "@azure/abort-controller";
 import {
   BaseRequestPolicy,
   RequestPolicy,
@@ -10,8 +11,8 @@ import {
 import { WebResourceLike } from "../webResource";
 import { HttpOperationResponse } from "../httpOperationResponse";
 import { Constants } from "../util/constants";
+import { DEFAULT_CLIENT_MAX_RETRY_COUNT } from "../util/throttlingRetryStrategy";
 import { delay } from "../util/delay";
-import { AbortError } from "@azure/abort-controller";
 
 type ResponseHandler = (
   httpRequest: WebResourceLike,
@@ -37,6 +38,7 @@ const StandardAbortMessage = "The operation was aborted.";
  */
 export class ThrottlingRetryPolicy extends BaseRequestPolicy {
   private _handleResponse: ResponseHandler;
+  private numberOfRetries = 0;
 
   constructor(
     nextPolicy: RequestPolicy,
@@ -48,13 +50,15 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy {
   }
 
   public async sendRequest(httpRequest: WebResourceLike): Promise<HttpOperationResponse> {
-    return this._nextPolicy.sendRequest(httpRequest.clone()).then((response) => {
-      if (response.status !== StatusCodes.TooManyRequests) {
-        return response;
-      } else {
-        return this._handleResponse(httpRequest, response);
-      }
-    });
+    const response = await this._nextPolicy.sendRequest(httpRequest.clone());
+    if (
+      response.status !== StatusCodes.TooManyRequests &&
+      response.status !== StatusCodes.ServiceUnavailable
+    ) {
+      return response;
+    } else {
+      return this._handleResponse(httpRequest, response);
+    }
   }
 
   private async _defaultResponseHandler(
@@ -70,14 +74,22 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy {
         retryAfterHeader
       );
       if (delayInMs) {
+        this.numberOfRetries += 1;
+
         await delay(delayInMs, undefined, {
           abortSignal: httpRequest.abortSignal,
           abortErrorMsg: StandardAbortMessage
         });
+
         if (httpRequest.abortSignal?.aborted) {
           throw new AbortError(StandardAbortMessage);
         }
-        return this._nextPolicy.sendRequest(httpRequest);
+
+        if (this.numberOfRetries < DEFAULT_CLIENT_MAX_RETRY_COUNT) {
+          return this.sendRequest(httpRequest);
+        } else {
+          return this._nextPolicy.sendRequest(httpRequest);
+        }
       }
     }
 

sdk/core/core-http/src/util/constants.ts

@@ -52,7 +52,8 @@ export const Constants = {
     },
 
     StatusCodes: {
-      TooManyRequests: 429
+      TooManyRequests: 429,
+      ServiceUnavailable: 503
     }
   },
 

sdk/core/core-http/src/util/throttlingRetryStrategy.ts

@@ -0,0 +1,7 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+/**
+ * Maximum number of retries for the throttling retry policy
+ */
+export const DEFAULT_CLIENT_MAX_RETRY_COUNT = 3;

sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts

@@ -72,7 +72,7 @@ describe("ThrottlingRetryPolicy", () => {
       assert.deepEqual(response.request, request);
     });
 
-    it("should do nothing when status code is not 429", async () => {
+    it("should do nothing when status code is not 429 nor 503", async () => {
       const request = new WebResource();
       const mockResponse = {
         status: 400,
@@ -114,6 +114,120 @@ describe("ThrottlingRetryPolicy", () => {
       assert.deepEqual(response, mockResponse);
     });
 
+    it("should pass the response to the handler if the status code equals 503", async () => {
+      const request = new WebResource();
+      const mockResponse = {
+        status: 503,
+        headers: new HttpHeaders({
+          "Retry-After": "100"
+        }),
+        request: request
+      };
+      const policy = createDefaultThrottlingRetryPolicy(mockResponse, (_, response) => {
+        delete (response.request as any).requestId;
+        delete (mockResponse.request as any).requestId;
+        assert.deepEqual(response, mockResponse);
+        return Promise.resolve(response);
+      });
+
+      const response = await policy.sendRequest(request);
+      delete (request as any).requestId;
+      delete (response.request as any).requestId;
+      assert.deepEqual(response, mockResponse);
+    });
+
+    it("if the status code equals 429, it should retry up to 3 times", async () => {
+      const request = new WebResource();
+      const status = 429;
+      const retryResponse = {
+        status,
+        headers: new HttpHeaders({
+          "Retry-After": "1"
+        }),
+        request
+      };
+      const responses: HttpOperationResponse[] = [
+        retryResponse,
+        retryResponse,
+        retryResponse,
+        // This one should be returned
+        {
+          status,
+          headers: new HttpHeaders({
+            "Retry-After": "1",
+            "final-response": "final-response"
+          }),
+          request
+        }
+      ];
+
+      const clock = sinon.useFakeTimers();
+
+      const policy = new ThrottlingRetryPolicy(
+        {
+          async sendRequest(): Promise<HttpOperationResponse> {
+            return responses.shift()!;
+          }
+        },
+        new RequestPolicyOptions()
+      );
+
+      const promise = policy.sendRequest(request);
+      clock.tickAsync(3000);
+
+      const response = await promise;
+      assert.deepEqual(response.status, status);
+      assert.equal(response.headers.get("final-response"), "final-response");
+
+      clock.restore();
+    });
+
+    it("if the status code equals 503, it should retry up to 3 times", async () => {
+      const request = new WebResource();
+      const status = 503;
+      const retryResponse = {
+        status,
+        headers: new HttpHeaders({
+          "Retry-After": "1"
+        }),
+        request
+      };
+      const responses: HttpOperationResponse[] = [
+        retryResponse,
+        retryResponse,
+        retryResponse,
+        // This one should be returned
+        {
+          status,
+          headers: new HttpHeaders({
+            "Retry-After": "1",
+            "final-response": "final-response"
+          }),
+          request
+        }
+      ];
+
+      const clock = sinon.useFakeTimers();
+
+      const policy = new ThrottlingRetryPolicy(
+        {
+          async sendRequest(): Promise<HttpOperationResponse> {
+            return responses.shift()!;
+          }
+        },
+        new RequestPolicyOptions()
+      );
+
+      const promise = policy.sendRequest(request);
+      clock.tickAsync(3000);
+
+      const response = await promise;
+      assert.deepEqual(response.status, status);
+      assert.equal(response.headers.get("final-response"), "final-response");
+
+      clock.restore();
+    });
+
     it("should honor the abort signal passed", async () => {
       const request = new WebResource(
         "https://fakeservice.io",

sdk/core/core-rest-pipeline/CHANGELOG.md

@@ -6,6 +6,12 @@
 
 - Fixed an issue where `proxySettings` does not work when there is username but no password [Issue 15720](https://github.com/Azure/azure-sdk-for-js/issues/15720)
 
+### Features Added
+
+- Added support for the `Retry-After` header on responses with status code 503, Service Unavailable.
+- The `ExponentialRetryPolicy` will now ignore `503` responses if they have the `Retry-After` header.
+- Added support for multiple retries on the `ThrottlingRetryPolicy` (up to 3 by default).
+
 ### Breaking Changes
 
 - Updated @azure/core-tracing to version `1.0.0-preview.12`. See [@azure/core-tracing CHANGELOG](https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/core/core-tracing/CHANGELOG.md) for details about breaking changes with tracing.

sdk/core/core-rest-pipeline/src/policies/exponentialRetryPolicy.ts

@@ -70,7 +70,12 @@ export function exponentialRetryPolicy(
    * @param retryData -  The retry data.
    * @returns True if the operation qualifies for a retry; false otherwise.
    */
-  function shouldRetry(statusCode: number | undefined, retryData: RetryData): boolean {
+  function shouldRetry(response: PipelineResponse | undefined, retryData: RetryData): boolean {
+    const statusCode = response?.status;
+    if (statusCode === 503 && response?.headers.get("Retry-After")) {
+      return false;
+    }
+
     if (
       statusCode === undefined ||
       (statusCode < 500 && statusCode !== 408) ||
@@ -126,7 +131,7 @@ export function exponentialRetryPolicy(
   ): Promise<PipelineResponse> {
     retryData = updateRetryData(retryData, requestError);
     const isAborted = request.abortSignal?.aborted;
-    if (!isAborted && shouldRetry(response?.status, retryData)) {
+    if (!isAborted && shouldRetry(response, retryData)) {
       logger.info(`Retrying request in ${retryData.retryInterval}`);
       try {
         await delay(retryData.retryInterval);

sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts

@@ -10,6 +10,11 @@ import { delay } from "../util/helpers";
  */
 export const throttlingRetryPolicyName = "throttlingRetryPolicy";
 
+/**
+ * Maximum number of retries for the throttling retry policy
+ */
+export const DEFAULT_CLIENT_MAX_RETRY_COUNT = 3;
+
 /**
  * A policy that retries when the server sends a 429 response with a Retry-After header.
  *
@@ -22,21 +27,23 @@ export function throttlingRetryPolicy(): PipelinePolicy {
   return {
     name: throttlingRetryPolicyName,
     async sendRequest(request: PipelineRequest, next: SendRequest): Promise<PipelineResponse> {
-      const response = await next(request);
-      if (response.status !== 429) {
-        return response;
-      }
+      let response = await next(request);
 
-      const retryAfterHeader = response.headers.get("Retry-After");
-
-      if (retryAfterHeader) {
+      for (let count = 0; count < DEFAULT_CLIENT_MAX_RETRY_COUNT; count++) {
+        if (response.status !== 429 && response.status !== 503) {
+          return response;
+        }
+        const retryAfterHeader = response.headers.get("Retry-After");
+        if (!retryAfterHeader) {
+          break;
+        }
         const delayInMs = parseRetryAfterHeader(retryAfterHeader);
-        if (delayInMs) {
-          await delay(delayInMs);
-          return next(request);
+        if (!delayInMs) {
+          break;
         }
+        await delay(delayInMs);
+        response = await next(request);
       }
-
       return response;
     }
   };