fix(core): Exclude client reports from offline queuing (#7226)

Author: timfish

packages/core/src/transports/offline.ts

@@ -1,22 +1,10 @@
 import type { Envelope, InternalBaseTransportOptions, Transport, TransportMakeRequestResponse } from '@sentry/types';
-import { forEachEnvelopeItem, logger, parseRetryAfterHeader } from '@sentry/utils';
+import { envelopeContainsItemType, logger, parseRetryAfterHeader } from '@sentry/utils';
 
 export const MIN_DELAY = 100; // 100 ms
 export const START_DELAY = 5_000; // 5 seconds
 const MAX_DELAY = 3.6e6; // 1 hour
 
-function isReplayEnvelope(envelope: Envelope): boolean {
-  let isReplay = false;
-
-  forEachEnvelopeItem(envelope, (_, type) => {
-    if (type === 'replay_event') {
-      isReplay = true;
-    }
-  });
-
-  return isReplay;
-}
-
 function log(msg: string, error?: Error): void {
   __DEBUG_BUILD__ && logger.info(`[Offline]: ${msg}`, error);
 }
@@ -74,7 +62,8 @@ export function makeOfflineTransport<TO>(
       // We don't queue Session Replay envelopes because they are:
       // - Ordered and Replay relies on the response status to know when they're successfully sent.
       // - Likely to fill the queue quickly and block other events from being sent.
-      if (isReplayEnvelope(env)) {
+      // We also want to drop client reports because they can be generated when we retry sending events while offline.
+      if (envelopeContainsItemType(env, ['replay_event', 'replay_recording', 'client_report'])) {
         return false;
       }
 

packages/core/test/lib/transports/offline.test.ts

@@ -1,4 +1,5 @@
 import type {
+  ClientReport,
   Envelope,
   EventEnvelope,
   EventItem,
@@ -9,6 +10,7 @@ import type {
   TransportMakeRequestResponse,
 } from '@sentry/types';
 import {
+  createClientReportEnvelope,
   createEnvelope,
   createEventEnvelopeHeaders,
   dsnFromString,
@@ -54,6 +56,25 @@ const RELAY_ENVELOPE = createEnvelope<ReplayEnvelope>(
   ],
 );
 
+const DEFAULT_DISCARDED_EVENTS: ClientReport['discarded_events'] = [
+  {
+    reason: 'before_send',
+    category: 'error',
+    quantity: 30,
+  },
+  {
+    reason: 'network_error',
+    category: 'transaction',
+    quantity: 23,
+  },
+];
+
+const CLIENT_REPORT_ENVELOPE = createClientReportEnvelope(
+  DEFAULT_DISCARDED_EVENTS,
+  'https://[email protected]/1337',
+  123456,
+);
+
 const transportOptions = {
   recordDroppedEvent: () => undefined, // noop
   textEncoder: new TextEncoder(),
@@ -288,6 +309,23 @@ describe('makeOfflineTransport', () => {
     expect(getCalls()).toEqual([]);
   });
 
+  it('should not store client report envelopes on send failure', async () => {
+    const { getCalls, store } = createTestStore();
+    const { getSendCount, baseTransport } = createTestTransport(new Error());
+    const queuedCount = 0;
+    const transport = makeOfflineTransport(baseTransport)({
+      ...transportOptions,
+      createStore: store,
+      shouldStore: () => true,
+    });
+    const result = transport.send(CLIENT_REPORT_ENVELOPE);
+
+    await expect(result).rejects.toBeInstanceOf(Error);
+    expect(queuedCount).toEqual(0);
+    expect(getSendCount()).toEqual(0);
+    expect(getCalls()).toEqual([]);
+  });
+
   it(
     'Follows the Retry-After header',
     async () => {

packages/utils/src/envelope.ts

@@ -6,7 +6,6 @@ import type {
   DataCategory,
   DsnComponents,
   Envelope,
-  EnvelopeItem,
   EnvelopeItemType,
   Event,
   EventEnvelopeHeaders,
@@ -41,16 +40,32 @@ export function addItemToEnvelope<E extends Envelope>(envelope: E, newItem: E[1]
 /**
  * Convenience function to loop through the items and item types of an envelope.
  * (This function was mostly created because working with envelope types is painful at the moment)
+ *
+ * If the callback returns true, the rest of the items will be skipped.
  */
 export function forEachEnvelopeItem<E extends Envelope>(
   envelope: Envelope,
-  callback: (envelopeItem: E[1][number], envelopeItemType: E[1][number][0]['type']) => void,
-): void {
+  callback: (envelopeItem: E[1][number], envelopeItemType: E[1][number][0]['type']) => boolean | void,
+): boolean {
   const envelopeItems = envelope[1];
-  envelopeItems.forEach((envelopeItem: EnvelopeItem) => {
+
+  for (const envelopeItem of envelopeItems) {
     const envelopeItemType = envelopeItem[0].type;
-    callback(envelopeItem, envelopeItemType);
-  });
+    const result = callback(envelopeItem, envelopeItemType);
+
+    if (result) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
+/**
+ * Returns true if the envelope contains any of the given envelope item types
+ */
+export function envelopeContainsItemType(envelope: Envelope, types: EnvelopeItemType[]): boolean {
+  return forEachEnvelopeItem(envelope, (_, type) => types.includes(type));
 }
 
 /**