fix(utils): Consider 429 responses in transports (#5062)

getsentry · May 11, 2022 · 5beee11 · 5beee11
1 parent 46400ff
commit 5beee11
Show file tree

Hide file tree

Showing 9 changed files with 70 additions and 40 deletions.
diff --git a/packages/browser/src/transports/fetch.ts b/packages/browser/src/transports/fetch.ts
@@ -21,6 +21,7 @@ export function makeFetchTransport(
     };
 
     return nativeFetch(options.url, requestOptions).then(response => ({
+      statusCode: response.status,
       headers: {
         'x-sentry-rate-limits': response.headers.get('X-Sentry-Rate-Limits'),
         'retry-after': response.headers.get('Retry-After'),

diff --git a/packages/browser/src/transports/xhr.ts b/packages/browser/src/transports/xhr.ts
@@ -26,13 +26,13 @@ export function makeXHRTransport(options: BrowserTransportOptions): Transport {
 
       xhr.onreadystatechange = (): void => {
         if (xhr.readyState === XHR_READYSTATE_DONE) {
-          const response = {
+          resolve({
+            statusCode: xhr.status,
             headers: {
               'x-sentry-rate-limits': xhr.getResponseHeader('X-Sentry-Rate-Limits'),
               'retry-after': xhr.getResponseHeader('Retry-After'),
             },
-          };
-          resolve(response);
+          });
         }
       };
 

diff --git a/packages/core/src/transports/base.ts b/packages/core/src/transports/base.ts
@@ -70,13 +70,11 @@ export function createTransport(
 
     const requestTask = (): PromiseLike<void> =>
       makeRequest({ body: serializeEnvelope(filteredEnvelope) }).then(
-        ({ headers }): void => {
-          if (headers) {
-            rateLimits = updateRateLimits(rateLimits, headers);
-          }
+        response => {
+          rateLimits = updateRateLimits(rateLimits, response);
         },
         error => {
-          IS_DEBUG_BUILD && logger.error('Failed while recording event:', error);
+          IS_DEBUG_BUILD && logger.error('Failed while sending event:', error);
           recordEnvelopeLoss('network_error');
         },
       );

diff --git a/packages/node/src/transports/http.ts b/packages/node/src/transports/http.ts
@@ -113,6 +113,7 @@ function createRequestExecutor(
           const rateLimitsHeader = res.headers['x-sentry-rate-limits'] ?? null;
 
           resolve({
+            statusCode: res.statusCode,
             headers: {
               'retry-after': retryAfterHeader,
               'x-sentry-rate-limits': Array.isArray(rateLimitsHeader) ? rateLimitsHeader[0] : rateLimitsHeader,

diff --git a/packages/node/test/transports/http.test.ts b/packages/node/test/transports/http.test.ts
@@ -237,10 +237,7 @@ describe('makeNewHttpTransport()', () => {
   it('should register TransportRequestExecutor that returns the correct object from server response (rate limit)', async () => {
     await setupTestServer({
       statusCode: RATE_LIMIT,
-      responseHeaders: {
-        'Retry-After': '2700',
-        'X-Sentry-Rate-Limits': '60::organization, 2700::organization',
-      },
+      responseHeaders: {},
     });
 
     makeNodeTransport(defaultOptions);
@@ -253,10 +250,7 @@ describe('makeNewHttpTransport()', () => {
 
     await expect(executorResult).resolves.toEqual(
       expect.objectContaining({
-        headers: {
-          'retry-after': '2700',
-          'x-sentry-rate-limits': '60::organization, 2700::organization',
-        },
+        statusCode: RATE_LIMIT,
       }),
     );
   });
@@ -276,6 +270,7 @@ describe('makeNewHttpTransport()', () => {
 
     await expect(executorResult).resolves.toEqual(
       expect.objectContaining({
+        statusCode: SUCCESS,
         headers: {
           'retry-after': null,
           'x-sentry-rate-limits': null,
@@ -303,6 +298,7 @@ describe('makeNewHttpTransport()', () => {
 
     await expect(executorResult).resolves.toEqual(
       expect.objectContaining({
+        statusCode: SUCCESS,
         headers: {
           'retry-after': '2700',
           'x-sentry-rate-limits': '60::organization, 2700::organization',
@@ -330,6 +326,7 @@ describe('makeNewHttpTransport()', () => {
 
     await expect(executorResult).resolves.toEqual(
       expect.objectContaining({
+        statusCode: RATE_LIMIT,
         headers: {
           'retry-after': '2700',
           'x-sentry-rate-limits': '60::organization, 2700::organization',

diff --git a/packages/node/test/transports/https.test.ts b/packages/node/test/transports/https.test.ts
@@ -290,10 +290,7 @@ describe('makeNewHttpsTransport()', () => {
   it('should register TransportRequestExecutor that returns the correct object from server response (rate limit)', async () => {
     await setupTestServer({
       statusCode: RATE_LIMIT,
-      responseHeaders: {
-        'Retry-After': '2700',
-        'X-Sentry-Rate-Limits': '60::organization, 2700::organization',
-      },
+      responseHeaders: {},
     });
 
     makeNodeTransport(defaultOptions);
@@ -306,10 +303,7 @@ describe('makeNewHttpsTransport()', () => {
 
     await expect(executorResult).resolves.toEqual(
       expect.objectContaining({
-        headers: {
-          'retry-after': '2700',
-          'x-sentry-rate-limits': '60::organization, 2700::organization',
-        },
+        statusCode: RATE_LIMIT,
       }),
     );
   });
@@ -329,6 +323,7 @@ describe('makeNewHttpsTransport()', () => {
 
     await expect(executorResult).resolves.toEqual(
       expect.objectContaining({
+        statusCode: SUCCESS,
         headers: {
           'retry-after': null,
           'x-sentry-rate-limits': null,
@@ -356,6 +351,7 @@ describe('makeNewHttpsTransport()', () => {
 
     await expect(executorResult).resolves.toEqual(
       expect.objectContaining({
+        statusCode: SUCCESS,
         headers: {
           'retry-after': '2700',
           'x-sentry-rate-limits': '60::organization, 2700::organization',
@@ -383,6 +379,7 @@ describe('makeNewHttpsTransport()', () => {
 
     await expect(executorResult).resolves.toEqual(
       expect.objectContaining({
+        statusCode: RATE_LIMIT,
         headers: {
           'retry-after': '2700',
           'x-sentry-rate-limits': '60::organization, 2700::organization',

diff --git a/packages/types/src/transport.ts b/packages/types/src/transport.ts
@@ -7,6 +7,7 @@ export type TransportRequest = {
 };
 
 export type TransportMakeRequestResponse = {
+  statusCode?: number;
   headers?: {
     [key: string]: string | null;
     'x-sentry-rate-limits': string | null;

diff --git a/packages/utils/src/ratelimit.ts b/packages/utils/src/ratelimit.ts
@@ -1,3 +1,5 @@
+import { TransportMakeRequestResponse } from '@sentry/types';
+
 // Intentionally keeping the key broad, as we don't know for sure what rate limit headers get returned from backend
 export type RateLimits = Record<string, number>;
 
@@ -43,7 +45,7 @@ export function isRateLimited(limits: RateLimits, category: string, now: number
  */
 export function updateRateLimits(
   limits: RateLimits,
-  headers: Record<string, string | null | undefined>,
+  { statusCode, headers }: TransportMakeRequestResponse,
   now: number = Date.now(),
 ): RateLimits {
   const updatedRateLimits: RateLimits = {
@@ -52,8 +54,8 @@ export function updateRateLimits(
 
   // "The name is case-insensitive."
   // https://developer.mozilla.org/en-US/docs/Web/API/Headers/get
-  const rateLimitHeader = headers['x-sentry-rate-limits'];
-  const retryAfterHeader = headers['retry-after'];
+  const rateLimitHeader = headers && headers['x-sentry-rate-limits'];
+  const retryAfterHeader = headers && headers['retry-after'];
 
   if (rateLimitHeader) {
     /**
@@ -69,19 +71,21 @@ export function updateRateLimits(
      *     <reason_code> is an arbitrary string like "org_quota" - ignored by SDK
      */
     for (const limit of rateLimitHeader.trim().split(',')) {
-      const parameters = limit.split(':', 2);
-      const headerDelay = parseInt(parameters[0], 10);
+      const [retryAfter, categories] = limit.split(':', 2);
+      const headerDelay = parseInt(retryAfter, 10);
       const delay = (!isNaN(headerDelay) ? headerDelay : 60) * 1000; // 60sec default
-      if (!parameters[1]) {
+      if (!categories) {
         updatedRateLimits.all = now + delay;
       } else {
-        for (const category of parameters[1].split(';')) {
+        for (const category of categories.split(';')) {
           updatedRateLimits[category] = now + delay;
         }
       }
     }
   } else if (retryAfterHeader) {
     updatedRateLimits.all = now + parseRetryAfterHeader(retryAfterHeader, now);
+  } else if (statusCode === 429) {
+    updatedRateLimits.all = now + 60 * 1000;
   }
 
   return updatedRateLimits;

diff --git a/packages/utils/test/ratelimit.test.ts b/packages/utils/test/ratelimit.test.ts
@@ -70,66 +70,73 @@ describe('updateRateLimits()', () => {
   test('should update the `all` category based on `retry-after` header ', () => {
     const rateLimits: RateLimits = {};
     const headers = {
+      'x-sentry-rate-limits': null,
       'retry-after': '42',
     };
-    const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
+    const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
     expect(updatedRateLimits.all).toEqual(42 * 1000);
   });
 
   test('should update a single category based on `x-sentry-rate-limits` header', () => {
     const rateLimits: RateLimits = {};
     const headers = {
+      'retry-after': null,
       'x-sentry-rate-limits': '13:error',
     };
-    const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
+    const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
     expect(updatedRateLimits.error).toEqual(13 * 1000);
   });
 
   test('should update multiple categories based on `x-sentry-rate-limits` header', () => {
     const rateLimits: RateLimits = {};
     const headers = {
+      'retry-after': null,
       'x-sentry-rate-limits': '13:error;transaction',
     };
-    const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
+    const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
     expect(updatedRateLimits.error).toEqual(13 * 1000);
     expect(updatedRateLimits.transaction).toEqual(13 * 1000);
   });
 
   test('should update multiple categories with different values based on multi `x-sentry-rate-limits` header', () => {
     const rateLimits: RateLimits = {};
     const headers = {
+      'retry-after': null,
       'x-sentry-rate-limits': '13:error,15:transaction',
     };
-    const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
+    const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
     expect(updatedRateLimits.error).toEqual(13 * 1000);
     expect(updatedRateLimits.transaction).toEqual(15 * 1000);
   });
 
   test('should use last entry from multi `x-sentry-rate-limits` header for a given category', () => {
     const rateLimits: RateLimits = {};
     const headers = {
+      'retry-after': null,
       'x-sentry-rate-limits': '13:error,15:transaction;error',
     };
-    const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
+    const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
     expect(updatedRateLimits.error).toEqual(15 * 1000);
     expect(updatedRateLimits.transaction).toEqual(15 * 1000);
   });
 
   test('should fallback to `all` if `x-sentry-rate-limits` header is missing a category', () => {
     const rateLimits: RateLimits = {};
     const headers = {
+      'retry-after': null,
       'x-sentry-rate-limits': '13',
     };
-    const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
+    const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
     expect(updatedRateLimits.all).toEqual(13 * 1000);
   });
 
   test('should use 60s default if delay in `x-sentry-rate-limits` header is malformed', () => {
     const rateLimits: RateLimits = {};
     const headers = {
+      'retry-after': null,
       'x-sentry-rate-limits': 'x',
     };
-    const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
+    const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
     expect(updatedRateLimits.all).toEqual(60 * 1000);
   });
 
@@ -138,9 +145,10 @@ describe('updateRateLimits()', () => {
       error: 1337,
     };
     const headers = {
+      'retry-after': null,
       'x-sentry-rate-limits': '13:transaction',
     };
-    const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
+    const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
     expect(updatedRateLimits.error).toEqual(1337);
     expect(updatedRateLimits.transaction).toEqual(13 * 1000);
   });
@@ -151,8 +159,31 @@ describe('updateRateLimits()', () => {
       'retry-after': '42',
       'x-sentry-rate-limits': '13:error',
     };
-    const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
+    const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
     expect(updatedRateLimits.error).toEqual(13 * 1000);
     expect(updatedRateLimits.all).toBeUndefined();
   });
+
+  test('should apply a global rate limit of 60s when no headers are provided on a 429 status code', () => {
+    const rateLimits: RateLimits = {};
+    const updatedRateLimits = updateRateLimits(rateLimits, { statusCode: 429 }, 0);
+    expect(updatedRateLimits.all).toBe(60_000);
+  });
+
+  test('should not apply a global rate limit specific headers are provided on a 429 status code', () => {
+    const rateLimits: RateLimits = {};
+    const headers = {
+      'retry-after': null,
+      'x-sentry-rate-limits': '13:error',
+    };
+    const updatedRateLimits = updateRateLimits(rateLimits, { statusCode: 429, headers }, 0);
+    expect(updatedRateLimits.error).toEqual(13 * 1000);
+    expect(updatedRateLimits.all).toBeUndefined();
+  });
+
+  test('should not apply a default rate limit on a non-429 status code', () => {
+    const rateLimits: RateLimits = {};
+    const updatedRateLimits = updateRateLimits(rateLimits, { statusCode: 200 }, 0);
+    expect(updatedRateLimits).toEqual(rateLimits);
+  });
 });