Merge pull request #6165 from getsentry/fix/callback-error-handling-replay-sampling

fix(core): [Callback Errors 7] Handle replay sampling failures

Author: adinauer

CHANGELOG.md

@@ -64,6 +64,7 @@
   - Add `DiscardReason.CALLBACK_ERROR` and use it for telemetry dropped when a `beforeSend*` callback throws. `OnDiscardCallback` can now receive this value.
   - Drop telemetry and record `callback_error` when a customer event processor throws instead of continuing with a potentially partially processed item. SDK-owned processor failures are logged and processing continues without a `callback_error` client report.
   - Drop breadcrumbs when `beforeBreadcrumb` throws instead of storing exception details on the breadcrumb.
+  - Skip replay capture when `beforeErrorSampling` throws, while still sending the error event ([#6165](https://github.com/getsentry/sentry-java/pull/6165))
   - When `tracesSampler` throws, drop the transaction and record `callback_error` instead of inheriting the parent sampling decision or falling back to `tracesSampleRate` ([#6163](https://github.com/getsentry/sentry-java/pull/6163))
   - When `profilesSampler` throws, disable profiling instead of falling back to `profilesSampleRate` or inheriting the parent's profiling decision. Trace sampling is unchanged ([#6164](https://github.com/getsentry/sentry-java/pull/6164))
 - Disable URL caching when reading `META-INF/MANIFEST.MF` files during version detection so that the SDK no longer keeps jar file handles open for the life of the process ([#6124](https://github.com/getsentry/sentry-java/pull/6124)

sentry/src/main/java/io/sentry/SentryClient.java

@@ -233,9 +233,14 @@ private boolean shouldApplyScopeData(final @NotNull CheckIn event, final @NotNul
               .getLogger()
               .log(
                   SentryLevel.ERROR,
-                  "The beforeErrorSampling callback threw an exception. Proceeding with replay capture.",
+                  "The beforeErrorSampling callback threw an exception. Skipping replay capture.",
                   e);
-          shouldCaptureReplay = true;
+          if (!SentryId.EMPTY_ID.equals(options.getReplayController().getReplayId())) {
+            options
+                .getClientReportRecorder()
+                .recordLostEvent(DiscardReason.CALLBACK_ERROR, DataCategory.Replay);
+          }
+          shouldCaptureReplay = false;
         }
       }
       if (shouldCaptureReplay) {

sentry/src/test/java/io/sentry/SentryClientTest.kt

@@ -3973,24 +3973,78 @@ class SentryClientTest {
   }
 
   @Test
-  fun `beforeErrorSampling throwing exception proceeds with captureReplay`() {
-    var called = false
-    fixture.sentryOptions.setReplayController(
-      object : ReplayController by NoOpReplayController.getInstance() {
-        override fun captureReplay(isTerminating: Boolean?): SentryId {
-          called = true
-          return SentryId.EMPTY_ID
+  fun `beforeErrorSampling throwing exception skips replay but still sends errors and crashes`() {
+    val replayController = mock<ReplayController>()
+    whenever(replayController.replayId).thenReturn(SentryId())
+    whenever(replayController.captureReplay(anyOrNull())).thenReturn(SentryId.EMPTY_ID)
+    fixture.sentryOptions.setReplayController(replayController)
+    val logger = mock<ILogger>()
+    fixture.sentryOptions.setLogger(logger)
+    val exception = RuntimeException("test")
+    fixture.sentryOptions.sessionReplay.beforeErrorSampling =
+      SentryReplayOptions.BeforeErrorSamplingCallback { _, _ ->
+        throw exception
+      }
+    val sut = fixture.getSut()
+    val events =
+      listOf(true, false).map { handled ->
+        SentryEvent().apply {
+          exceptions =
+            listOf(
+              SentryException().apply { mechanism = Mechanism().apply { isHandled = handled } }
+            )
         }
       }
+
+    events.forEach { event ->
+      assertThat(sut.captureEvent(event)).isEqualTo(event.eventId)
+    }
+
+    verify(replayController, never()).captureReplay(anyOrNull())
+    val envelopes = argumentCaptor<SentryEnvelope>()
+    verify(fixture.transport, times(2)).send(envelopes.capture(), anyOrNull())
+    assertThat(envelopes.allValues.map { getEventFromData(it.items.first().data).eventId })
+      .containsExactlyElementsIn(events.map { it.eventId })
+      .inOrder()
+    verify(logger, times(2))
+      .log(
+        SentryLevel.ERROR,
+        "The beforeErrorSampling callback threw an exception. Skipping replay capture.",
+        exception,
+      )
+    assertClientReport(
+      fixture.sentryOptions.clientReportRecorder,
+      listOf(DiscardedEvent(DiscardReason.CALLBACK_ERROR.reason, DataCategory.Replay.category, 2)),
     )
+  }
+
+  @Test
+  fun `beforeErrorSampling throwing exception does not prevent replay capture for subsequent events`() {
+    val replayController = mock<ReplayController>()
+    whenever(replayController.replayId).thenReturn(SentryId.EMPTY_ID)
+    whenever(replayController.captureReplay(anyOrNull())).thenReturn(SentryId.EMPTY_ID)
+    fixture.sentryOptions.setReplayController(replayController)
+    var invocations = 0
     fixture.sentryOptions.sessionReplay.beforeErrorSampling =
       SentryReplayOptions.BeforeErrorSamplingCallback { _, _ ->
-        throw RuntimeException("test")
+        invocations++
+        if (invocations == 1) {
+          throw RuntimeException("test")
+        }
+        true
       }
     val sut = fixture.getSut()
 
     sut.captureEvent(SentryEvent().apply { exceptions = listOf(SentryException()) })
-    assertTrue(called)
+    verify(replayController, never()).captureReplay(anyOrNull())
+
+    val event = SentryEvent().apply { exceptions = listOf(SentryException()) }
+    assertThat(sut.captureEvent(event)).isEqualTo(event.eventId)
+
+    assertThat(invocations).isEqualTo(2)
+    verify(replayController).captureReplay(false)
+    verify(fixture.transport, times(2)).send(any(), anyOrNull())
+    assertClientReport(fixture.sentryOptions.clientReportRecorder, emptyList())
   }
 
   @Test