fix(replay): Ensure handleRecordingEmit aborts when event is not added

Author: mydea

packages/replay/src/replay.ts

@@ -38,7 +38,7 @@ import type {
   Timeouts,
 } from './types';
 import { ReplayEventTypeCustom } from './types';
-import { addEvent } from './util/addEvent';
+import { addEvent, addEventSync } from './util/addEvent';
 import { addGlobalListeners } from './util/addGlobalListeners';
 import { addMemoryEntry } from './util/addMemoryEntry';
 import { createBreadcrumb } from './util/createBreadcrumb';
@@ -692,7 +692,8 @@ export class ReplayContainer implements ReplayContainerInterface {
       });
 
       this.addUpdate(() => {
-        void addEvent(this, {
+        // Return `false` if the event _was_ added, as that means we schedule a flush
+        return !addEventSync(this, {
           type: ReplayEventTypeCustom,
           timestamp: breadcrumb.timestamp || 0,
           data: {

packages/replay/src/util/addEvent.ts

@@ -13,39 +13,46 @@ function isCustomEvent(event: RecordingEvent): event is ReplayFrameEvent {
 
 /**
  * Add an event to the event buffer.
+ * In contrast to `addEvent`, this does not return a promise & does not wait for the adding of the event to succeed/fail.
+ * Instead this returns `true` if we tried to add the event, else false.
+ * It returns `false` e.g. if we are paused, disabled, or out of the max replay duration.
+ *
+ * `isCheckout` is true if this is either the very first event, or an event triggered by `checkoutEveryNms`.
+ */
+export function addEventSync(replay: ReplayContainer, event: RecordingEvent, isCheckout?: boolean): boolean {
+  if (!shouldAddEvent(replay, event)) {
+    return false;
+  }
+
+  void _addEvent(replay, event, isCheckout);
+
+  return true;
+}
+
+/**
+ * Add an event to the event buffer.
+ * Resolves to `null` if no event was added, else to `void`.
+ *
  * `isCheckout` is true if this is either the very first event, or an event triggered by `checkoutEveryNms`.
  */
 export async function addEvent(
   replay: ReplayContainer,
   event: RecordingEvent,
   isCheckout?: boolean,
 ): Promise<AddEventResult | null> {
-  if (!replay.eventBuffer) {
-    // This implies that `_isEnabled` is false
+  if (!shouldAddEvent(replay, event)) {
     return null;
   }
 
-  if (replay.isPaused()) {
-    // Do not add to event buffer when recording is paused
-    return null;
-  }
-
-  const timestampInMs = timestampToMs(event.timestamp);
-
-  // Throw out events that happen more than 5 minutes ago. This can happen if
-  // page has been left open and idle for a long period of time and user
-  // comes back to trigger a new session. The performance entries rely on
-  // `performance.timeOrigin`, which is when the page first opened.
-  if (timestampInMs + replay.timeouts.sessionIdlePause < Date.now()) {
-    return null;
-  }
+  return _addEvent(replay, event, isCheckout);
+}
 
-  // Throw out events that are +60min from the initial timestamp
-  if (timestampInMs > replay.getContext().initialTimestamp + replay.getOptions().maxReplayDuration) {
-    logInfo(
-      `[Replay] Skipping event with timestamp ${timestampInMs} because it is after maxReplayDuration`,
-      replay.getOptions()._experiments.traceInternals,
-    );
+async function _addEvent(
+  replay: ReplayContainer,
+  event: RecordingEvent,
+  isCheckout?: boolean,
+): Promise<AddEventResult | null> {
+  if (!replay.eventBuffer) {
     return null;
   }
 
@@ -81,6 +88,34 @@ export async function addEvent(
   }
 }
 
+function shouldAddEvent(replay: ReplayContainer, event: RecordingEvent): boolean {
+  if (!replay.eventBuffer || replay.isPaused() || !replay.isEnabled()) {
+    // This implies that `_isEnabled` is false
+    return false;
+  }
+
+  const timestampInMs = timestampToMs(event.timestamp);
+
+  // Throw out events that happen more than 5 minutes ago. This can happen if
+  // page has been left open and idle for a long period of time and user
+  // comes back to trigger a new session. The performance entries rely on
+  // `performance.timeOrigin`, which is when the page first opened.
+  if (timestampInMs + replay.timeouts.sessionIdlePause < Date.now()) {
+    return false;
+  }
+
+  // Throw out events that are +60min from the initial timestamp
+  if (timestampInMs > replay.getContext().initialTimestamp + replay.getOptions().maxReplayDuration) {
+    logInfo(
+      `[Replay] Skipping event with timestamp ${timestampInMs} because it is after maxReplayDuration`,
+      replay.getOptions()._experiments.traceInternals,
+    );
+    return false;
+  }
+
+  return true;
+}
+
 function maybeApplyCallback(
   event: RecordingEvent,
   callback: ReplayPluginOptions['beforeAddRecordingEvent'],

packages/replay/src/util/handleRecordingEmit.ts

@@ -3,7 +3,7 @@ import { logger } from '@sentry/utils';
 
 import { saveSession } from '../session/saveSession';
 import type { AddEventResult, RecordingEvent, ReplayContainer, ReplayOptionFrameEvent } from '../types';
-import { addEvent } from './addEvent';
+import { addEvent, addEventSync } from './addEvent';
 import { logInfo } from './log';
 
 type RecordingEmitCallback = (event: RecordingEvent, isCheckout?: boolean) => void;
@@ -40,9 +40,12 @@ export function getHandleRecordingEmit(replay: ReplayContainer): RecordingEmitCa
         replay.setInitialState();
       }
 
-      // We need to clear existing events on a checkout, otherwise they are
-      // incremental event updates and should be appended
-      void addEvent(replay, event, isCheckout);
+      // If the event is not added (e.g. due to being paused, disabled, or out of the max replay duration),
+      // Skip all further steps
+      if (!addEventSync(replay, event, isCheckout)) {
+        // Return true to skip scheduling a debounced flush
+        return true;
+      }
 
       // Different behavior for full snapshots (type=2), ignore other event types
       // See https://github.com/rrweb-io/rrweb/blob/d8f9290ca496712aa1e7d472549480c4e7876594/packages/rrweb/src/types.ts#L16