Skip to content

fix(android): drop native events when BeforeSend conversion fails - #5868

Open
osamashtayat wants to merge 2 commits into
getsentry:mainfrom
osamashtayat:fix/android-before-send-conversion
Open

osamashtayat wants to merge 2 commits into
getsentry:mainfrom
osamashtayat:fix/android-before-send-conversion

Conversation

@osamashtayat

Copy link
Copy Markdown

Description

Problem

On Android, when native BeforeSend is enabled, a Java/NDK event that cannot be converted to or from a managed SentryEvent is currently sent unfiltered.

BeforeSendCallback.Execute converts the original Java event to a managed event, runs the user's BeforeSend, then converts the result back to Java. If either conversion throws, the exception handler logs the error and returns e — the original Java event. That means:

  • if the input conversion fails, the user's BeforeSend never runs and the raw event is sent;
  • if the output conversion fails, the user's redactions exist only on the managed copy and the original unredacted event is sent.

Users enable Native.EnableBeforeSend specifically to redact native events, so sending the raw event when filtering cannot run defeats the purpose.

Root cause

BeforeSendCallback.Execute wraps ToSentryEvent, ToHint, the user callback, and ToJavaSentryEvent in one try block. Its catch returns the original Java event:

catch (Exception exception)
{
    _options.LogError(exception, "Before Send Error");
    return e;
}

Returning e bypasses whatever the managed filter did, or would have done.

Fix

Return null on conversion failure instead of returning the original event:

catch (Exception exception)
{
    _options.LogError(exception, "Before Send Error");
    return null;
}

The Java SDK treats a null BeforeSend result as a drop and records a before_send client report. This is consistent with BeforeSendWrapper, which already returns null when the user's callback throws (#5610), and with the other Android callback bridges.

Successful conversions, explicit callback drops, and SIGSEGV suppression are unchanged. There is no public API change and no new dependency.

Tests

Added BeforeSendCallbackTests with three Android device tests:

  • input conversion failure drops the event before the user callback runs;
  • output conversion failure drops the event after the callback redacted ServerName;
  • a successful redaction returns a Java event with the same event ID and no ServerName.

Verified on an API 34 arm64 emulator using the repository's device-test script:

  • 2,957 tests run, 2,930 passed, 0 failed, 27 skipped
  • all three new tests present and passing

Also green:

  • Sentry.Tests on .NET 8, 9, and 10 (0 failures)
  • Sentry.Tests net10.0-android36.0 build (0 warnings, 0 errors)
  • dotnet format and git diff --check

The full macOS solution build could not complete on the local machine because of unrelated host tooling issues (Intel protoc on Apple Silicon, sample DSN placeholders, and the Cocoa-from-source step). Android CI should be the authoritative device check.

Maintainer decision

Android.SuppressSegfaults alone also installs this bridge. With this change, an event whose conversion fails is dropped even if it might not have been a SIGSEGV.

I recommend dropping it because the SDK cannot evaluate the configured suppression rule. Please confirm this is the desired policy.

Issues

Closes #5764

@github-actions github-actions Bot added the risk: medium PR risk score: medium label Nov 4, 2026
@codecov

codecov Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.67%. Comparing base (a9b0062) to head (0a1db4a).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5868      +/-   ##
==========================================
+ Coverage   76.57%   76.67%   +0.10%     
==========================================
  Files         528      528              
  Lines       19624    19524     -100     
  Branches     3873     3840      -33     
==========================================
- Hits        15028    14971      -57     
+ Misses       3711     3669      -42     
+ Partials      885      884       -1     

☔ 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

Copy link
Copy Markdown
Collaborator

Thanks for the PR @osamashtayat ❤️

Maintainer decision

Android.SuppressSegfaults alone also installs this bridge. With this change, an event whose conversion fails is dropped even if it might not have been a SIGSEGV.

I recommend dropping it because the SDK cannot evaluate the configured suppression rule. Please confirm this is the desired policy.

Hm... that's a fair question.

One option would be to change:

- return null;
+ return options.Native.EnableBeforeSend && options.BeforeSendInternal != null ? null : e;

So you'd only return null (dropping the event) if the SDK user had set EnableBeforeSend = true and wired up a before send handler. If someone had just turned on the sigsegv handling, it wouldn't be triggered.

That seems like a hack/workaround though - inside this handler we're trying to guess/infer whether the code is being executed to handle a segv signal or not and adapt the behaviour of the code accordingly. Probably a cleaner solution would be to have the sigsegv errors trigger a different code path to the custom beforesend handlers that SDK users wire up.

Java’s SentryOptions has a single BeforeSend slot, so the two jobs need to be chained... so something like this in SentrySdk.cs:

JavaSdk.SentryOptions.IBeforeSendCallback? beforeSend = null;
if (options.Native.EnableBeforeSend && options.BeforeSendInternal is not null)
{
    beforeSend = new BeforeSendCallback(BeforeSendWrapper(options), options, o);
}
if (options.Android.SuppressSegfaults)
{
    beforeSend = new SegfaultSuppressionCallback(beforeSend);
}
o.BeforeSend = beforeSend;

@jamescrosswell

Copy link
Copy Markdown
Collaborator

To fill in the gap above: SegfaultSuppressionCallback would be a new Java-side callback that checks the Java event directly, so no conversion happens:

internal class SegfaultSuppressionCallback : JavaObject, JavaSdk.SentryOptions.IBeforeSendCallback
{
    private readonly JavaSdk.SentryOptions.IBeforeSendCallback? _next;

    public SegfaultSuppressionCallback(JavaSdk.SentryOptions.IBeforeSendCallback? next) => _next = next;

    public JavaSdk.SentryEvent? Execute(JavaSdk.SentryEvent e, JavaSdk.Hint h) =>
        e.Exceptions?.FirstOrDefault() is { Type: "SIGSEGV", Value: "Segfault" }
            ? null
            : _next is null ? e : _next.Execute(e, h);
}

With that in place, BeforeSendWrapper can drop its SIGSEGV branch and its "not enabled, return as-is" branch, and BeforeSendCallback can keep your return null unconditionally, because it's only installed when there's a user filter. The existing BeforeSendWrapper_*SIGSEGV* tests would move to the new class.

(A Java EventProcessor looks like the more natural separate hook, but tombstone events (Native.TombstoneEnabled) are "backfillable" and skip plain processors, so it would miss exactly the SIGSEGV/Segfault events we want to suppress. The BeforeSend slot sees everything.)

@osamashtayat

Copy link
Copy Markdown
Author

Thanks for the review @jamescrosswell. I addressed both comments in aac8565. Segfault suppression now checks the first exception on the Java event directly. An exact SIGSEGV / Segfault match is dropped before the user filter runs. Other events go to the user filter if one is enabled; with suppression alone, the original Java event passes through without conversion.

Both callbacks share Java’s BeforeSend slot, so suppression still applies to tombstone events. The managed bridge is installed only for an enabled user filter, and it still drops the event if conversion fails rather than sending unfiltered data.

I ran the Android emulator suite: 2,933 passed, 0 failed, 27 skipped. All 12 callback-related cases passed, including the conversion regressions and the new tests for callback ordering and preserving a filter’s edits or drop decision.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk: medium PR risk score: medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Android: native events are sent unfiltered when BeforeSend conversion fails

2 participants