Don't resend envelopes that get rejected by the server (#3938)

getsentry · Feb 9, 2025 · 337faa7 · 337faa7
1 parent d197cb2
commit 337faa7
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 2 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@
 ### Fixes
 
 - OTel activities that are marked as not recorded are no longer sent to Sentry ([#3890](https://github.com/getsentry/sentry-dotnet/pull/3890))
+- Fixed envelopes with oversized attachments getting stuck in __processing ([#3938](https://github.com/getsentry/sentry-dotnet/pull/3938))
 - Unknown stack frames in profiles on .NET 8+ ([#3942](https://github.com/getsentry/sentry-dotnet/pull/3942))
 - Deduplicate profiling stack frames ([#3941](https://github.com/getsentry/sentry-dotnet/pull/3941))
 

diff --git a/src/Sentry/Internal/DiscardReason.cs b/src/Sentry/Internal/DiscardReason.cs
@@ -4,6 +4,7 @@ namespace Sentry.Internal;
 {
     // See https://develop.sentry.dev/sdk/client-reports/ for list
     public static DiscardReason BeforeSend = new("before_send");
+    public static DiscardReason BufferOverflow = new("buffer_overflow");
     public static DiscardReason CacheOverflow = new("cache_overflow");
     public static DiscardReason EventProcessor = new("event_processor");
     public static DiscardReason NetworkError = new("network_error");

diff --git a/src/Sentry/Internal/Http/CachingTransport.cs b/src/Sentry/Internal/Http/CachingTransport.cs
@@ -300,13 +300,22 @@ private async Task ProcessCacheAsync(CancellationToken cancellation)
         }
     }
 
-    private static bool IsNetworkError(Exception exception) =>
+    private static bool IsNetworkUnavailableError(Exception exception) =>
         exception switch
         {
+            // TODO: Could we make this more specific? Lots of these errors are unrelated to network availability.
             HttpRequestException or WebException or IOException or SocketException => true,
             _ => false
         };
 
+    private static bool IsRejectedByServer(Exception ex)
+    {
+        // When envelopes are too big, the server will reset the connection as soon as the maximum size is exceeded
+        // (it doesn't wait for us to finish sending the whole envelope).
+        return ex is SocketException { ErrorCode: 32 /* Broken pipe */ }
+            || (ex.InnerException is { } innerException && IsRejectedByServer(innerException));
+    }
+
     private async Task InnerProcessCacheAsync(string file, CancellationToken cancellation)
     {
         if (_options.NetworkStatusListener is { Online: false } listener)
@@ -346,8 +355,15 @@ private async Task InnerProcessCacheAsync(string file, CancellationToken cancell
                         // Let the worker catch, log, wait a bit and retry.
                         throw;
                     }
-                    catch (Exception ex) when (IsNetworkError(ex))
+                    catch (Exception ex) when (IsRejectedByServer(ex))
+                    {
+                        _options.ClientReportRecorder.RecordDiscardedEvents(DiscardReason.BufferOverflow, envelope);
+                        _options.LogError(ex, "Failed to send cached envelope: {0}. The envelope is likely too big and will be discarded.", file);
+                    }
+                    catch (Exception ex) when (IsNetworkUnavailableError(ex))
                     {
+                        // TODO: Envelopes could end up in an infinite loop here. We should consider implementing some
+                        // kind backoff strategy and a retry limit... then drop the envelopes if the limit is exceeded.
                         if (_options.NetworkStatusListener is PollingNetworkStatusListener pollingListener)
                         {
                             pollingListener.Online = false;

diff --git a/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs b/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs
@@ -711,6 +711,43 @@ public async Task TransportResumesWhenNetworkComesBackOnline()
         envelopes.Should().NotBeEmpty();
     }
 
+    [Fact]
+    public async Task FlushAsync_RejectedByServer_DiscardsEnvelope()
+    {
+        // Arrange
+        var listener = Substitute.For<INetworkStatusListener>();
+        listener.Online.Returns(_ => true);
+
+        using var cacheDirectory = new TempDirectory();
+        var options = new SentryOptions
+        {
+            Dsn = ValidDsn,
+            DiagnosticLogger = _logger,
+            Debug = true,
+            CacheDirectoryPath = cacheDirectory.Path,
+            NetworkStatusListener = listener,
+            ClientReportRecorder = Substitute.For<IClientReportRecorder>()
+        };
+
+        using var envelope = Envelope.FromEvent(new SentryEvent());
+
+        var innerTransport = Substitute.For<ITransport>();
+        innerTransport.SendEnvelopeAsync(Arg.Any<Envelope>(), Arg.Any<CancellationToken>())
+            .Returns(_ => throw new SocketException(32 /* Bad pipe exception */));
+        await using var transport = CachingTransport.Create(innerTransport, options, startWorker: false);
+
+        // Act
+        await transport.SendEnvelopeAsync(envelope);
+        await transport.FlushAsync();
+
+        // Assert
+        foreach (var item in envelope.Items)
+        {
+            options.ClientReportRecorder.Received(1)
+                .RecordDiscardedEvent(DiscardReason.BufferOverflow, item.DataCategory);
+        }
+    }
+
     [Fact]
     public async Task DoesntWriteSentAtHeaderToCacheFile()
     {