Repository: urql-graphql/urql
Pull request #3478 (2024-01-17 12:48:34)

fix: retry delay not persisting between calls
<!--
  Thanks for opening a pull request! We appreciate your dedication and help!
  Before submitting your pull request, please make sure to read our CONTRIBUTING guide.

  The best contribution is always a PR, but please make sure to open an issue or discuss
  your changes first, if you’re looking to submit a larger PR.

  If this PR is already related to an issue, please reference it like so:
  Resolves #123
-->

## Summary

<!-- What's the motivation of this change? What does it solve? -->

Retry delays are not actually working as specified in the docs. Currently, retry delays are not persisted between retries, resulting in exponential backoff not actually increasing the delay amount as time goes on. The linear backoff also does not currently work.

I've added a unit test for this, as well as the change for the behavior to comply with what's actually stated in the docs.



## Set of changes

<!--
  Roughly list the changes you've made and which packages are affected.
  Leave some notes on what may be noteworthy files you've changed.
  And lastly, please let us know if you think this is a breaking change.
-->

- Made retry delays persist in the context, allowing backoffs to work
- Added unit test for retry delay
- Modified an existing unit test that was failing (I believe it might have been incorrect, but I'm not entirely sure, please verify! It seems like the retry value is meant to reset after a retry that was successful [so, a successful operation], but the unit test was checking the `retry` values after a failed/errored operation instead. However, the test was passing because the retry delays were never being carried across operations anyway.)

changeset-bot[bot]: ###  🦋  Changeset detected

Latest commit: fc5c7656bae1ac3ac92c0b0c915c81892e19f92a

**The changes in this PR will be included in the next version bump.**

<details><summary>This PR includes changesets to release 1 package</summary>

  | Name                 | Type  |
| -------------------- | ----- |
| @urql/exchange-retry | Patch |

</details>

Not sure what this means? [Click here  to learn what changesets are](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add another changeset to this PR](https://github.com/DoisKoh/urql/new/fix/retry-delay?filename=.changeset/calm-bugs-dream.md&value=---%0A%22%40urql%2Fexchange-retry%22%3A%20patch%0A---%0A%0Afix%3A%20retry%20delay%20not%20persisting%20between%20calls%0A)



Patch:
diff --git a/.changeset/silly-pigs-return.md b/.changeset/silly-pigs-return.md
new file mode 100644
index 00000000..38d51ccd
--- /dev/null
+++ b/.changeset/silly-pigs-return.md
@@ -0,0 +1,5 @@
+---
+'@urql/core': patch
+---
+
+Add back our cache-outcome on the document-cache, this was behind a development flag however in our normalized cache we always add it already
diff --git a/.changeset/witty-peas-love.md b/.changeset/witty-peas-love.md
new file mode 100644
index 00000000..850dad9d
--- /dev/null
+++ b/.changeset/witty-peas-love.md
@@ -0,0 +1,7 @@
+---
+'@urql/exchange-retry': patch
+---
+
+---
+
+Fixed the delay amount not increasing as retry count increases.
diff --git a/exchanges/retry/src/retryExchange.ts b/exchanges/retry/src/retryExchange.ts
index 98bd020f..2410fc46 100644
--- a/exchanges/retry/src/retryExchange.ts
+++ b/exchanges/retry/src/retryExchange.ts
@@ -111,7 +111,7 @@ interface RetryState {
 export const retryExchange = (options: RetryExchangeOptions): Exchange => {
   const { retryIf, retryWith } = options;
   const MIN_DELAY = options.initialDelayMs || 1000;
-  const MAX_DELAY = options.maxDelayMs || 15000;
+  const MAX_DELAY = options.maxDelayMs || 15_000;
   const MAX_ATTEMPTS = options.maxNumberAttempts || 2;
   const RANDOM_DELAY =
     options.randomDelay != null ? !!options.randomDelay : true;
@@ -133,12 +133,22 @@ export const retryExchange = (options: RetryExchangeOptions): Exchange => {
           let delayAmount = retry.delay || MIN_DELAY;
 
           const backoffFactor = Math.random() + 1.5;
-          // if randomDelay is enabled and it won't exceed the max delay, apply a random
-          // amount to the delay to avoid thundering herd problem
-          if (RANDOM_DELAY && delayAmount * backoffFactor < MAX_DELAY) {
-            delayAmount *= backoffFactor;
+          if (RANDOM_DELAY) {
+            // if randomDelay is enabled and it won't exceed the max delay, apply a random
+            // amount to the delay to avoid thundering herd problem
+            if (delayAmount * backoffFactor < MAX_DELAY) {
+              delayAmount *= backoffFactor;
+            } else {
+              delayAmount = MAX_DELAY;
+            }
+          } else {
+            // otherwise, increase the delay proportionately by the initial delay
+            delayAmount = Math.min(retryCount * MIN_DELAY, MAX_DELAY);
           }
 
+          // ensure the delay is carried over to the next context
+          retry.delay = delayAmount;
+
           // We stop the retries if a teardown event for this operation comes in
           // But if this event comes through regularly we also stop the retries, since it's
           // basically the query retrying itself, no backoff should be added!
@@ -158,6 +168,7 @@ export const retryExchange = (options: RetryExchangeOptions): Exchange => {
             operation,
             data: {
               retryCount,
+              delayAmount,
             },
           });
 
diff --git a/packages/core/src/exchanges/cache.ts b/packages/core/src/exchanges/cache.ts
index 31b9202f..b3f53ebe 100755
--- a/packages/core/src/exchanges/cache.ts
+++ b/packages/core/src/exchanges/cache.ts
@@ -86,14 +86,12 @@ export const cacheExchange: Exchange = ({ forward, client, dispatchDebug }) => {
             data: null,
           });
 
-        if (process.env.NODE_ENV !== 'production') {
-          result = {
-            ...result,
-            operation: addMetadata(operation, {
-              cacheOutcome: cachedResult ? 'hit' : 'miss',
-            }),
-          };
-        }
+        result = {
+          ...result,
+          operation: addMetadata(operation, {
+            cacheOutcome: cachedResult ? 'hit' : 'miss',
+          }),
+        };
 
         if (operation.context.requestPolicy === 'cache-and-network') {
           result.stale = true;


Test patch:
diff --git a/exchanges/retry/src/retryExchange.test.ts b/exchanges/retry/src/retryExchange.test.ts
index 13de90f8..a56ee8c4 100644
--- a/exchanges/retry/src/retryExchange.test.ts
+++ b/exchanges/retry/src/retryExchange.test.ts
@@ -211,15 +211,15 @@ it('should reset the retry counter if an operation succeeded first', () => {
       return fromArray([
         {
           operation: forwardOp,
-          data: queryOneData,
+          error: queryOneError,
         } as any,
         {
           operation: forwardOp,
-          error: queryOneError,
+          data: queryOneData,
         } as any,
       ]);
     } else {
-      expect(forwardOp.context.retry).toEqual({ count: 1, delay: null });
+      expect(forwardOp.context.retry).toEqual({ count: 0, delay: null });
 
       return fromValue({
         operation: forwardOp,
@@ -396,3 +396,61 @@ it('should allow retryWhen to return new operations when retrying', () => {
   expect(response.mock.calls[1][0]).toHaveProperty('context.counter', 1);
   expect(response.mock.calls[2][0]).toHaveProperty('context.counter', 2);
 });
+
+it('should increase retries by initialDelayMs for each subsequent failure', () => {
+  const errorWithNetworkError = {
+    ...queryOneError,
+    networkError: 'scary network error',
+  };
+  const response = vi.fn((forwardOp: Operation): OperationResult => {
+    expect(forwardOp.key).toBe(op.key);
+    return {
+      operation: forwardOp,
+      // @ts-ignore
+      error: errorWithNetworkError,
+    };
+  });
+
+  const result = vi.fn();
+  const forward: ExchangeIO = ops$ => {
+    return pipe(ops$, map(response));
+  };
+
+  const retryWith = vi.fn((_error, operation) => {
+    return makeOperation(operation.kind, operation, {
+      ...operation.context,
+      counter: (operation.context?.counter || 0) + 1,
+    });
+  });
+
+  const fixedDelayMs = 50;
+
+  const fixedDelayOptions = {
+    ...mockOptions,
+    randomDelay: false,
+    initialDelayMs: fixedDelayMs,
+  };
+
+  pipe(
+    retryExchange({
+      ...fixedDelayOptions,
+      retryIf: undefined,
+      retryWith,
+    })({
+      forward,
+      client,
+      dispatchDebug,
+    })(ops$),
+    tap(result),
+    publish
+  );
+
+  next(op);
+
+  // delay between each call should be increased by initialDelayMs
+  // (e.g. if initialDelayMs is 5s, first retry is waits 5 seconds, second retry waits 10 seconds)
+  for (let i = 1; i <= fixedDelayOptions.maxNumberAttempts; i++) {
+    expect(response).toHaveBeenCalledTimes(i);
+    vi.advanceTimersByTime(i * fixedDelayOptions.initialDelayMs);
+  }
+});