feat(#575): retry failed HTTP requests #583

m5r · 2023-10-25T09:42:05Z

Description

Leverage async-retry to retry failed HTTP requests

#575

Code review items

Readable: Concise, well named, follows the style guide, documented if necessary.
Documented: Configuration and user documentation on cht-docs
Tested: Unit and/or integration tests where appropriate
Backwards compatible: Works with existing data and configuration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

jkuester

I concur with Gareth's comment that this is probably a more viable approach than #590.

The implementation here looks great! I had a few questions regarding the tests, particular around trying to avoid having to increase the timeone on so many tests. Once those are resolved this is good to go!

jkuester · 2023-12-01T15:53:15Z

src/lib/api.js

@@ -7,6 +8,13 @@ const url = require('url');

 const cache = new Map();

+const _request = (method) => (...args) => retry(() => rpn[method](...args), { retries: 5, randomize: false, factor: 1.5 });


Just curious, but what was the reason for factor: 1.5 instead of just using the default 2? I guess this will make it retry faster?

Correct, the default config chooses a random factor between 1 and 2 for each retry. Worst case scenario was making the user wait >60 seconds when all 6 requests failed so making it retry faster and consistent across failures with no random made more sense

jkuester · 2023-12-01T15:59:46Z

test/lib/api.spec.js

-      mockRequest.onCall(0).resolves([]);
-      mockRequest.onCall(1).rejects({ error: 'random' });
+      mockRequest.get.onCall(0).resolves([]);
+      mockRequest.put.onCall(0).resolves({ ok: true });


Should this actually reject with an error? (That is what the test was originally doing....)

I think this test might have unintentionally been broken since there is no expect.fail at the end of the try-block to make sure an exception was actually thrown...

jkuester · 2023-12-01T16:06:04Z

test/lib/warn-upload-overwrite.spec.js

+    api = rewire('../../src/lib/api');
+    warnUploadOverwrite = rewire('../../src/lib/warn-upload-overwrite');


Why do we need to rewire these again here in the afterEach?

That's the fun part of my latest problems with cht-conf tests...

In https://github.com/medic/cht-conf/blob/575-retry-on-failure/src/lib/api.js#L9 you will notice this on line 9 const cache = new Map();.
This map basically caches some common responses from the API that can be reused between cht-conf commands like how some things in couch are configured. It took me a while to figure this out but this cache variable wouldn't get reset across tests, the map was still populated with the API response the first test had cached and it was causing tests in this file to fail.
They were not failing before because the API response mocks were incomplete and it didn't matter much because the command could keep going without this information. But with the retry mechanism, a lack of API response means it gets retried a couple times before moving on and that made a lot of tests run over the timeout.

Back to your question, we need to rewire these because we need to reset the cache variable. I tried with just api.__set__('cache', new Map()); in the beforeEach but it wasn't enough to reset it. So we rewire api and then we rewire warnUploadOverwrite to give it the newly rewired api.

test/lib/warn-upload-overwrite.spec.js

jkuester · 2023-12-01T16:42:12Z

test/fn/watch-project.spec.js

+    api.giveResponses(
+      {
+        status: 200,
+        body: { version: '3.5.0' },
+      },
+    );


Why did you add this? The tests run fine for me locally without it....

It times out locally without this mocked response, my guess is it's because it calls const version = await getValidApiVersion(); on line 171 https://github.com/medic/cht-conf/blob/575-retry-on-failure/src/fn/upload-custom-translations.js#L171

jkuester · 2023-12-01T17:00:49Z

test/fn/create-users.spec.js

@@ -12,7 +12,9 @@ const userPrompt = rewire('../../src/lib/user-prompt');
 const readLine = require('readline-sync');
 const mockTestDir = testDir => sinon.stub(environment, 'pathToProject').get(() => testDir);

-describe('create-users', () => {
+describe('create-users', function () {
+  this.timeout(15000);


So, I understand that these tests are kind of all over the place in terms of how they are a wierd mix of unit and integration tests (hitting the api stub). However, having all these various tests trip the async-retry logic is super inefficient and that time is wasted on every test run.

To me, it seems that the 'proper' fix would be to actually separate the tests out so that we have a few integration tests that call through to the api stub, but the rest are unit tests that just hit a mock of the api.js (and never actually make any HTTP calls at all...). But, I don't know that we want to try and take a big refactor of the tests on at this point. So, here is my compromise suggestion:

Do you think it makes sense in these tests to override the request property in the api.js (similar to what you did in api.spec.js), but for these tests just have it directly call rpn instead of wrapping it in the retry? That should allow these tests to continue functioning as they did originally, but without needing the extra time/setup for a bunch of retries....

I agree with you here, I was a bit disappointed to cause the tests to take so much longer (going from ~1 minute to ~7 minutes because of the repeated timeouts) but I think I've found a reasonable compromise.
I went ahead and overrode request to directly all rpn in the related tests and I added a few tests to cover the retry mechanism in the api tests that will confidently tell us if we happen to break this feature

…reaking change

…ranslations for multiple languages` conflicting with `request` mock in `warn-upload-overwrite > prompts when attempting to overwrite docs > shows diff when local is different from remote and the user requests a diff`

…lations > medic-3.x > 3.0.0` conflicting with `request` mock in `warn-upload-overwrite > prompts when attempting to overwrite docs > shows diff when local is different from remote and the user requests a diff`

medic-ci · 2023-12-06T17:09:17Z

🎉 This PR is included in version 3.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

m5r force-pushed the 575-retry-on-failure branch from 6b4fffb to 3f280f0 Compare November 15, 2023 17:28

garethbowen mentioned this pull request Nov 23, 2023

Use AuthSession cookie for login instead of basic auth #582

Open

m5r force-pushed the 575-retry-on-failure branch 6 times, most recently from 2e543f9 to 0866938 Compare November 28, 2023 14:48

m5r mentioned this pull request Nov 29, 2023

feat(#575): retry failed commands #590

Closed

m5r marked this pull request as ready for review November 29, 2023 22:47

m5r requested review from kennsippell and jkuester November 29, 2023 22:53

m5r changed the title ~~Retry failed requests~~ feat(#575): retry failed HTTP requests Nov 29, 2023

garethbowen mentioned this pull request Nov 29, 2023

add retry to CHT Conf #575

Closed

jkuester approved these changes Dec 1, 2023

View reviewed changes

m5r added 15 commits December 6, 2023 17:59

try with async-retry, 0 retries to not break tests, should not be a b…

7535721

…reaking change

fix api tests

ff5532e

fix warn-upload-overwrite tests

e1dbf7b

fix api tests - baseline working with 0 retries

0ce6cd3

remove console log

53e248b

halfway there

26436df

one mo'

0cf78b6

done fixing create-users tests

166c210

done fixing watch-project tests

83ca2da

quick n dirty to see whether the CI passes too

af6b017

done fixing upload-custom-translations tests

2e9b538

fix extra long timeout

9fffe82

fix upload-custom-translations.spec.js

a248bac

try skipping watch-project.spec.js

b221f10

m5r added 6 commits December 6, 2023 18:00

fix upload-custom-translations > medic-2.x and `upload-custom-trans…

969b5b5

…lations > medic-3.x > 3.0.0` conflicting with `request` mock in `warn-upload-overwrite > prompts when attempting to overwrite docs > shows diff when local is different from remote and the user requests a diff`

fix rewire not overriding cache in api.js

52c2084

temp skipping watch project

c6f666c

temp

5642374

bypass the retry mechanism on HTTP requests in tests

74bb264

test api retry mechanism

49fbd94

m5r force-pushed the 575-retry-on-failure branch from 83dc4e5 to 49fbd94 Compare December 6, 2023 17:01

regenerate lock file

2a62dc3

m5r merged commit bc5b0bc into main Dec 6, 2023
12 checks passed

m5r deleted the 575-retry-on-failure branch December 6, 2023 17:08

medic-ci added the released label Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(#575): retry failed HTTP requests #583

feat(#575): retry failed HTTP requests #583

m5r commented Oct 25, 2023 •

edited

Loading

jkuester left a comment

jkuester Dec 1, 2023

m5r Dec 5, 2023

jkuester Dec 1, 2023

jkuester Dec 1, 2023

m5r Dec 5, 2023

jkuester Dec 1, 2023

m5r Dec 5, 2023

jkuester Dec 1, 2023

m5r Dec 5, 2023

medic-ci commented Dec 6, 2023

		@@ -7,6 +8,13 @@ const url = require('url');

		const cache = new Map();

		const _request = (method) => (...args) => retry(() => rpn[method](...args), { retries: 5, randomize: false, factor: 1.5 });

		api = rewire('../../src/lib/api');
		warnUploadOverwrite = rewire('../../src/lib/warn-upload-overwrite');

feat(#575): retry failed HTTP requests #583

feat(#575): retry failed HTTP requests #583

Conversation

m5r commented Oct 25, 2023 • edited Loading

Description

Code review items

License

jkuester 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

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

medic-ci commented Dec 6, 2023

m5r commented Oct 25, 2023 •

edited

Loading