Fail gracefully with malformed envelopes (#2371)

* Fixed #2357

* Updated Changelog

Author: jamescrosswell

CHANGELOG.md

@@ -20,6 +20,7 @@
  - NOTE: This is a potentially breaking change, as the `TracesSampleRate` property has been made nullable.
    Though extremely uncommon, if you are _retrieving_ the `TracesSampleRate` property for some reason, you will need to account for nulls.
    However, there is no change to the behavior or _typical_ usage of either of these properties.
+- CachedTransport gracefully handles malformed envelopes during processing  ([#2371](https://github.com/getsentry/sentry-dotnet/pull/2371))
 
 ### Dependencies
 

src/Sentry/Internal/Http/CachingTransport.cs

@@ -290,47 +290,59 @@ private async Task InnerProcessCacheAsync(string file, CancellationToken cancell
 
         _options.LogDebug("Reading cached envelope: {0}", file);
 
-        var stream = _fileSystem.OpenFileForReading(file);
+        try
+        {
+            var stream = _fileSystem.OpenFileForReading(file);
 #if NETFRAMEWORK || NETSTANDARD2_0
-        using (stream)
+            using (stream)
 #else
-        await using (stream.ConfigureAwait(false))
+            await using (stream.ConfigureAwait(false))
 #endif
-        using (var envelope = await Envelope.DeserializeAsync(stream, cancellation).ConfigureAwait(false))
-        {
-            // Don't even try to send it if we are requesting cancellation.
-            cancellation.ThrowIfCancellationRequested();
-
-            try
             {
-                _options.LogDebug("Sending cached envelope: {0}", envelope.TryGetEventId(_options.DiagnosticLogger));
+                using (var envelope = await Envelope.DeserializeAsync(stream, cancellation).ConfigureAwait(false))
+                {
+                    // Don't even try to send it if we are requesting cancellation.
+                    cancellation.ThrowIfCancellationRequested();
 
-                await _innerTransport.SendEnvelopeAsync(envelope, cancellation).ConfigureAwait(false);
-            }
-            // OperationCancel should not log an error
-            catch (OperationCanceledException ex)
-            {
-                _options.LogDebug("Canceled sending cached envelope: {0}, retrying after a delay.", ex, file);
-                // Let the worker catch, log, wait a bit and retry.
-                throw;
-            }
-            catch (Exception ex) when (ex is HttpRequestException or WebException or SocketException or IOException)
-            {
-                _options.LogError("Failed to send cached envelope: {0}, retrying after a delay.", ex, file);
-                // Let the worker catch, log, wait a bit and retry.
-                throw;
-            }
-            catch (Exception ex) when (ex.Source == "FakeFailingTransport")
-            {
-                // HACK: Deliberately sent from unit tests to avoid deleting the file from processing
-                return;
-            }
-            catch (Exception ex)
-            {
-                _options.ClientReportRecorder.RecordDiscardedEvents(DiscardReason.CacheOverflow, envelope);
-                LogFailureWithDiscard(file, ex);
+                    try
+                    {
+                        _options.LogDebug("Sending cached envelope: {0}",
+                            envelope.TryGetEventId(_options.DiagnosticLogger));
+
+                        await _innerTransport.SendEnvelopeAsync(envelope, cancellation).ConfigureAwait(false);
+                    }
+                    // OperationCancel should not log an error
+                    catch (OperationCanceledException ex)
+                    {
+                        _options.LogDebug("Canceled sending cached envelope: {0}, retrying after a delay.", ex, file);
+                        // Let the worker catch, log, wait a bit and retry.
+                        throw;
+                    }
+                    catch (Exception ex) when (ex is HttpRequestException or WebException or SocketException
+                                                   or IOException)
+                    {
+                        _options.LogError("Failed to send cached envelope: {0}, retrying after a delay.", ex, file);
+                        // Let the worker catch, log, wait a bit and retry.
+                        throw;
+                    }
+                    catch (Exception ex) when (ex.Source == "FakeFailingTransport")
+                    {
+                        // HACK: Deliberately sent from unit tests to avoid deleting the file from processing
+                        return;
+                    }
+                    catch (Exception ex)
+                    {
+                        _options.ClientReportRecorder.RecordDiscardedEvents(DiscardReason.CacheOverflow, envelope);
+                        LogFailureWithDiscard(file, ex);
+                    }
+                }
             }
         }
+        catch (JsonException ex)
+        {
+            // Log deserialization errors
+            LogFailureWithDiscard(file, ex);
+        }
 
         // Envelope & file stream must be disposed prior to reaching this point
 

test/Sentry.Tests/Internals/Http/CachingTransportTests.cs

@@ -292,6 +292,39 @@ public async Task AwareOfExistingFiles()
         innerTransport.GetSentEnvelopes().Should().HaveCount(3);
     }
 
+    [Fact]
+    public async Task Handle_Malformed_Envelopes_Gracefully()
+    {
+        // Arrange
+        using var cacheDirectory = new TempDirectory(_fileSystem);
+        var options = new SentryOptions
+        {
+            Dsn = ValidDsn,
+            DiagnosticLogger = _logger,
+            Debug = true,
+            CacheDirectoryPath = cacheDirectory.Path,
+            FileSystem = _fileSystem
+        };
+        var cacheDirectoryPath =
+            options.TryGetProcessSpecificCacheDirectoryPath() ??
+            throw new InvalidOperationException("Cache directory or DSN is not set.");
+        var processingDirectoryPath = Path.Combine(cacheDirectoryPath, "__processing");
+        var fileName = $"{Guid.NewGuid()}.envelope";
+        var filePath = Path.Combine(processingDirectoryPath, fileName);
+
+        _fileSystem.CreateDirectory(processingDirectoryPath);   // Create the processing directory
+        _fileSystem.CreateFileForWriting(filePath).Dispose(); // Make a malformed envelope... just an empty file
+        _fileSystem.FileExists(filePath).Should().BeTrue();
+
+        // Act
+        using var innerTransport = new FakeTransport();
+        await using var transport = CachingTransport.Create(innerTransport, options, startWorker: false);
+        await transport.FlushAsync(); // Flush the worker to process
+
+        // Assert
+        _fileSystem.FileExists(filePath).Should().BeFalse();
+    }
+
     [Fact]
     public async Task NonTransientExceptionShouldLog()
     {