fix(replay): Ensure we do not try to flush when we force stop replay

mydea · mydea · commit 009ce3448991 · 2023-08-10T12:49:57.000+02:00
diff --git a/packages/browser-integration-tests/suites/replay/eventBufferError/template.html b/packages/browser-integration-tests/suites/replay/eventBufferError/template.html
@@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+  </head>
+  <body>
+    <button onclick="console.log('Test log')" id="button1">Click me</button>
+    <button onclick="console.log('Test log 2')" id="button2">Click me</button>
+  </body>
+</html>
diff --git a/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts b/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts
@@ -0,0 +1,68 @@
+import { expect } from '@playwright/test';
+
+import { sentryTest } from '../../../utils/fixtures';
+import {
+  getReplaySnapshot,
+  REPLAY_DEFAULT_FLUSH_MAX_DELAY,
+  shouldSkipReplayTest,
+  waitForReplayRequest,
+} from '../../../utils/replayHelpers';
+
+sentryTest('should stop recording when running into eventBuffer error', async ({ getLocalTestPath, page }) => {
+  if (shouldSkipReplayTest()) {
+    sentryTest.skip();
+  }
+
+  let called = 0;
+
+  await page.route('https://dsn.ingest.sentry.io/**/*', route => {
+    called++;
+
+    return route.fulfill({
+      status: 200,
+    });
+  });
+
+  const url = await getLocalTestPath({ testDir: __dirname });
+  await page.goto(url);
+
+  await waitForReplayRequest(page);
+
+  expect(called).toBe(1);
+  const replay = await getReplaySnapshot(page);
+  expect(replay._isEnabled).toBe(true);
+
+  called = 0;
+
+  /**
+   * We test the following here:
+   * 1. First click should add an event (so the eventbuffer is not empty)
+   * 2. Second click should throw an error in eventBuffer (which should lead to stopping the replay)
+   * 3. Nothing should be sent to API, as we stop the replay due to the eventBuffer error.
+   */
+  await page.evaluate(`
+window._count = 0;
+window._addEvent = window.Replay._replay.eventBuffer.addEvent.bind(window.Replay._replay.eventBuffer);
+window.Replay._replay.eventBuffer.addEvent = (...args) => {
+  window._count++;
+  if (window._count === 2) {
+    throw new Error('provoked error');
+  }
+  window._addEvent(...args);
+};
+`);
+
+  void page.click('#button1');
+  void page.click('#button2');
+
+  // Should immediately skip retrying and just cancel, no backoff
+  // This waitForTimeout call should be okay, as we're not checking for any
+  // further network requests afterwards.
+  await page.waitForTimeout(REPLAY_DEFAULT_FLUSH_MAX_DELAY + 100);
+
+  expect(called).toBe(0);
+
+  const replay2 = await getReplaySnapshot(page);
+
+  expect(replay2._isEnabled).toBe(false);
+});
diff --git a/packages/replay/src/integration.ts b/packages/replay/src/integration.ts
@@ -263,7 +263,7 @@ Sentry.init({ replaysOnErrorSampleRate: ${errorSampleRate} })`,
       return Promise.resolve();
     }
 
-    return this._replay.stop();
+    return this._replay.stop({ forceFlush: this._replay.recordingMode === 'session' });
   }
 
   /**
diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts
@@ -368,7 +368,7 @@ export class ReplayContainer implements ReplayContainerInterface {
    * Currently, this needs to be manually called (e.g. for tests). Sentry SDK
    * does not support a teardown
    */
-  public async stop(reason?: string): Promise<void> {
+  public async stop({ forceFlush = false, reason }: { forceFlush?: boolean; reason?: string } = {}): Promise<void> {
     if (!this._isEnabled) {
       return;
     }
@@ -388,7 +388,7 @@ export class ReplayContainer implements ReplayContainerInterface {
       this._debouncedFlush.cancel();
       // See comment above re: `_isEnabled`, we "force" a flush, ignoring the
       // `_isEnabled` state of the plugin since it was disabled above.
-      if (this.recordingMode === 'session') {
+      if (forceFlush) {
         await this._flush({ force: true });
       }
 
@@ -777,7 +777,7 @@ export class ReplayContainer implements ReplayContainerInterface {
     this.session = session;
 
     if (!this.session.sampled) {
-      void this.stop('session not refreshed');
+      void this.stop({ reason: 'session not refreshed' });
       return false;
     }
 
@@ -1099,7 +1099,7 @@ export class ReplayContainer implements ReplayContainerInterface {
       // This means we retried 3 times and all of them failed,
       // or we ran into a problem we don't want to retry, like rate limiting.
       // In this case, we want to completely stop the replay - otherwise, we may get inconsistent segments
-      void this.stop('sendReplay');
+      void this.stop({ reason: 'sendReplay' });
 
       const client = getCurrentHub().getClient();
 
@@ -1223,7 +1223,7 @@ export class ReplayContainer implements ReplayContainerInterface {
 
     // Stop replay if over the mutation limit
     if (overMutationLimit) {
-      void this.stop('mutationLimit');
+      void this.stop({ reason: 'mutationLimit', forceFlush: this.recordingMode === 'session' });
       return false;
     }
 
diff --git a/packages/replay/src/types/replay.ts b/packages/replay/src/types/replay.ts
@@ -451,7 +451,7 @@ export interface ReplayContainer {
   getContext(): InternalEventContext;
   initializeSampling(): void;
   start(): void;
-  stop(reason?: string): Promise<void>;
+  stop(options?: { reason?: string; forceflush?: boolean }): Promise<void>;
   pause(): void;
   resume(): void;
   startRecording(): void;
diff --git a/packages/replay/src/util/addEvent.ts b/packages/replay/src/util/addEvent.ts
@@ -71,7 +71,7 @@ export async function addEvent(
     const reason = error && error instanceof EventBufferSizeExceededError ? 'addEventSizeExceeded' : 'addEvent';
 
     __DEBUG_BUILD__ && logger.error(error);
-    await replay.stop(reason);
+    await replay.stop({ reason });
 
     const client = getCurrentHub().getClient();
 

Original file line number	Diff line number	Diff line change
@@ -263,7 +263,7 @@ Sentry.init({ replaysOnErrorSampleRate: ${errorSampleRate} })`,
`263`	`263`	`return Promise.resolve();`
`264`	`264`	`}`
`265`	`265`
`266`		`- return this._replay.stop();`
	`266`	`+ return this._replay.stop({ forceFlush: this._replay.recordingMode === 'session' });`
`267`	`267`	`}`
`268`	`268`
`269`	`269`	`/**`