feat(minidump): Allow multiple exceptions#6063
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| minidump, | ||
| ctx.processing.project_info | ||
| ); | ||
| metrics.bytes_ingested_event_minidump = (minidump.attachment_body_size() as u64).into(); | ||
| } | ||
| if let Some(acr) = &apple_crash_report { | ||
| crate::utils::process_apple_crash_report( | ||
| event.get_or_insert_with(Default::default) | ||
| event.get_or_insert_with(Default::default), ctx.processing.project_info | ||
| ); | ||
| metrics.bytes_ingested_event_applecrashreport = (acr.len() as u64).into(); | ||
| } |
There was a problem hiding this comment.
Bug: With the MinidumpMultiException feature enabled, processing an Unreal report with both a minidump and Apple Crash Report incorrectly places the Apple placeholder first, violating the minidump-first requirement.
Severity: MEDIUM
Suggested Fix
To ensure the minidump placeholder remains at index 0, modify the logic for when both report types are present. One solution is to insert the Apple Crash Report placeholder at index 1 instead of 0. Alternatively, add a check to see if a minidump placeholder already exists and adjust the insertion logic accordingly.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: relay-server/src/processing/errors/errors/unreal.rs#L103-L115
Potential issue: When the `MinidumpMultiException` feature flag is enabled for an Unreal
Engine project, and a crash report contains both a minidump and an Apple Crash Report,
the processing logic results in an incorrect exception order. The `process_minidump`
function correctly adds a placeholder at index 0, but the subsequent call to
`process_apple_crash_report` also inserts its placeholder at index 0. This pushes the
minidump placeholder to index 1, violating the requirement that the minidump placeholder
must be the first exception in the list for downstream processing.
Also affects:
relay-server/src/utils/native.rs:71~96
Did we get this right? 👍 / 👎 to inform future reviews.
| event.get_or_insert_with(Default::default), | ||
| minidump, | ||
| ctx.processing.project_info | ||
| ); | ||
| metrics.bytes_ingested_event_minidump = (minidump.attachment_body_size() as u64).into(); | ||
| } | ||
| if let Some(acr) = &apple_crash_report { | ||
| crate::utils::process_apple_crash_report( | ||
| event.get_or_insert_with(Default::default) | ||
| event.get_or_insert_with(Default::default), ctx.processing.project_info | ||
| ); | ||
| metrics.bytes_ingested_event_applecrashreport = (acr.len() as u64).into(); |
There was a problem hiding this comment.
Bug: With MinidumpMultiException enabled, an Unreal event with both minidump and Apple Crash Report attachments will incorrectly place the ACR placeholder before the minidump placeholder.
Severity: MEDIUM
Suggested Fix
The logic for adding native placeholders in native.rs should be modified. Before inserting a new native placeholder like an Apple Crash Report, the code should check if a minidump placeholder already exists at index 0. If it does, the new placeholder should be inserted at index 1 or appended to the list, rather than being prepended, to preserve the minidump placeholder's primary position.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: relay-server/src/processing/errors/errors/unreal.rs#L104-L114
Potential issue: When the `MinidumpMultiException` feature flag is enabled, processing
an Unreal event that contains both a minidump and an Apple Crash Report attachment will
lead to an incorrect exception order. The code first calls `process_minidump`, which
correctly inserts a minidump placeholder at the start of the exceptions list. It then
calls `process_apple_crash_report`, which also inserts its placeholder at the start of
the list, pushing the minidump placeholder to the second position. This violates the
documented requirement that the minidump placeholder must be the first exception, which
can cause incorrect processing by Sentry consumers.
Also affects:
relay-server/src/utils/native.rs:71~96
1 participant
Sentry consumers except the first exception in the minidump event to be the minidump placeholder. But AFAICT there's no reason to not allow additional exception entries after the placeholder. Relax the condition behind a feature flag so we can test it with an internal project.
ref: #6046