From 8ed1dce0272709a35ed72f99ec0f0852940159cf Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Wed, 3 Dec 2025 12:59:41 +0100 Subject: [PATCH 1/3] Discard envelopes on 4xx and 5xx response --- .../sentry/transport/AsyncHttpTransport.java | 19 +++---- .../AsyncHttpTransportClientReportTest.kt | 45 ++++++++++++--- .../transport/AsyncHttpTransportTest.kt | 56 ++++++++++++++++++- 3 files changed, 99 insertions(+), 21 deletions(-) diff --git a/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java b/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java index e6f1be3d952..b664302f1e0 100644 --- a/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java +++ b/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); } } diff --git a/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportClientReportTest.kt b/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportClientReportTest.kt index efb9597277d..ec05af3df36 100644 --- a/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportClientReportTest.kt +++ b/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() var executor = mock() var rateLimiter = mock() + var envelopeCache = mock() var sentryOptions: SentryOptions = SentryOptions().apply { dsn = dsnString setSerializer(mock()) - setEnvelopeDiskCache(mock()) + setEnvelopeDiskCache(envelopeCache) } var clientReportRecorder = mock() 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 diff --git a/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt b/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt index 27fc7dba0cb..90bd05069ef 100644 --- a/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt +++ b/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 From 2be9cd7acaa3347f86540e7e5f430e2525011fa5 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Wed, 3 Dec 2025 14:17:15 +0100 Subject: [PATCH 2/3] do not discard on 429 --- .../java/io/sentry/transport/AsyncHttpTransport.java | 12 +++++------- .../io/sentry/transport/AsyncHttpTransportTest.kt | 4 ++-- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java b/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java index b664302f1e0..984943cf03b 100644 --- a/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java +++ b/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java @@ -309,14 +309,12 @@ public void run() { options.getLogger().log(SentryLevel.ERROR, message); - if (result.getResponseCode() >= 400) { + // ignore e.g. 429 as we're not the ones actively dropping + if (result.getResponseCode() >= 400 && result.getResponseCode() != 429) { 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); - } + options + .getClientReportRecorder() + .recordLostEnvelope(DiscardReason.NETWORK_ERROR, envelopeWithClientReport); } throw new IllegalStateException(message); diff --git a/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt b/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt index 90bd05069ef..a0aac4888ec 100644 --- a/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt +++ b/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt @@ -157,7 +157,7 @@ class AsyncHttpTransportTest { } @Test - fun `discards envelope after unsuccessful send 429`() { + fun `stores envelope after unsuccessful send 429`() { // given val envelope = SentryEnvelope.from(fixture.sentryOptions.serializer, createSession(), null) whenever(fixture.transportGate.isConnected).thenReturn(true) @@ -180,7 +180,7 @@ class AsyncHttpTransportTest { order.verify(fixture.sentryOptions.envelopeDiskCache).storeEnvelope(eq(envelope), anyOrNull()) order.verify(fixture.connection).send(eq(envelope)) - order.verify(fixture.sentryOptions.envelopeDiskCache).discard(eq(envelope)) + verify(fixture.sentryOptions.envelopeDiskCache, never()).discard(any()) } @Test From 999fe4f3cf3c4e101fd2783c96ead41418cf4b05 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Wed, 3 Dec 2025 14:25:03 +0100 Subject: [PATCH 3/3] changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cce62395402..b947bc575f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,9 @@ SentryAndroid.init( - Avoid forking `rootScopes` for Reactor if current thread has `NoOpScopes` ([#4793](https://github.com/getsentry/sentry-java/pull/4793)) - This reduces the SDKs overhead by avoiding unnecessary scope forks +- 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 where too many events are sent at once + - We also do not have to re-send on a `4xx` since that's likely an SDK (e.g. wrong serialization) or envelope problem (e.g. too large) and sending it again won't change the result ### Fixes