From 31169c33f939b48358aefeb9a47d2028a556f49d Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Wed, 5 Feb 2025 16:47:24 +1300 Subject: [PATCH 1/4] Envelopes that are too big and get rejected by the server do not get sent again --- src/Sentry/Internal/DiscardReason.cs | 1 + src/Sentry/Internal/Http/CachingTransport.cs | 20 ++++++++++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/Sentry/Internal/DiscardReason.cs b/src/Sentry/Internal/DiscardReason.cs index d132f761ba..3c5d3d8ed9 100644 --- a/src/Sentry/Internal/DiscardReason.cs +++ b/src/Sentry/Internal/DiscardReason.cs @@ -7,6 +7,7 @@ namespace Sentry.Internal; public static DiscardReason CacheOverflow = new("cache_overflow"); public static DiscardReason EventProcessor = new("event_processor"); public static DiscardReason NetworkError = new("network_error"); + public static DiscardReason SendError = new("send_error"); public static DiscardReason QueueOverflow = new("queue_overflow"); public static DiscardReason RateLimitBackoff = new("ratelimit_backoff"); public static DiscardReason SampleRate = new("sample_rate"); diff --git a/src/Sentry/Internal/Http/CachingTransport.cs b/src/Sentry/Internal/Http/CachingTransport.cs index f6d4c9b856..8f9a6eb973 100644 --- 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(ex.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.SendError, 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; From 75ad4c88e7af2cbad0dbdf77b38f9b62cfbc3fe9 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Thu, 6 Feb 2025 11:42:53 +1300 Subject: [PATCH 2/4] Added unit test --- .../Internals/Http/CachingTransportTests.cs | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs b/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs index 05df93f02b..5a22302dbd 100644 --- 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(); + 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() + }; + + using var envelope = Envelope.FromEvent(new SentryEvent()); + + var innerTransport = Substitute.For(); + innerTransport.SendEnvelopeAsync(Arg.Any(), Arg.Any()) + .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.SendError, item.DataCategory); + } + } + [Fact] public async Task DoesntWriteSentAtHeaderToCacheFile() { From 2529ab5602e3962ab45f06a087fcb8a69dd12f80 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Thu, 6 Feb 2025 11:46:13 +1300 Subject: [PATCH 3/4] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index af09754d95..b69eb7d39c 100644 --- 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)) ## 5.1.0 From 53de776ac343a923ed56445a4cd596e2e76eab80 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Fri, 7 Feb 2025 11:56:20 +1300 Subject: [PATCH 4/4] Review feedback --- src/Sentry/Internal/DiscardReason.cs | 2 +- src/Sentry/Internal/Http/CachingTransport.cs | 4 ++-- test/Sentry.Tests/Internals/Http/CachingTransportTests.cs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Sentry/Internal/DiscardReason.cs b/src/Sentry/Internal/DiscardReason.cs index 3c5d3d8ed9..11a35fa2a3 100644 --- a/src/Sentry/Internal/DiscardReason.cs +++ b/src/Sentry/Internal/DiscardReason.cs @@ -4,10 +4,10 @@ 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"); - public static DiscardReason SendError = new("send_error"); public static DiscardReason QueueOverflow = new("queue_overflow"); public static DiscardReason RateLimitBackoff = new("ratelimit_backoff"); public static DiscardReason SampleRate = new("sample_rate"); diff --git a/src/Sentry/Internal/Http/CachingTransport.cs b/src/Sentry/Internal/Http/CachingTransport.cs index 8f9a6eb973..b5ed2cb34a 100644 --- a/src/Sentry/Internal/Http/CachingTransport.cs +++ b/src/Sentry/Internal/Http/CachingTransport.cs @@ -313,7 +313,7 @@ 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(ex.InnerException)); + || (ex.InnerException is { } innerException && IsRejectedByServer(innerException)); } private async Task InnerProcessCacheAsync(string file, CancellationToken cancellation) @@ -357,7 +357,7 @@ private async Task InnerProcessCacheAsync(string file, CancellationToken cancell } catch (Exception ex) when (IsRejectedByServer(ex)) { - _options.ClientReportRecorder.RecordDiscardedEvents(DiscardReason.SendError, envelope); + _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)) diff --git a/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs b/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs index 5a22302dbd..e903b7f3ca 100644 --- a/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs +++ b/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs @@ -744,7 +744,7 @@ public async Task FlushAsync_RejectedByServer_DiscardsEnvelope() foreach (var item in envelope.Items) { options.ClientReportRecorder.Received(1) - .RecordDiscardedEvent(DiscardReason.SendError, item.DataCategory); + .RecordDiscardedEvent(DiscardReason.BufferOverflow, item.DataCategory); } }