feat(replay): Stop without retry when receiving bad API response (#6773)

mydea · web-flow · commit 81ef0cabe3fb · 2023-01-16T13:20:59.000+01:00
diff --git a/packages/integration-tests/suites/replay/errorResponse/template.html b/packages/integration-tests/suites/replay/errorResponse/template.html
@@ -0,0 +1,9 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+  </head>
+  <body>
+    <button onclick="console.log('Test log')">Click me</button>
+  </body>
+</html>
diff --git a/packages/integration-tests/suites/replay/errorResponse/test.ts b/packages/integration-tests/suites/replay/errorResponse/test.ts
@@ -0,0 +1,37 @@
+import { expect } from '@playwright/test';
+
+import { sentryTest } from '../../../utils/fixtures';
+import { getReplaySnapshot } from '../../../utils/helpers';
+
+sentryTest('errorResponse', async ({ getLocalTestPath, page }) => {
+  // Currently bundle tests are not supported for replay
+  if (process.env.PW_BUNDLE && process.env.PW_BUNDLE.startsWith('bundle_')) {
+    sentryTest.skip();
+  }
+
+  let called = 0;
+
+  await page.route('https://dsn.ingest.sentry.io/**/*', route => {
+    called++;
+
+    return route.fulfill({
+      status: 400,
+    });
+  });
+
+  const url = await getLocalTestPath({ testDir: __dirname });
+  await page.goto(url);
+
+  await page.click('button');
+  await page.waitForTimeout(300);
+
+  expect(called).toBe(1);
+
+  // Should immediately skip retrying and just cancel, no backoff
+  await page.waitForTimeout(5001);
+
+  expect(called).toBe(1);
+  const replay = await getReplaySnapshot(page);
+
+  expect(replay['_isEnabled']).toBe(false);
+});
diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts
@@ -234,6 +234,7 @@ export class ReplayContainer implements ReplayContainerInterface {
       this._stopRecording?.();
       this.eventBuffer?.destroy();
       this.eventBuffer = null;
+      this._debouncedFlush.cancel();
     } catch (err) {
       this._handleException(err);
     }
@@ -907,7 +908,7 @@ export class ReplayContainer implements ReplayContainerInterface {
     if (rateLimitDuration > 0) {
       __DEBUG_BUILD__ && logger.warn('[Replay]', `Rate limit hit, pausing replay for ${rateLimitDuration}ms`);
       this.pause();
-      this._debouncedFlush && this._debouncedFlush.cancel();
+      this._debouncedFlush.cancel();
 
       setTimeout(() => {
         __DEBUG_BUILD__ && logger.info('[Replay]', 'Resuming replay after rate limit');
diff --git a/packages/replay/src/util/sendReplay.ts b/packages/replay/src/util/sendReplay.ts
@@ -2,7 +2,7 @@ import { captureException, setContext } from '@sentry/core';
 
 import { RETRY_BASE_INTERVAL, RETRY_MAX_COUNT, UNABLE_TO_SEND_REPLAY } from '../constants';
 import type { SendReplayData } from '../types';
-import { RateLimitError, sendReplayRequest } from './sendReplayRequest';
+import { RateLimitError, sendReplayRequest, TransportStatusCodeError } from './sendReplayRequest';
 
 /**
  * Finalize and send the current replay event to Sentry
@@ -25,7 +25,7 @@ export async function sendReplay(
     await sendReplayRequest(replayData);
     return true;
   } catch (err) {
-    if (err instanceof RateLimitError) {
+    if (err instanceof RateLimitError || err instanceof TransportStatusCodeError) {
       throw err;
     }
 
diff --git a/packages/replay/src/util/sendReplayRequest.ts b/packages/replay/src/util/sendReplayRequest.ts
@@ -116,12 +116,20 @@ export async function sendReplayRequest({
   }
 
   // TODO (v8): we can remove this guard once transport.send's type signature doesn't include void anymore
-  if (response) {
-    const rateLimits = updateRateLimits({}, response);
-    if (isRateLimited(rateLimits, 'replay')) {
-      throw new RateLimitError(rateLimits);
-    }
+  if (!response) {
+    return response;
   }
+
+  const rateLimits = updateRateLimits({}, response);
+  if (isRateLimited(rateLimits, 'replay')) {
+    throw new RateLimitError(rateLimits);
+  }
+
+  // If the status code is invalid, we want to immediately stop & not retry
+  if (typeof response.statusCode === 'number' && (response.statusCode < 200 || response.statusCode >= 300)) {
+    throw new TransportStatusCodeError(response.statusCode);
+  }
+
   return response;
 }
 
@@ -136,3 +144,12 @@ export class RateLimitError extends Error {
     this.rateLimits = rateLimits;
   }
 }
+
+/**
+ * This error indicates that the transport returned an invalid status code.
+ */
+export class TransportStatusCodeError extends Error {
+  public constructor(statusCode: number) {
+    super(`Transport returned status code ${statusCode}`);
+  }
+}
diff --git a/packages/replay/test/integration/coreHandlers/handleGlobalEvent.test.ts b/packages/replay/test/integration/coreHandlers/handleGlobalEvent.test.ts
@@ -26,7 +26,6 @@ describe('Integration | coreHandlers | handleGlobalEvent', () => {
         replaysSessionSampleRate: 0.0,
         replaysOnErrorSampleRate: 1.0,
       },
-      autoStart: false,
     }));
   });
 
diff --git a/packages/replay/test/mocks/resetSdkMock.ts b/packages/replay/test/mocks/resetSdkMock.ts
@@ -5,7 +5,7 @@ import type { DomHandler } from './../types';
 import type { MockSdkParams } from './mockSdk';
 import { mockSdk } from './mockSdk';
 
-export async function resetSdkMock({ replayOptions, sentryOptions }: MockSdkParams): Promise<{
+export async function resetSdkMock({ replayOptions, sentryOptions, autoStart }: MockSdkParams): Promise<{
   domHandler: DomHandler;
   mockRecord: RecordMock;
   replay: ReplayContainer;
@@ -30,6 +30,7 @@ export async function resetSdkMock({ replayOptions, sentryOptions }: MockSdkPara
   const { replay } = await mockSdk({
     replayOptions,
     sentryOptions,
+    autoStart,
   });
 
   // XXX: This is needed to ensure `domHandler` is set
