fix: drop screenshots for events filtered by before_send#2642
Draft
JoshuaMoelans wants to merge 2 commits intomainfrom
Draft
fix: drop screenshots for events filtered by before_send#2642JoshuaMoelans wants to merge 2 commits intomainfrom
before_send#2642JoshuaMoelans wants to merge 2 commits intomainfrom
Conversation
Contributor
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Bug Fixes 🐛
🤖 This preview updates automatically when you update the PR. |
Contributor
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Fixes
- drop screenshots for events filtered by `before_send` ([#2642](https://github.com/getsentry/sentry-unity/pull/2642))If none of the above apply, you can opt out of this check by adding |
The previous test called processor.Process() directly, bypassing DoSendEvent entirely. It passed after the WasCaptured fix only because the flag defaults to false — not because DoSendEvent actively left it false after before_send dropped the event. The new tests register the screenshot processor via AddEventProcessor and trigger events through SentrySdk.CaptureMessage, so the event flows through the real pipeline. A positive control test confirms screenshots are sent when events are captured. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2641
Since screenshots await
EndOfFrame(), we start a coroutine that ends up sending an envelope with just the screenshot item. However, in the meantime, the related event might have been discarded by thebefore_sendfilter. In this case, we don't want to send unnecessary data over to Relay.As a temporary workaround, setting
SetBeforeCaptureScreenshotto the same filter used inSetBeforeSendprevents these screenshots from being sent.Proposed Fix
Add internal
bool WasCapturedtoSentryEvent(sentry-dotnet/SentryEvent.cs:249) — a flag that tracks whether an event actually made it through the entire send pipeline. Defaults tofalse, which is the safe default: if the flag is never set, the screenshot is skipped rather than sent as an orphan.Set it to
trueon the success path inDoSendEvent(sentry-dotnet/SentryClient.cs:416) — placed immediately afterCaptureEnvelopereturns true, which is the only point where we know the event was actually sent. The flag is set on the original @event parameter (not processedEvent) because event processors andbefore_sendcan swap the event object, but the screenshot coroutine's closure holds a reference to the original. All five drop paths (exception filters, event processors, before_send, sampling, transport failure) return early and never reach this line, leaving the flag false.Check the flag in the screenshot coroutine before capturing (ScreenshotEventProcessor.cs:48) — after
WaitForEndOfFrameresumes the coroutine (at which pointDoSendEventhas already completed synchronously), we readWasCaptured. If false, we yield break and skip the screenshot entirely. This check runs before any screenshot capture or CaptureAttachment call, preventing orphaned attachment envelopes from ever being created.Related PRs:
internal bool CaptureAttachmenttoHubsentry-dotnet#4357