feat: add exponential retries on merge errors

[ricardoatsouza]

This Pull Request fulfills the following requirements:

• The commit message follows our guidelines.
• Tests for the changes have been added if needed.
• Does not affect documentation or it has been added or updated.

Resolves

Add an exponential retry on merge error, basically retrying up to three times (1 second after the first failure, then 4 seconds later and, finally, again 9 seconds later). If all three fail, an error is thrown.

[github-actions]

Pull Request Test Coverage Report for Build 194556948

• 25 of 25 (100.0%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+1.3%) to 96.313%

---

Totals
Change from base Build 189737946:	1.3%
Covered Lines:	141
Relevant Lines:	145

---

💛 - Coveralls

[viestat]

Why is the distfolder removed? As far as I know, GitHub Actions do not work with TypeScript. Did something changed?

[viestat]

Looks good the only thing I am missing is the README changes, there we can describe how to configure the number of retires.

[ricardoatsouza]

Looks good the only thing I am missing is the README changes, there we can describe how to configure the number of retires.

README.MD updated in c97bfc0.

[JrSchild]

See comment

[merlinnot]

Only some errors should be retried, I don't think we should retry in all cases. As an example, the "out of sync" error is not retriable, as the branch must be rebased. Some other errors, such as the one originally reported, are cause by GitHub not being done processing yet. This should be retried.

[JrSchild]

One small comment. This is indeed better with .times(3). Also see Natan's review.

[JrSchild]

Also we should overwrite Date.now or something similar to speed up the tests. We should never actually be backing off that long during the test.

[ricardoatsouza]

@JrSchild @merlinnot

Added parameter to speed up tests: ef0cedc

Retrying only on the reported error: 7cf6771 ( appreciate feedback on this one)

[JrSchild]

@ricardoatsouza can you please revert #ef0cedc3674f849c0f0120c0d0d990d2f28537f1. You are exposing an internal implementation by changing the interface, but the option is never used in production. Also starting the backoff from 100ms instead is definitelly not the correct way to "speed up the test". Any timers that run longer than the an immediate interval should be avoided as much as possible. See our (internal) firestore-extensions package (retries.spec.ts) on how to properly test it.

Can you add a test that makes sure backoff does not happen if shouldRetry does not match the error?

[ricardoatsouza]

@ricardoatsouza can you please revert #ef0cedc3674f849c0f0120c0d0d990d2f28537f1. You are exposing an internal implementation by changing the interface, but the option is never used in production. Also starting the backoff from 100ms instead is definitelly not the correct way to "speed up the test". Any timers that run longer than the an immediate interval should be avoided as much as possible. See our (internal) firestore-extensions package (retries.spec.ts) on how to properly test it.

It's not the wrong way either. Regardless:

• Removed in 2032586
• Using mock in aabf783

Can you add a test that makes sure backoff does not happen if shouldRetry does not match the error?

Added in d5400ab