refactor: update retry function

[FaybianB]

Motivation

Update retry function.

Description

Updates the retry function to follow a more common pattern used in other libraries such as retry and retry-request, which, if retries=1 it will call the function and on failure retry it once.

Resolves #6447

Closes #6447

[CLAassistant]

All committers have signed the CLA.

[nflaig]

@FaybianB Thanks for looking into this. The change looks good to me, but it would be good to check all callers of this function and see if we need to reduce retries passed by 1. I would expect it doesn't matter much for most operation but getting an overview would be great.

[FaybianB]

@nflaig Here are all the functions I've found that call the retry function. A lot of the supplied retry values are 20 and 60. Do you think any of these should be subtracted by 1?:

• lodestar/packages/api/src/utils/client/httpClient.ts

  Lines 187 to 192 in c7c9941

  	return retry(
  	async (_attempt) => {
  	return this.requestWithBodyWithFallbacks<T>(opts, getBody);
  	},
  	{retries: opts.retryAttempts, retryDelay: 200}
  	);

• lodestar/packages/cli/test/utils/validator.ts

  Line 64 in c7c9941

  await retry(() => keymanagerClient.listRemoteKeys(), {retryDelay: 500, retries: 20});

• lodestar/packages/cli/test/e2e/runDevCmd.test.ts

  Line 32 in c7c9941

  await retry(() => client.node.getHealth().then((res) => ApiError.assert(res)), {retryDelay: 1000, retries: 60});

• lodestar/packages/cli/test/e2e/voluntaryExit.test.ts

  Lines 47 to 54 in c7c9941

  	await retry(
  	async () => {
  	const head = await client.beacon.getBlockHeader("head");
  	ApiError.assert(head);
  	if (head.response.data.header.message.slot < 1) throw Error("pre-genesis");
  	},
  	{retryDelay: 1000, retries: 20}
  	);

• lodestar/packages/cli/test/e2e/voluntaryExit.test.ts

  Lines 80 to 92 in c7c9941

  	await retry(
  	async () => {
  	const res = await client.beacon.getStateValidator("head", pubkey);
  	ApiError.assert(res);
  	if (res.response.data.status !== "active_exiting") {
  	throw Error("Validator not exiting");
  	} else {
  	// eslint-disable-next-line no-console
  	console.log(`Confirmed validator ${pubkey} = ${res.response.data.status}`);
  	}
  	},
  	{retryDelay: 1000, retries: 20}
  	);

• lodestar/packages/utils/test/unit/retry.test.ts

  Line 43 in c7c9941

  expect(await retry(fn, opts)).toEqual(result);

• lodestar/packages/cli/test/e2e/voluntaryExitRemoteSigner.test.ts

  Lines 67 to 74 in c7c9941

  	await retry(
  	async () => {
  	const head = await client.beacon.getBlockHeader("head");
  	ApiError.assert(head);
  	if (head.response.data.header.message.slot < 1) throw Error("pre-genesis");
  	},
  	{retryDelay: 1000, retries: 20}
  	);

• lodestar/packages/cli/test/e2e/voluntaryExitRemoteSigner.test.ts

  Lines 95 to 107 in efb684a

  	await retry(
  	async () => {
  	const res = await client.beacon.getStateValidator("head", pubkey);
  	ApiError.assert(res);
  	if (res.response.data.status !== "active_exiting") {
  	throw Error("Validator not exiting");
  	} else {
  	// eslint-disable-next-line no-console
  	console.log(`Confirmed validator ${pubkey} = ${res.response.data.status}`);
  	}
  	},
  	{retryDelay: 1000, retries: 20}
  	);

• lodestar/packages/cli/test/e2e/voluntaryExitFromApi.test.ts

  Lines 54 to 61 in c7c9941

  	await retry(
  	async () => {
  	const head = await beaconClient.getBlockHeader("head");
  	ApiError.assert(head);
  	if (head.response.data.header.message.slot < 1) throw Error("pre-genesis");
  	},
  	{retryDelay: 1000, retries: 20}
  	);

• lodestar/packages/cli/test/e2e/voluntaryExitFromApi.test.ts

  Lines 81 to 93 in efb684a

  	await retry(
  	async () => {
  	const res = await beaconClient.getStateValidator("head", pubkeyToExit);
  	ApiError.assert(res);
  	if (res.response.data.status !== "active_exiting") {
  	throw Error("Validator not exiting");
  	} else {
  	// eslint-disable-next-line no-console
  	console.log(`Confirmed validator ${pubkeyToExit} = ${res.response.data.status}`);
  	}
  	},
  	{retryDelay: 1000, retries: 20}
  	);

• lodestar/packages/cli/test/e2e/blsToExecutionchange.test.ts

  Lines 45 to 52 in c7c9941

  	await retry(
  	async () => {
  	const head = await client.beacon.getBlockHeader("head");
  	ApiError.assert(head);
  	if (head.response.data.header.message.slot < 1) throw Error("pre-genesis");
  	},
  	{retryDelay: 1000, retries: 60}
  	);

• lodestar/packages/spec-test-util/src/downloadTests.ts

  Lines 69 to 93 in c7c9941

  	await retry(
  	// async (bail) => {
  	async () => {
  	const {data, headers} = await axios({
  	method: "get",
  	url,
  	responseType: "stream",
  	timeout: 30 * 60 * 1000,
  	});
  	const totalSize = headers["content-length"] as string;
  	log(`Downloading ${url} - ${totalSize} bytes`);
  	// extract tar into output directory
  	await promisify(stream.pipeline)(data, extractTar({cwd: outputDir}));
  	log(`Downloaded ${url}`);
  	},
  	{
  	retries: 3,
  	onRetry: (e, attempt) => {
  	log(`Download attempt ${attempt} for ${url} failed: ${e.message}`);
  	},
  	}
  	);

[nflaig]

Thanks @FaybianB for looking into it

We should be fine to keep the 20/60 as is, but might wanna update these

lodestar/packages/spec-test-util/src/downloadTests.ts

Line 88 in c7c9941

retries: 3,

lodestar/packages/beacon-node/src/execution/builder/http.ts

Line 121 in 0e9fc49

const res = await this.api.submitBlindedBlock(signedBlindedBlock, {retryAttempts: 3});

I think 3 requests in total should be sufficient here, and we keep previous behavior

[nflaig]

Just noticed we use external dependency in download tests instead of our own retry implementation. Good opportunity to clean up this dependency

lodestar/packages/spec-test-util/src/downloadTests.ts

Line 9 in c7c9941

import retry from "async-retry";

lodestar/packages/spec-test-util/package.json

Line 66 in 533db97

"async-retry": "^1.3.3",

Edit: Noticed this requires more changes, I will push a PR for this separately (see #6458)

[nflaig]

@FaybianB I noticed few more cases, we use the retry function in our http clients which are configured via retryAttempts, need to double check these as well

e.g. default value here should probably be changed to 2

lodestar/packages/beacon-node/src/execution/engine/http.ts

Line 75 in 6b01690

retryAttempts: 3,

and we need to make sure to set default value here to 0 (meaning no retries by default)

lodestar/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts

Line 169 in 6b01690

retries: opts?.retryAttempts ?? this.opts?.retryAttempts ?? 1,

(or maybe more elegant would be to not wrap call into retry function if opts?.retryAttempts ?? this.opts?.retryAttempts is not set / undefined

[FaybianB]

@nflaig I've updated the other areas referring to retryAttempts. Also, I've added an additional check in the retry function to check if we're on the last attempt and bypasses the shouldRetry logic if so:

https://github.com/ChainSafe/lodestar/pull/6451/files#diff-7425121b21e5d20801cc486ce948c1ceb550d27819e3fd95177b894c20114e4cR39-R43

I thought this was a cleaner solution than updating the retry tests to subtract 1 from the retryCount:

lodestar/packages/beacon-node/test/e2e/eth1/jsonRpcHttpClient.test.ts

Line 202 in d1dc217

expect(retryCount).toBeWithMessage(retryAttempts, "ENOTFOUND should be retried before failing");

Do you agree or do you think I should update the tests instead?

[codecov]

Codecov Report

Merging #6451 (ca7dfde) into unstable (0a12b1b) will increase coverage by 0.00%.
Report is 1 commits behind head on unstable.
The diff coverage is 87.50%.

Additional details and impacted files

@@            Coverage Diff            @@
##           unstable    #6451   +/-   ##
=========================================
  Coverage     61.63%   61.63%           
=========================================
  Files           553      553           
  Lines         57974    57998   +24     
  Branches       1834     1837    +3     
=========================================
+ Hits          35734    35749   +15     
- Misses        22203    22210    +7     
- Partials         37       39    +2     

[nflaig]

This used async-retry library, keeping retries as is, to not change previous behavior

[nflaig]

Do you agree or do you think I should update the tests instead?

Good catch! This makes sense to me, I noticed there is another case where checking if we are at max attempts is important because otherwise, we will also sleep one more time before throwing the last error which is not ideal.

I thought this was a cleaner solution than updating the retry tests to subtract 1 from the retryCount:

The tests are incorrect, incrementing retryCount in the last iteration is wrong as it will not do another retry. Also noticed tests use retryCount to track request count, pushed some updates to the test to fix this and make them pass.

[nflaig]

LGTM, thanks @FaybianB for the thoroughness!

[wemeetagain]

🎉 This PR is included in v1.17.0 🎉