Discard envelopes on 4xx and 5xx response (#4950)

* Discard envelopes on 4xx and 5xx response

* do not discard on 429

* changelog

* also discard cached envelope on 429

* move changelog; reword

* shorten changelog entry

Author: adinauer

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Changelog
 
+## Unreleased
+
+### Improvements
+
+- Discard envelopes on `4xx` and `5xx` response ([#4950](https://github.com/getsentry/sentry-java/pull/4950))
+  - This aims to not overwhelm Sentry after an outage or load shedding (including HTTP 429) where too many events are sent at once
+
+
 ## 8.29.0
 
 ### Fixes

sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java

@@ -309,18 +309,13 @@ public void run() {
 
             options.getLogger().log(SentryLevel.ERROR, message);
 
-            // ignore e.g. 429 as we're not the ones actively dropping
-            if (result.getResponseCode() >= 400 && result.getResponseCode() != 429) {
-              if (!cached) {
-                HintUtils.runIfDoesNotHaveType(
-                    hint,
-                    Retryable.class,
-                    (hint) -> {
-                      options
-                          .getClientReportRecorder()
-                          .recordLostEnvelope(
-                              DiscardReason.NETWORK_ERROR, envelopeWithClientReport);
-                    });
+            if (result.getResponseCode() >= 400) {
+              envelopeCache.discard(envelope);
+              // ignore e.g. 429 as we're not the ones actively dropping
+              if (result.getResponseCode() != 429) {
+                options
+                    .getClientReportRecorder()
+                    .recordLostEnvelope(DiscardReason.NETWORK_ERROR, envelopeWithClientReport);
               }
             }
 

sentry/src/test/java/io/sentry/transport/AsyncHttpTransportClientReportTest.kt

@@ -4,16 +4,20 @@ import io.sentry.SentryEnvelope
 import io.sentry.SentryOptions
 import io.sentry.SentryOptionsManipulator
 import io.sentry.Session
+import io.sentry.cache.IEnvelopeCache
 import io.sentry.clientreport.ClientReportTestHelper.Companion.retryableHint
 import io.sentry.clientreport.ClientReportTestHelper.Companion.retryableUncaughtExceptionHint
 import io.sentry.clientreport.ClientReportTestHelper.Companion.uncaughtExceptionHint
 import io.sentry.clientreport.DiscardReason
 import io.sentry.clientreport.IClientReportRecorder
 import io.sentry.dsnString
+import io.sentry.hints.Retryable
 import io.sentry.protocol.User
+import io.sentry.util.HintUtils
 import java.io.IOException
 import kotlin.test.Test
 import kotlin.test.assertFailsWith
+import kotlin.test.assertFalse
 import org.mockito.kotlin.any
 import org.mockito.kotlin.anyOrNull
 import org.mockito.kotlin.eq
@@ -31,11 +35,12 @@ class AsyncHttpTransportClientReportTest {
     var transportGate = mock<ITransportGate>()
     var executor = mock<QueuedThreadPoolExecutor>()
     var rateLimiter = mock<RateLimiter>()
+    var envelopeCache = mock<IEnvelopeCache>()
     var sentryOptions: SentryOptions =
       SentryOptions().apply {
         dsn = dsnString
         setSerializer(mock())
-        setEnvelopeDiskCache(mock())
+        setEnvelopeDiskCache(envelopeCache)
       }
     var clientReportRecorder = mock<IClientReportRecorder>()
     val envelopeBeforeAttachingClientReport =
@@ -66,23 +71,36 @@ class AsyncHttpTransportClientReportTest {
       .attachReportToEnvelope(same(fixture.envelopeBeforeAttachingClientReport))
     verify(fixture.clientReportRecorder, never()).recordLostEnvelope(any(), any())
     verifyNoMoreInteractions(fixture.clientReportRecorder)
+    verify(fixture.envelopeCache).discard(fixture.envelopeBeforeAttachingClientReport)
   }
 
   @Test
-  fun `does not record lost envelope on 500 error for retryable`() {
+  fun `records lost envelope on 500 error for retryable`() {
     // given
     givenSetup(TransportResult.error(500))
+    whenever(
+        fixture.envelopeCache.storeEnvelope(eq(fixture.envelopeBeforeAttachingClientReport), any())
+      )
+      .thenReturn(true)
 
     // when
+    val retryableHint = retryableHint()
     assertFailsWith(java.lang.IllegalStateException::class) {
-      fixture.getSUT().send(fixture.envelopeBeforeAttachingClientReport, retryableHint())
+      fixture.getSUT().send(fixture.envelopeBeforeAttachingClientReport, retryableHint)
     }
 
     // then
     verify(fixture.clientReportRecorder, times(1))
       .attachReportToEnvelope(same(fixture.envelopeBeforeAttachingClientReport))
-    verify(fixture.clientReportRecorder, never()).recordLostEnvelope(any(), any())
+    verify(fixture.clientReportRecorder, times(1))
+      .recordLostEnvelope(
+        eq(DiscardReason.NETWORK_ERROR),
+        same(fixture.envelopeAfterAttachingClientReport),
+      )
     verifyNoMoreInteractions(fixture.clientReportRecorder)
+    val sentrySdkHint = HintUtils.getSentrySdkHint(retryableHint)
+    assertFalse((sentrySdkHint as Retryable).isRetry)
+    verify(fixture.envelopeCache).discard(fixture.envelopeBeforeAttachingClientReport)
   }
 
   @Test
@@ -104,23 +122,36 @@ class AsyncHttpTransportClientReportTest {
         same(fixture.envelopeAfterAttachingClientReport),
       )
     verifyNoMoreInteractions(fixture.clientReportRecorder)
+    verify(fixture.envelopeCache).discard(fixture.envelopeBeforeAttachingClientReport)
   }
 
   @Test
-  fun `does not record lost envelope on 400 error for retryable`() {
+  fun `records lost envelope on 400 error for retryable`() {
     // given
     givenSetup(TransportResult.error(400))
+    whenever(
+        fixture.envelopeCache.storeEnvelope(eq(fixture.envelopeBeforeAttachingClientReport), any())
+      )
+      .thenReturn(true)
 
     // when
+    val retryableHint = retryableHint()
     assertFailsWith(java.lang.IllegalStateException::class) {
-      fixture.getSUT().send(fixture.envelopeBeforeAttachingClientReport, retryableHint())
+      fixture.getSUT().send(fixture.envelopeBeforeAttachingClientReport, retryableHint)
     }
 
     // then
     verify(fixture.clientReportRecorder, times(1))
       .attachReportToEnvelope(same(fixture.envelopeBeforeAttachingClientReport))
-    verify(fixture.clientReportRecorder, never()).recordLostEnvelope(any(), any())
+    verify(fixture.clientReportRecorder, times(1))
+      .recordLostEnvelope(
+        eq(DiscardReason.NETWORK_ERROR),
+        same(fixture.envelopeAfterAttachingClientReport),
+      )
     verifyNoMoreInteractions(fixture.clientReportRecorder)
+    val sentrySdkHint = HintUtils.getSentrySdkHint(retryableHint)
+    assertFalse((sentrySdkHint as Retryable).isRetry)
+    verify(fixture.envelopeCache).discard(fixture.envelopeBeforeAttachingClientReport)
   }
 
   @Test

sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt

@@ -107,7 +107,7 @@ class AsyncHttpTransportTest {
   }
 
   @Test
-  fun `stores envelope after unsuccessful send`() {
+  fun `discards envelope after unsuccessful send 500`() {
     // given
     val envelope = SentryEnvelope.from(fixture.sentryOptions.serializer, createSession(), null)
     whenever(fixture.transportGate.isConnected).thenReturn(true)
@@ -128,7 +128,59 @@ class AsyncHttpTransportTest {
     order.verify(fixture.sentryOptions.envelopeDiskCache).storeEnvelope(eq(envelope), anyOrNull())
 
     order.verify(fixture.connection).send(eq(envelope))
-    verify(fixture.sentryOptions.envelopeDiskCache, never()).discard(any())
+    order.verify(fixture.sentryOptions.envelopeDiskCache).discard(eq(envelope))
+  }
+
+  @Test
+  fun `discards envelope after unsuccessful send 400`() {
+    // given
+    val envelope = SentryEnvelope.from(fixture.sentryOptions.serializer, createSession(), null)
+    whenever(fixture.transportGate.isConnected).thenReturn(true)
+    whenever(fixture.rateLimiter.filter(eq(envelope), anyOrNull())).thenReturn(envelope)
+    whenever(fixture.connection.send(any())).thenReturn(TransportResult.error(400))
+
+    // when
+    try {
+      fixture.getSUT().send(envelope)
+    } catch (e: IllegalStateException) {
+      // expected - this is how the AsyncConnection signals failure to the executor for it to retry
+    }
+
+    // then
+    val order = inOrder(fixture.connection, fixture.sentryOptions.envelopeDiskCache)
+
+    // because storeBeforeSend is enabled by default
+    order.verify(fixture.sentryOptions.envelopeDiskCache).storeEnvelope(eq(envelope), anyOrNull())
+
+    order.verify(fixture.connection).send(eq(envelope))
+    order.verify(fixture.sentryOptions.envelopeDiskCache).discard(eq(envelope))
+  }
+
+  @Test
+  fun `discards envelope after unsuccessful send 429`() {
+    // given
+    val envelope = SentryEnvelope.from(fixture.sentryOptions.serializer, createSession(), null)
+    whenever(fixture.transportGate.isConnected).thenReturn(true)
+    whenever(fixture.rateLimiter.filter(eq(envelope), anyOrNull())).thenReturn(envelope)
+    whenever(fixture.sentryOptions.envelopeDiskCache.storeEnvelope(any(), anyOrNull()))
+      .thenReturn(true)
+    whenever(fixture.connection.send(any())).thenReturn(TransportResult.error(429))
+
+    // when
+    try {
+      fixture.getSUT().send(envelope)
+    } catch (e: IllegalStateException) {
+      // expected - this is how the AsyncConnection signals failure to the executor for it to retry
+    }
+
+    // then
+    val order = inOrder(fixture.connection, fixture.sentryOptions.envelopeDiskCache)
+
+    // because storeBeforeSend is enabled by default
+    order.verify(fixture.sentryOptions.envelopeDiskCache).storeEnvelope(eq(envelope), anyOrNull())
+
+    order.verify(fixture.connection).send(eq(envelope))
+    order.verify(fixture.sentryOptions.envelopeDiskCache).discard(eq(envelope))
   }
 
   @Test