fix: discard corrupt cache files instead of looping on them (resulting in an OOM exception) - #5507
Conversation
Header read caps at 64 KB, and the discard log no longer reads the whole file.
|
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 |
|
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 Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5507 +/- ##
==========================================
- Coverage 74.71% 74.70% -0.01%
==========================================
Files 515 515
Lines 18930 18948 +18
Branches 3692 3696 +4
==========================================
+ Hits 14143 14155 +12
- Misses 3905 3909 +4
- Partials 882 884 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jamescrosswell
left a comment
jamescrosswell
left a comment
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| // Only corrupt files get here and they can be huge, so don't read the whole thing |
There was a problem hiding this comment.
| // 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 👍🏻
There was a problem hiding this comment.
These are all very good feedback, I'll get around to it at some point. For now, I just wanted our death loop fixed ^^
There was a problem hiding this comment.
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.
A fixed MaxLineLength on StreamExtensions baked "this is only ever used to read envelope headers, and those are small" into a general-purpose stream helper. The knowledge of what a reasonable length looks like belongs with the callers, and keeping it there means adding a field to a header can't silently invalidate an assumption living in an unrelated file. ReadLineAsync now takes an optional maxLength and only enforces a cap when one is supplied. Envelope and EnvelopeItem each declare their own limit next to a comment describing what that particular header actually contains. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Catching a whitelist of exception types is how this bug happened: the runaway
header read surfaces as OutOfMemoryException, or as IOException("Stream was too
long") once the buffer passes the 2 GB array limit, and neither is a JsonException,
so the file was never discarded and came back on every launch. Adding
InvalidDataException to the list fixes the case we hit but leaves us guessing at
what a parser can throw when handed arbitrary corrupted bytes.
Scope the try to the deserialize call instead and discard on anything other than
OperationCanceledException. A file we cannot deserialize is a file we can never
send, whatever the exception type. We may throw away the odd envelope on a
transient read error, which beats a corrupt file stalling the cache indefinitely.
The try deliberately covers only Envelope.DeserializeAsync rather than widening the
existing one, which also wrapped the send block. Those catches rethrow on
cancellation and on network-unavailable errors so the worker retries the file
later; swallowing them would delete every envelope buffered during an outage.
Narrowing that outer catch instead is no better, because IsNetworkUnavailableError
matches bare IOException and would classify the corrupt-file read as a network blip.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
risk: high
Co-authored-by: James Crosswell <jamescrosswell@users.noreply.github.com>
jamescrosswell
left a comment
jamescrosswell
left a comment
There was a problem hiding this comment.
@lgarczyn I merged in the changes we discussed (figured you might not easily find the time). Thank you very much for the contribution!
Thank you so much! |
jamescrosswell
changed the title
2 participants
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