Merge pull request #239 from contentful/fix-retry-on-error

fix: retry on error [ZEND-1616]

src/rate-limit.ts

@@ -1,14 +1,15 @@
 import { noop } from './utils'
 import type { AxiosInstance } from './types'
 
-const attempts: Record<string, number> = {}
-let networkErrorAttempts = 0
-
 const delay = (ms: number): Promise<void> =>
   new Promise((resolve) => {
     setTimeout(resolve, ms)
   })
 
+const defaultWait = (attempts: number): number => {
+  return Math.pow(Math.SQRT2, attempts)
+}
+
 export default function rateLimit(instance: AxiosInstance, maxRetry = 5): void {
   const { responseLogger = noop, requestLogger = noop } = instance.defaults
 
@@ -30,47 +31,30 @@ export default function rateLimit(instance: AxiosInstance, maxRetry = 5): void {
       return response
     },
     function (error) {
-      let { response } = error
+      const { response } = error
       const { config } = error
       responseLogger(error)
       // Do not retry if it is disabled or no request config exists (not an axios error)
       if (!config || !instance.defaults.retryOnError) {
         return Promise.reject(error)
       }
 
+      // Retried already for max attempts
+      const doneAttempts = config.attempts || 1
+      if (doneAttempts > maxRetry) {
+        error.attempts = config.attempts
+        return Promise.reject(error)
+      }
+
       let retryErrorType = null
-      let wait = 0
+      let wait = defaultWait(doneAttempts)
 
       // Errors without response did not receive anything from the server
       if (!response) {
         retryErrorType = 'Connection'
-        networkErrorAttempts++
-
-        if (networkErrorAttempts > maxRetry) {
-          error.attempts = networkErrorAttempts
-          return Promise.reject(error)
-        }
-
-        wait = Math.pow(Math.SQRT2, networkErrorAttempts)
-        response = {}
-      } else {
-        networkErrorAttempts = 0
-      }
-
-      if (response.status >= 500 && response.status < 600) {
+      } else if (response.status >= 500 && response.status < 600) {
         // 5** errors are server related
         retryErrorType = `Server ${response.status}`
-        const headers = response.headers || {}
-        const requestId = headers['x-contentful-request-id'] || null
-        attempts[requestId] = attempts[requestId] || 0
-        attempts[requestId]++
-
-        // we reject if there are too many errors with the same request id or request id is not defined
-        if (attempts[requestId] > maxRetry || !requestId) {
-          error.attempts = attempts[requestId]
-          return Promise.reject(error)
-        }
-        wait = Math.pow(Math.SQRT2, attempts[requestId])
       } else if (response.status === 429) {
         // 429 errors are exceeded rate limit exceptions
         retryErrorType = 'Rate limit'
@@ -88,6 +72,9 @@ export default function rateLimit(instance: AxiosInstance, maxRetry = 5): void {
           `${retryErrorType} error occurred. Waiting for ${wait} ms before retrying...`
         )
 
+        // increase attempts counter
+        config.attempts = doneAttempts + 1
+
         /* Somehow between the interceptor and retrying the request the httpAgent/httpsAgent gets transformed from an Agent-like object
          to a regular object, causing failures on retries after rate limits. Removing these properties here fixes the error, but retry
          requests still use the original http/httpsAgent property */

src/types.ts

@@ -106,4 +106,10 @@ export type CreateHttpClientParams = {
    * @param 1-30 (fixed number of limit), 'auto' (calculated limit based on current tier), '0%' - '100%' (calculated % limit based on tier)
    */
   throttle?: 'auto' | string | number
+
+  /**
+   * Optional how often the current request has been retried
+   * @default 0
+   */
+  attempt?: number
 }

test/unit/rate-limit-test.spec.ts

@@ -83,38 +83,6 @@ it('Retry on 5** - multiple errors', async () => {
   expect(response.data).toEqual('works #2')
 })
 
-it('Retry on 5** - multiple errors - reach/exceed limit', async () => {
-  const { client } = setup({ retryLimit: 7 })
-  mock.onGet('/rate-limit-me').replyOnce(500, 'Server Error', { 'x-contentful-request-id': 12345 })
-  mock.onGet('/rate-limit-me').replyOnce(500, 'Server Error', { 'x-contentful-request-id': 12345 })
-  mock
-    .onGet('/rate-limit-me')
-    .replyOnce(503, 'Another Server Error', { 'x-contentful-request-id': 12345 })
-  mock.onGet('/rate-limit-me').replyOnce(500, 'Server Error', { 'x-contentful-request-id': 12345 })
-  mock
-    .onGet('/rate-limit-me')
-    .replyOnce(503, 'Another Server Error', { 'x-contentful-request-id': 12345 })
-  mock.onGet('/rate-limit-me').replyOnce(500, 'Server Error', { 'x-contentful-request-id': 12345 })
-  mock
-    .onGet('/rate-limit-me')
-    .replyOnce(503, 'Another Server Error', { 'x-contentful-request-id': 12345 })
-  mock.onGet('/rate-limit-me').replyOnce(200, 'works')
-  mock.onGet('/rate-limit-me').replyOnce(500, 'Server Error', { 'x-contentful-request-id': 12345 })
-
-  expect.assertions(3)
-
-  const response = await client.get('/rate-limit-me')
-
-  expect(response.data).toBeDefined()
-  expect(response.data).toEqual('works')
-
-  try {
-    await client.get('/rate-limit-me')
-  } catch (error) {
-    expect(error.message).toEqual('Request failed with status code 500')
-  }
-})
-
 it('Retry on network error', async () => {
   const { client } = setupWithOneRetry()
   mock.onGet('/rate-limit-me').networkError()
@@ -166,21 +134,22 @@ it('Should Fail if it hits maxRetries', async () => {
   }
 })
 
-it('Rejects error straight away when X-Contentful-Request-Id header is missing', async () => {
+it('Retry on responses when X-Contentful-Request-Id header is missing', async () => {
   const { client } = setupWithOneRetry()
   mock.onGet('/error').replyOnce(500, 'error attempt')
+  mock.onGet('/error').replyOnce(500, 'error attempt 2')
   mock.onGet('/error').replyOnce(200, 'works')
 
-  expect.assertions(4)
+  expect.assertions(5)
 
   try {
     await client.get('/error')
   } catch (error) {
-    expect(error.response.data).toEqual('error attempt')
+    expect(error.response.data).toEqual('error attempt 2')
+    expect(logHandlerStub).toHaveBeenCalledTimes(1)
+    expect(logHandlerStub.mock.calls[0][0]).toEqual('warning')
     expect(error.message).toEqual('Request failed with status code 500')
-    expect(error.attempts).toEqual(1)
-    // did not log anything
-    expect(logHandlerStub).toBeCalledTimes(0)
+    expect(error.attempts).toEqual(2)
   }
 })