fix: Don't synthesize event IDs for requires_event items - #6350
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
nit: I think having a metric/log in the existing NoEventId checks might be nice, just to confirm for us we did not break anything for the users. |
| #[cfg(feature = "processing")] | ||
| Self::NoEventId => Some(Outcome::Invalid( | ||
| crate::services::outcome::DiscardReason::Internal, | ||
| crate::services::outcome::DiscardReason::InvalidEventId, |
There was a problem hiding this comment.
For future reference, these are deprecated and we want to probably grab the EventID from the payload rather than the headers: INGEST-1174.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a534507. Configure here.
a534507 to
25fe72a
Compare
| if !has_event_id { | ||
| return Err(Error::NoEventId); | ||
| } |
There was a problem hiding this comment.
Bug: The new validation in validate.rs unconditionally rejects replays without an event_id, breaking backward compatibility for StandaloneRecording payloads from some SDKs.
Severity: HIGH
Suggested Fix
Modify the validation logic in validate::validate to not require an event_id for StandaloneRecording payloads. The check for replay.payload.event() should happen before the event_id check. If replay.payload.event() is None, the function should return Ok(()) to allow these specific payloads through, preserving backward compatibility.
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/replays/validate.rs#L18-L20
Potential issue: A new validation check in
`relay-server/src/processing/replays/validate.rs` requires all replays to have an
`event_id`. This check is performed before the payload type is evaluated. As a result,
`StandaloneRecording` payloads, which are intentionally supported for backward
compatibility with older SDKs and may not have an `event_id`, will be incorrectly
rejected. This contradicts a comment in `process.rs` which explicitly states this
support was added to "not break" SDKs that send recording items without event items.
These replays will be discarded with an `Error::NoEventId`.
There was a problem hiding this comment.
They were already meant to be removed, this check existed the entire time. It just didn't work because we invented an event ID on ingestion.
Co-authored-by: Joris Bayer <joris.bayer@sentry.io>
3 participants
Currently Relay fills in missing event IDs on envelopes if there are items present for which
requires_eventis true. This seems like a mistake both theoretically and practically:This PR fixes the problem by gating the synthesis behind
creates_event(false for attachments in general) instead ofrequires_event(true for all attachments).It also makes some additional improvements:
processso these attachments are rejected as early as possible.InternaltoInvalidEventId.Finally, it also adds an integration test.
Effect on other item types
Changing the event ID synthesis from
requires_eventtocreates_eventalso means that envelopes containing solelyFormData,UserReport,Profile, orReplayEventitems will no longer have an event ID synthesized. In the case ofUserReportandReplayEventthere is already a check that could previously never trip, exactly like the one for attachments.Fixes #6349. Fixes RELAY-278.