fix: discard corrupt cache files instead of OOM looping on them

[lgarczyn]

Header read caps at 64 KB, and the discard log no longer reads the whole file.

One mac dev was stuck at 300+Gb usage because sentry was trying to read aarge corrupted dump

test: cover the corrupt cache discard through SentrySdk.Init

Closes #5510

[jamescrosswell]

Hi @lgarczyn - thanks for the contribution.

Could I get a bit of context for this? I don't think any issue was raised. What circumstances were you running into problems? What happens vs what you expect to happen? Is there an easy way to reproduce this?

Thanks in advance.

[lgarczyn]

Hi @lgarczyn - thanks for the contribution.

Could I get a bit of context for this? I don't think any issue was raised. What circumstances were you running into problems? What happens vs what you expect to happen? Is there an easy way to reproduce this?

Thanks in advance.

Hello!

One of our designer's mac computer crashed.

When it restarted, it was extremely sluggish, with insane memory usage.

Trying to debug it, we found out sentry was trying to load a giant log or dmp, failing, and then just trying again.

This is to try and mitigate it

[lgarczyn]

A crash mid-write leaves a big NUL-filled envelope in the cache.

ReadLineAsync has no length cap, so the header read OOMs.

OutOfMemoryException isn't JsonException, so the discard catch never fires

MoveUnprocessedFilesBackToCache starts the loop again the file every launch.

Manual fix: Deleting the cache by hand.

This PR: Cap the header read at 64 KB, route InvalidDataException through the existing discard, and limit LogFailureWithDiscard, so it doesn't try to pickup 100Gb file.

Test: cover the corrupt cache discard through SentrySdk.Init. i didn't try to reproduce an actual OOM, because, tbh, I'm not sure how'd you'd test that.

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.76%. Comparing base (4425201) to head (f459788).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Sentry/Internal/Http/CachingTransport.cs	76.47%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5507      +/-   ##
==========================================
- Coverage   74.76%   74.76%   -0.01%     
==========================================
  Files         513      513              
  Lines       18759    18772      +13     
  Branches     3669     3673       +4     
==========================================
+ Hits        14025    14034       +9     
- Misses       3863     3865       +2     
- Partials      871      873       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jamescrosswell]

Thanks @lgarczyn - that makes sense. I've raised #5510 to capture the key context.

Generally your change looks good. I made a couple of suggestions to your code.

Additionally, it might be nice to address a similar issue in EnvelopeItem.DeserializePayloadAsync... this takes length straight from the item header and does (int)(payloadLength ?? stream.Length). A header with a bogus length gives either an unchecked overflow or an OOM, and a large file with no length key at all overflows on (int)stream.Length.

That one is maybe a bit trickier... Ideally we'd validate the length against the remaining stream length and throw InvalidDataException if there was an inconsistency... so something like:

if (payloadLength is > int.MaxValue or < 0)
{
    throw new InvalidDataException($"Envelope item length {payloadLength} is not a valid buffer size.");
}
var remaining = stream.Length - stream.Position;
if (payloadLength > remaining)
{
    throw new InvalidDataException($"Envelope item claims {payloadLength} bytes but only {remaining} remain.");
}

That should work for us since we ensure CanSeek for the stream... meaning we shouldn't get NotSupportedException on Stream.Length.

[jamescrosswell]

This bakes an assumption ("this extension method will just be used to read headers, which are small") into a general-purpose stream helper.

Maybe instead we have an optional nullable maxLength parameter that Envelope/EnvelopeItem pass in (the callers carry context for what a reasonable length might be in the specific situation it's being used).

[jamescrosswell]

This fixes the case you ran into. If someone discovers a new possible error in this code path, we'd have to make another fix to whitelist the new exception type.

For example,Envelope.DeserializeHeaderAsync throws InvalidOperationException("Envelope header is malformed.") when the header is valid JSON but not an object.

I'd suggest wrapping just the Envelope.DeserializeAsync call in its own try..catch and discarding on anything except OperationCanceledException. We might throw away the odd file on a transient disk error or something but I think that's better than locking up the process with an OOM.

[jamescrosswell]

Suggested change

	// Only corrupt files get here and they can be huge, so don't read the whole thing
	/// <summary>
	/// Only corrupt files get here and they can be huge, so don't read the whole thing
	/// </summary>

Just for consistency... we usually XML comment methods in the repo (even if they're private).

It's a good change though 👍🏻

[lgarczyn]

These are all very good feedback, I'll get around to it at some point. For now, I just wanted our death loop fixed ^^

[jamescrosswell]

I'll get around to it at some point. For now, I just wanted our death loop fixed

If you don't have time to implement those changes, I can do it from my side...

I agree we should get this out as soon as possible but worth making the changes now before a merge - it's very hard to circle back on things otherwise and we just accumulate tech debt.