feat(node): add exponential retry mechanism with idempotency headers #4462

michaldziuba03 · 2023-10-09T05:11:09Z

What change does this PR introduce?

This PR introduces exponential retry mechanism with idempotency headers for Node SDK.

PR contains:

adding idempotency key header for each POST and PATCH request
configurable exponential retry mechanism
tests

Why was this change needed?

Closes #4452

Other information (Screenshots)

packages/node/src/lib/novu.ts

michaldziuba03 · 2023-10-10T23:25:23Z

@Cliftonz I'm wondering about the retry condition. I think it should be clearly defined in requirements.

What I plan to do:

for idempotent http methods SDK will retry on 50x errors + network errors without response. I will not apply Idempotency-Key header for these idempotent methods.

for non-idempotent http methods SDK will retry only on network errors without response / maybe on 409 Conflict. I think most 50x errors will be replayed from the cache anyway, so request errored with 50x status shouldn't be retried.

dammy001 · 2023-10-11T07:12:36Z

@Cliftonz I'm wondering about the retry condition. I think it should be clearly defined in requirements.

What I plan to do:

for idempotent http methods SDK will retry on 50x errors + network errors without response. I will not apply Idempotency-Key header for these idempotent methods.

for non-idempotent http methods SDK will retry only on network errors without response / maybe on 409 Conflict. I think most 50x errors will be replayed from the cache anyway, so request errored with 50x status shouldn't be retried.

And also, there should be an option for setting retryCondition callback should from userland.

Cliftonz · 2023-10-11T11:33:58Z

@Cliftonz I'm wondering about the retry condition. I think it should be clearly defined in requirements.

What I plan to do:

for idempotent http methods SDK will retry on 50x errors + network errors without response. I will not apply Idempotency-Key header for these idempotent methods.

for non-idempotent http methods SDK will retry only on network errors without response / maybe on 409 Conflict. I think most 50x errors will be replayed from the cache anyway, so request errored with 50x status shouldn't be retried.

Just to make sure we are on the same page, these should be configurable by the client.
By default we want RetryMax, WaitMin, WaitMax , and InitialDelay to be resonable values like
RetryMax: 0
Waitmin: 1
Waitmax: 2
InitialDelay: 0.5

As for applying the the idempotency header, you should only do it to POST and PATCH methods however we can add it to all calls its just not preferred.

As for which errors that should be retried, not all errors are considered equal and here are the Errors that should be considered for retrying:
- 408 Request Timeout
- 429 Too Many Requests
- 500 Internal Server Error
- 502 Bad Gateway
- 503 Service Unavailable
- 504 Gateway Timeout

Errors that generally should not be retried without addressing the cause:
- 400 Bad Request
- 401 Unauthorized
- 403 Forbidden
- 404 Not Found
- 405 Method Not Allowed
- 413 Payload Too Large
- 414 URI Too Long
- 415 Unsupported Media Type
- 422 Unprocessable Entity

Cliftonz · 2023-10-11T11:34:43Z

@michaldziuba03 I also agree with @dammy001 we should provide a callback for the user to dynamically select the condition if they so want.

packages/node/src/index.ts

michaldziuba03 · 2023-10-12T22:59:14Z

packages/node/src/lib/retries.ts

+  axios.interceptors.request.use((axiosConfig) => {
+    // don't attach idempotency key for idempotent methods
+    if (
+      axiosConfig.method &&
+      !NON_IDEMPOTENT_METHODS.includes(axiosConfig.method)
+    ) {
+      return axiosConfig;
+    }
+
+    const idempotencyKey = axiosConfig.headers['Idempotency-Key'];
+    // that means intercepted request is retried, so don't generate new idempotency key
+    if (idempotencyKey) {
+      return axiosConfig;
+    }
+
+    axiosConfig.headers['Idempotency-Key'] = uuid();
+
+    return axiosConfig;
+  });


I'm using axios interceptor to attach idempotency key header to each request.

Idempotency key is attached only to POST and PATCH methods because they are not idempotent by definition.

michaldziuba03 · 2023-10-12T23:12:57Z

packages/node/src/lib/retries.ts

+  // retry on TCP/IP error codes like ECONNRESET
+  if (isNetworkError(err)) {
+    return true;
+  }


isNetworkError is imported from axios-retry library.

As we know HTTP is based on TCP/IP protocol and sometimes we facing issues on this network level.

TCP/IP error typically contains some code like "ECONNRESET" and isNetworkError uses under the hood library is-retry-allowed, because not every network error should be retried.

michaldziuba03 · 2023-10-12T23:20:38Z

packages/node/src/lib/retries.ts

+  if (err.response.status >= 500 && err.response.status <= 599) {
+    return true;
+  }
+
+  if (RETRIABLE_HTTP_CODES.includes(err.response.status)) {
+    return true;
+  }


On HTTP level, I decided to retry on every 5xx error, 408 (Request Timeout) and 429 (Too Many Requests)

You should also retry on 422 but regen the idempotency key

packages/node/src/lib/retries.spec.ts

michaldziuba03 · 2023-10-12T23:31:04Z

packages/node/package.json

@@ -77,7 +81,7 @@
    "preset": "ts-jest",
    "testEnvironment": "node",
    "moduleNameMapper": {
-      "axios": "axios/dist/node/axios.cjs"
+      "^axios$": "axios/dist/node/axios.cjs"


without that change, tests for retries don't work.

I observed strange behavior - it sends GET request to 127.0.0.1:80 even when you send POST request to baseUrl, it messes up the request config.

change to ^axios$ fixed that issue. This is also recommendation inside nock readme for axios.

I also tested manually using symlink in testing repo's node_modules and retries work as expected.

Cliftonz · 2023-10-12T23:33:36Z

@michaldziuba03 Great overview of the PR!

michaldziuba03 · 2023-10-13T01:51:04Z

packages/node/src/lib/retries.ts

+    onRetry(_retryCount, error, requestConfig) {
+      if (
+        error.response?.status === 422 &&
+        requestConfig.headers &&
+        requestConfig.method &&
+        NON_IDEMPOTENT_METHODS.includes(requestConfig.method)
+      ) {
+        requestConfig.headers['Idempotency-Key'] = uuid();
+      }
+    },
+  });


@Cliftonz regenerating idempotency key for HTTP status 422 Unprocessable Content

michaldziuba03 · 2023-10-13T01:56:58Z

@Cliftonz @scopsy PR is ready for judgment :)

packages/node/package.json

packages/node/src/lib/retries.spec.ts

packages/node/src/lib/novu.ts

packages/node/src/lib/retries.spec.ts

rifont · 2023-10-25T17:03:46Z

packages/node/src/lib/retries.spec.ts

+    const result = await novuClient.topics.list({});
+    expect(result.status).toEqual(200);
+  });
+});


Nice test 🙌

rifont · 2023-10-25T17:05:54Z

packages/node/src/lib/retries.spec.ts

+    expect(result.request.headers['idempotency-key']).toBeUndefined();
+  });
+
+  it('should not retry on common 4xx errors', async () => {


Nit: common isn't quite the correct term here, non-recoverable would be more descriptive.

Secondly, this is a good case for using a jest loop to test all relevant 4xx errors. It will make the tests stronger and more declarative.

rifont · 2023-10-25T18:33:08Z

packages/node/src/lib/retries.spec.ts

+
+    expect(result.status).toEqual(200);
+    expect(result.request.headers['idempotency-key']).toBeUndefined();
+  });


This test is great. Could we add another test using the same format with times(4) and verify that a 500 is received?

packages/node/src/lib/retries.ts

rifont · 2023-10-25T18:47:26Z

packages/node/src/lib/retries.ts

+
+  if (!err.response) {
+    return true;
+  }


It's not clear why we're retrying when a response is unavailable. Could you please shed some light on this one?

I think it's mistake (should return false) - if statements after this check are dependent on err.response (possibly undefined). In new changes I made it look cleaner.

michaldziuba03 · 2023-11-03T03:01:44Z

packages/node/src/lib/retry.spec.ts

+  it('should generate different idempotency-key for each request', async () => {
+    nock(BACKEND_URL)
+      .post(TRIGGER_PATH)
+      .reply(500, { message: 'Server Exception' });
+
+    nock(BACKEND_URL)
+      .post(TRIGGER_PATH)
+      .times(10)
+      .reply(201, { acknowledged: true, transactionId: '1003' });
+
+    const idempotencyKeys: string[] = [];
+
+    for (let i = 0; i < 10; i++) {
+      const result = await novu.trigger('fake-workflow', {
+        to: { subscriberId: '123' },
+        payload: {},
+      });
+
+      idempotencyKeys.push(result.request?.headers['idempotency-key']);
+    }
+
+    expect(allEqual(idempotencyKeys)).toEqual(false);
+  });


new test case - I want to make sure that idempotency key is different between requests

Nice catch 🎉

packages/node/src/lib/retry.spec.ts

michaldziuba03 · 2023-11-03T03:11:59Z

packages/node/src/lib/retry.ts

+
+const NON_IDEMPOTENT_METHODS = ['post', 'patch'];
+const IDEMPOTENCY_KEY = 'Idempotency-Key'; // header key
+
+const DEFAULT_RETRY_MAX = 0;
+const DEFAULT_WAIT_MIN = 1;
+const DEFAULT_WAIT_MAX = 30;
+


new global variables - for default values as suggested in review + I decided to move string value for header key to IDEMPOTENCY_KEY variable (to avoid "magic strings" as it's used in multiple places)

michaldziuba03 · 2023-11-05T19:23:48Z

@rifont

rifont · 2023-11-07T09:33:15Z

packages/node/src/lib/retry.spec.ts

+
+jest.setTimeout(15000);
+
+const allEqual = (arr: Array<string>) => arr.every((val) => val === arr[0]);


Suggested change

const allEqual = (arr: Array<string>) => arr.every((val) => val === arr[0]);

const hasUniqueOnly = (arr: Array<string>) => Array.from(new Set(arr)).length === arr.length);

Consider the case where every array item except the first is the same, like [1,2,2,2,2]. This function will currently return true, because it's only comparing entries against the first item, not all of the entries.

So we'll need to instead verify that the number of unique entries in the array is equal to the number of total entries. new Set(Array<any>) constructs a Set comprising unique only values. We can then transform the Set to an Array for a length comparison. See https://stackoverflow.com/a/14438954 for reference.

We should also use the convention of is/has (is for singular test against a primitive, has for plural test, like an array) prefix for a boolean function, to provide a better description for the result.

Lastly, it's generally a good idea to also add tests for helper functions. If these helper functions aren't working as expected, how can we be sure that the feature under test is too?

I disagree with second sentence but I understand where problem is. Function allEqual (currently renamed to hasAllEqual) works well when our goal is to check if all items are equal (it iterates over all elements and compares them to the first element, it also compares first element with itself). This is the goal when we check in tests if idempotency-keys are equal between retries (in the scope of same request).

I agree this function is used in wrong way when we check if idempotency-keys are unique between requests - we expect allEqual to return false. Negation of this function does not guarantee that all items are unique, so this expectation will pass in case of [1,2,2,2,2] scenario, but shouldn't.

I decided to let them coexist :)

rifont · 2023-11-07T09:37:05Z

packages/node/src/lib/retry.spec.ts

+    await expect(novu.topics.list({})).rejects.toMatchObject({
+      response: { status: 500 },
+    });
+  });


Great job 👏

rifont · 2023-11-07T09:37:32Z

packages/node/src/lib/retry.spec.ts

+  it('should generate different idempotency-key for each request', async () => {
+    nock(BACKEND_URL)
+      .post(TRIGGER_PATH)
+      .reply(500, { message: 'Server Exception' });
+
+    nock(BACKEND_URL)
+      .post(TRIGGER_PATH)
+      .times(10)
+      .reply(201, { acknowledged: true, transactionId: '1003' });
+
+    const idempotencyKeys: string[] = [];
+
+    for (let i = 0; i < 10; i++) {
+      const result = await novu.trigger('fake-workflow', {
+        to: { subscriberId: '123' },
+        payload: {},
+      });
+
+      idempotencyKeys.push(result.request?.headers['idempotency-key']);
+    }
+
+    expect(allEqual(idempotencyKeys)).toEqual(false);
+  });


Nice catch 🎉

packages/node/src/lib/retry.ts

rifont

Great job @michaldziuba03 🙌

rifont · 2023-11-20T08:40:26Z

packages/node/src/lib/retry.spec.ts

+  describe('defaultRetryCondition function', () => {
+    test.each<[number, string]>(NON_RECOVERABLE_ERRORS)(
+      'should return false when HTTP status is %i',
+      (status) => {
+        const err = new HttpError(status);
+        expect(defaultRetryCondition(err as AxiosError)).toEqual(false);
+      }
+    );
+
+    test.each<number>(RETRYABLE_HTTP_CODES)(
+      'should return true when HTTP status is %i',
+      (status) => {
+        const err = new HttpError(status);
+        expect(defaultRetryCondition(err as AxiosError)).toEqual(true);
+      }
+    );
+
+    it('should return true when HTTP status is 500', () => {
+      const err = new HttpError(500);
+      expect(defaultRetryCondition(err as AxiosError)).toEqual(true);
+    });
+
+    it('should return true when network code is ECONNRESET', () => {
+      const err = new NetworkError('ECONNRESET');
+      expect(defaultRetryCondition(err as AxiosError)).toEqual(true);
+    });
+
+    it('shoud return false on unknown error', () => {
+      const err = new Error('Unexpected error');
+      expect(defaultRetryCondition(err as AxiosError)).toEqual(false);
+    });
+  });
+
+  describe('hasAllEqual helper function', () => {
+    it('should return true when all items are equal', () => {
+      const arr = ['a', 'a', 'a', 'a'];
+      expect(hasAllEqual(arr)).toEqual(true);
+    });
+
+    it('should return false when items are not equal', () => {
+      const arr = ['a', 'b', 'b', 'b'];
+      expect(hasAllEqual(arr)).toEqual(false);
+    });
+  });
+
+  describe('hasUniqueOnly helper function', () => {
+    it('should return true when all items are unique', () => {
+      const arr = ['a', 'b', 'c', 'd'];
+      expect(hasUniqueOnly(arr)).toEqual(true);
+    });
+
+    it('should return false when items are not unique', () => {
+      const arr = ['a', 'a', 'c', 'd'];
+      expect(hasUniqueOnly(arr)).toEqual(false);
+    });
+  });
+});


Excellent tests 👍

hey @michaldziuba03 , just a few spellcheck issues to resolve then we are good to merge 🙌

@rifont done

michaldziuba03 · 2023-11-30T19:49:48Z

@rifont we are waiting for something before merge? Spellcheck issues were resolved week ago.

Let me know if there is something else to do.

rifont · 2023-12-01T11:57:27Z

@rifont we are waiting for something before merge? Spellcheck issues were resolved week ago.

Let me know if there is something else to do.

Hi @michaldziuba03 , we had some failing tests on next that we were seeking to fix. However we will now fix these separately. I will get this merged today. Thanks!

feat(node): add basic retry mechanism

41d77fd

github-actions bot added the community label Oct 9, 2023

dammy001 reviewed Oct 9, 2023

View reviewed changes

packages/node/src/lib/novu.ts Outdated Show resolved Hide resolved

michaldziuba03 added 2 commits October 11, 2023 23:51

feat: improve retry and add basic tests

b66965a

feat(node): add retry condition callback and more tests

503257c

michaldziuba03 commented Oct 12, 2023

View reviewed changes

packages/node/src/index.ts Outdated Show resolved Hide resolved

michaldziuba03 commented Oct 12, 2023

View reviewed changes

packages/node/src/lib/retries.spec.ts Outdated Show resolved Hide resolved

michaldziuba03 commented Oct 12, 2023

View reviewed changes

feat(node): regenerate idempotency key on status 422

7475e22

michaldziuba03 commented Oct 13, 2023

View reviewed changes

michaldziuba03 marked this pull request as ready for review October 13, 2023 01:55

dammy001 reviewed Oct 13, 2023

View reviewed changes

packages/node/package.json Show resolved Hide resolved

Cliftonz requested a review from rifont October 17, 2023 17:46

mayorJAY mentioned this pull request Oct 18, 2023

[Draft]: Added Exponential Retry Mechanism with Idempotency Headers novuhq/novu-java#92

Draft

dammy001 reviewed Oct 20, 2023

View reviewed changes

packages/node/src/lib/retries.spec.ts Outdated Show resolved Hide resolved

test(node): add cases for common 4xx errors

6670e32

rifont requested changes Oct 25, 2023

View reviewed changes

michaldziuba03 added 2 commits November 2, 2023 23:09

fix: merge and resolve git conflicts

41296c2

feat(node): improve retries feature

071d59f

michaldziuba03 commented Nov 3, 2023

View reviewed changes

packages/node/src/lib/retry.spec.ts Show resolved Hide resolved

michaldziuba03 commented Nov 3, 2023

View reviewed changes

michaldziuba03 requested a review from rifont November 3, 2023 03:13

Merge branch 'next' into nv-3009-add-exponential-retry

1323c04

Merge branch 'next' into nv-3009-add-exponential-retry

d70b9f8

rifont requested changes Nov 7, 2023

View reviewed changes

rifont and others added 3 commits November 7, 2023 10:19

Merge branch 'next' into nv-3009-add-exponential-retry

bf68b2d

Merge branch 'next' into nv-3009-add-exponential-retry

3474d5a

test(node): add more tests for retries feature

a3e150d

michaldziuba03 requested a review from rifont November 9, 2023 04:18

michaldziuba03 and others added 2 commits November 11, 2023 18:32

Merge branch 'novuhq:next' into nv-3009-add-exponential-retry

2d548b4

Merge branch 'next' into nv-3009-add-exponential-retry

208d803

rifont approved these changes Nov 20, 2023

View reviewed changes

rifont and others added 8 commits November 20, 2023 08:41

Merge branch 'next' into nv-3009-add-exponential-retry

bbcbb80

fix: retryable to cspell

024d8cd

Merge branch 'next' into nv-3009-add-exponential-retry

60a000d

Merge branch 'next' into nv-3009-add-exponential-retry

8479ad0

Merge branch 'next' into nv-3009-add-exponential-retry

b773f3a

Merge branch 'next' into nv-3009-add-exponential-retry

01649c2

Merge branch 'next' into nv-3009-add-exponential-retry

fe525ae

Merge branch 'next' into nv-3009-add-exponential-retry

c75140c

Merge branch 'next' into nv-3009-add-exponential-retry

1e8c1b5

rifont merged commit 4ea758c into novuhq:next Dec 1, 2023
24 of 26 checks passed

michaldziuba03 deleted the nv-3009-add-exponential-retry branch December 1, 2023 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(node): add exponential retry mechanism with idempotency headers #4462

feat(node): add exponential retry mechanism with idempotency headers #4462

michaldziuba03 commented Oct 9, 2023 •

edited

Loading

michaldziuba03 commented Oct 10, 2023 •

edited

Loading

dammy001 commented Oct 11, 2023

What I plan to do:

Cliftonz commented Oct 11, 2023

What I plan to do:

Cliftonz commented Oct 11, 2023

michaldziuba03 Oct 12, 2023

michaldziuba03 Oct 12, 2023 •

edited

Loading

michaldziuba03 Oct 12, 2023

Cliftonz Oct 12, 2023

michaldziuba03 Oct 12, 2023

Cliftonz commented Oct 12, 2023

michaldziuba03 Oct 13, 2023 •

edited

Loading

michaldziuba03 commented Oct 13, 2023

rifont Oct 25, 2023

rifont Oct 25, 2023

rifont Oct 25, 2023

rifont Oct 25, 2023

rifont Oct 25, 2023

michaldziuba03 Nov 3, 2023

michaldziuba03 Nov 3, 2023

rifont Nov 7, 2023

michaldziuba03 Nov 3, 2023 •

edited

Loading

michaldziuba03 commented Nov 5, 2023

rifont Nov 7, 2023

michaldziuba03 Nov 9, 2023

rifont Nov 7, 2023

rifont Nov 7, 2023

rifont left a comment

rifont Nov 20, 2023

rifont Nov 20, 2023

michaldziuba03 Nov 20, 2023

michaldziuba03 commented Nov 30, 2023

rifont commented Dec 1, 2023


		jest.setTimeout(15000);

		const allEqual = (arr: Array<string>) => arr.every((val) => val === arr[0]);

	const allEqual = (arr: Array<string>) => arr.every((val) => val === arr[0]);
	const hasUniqueOnly = (arr: Array<string>) => Array.from(new Set(arr)).length === arr.length);

feat(node): add exponential retry mechanism with idempotency headers #4462

feat(node): add exponential retry mechanism with idempotency headers #4462

Conversation

michaldziuba03 commented Oct 9, 2023 • edited Loading

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

michaldziuba03 commented Oct 10, 2023 • edited Loading

What I plan to do:

dammy001 commented Oct 11, 2023

What I plan to do:

Cliftonz commented Oct 11, 2023

What I plan to do:

Cliftonz commented Oct 11, 2023

Choose a reason for hiding this comment

michaldziuba03 Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cliftonz commented Oct 12, 2023

michaldziuba03 Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

michaldziuba03 commented Oct 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaldziuba03 Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

michaldziuba03 commented Nov 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rifont left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaldziuba03 commented Nov 30, 2023

rifont commented Dec 1, 2023

michaldziuba03 commented Oct 9, 2023 •

edited

Loading

michaldziuba03 commented Oct 10, 2023 •

edited

Loading

michaldziuba03 Oct 12, 2023 •

edited

Loading

michaldziuba03 Oct 13, 2023 •

edited

Loading

michaldziuba03 Nov 3, 2023 •

edited

Loading