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

mydea · web-flow · commit fc7344fb0e6e · 2023-08-10T16:54:05.000+02:00
In our `stop()` method, we always tried to force a final flush when in
`session` mode.
However, that may lead to weird behaviour when we internally `stop()`
due to a failure - e.g. think a send replay request fails, we do not
want to force a flush again in that case.

Note that in tests this seems to be generally passing because `flush()`
usually has a running `_flushLock` at this time and thus does not
attempt to flush again immediately but only schedules a flush later. I
added a test that properly failed for this before and is now fixed.
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,89 @@
+import { expect } from '@playwright/test';
+
+import { sentryTest } from '../../../utils/fixtures';
+import { envelopeRequestParser } from '../../../utils/helpers';
+import {
+  getDecompressedRecordingEvents,
+  getReplaySnapshot,
+  isReplayEvent,
+  REPLAY_DEFAULT_FLUSH_MAX_DELAY,
+  shouldSkipReplayTest,
+  waitForReplayRequest,
+} from '../../../utils/replayHelpers';
+
+sentryTest(
+  'should stop recording when running into eventBuffer error',
+  async ({ getLocalTestPath, page, forceFlushReplay }) => {
+    if (shouldSkipReplayTest()) {
+      sentryTest.skip();
+    }
+
+    await page.route('https://dsn.ingest.sentry.io/**/*', route => {
+      return route.fulfill({
+        status: 200,
+      });
+    });
+
+    const url = await getLocalTestPath({ testDir: __dirname });
+    await page.goto(url);
+
+    await waitForReplayRequest(page);
+    const replay = await getReplaySnapshot(page);
+    expect(replay._isEnabled).toBe(true);
+
+    await forceFlushReplay();
+
+    let called = 0;
+
+    await page.route('https://dsn.ingest.sentry.io/**/*', route => {
+      const event = envelopeRequestParser(route.request());
+
+      // We only want to count replays here
+      if (event && isReplayEvent(event)) {
+        const events = getDecompressedRecordingEvents(route.request());
+        // this makes sure we ignore e.g. mouse move events which can otherwise lead to flakes
+        if (events.length > 0) {
+          called++;
+        }
+      }
+
+      return route.fulfill({
+        status: 200,
+      });
+    });
+
+    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/browser-integration-tests/utils/replayHelpers.ts b/packages/browser-integration-tests/utils/replayHelpers.ts
@@ -267,7 +267,7 @@ function getOptionsEvents(replayRequest: Request): CustomRecordingEvent[] {
   return getAllCustomRrwebRecordingEvents(events).filter(data => data.tag === 'options');
 }
 
-function getDecompressedRecordingEvents(resOrReq: Request | Response): RecordingSnapshot[] {
+export function getDecompressedRecordingEvents(resOrReq: Request | Response): RecordingSnapshot[] {
   const replayRequest = getRequest(resOrReq);
   return (
     (replayEnvelopeRequestParser(replayRequest, 5) as eventWithTime[])
@@ -302,7 +302,7 @@ const replayEnvelopeRequestParser = (request: Request | null, envelopeIndex = 2)
   return envelope[envelopeIndex] as Event;
 };
 
-const replayEnvelopeParser = (request: Request | null): unknown[] => {
+export const replayEnvelopeParser = (request: Request | null): unknown[] => {
   // https://develop.sentry.dev/sdk/envelopes/
   const envelopeBytes = request?.postDataBuffer() || '';
 
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();
 
diff --git a/packages/replay/test/integration/flush.test.ts b/packages/replay/test/integration/flush.test.ts
@@ -472,10 +472,9 @@ describe('Integration | flush', () => {
     const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP + 100, type: 2 };
     mockRecord._emitter(TEST_EVENT);
 
-    await advanceTimers(DEFAULT_FLUSH_MIN_DELAY);
     await advanceTimers(160_000);
 
-    expect(mockFlush).toHaveBeenCalledTimes(2);
+    expect(mockFlush).toHaveBeenCalledTimes(1);
     expect(mockSendReplay).toHaveBeenCalledTimes(0);
     expect(replay.isEnabled()).toBe(false);
 

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`	`/**`