fix(android): mark tombstone and ANR exits as reported when the event is dropped - #6002
fix(android): mark tombstone and ANR exits as reported when the event is dropped#6002markushi wants to merge 2 commits into
Conversation
… is dropped Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📲 Install BuildsAndroid
|
| final @NotNull SentryId sentryId = scopes.captureEvent(report.getEvent(), report.getHint()); | ||
| final boolean isEventDropped = sentryId.equals(SentryId.EMPTY_ID); | ||
| if (!isEventDropped) { | ||
| if (isEventDropped) { | ||
| // A dropped event never reaches the envelope disk cache, which is where the last reported | ||
| // marker is normally written. Without writing it here, the very same exit would be turned | ||
| // into an event again on the next app start, ignoring the user's decision to drop it. | ||
| policy.markReported(exitInfo.getTimestamp()); | ||
| } else { |
There was a problem hiding this comment.
Bug: The code marks an exit event as reported if captureEvent returns SentryId.EMPTY_ID. This ID is also returned for transient I/O errors, causing events to be permanently lost instead of retried.
Severity: HIGH
Suggested Fix
The captureEvent method should provide a way to distinguish between intentional drops and transient failures. For example, it could throw a specific exception for I/O errors instead of returning SentryId.EMPTY_ID. The calling code in ApplicationExitInfoHistoryDispatcher should then catch this exception and avoid calling policy.markReported(), allowing the event to be retried later.
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:
sentry-android-core/src/main/java/io/sentry/android/core/ApplicationExitInfoHistoryDispatcher.java#L188-L195
Potential issue: The `captureEvent` method returns `SentryId.EMPTY_ID` for both
intentional event drops (e.g., via `beforeSend` or sampling) and transient failures like
an `IOException` during envelope processing. The new logic in
`ApplicationExitInfoHistoryDispatcher` treats any `SentryId.EMPTY_ID` return as an
intentional drop and calls `policy.markReported()`. This means if an application exit
event fails to be captured due to a temporary network or I/O issue, it will be
permanently marked as reported and will not be retried on the next application start,
leading to the loss of critical ANR or crash data.
Did we get this right? 👍 / 👎 to inform future reviews.
0xadam-brown
left a comment
There was a problem hiding this comment.
Nice to fix this 👍
Lgtm save for the ambiguity of the EMPTY_ID, as called out by SentryBot here.
📜 Description
The last reported marker (
last_tombstone_report/last_anr_report) was only written as a sideeffect of caching the envelope on disk. An event dropped by
beforeSendnever gets there, so thesame
ApplicationExitInfowas turned into an event again at every app start.ApplicationExitInfoHistoryDispatchernow writes the marker as well whencaptureEventreturnsSentryId.EMPTY_ID, through a newApplicationExitInfoPolicy.markReported(long). The successfulpath is unchanged.
💡 Motivation and Context
A discarded crash must stay discarded. This is how signal handler events already behave, because
OutboxSenderdeletes the outbox file independent of the result ofbeforeSend.Tombstoneevents discarded frombeforeSendare re-reported on every app launch #5973💚 How did you test it?
New test in
ApplicationExitIntegrationTestBase, so it runs for bothTombstoneIntegrationTestand
AnrV2IntegrationTest. It failed before the change and passes now. Full:sentry-android-core:testReleaseUnitTestis green.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
TombstonePolicydeletes the matching native outbox file before the capture, so native data cannotcome back if the merged event is lost. That is a separate defect.