[#IOPSC-118] Respond with 'too many requests' status code when creati…

…ng too much subscriptions for the same account (#202)

CreateSubscription/__tests__/handler.test.ts

@@ -6,6 +6,7 @@ import {
   SubscriptionContract,
   UserContract
 } from "@azure/arm-apimanagement/esm/models";
+import { RestError } from "@azure/ms-rest-js";
 import * as E from "fp-ts/lib/Either";
 import * as TE from "fp-ts/lib/TaskEither";
 import { ProductNamePayload } from "../../generated/definitions/ProductNamePayload";
@@ -315,4 +316,34 @@ describe("CreateSubscription", () => {
     });
     expect(E.isRight(UserInfo.decode((response as any).value))).toBeTruthy();
   });
+
+  it("should return too many requests if APIM respinds with 412", async () => {
+    mockUserListByService.mockImplementation(() =>
+      Promise.resolve([aFakeApimUser])
+    );
+    mockProductList.mockImplementation(() =>
+      Promise.resolve([aFakeApimProductContract])
+    );
+    mockSubscriptionCreateOrUpdate.mockImplementation(() =>
+      Promise.reject(new RestError("", "", 412))
+    );
+
+    const createSubscriptionHandler = CreateSubscriptionHandler(
+      fakeServicePrincipalCredentials,
+      fakeApimConfig
+    );
+
+    const response = await createSubscriptionHandler(
+      mockedContext as any,
+      undefined as any,
+      fakeParams as any,
+      fakePayload as any
+    );
+
+    expect(response).toEqual(
+      expect.objectContaining({
+        kind: "IResponseErrorTooManyRequests"
+      })
+    );
+  });
 });

CreateSubscription/handler.ts

@@ -17,18 +17,21 @@ import { Errors } from "io-ts";
 import {
   IResponseErrorInternal,
   IResponseErrorNotFound,
+  IResponseErrorTooManyRequests,
   IResponseSuccessJson,
   ResponseErrorNotFound,
+  ResponseErrorTooManyRequests,
   ResponseSuccessJson
 } from "@pagopa/ts-commons/lib/responses";
 import { NonEmptyString } from "@pagopa/ts-commons/lib/strings";
-import { flow, pipe } from "fp-ts/lib/function";
+import { flow, identity, pipe } from "fp-ts/lib/function";
 import { EmailAddress } from "../generated/definitions/EmailAddress";
 import { ProductNamePayload } from "../generated/definitions/ProductNamePayload";
 import { Subscription } from "../generated/definitions/Subscription";
 import {
   getApiClient,
   IAzureApimConfig,
+  isErrorStatusCode,
   IServicePrincipalCreds
 } from "../utils/apim";
 import { subscriptionContractToApiSubscription } from "../utils/conversions";
@@ -37,6 +40,7 @@ import {
   genericInternalValidationErrorHandler
 } from "../utils/errorHandler";
 import { CreateSubscriptionParamsMiddleware } from "../utils/middlewares/createSubscriptionParamsMiddleware";
+import { withRetry } from "../utils/retry";
 
 type ICreateSubscriptionHandler = (
   context: Context,
@@ -47,6 +51,7 @@ type ICreateSubscriptionHandler = (
   | IResponseSuccessJson<Subscription>
   | IResponseErrorInternal
   | IResponseErrorNotFound
+  | IResponseErrorTooManyRequests
 >;
 
 // eslint-disable-next-line prefer-arrow/prefer-arrow-functions
@@ -183,7 +188,7 @@ export function CreateSubscriptionHandler(
         )
       ),
       TE.chainW(taskResults =>
-        TE.tryCatch(
+        pipe(
           () =>
             taskResults.apimClient.subscription.createOrUpdate(
               azureApimConfig.apimResourceGroup,
@@ -196,11 +201,27 @@ export function CreateSubscriptionHandler(
                 state: "active"
               }
             ),
-          error =>
-            internalErrorHandler(
-              "Could not create the subscription.",
-              error as Error
-            )
+          // It turns out Azure API Management implements optimistic consistency on accounts
+          // Hence, when multiple subscriptions are added to the same account concurrently,
+          //   this API may return 412.
+          // This is an undocumented behaviour that arose in production.
+          // In accordance with Azure support, we decided to retry the request on such case
+          withRetry({
+            delayMS: 200,
+            maxAttempts: 3,
+            whileCondition: f => isErrorStatusCode(f, 412)
+          }),
+          retrieable => TE.tryCatch(retrieable, identity),
+          // If we get 412 even after retries, we respond with a too may request status
+          //   so we ask the client to retry by itself
+          TE.mapLeft(error =>
+            isErrorStatusCode(error, 412)
+              ? ResponseErrorTooManyRequests()
+              : internalErrorHandler(
+                  "Could not create the subscription.",
+                  error as Error
+                )
+          )
         )
       ),
       TE.chainW(

openapi/index.yaml

@@ -512,6 +512,8 @@ paths:
           description: Forbidden
         "404":
           description: Resource (User or Product) not found
+        "429":
+          description: Too Many Requests
         "500":
           description: Internal server error
 definitions:

package.json

@@ -59,7 +59,7 @@
     "@azure/arm-apimanagement": "^5.1.1",
     "@azure/cosmos": "^3.15.1",
     "@azure/graph": "^4.0.1",
-    "@azure/ms-rest-js": "^2.5.1",
+    "@azure/ms-rest-js": "^2.6.4",
     "@azure/ms-rest-nodeauth": "^2.0.6",
     "@pagopa/express-azure-functions": "^2.0.0",
     "@pagopa/io-functions-commons": "^25.5.2",

utils/__tests__/retry.test.ts

@@ -0,0 +1,125 @@
+import { withRetry } from "../retry";
+
+const aGoodResult = 42;
+const aBadResult = "bad things happen here";
+const anAlwaysGoodOperation = jest.fn(() => Promise.resolve(aGoodResult));
+const anAlwaysBadOperation = jest.fn(() => Promise.reject(aBadResult));
+const anOperationGoodAtAttemptNth = (n: number) => {
+  let count = 0;
+  return jest.fn(() => {
+    count++;
+    if (count < n) {
+      return Promise.reject(aBadResult);
+    } else {
+      return Promise.resolve(aGoodResult);
+    }
+  });
+};
+
+const sleep = ms => new Promise(done => setTimeout(done, ms));
+
+beforeEach(() => {
+  jest.clearAllMocks();
+});
+
+// check test helper
+describe("anOperationGoodAtAttemptNth", () => {
+  it("works as intended", () => {
+    const op = anOperationGoodAtAttemptNth(3);
+    const result1 = op();
+    const result2 = op();
+    const result3 = op();
+    const result4 = op();
+
+    expect(result1).rejects.toEqual(aBadResult);
+    expect(result2).rejects.toEqual(aBadResult);
+    expect(result3).resolves.toEqual(aGoodResult);
+    expect(result4).resolves.toEqual(aGoodResult);
+  });
+});
+
+describe("withRetry", () => {
+  it("should return a promise", async () => {
+    const retriable = withRetry()(anAlwaysGoodOperation);
+    const result = retriable();
+
+    // check is a Promise
+    expect(
+      result instanceof Object &&
+        "then" in result &&
+        typeof result.then === "function"
+    ).toBe(true);
+
+    const value = await result;
+    expect(value).toEqual(aGoodResult);
+    expect(anAlwaysGoodOperation).toBeCalledTimes(1);
+  });
+
+  it("should fail when the operations always fails", async () => {
+    const operation = anAlwaysBadOperation;
+    const retriable = withRetry()(operation);
+    const result = retriable();
+
+    try {
+      await result;
+      fail("expected to throw");
+    } catch (error) {
+      expect(error).toEqual(aBadResult);
+      expect(operation).toBeCalledTimes(3);
+    }
+  });
+
+  it("should fail when the operations fails more than retried times", async () => {
+    const operation = anOperationGoodAtAttemptNth(4);
+    const retriable = withRetry({ maxAttempts: 3 })(operation);
+    const result = retriable();
+
+    try {
+      await result;
+      fail("expected to throw");
+    } catch (error) {
+      expect(error).toEqual(aBadResult);
+      expect(operation).toBeCalledTimes(3);
+    }
+  });
+
+  it("should succeed when the operations fails less than retried times", async () => {
+    const operation = anOperationGoodAtAttemptNth(2);
+    const retriable = withRetry({ maxAttempts: 3 })(operation);
+    const result = retriable();
+
+    const value = await result;
+
+    expect(value).toEqual(aGoodResult);
+    expect(operation).toBeCalledTimes(2);
+  });
+
+  it("should not retry if whileCondition is not met", async () => {
+    const operation = anOperationGoodAtAttemptNth(2);
+    const retriable = withRetry({
+      maxAttempts: 3,
+      whileCondition: () => false
+    })(operation);
+    const result = retriable();
+
+    try {
+      await result;
+      fail("expected to throw");
+    } catch (error) {
+      expect(error).toEqual(aBadResult);
+      expect(operation).toBeCalledTimes(1);
+    }
+  });
+
+  it("should wait between invocations if a delay is provided", async () => {
+    const operation = anAlwaysBadOperation;
+    const waitFor = 1000;
+    const retriable = withRetry({ delayMS: waitFor })(operation);
+    const _ = retriable();
+
+    expect(operation).toBeCalledTimes(1);
+    await sleep(waitFor * 1.01);
+
+    expect(operation).toBeCalledTimes(2);
+  });
+});

utils/apim.ts

@@ -18,6 +18,7 @@ import {
   ResponseErrorNotFound
 } from "@pagopa/ts-commons/lib/responses";
 import { parse } from "fp-ts/lib/Json";
+import { RestError } from "@azure/ms-rest-js";
 export interface IServicePrincipalCreds {
   readonly clientId: string;
   readonly secret: string;
@@ -169,3 +170,27 @@ export const getSubscription = (
     ),
     chainApimMappedError
   );
+/**
+ * APIM sdk operations may raise untyped errors.
+ * This utility check if the error is of a specific status code
+ *
+ * @param error any error coming from an APIM sdk call
+ * @param statusCode status code to check against
+ * @returns whether the returned error is of that status code
+ */
+export const isErrorStatusCode = (
+  error: unknown,
+  statusCode: number
+): boolean => {
+  if (error === null) {
+    return false;
+  }
+  if (!(error instanceof RestError)) {
+    return false;
+  }
+  if (!error.statusCode) {
+    return false;
+  }
+
+  return error.statusCode === statusCode;
+};

utils/retry.ts

@@ -0,0 +1,41 @@
+/**
+ * Given an async operation, it retries the operation until
+ * the retry conditions are met and maximum attempts are fulfilled.
+ *
+ * An optional delay in milliseconds can be waited between attempts.
+ *
+ * @param operation the operation to be executed
+ * @param whileCondition stop retries when false. Default: alway true
+ * @param maxAttempts max total calls to operation. Default: 3
+ * @param delayMS delay between invocations. Default: 100ms
+ * @returns
+ */
+
+export const withRetry = ({
+  whileCondition = (_: unknown): true => true,
+  maxAttempts = 3,
+  delayMS = 100
+}: {
+  readonly whileCondition?: (failure: unknown) => boolean;
+  readonly maxAttempts?: number;
+  readonly delayMS?: number;
+} = {}) => <T>(operation: () => Promise<T>) => async (): Promise<T> => {
+  // eslint-disable-next-line functional/no-let
+  let remainingAttempts = maxAttempts;
+  // eslint-disable-next-line no-constant-condition
+  while (true) {
+    try {
+      return await operation();
+    } catch (error) {
+      remainingAttempts--;
+
+      if (!remainingAttempts || !whileCondition(error)) {
+        throw error;
+      }
+
+      if (delayMS) {
+        await new Promise(done => setTimeout(done, delayMS));
+      }
+    }
+  }
+};