fix(core): Make async event processing safe for crash and shutdown paths - #5878
Draft
runningcode wants to merge 4 commits into
Draft
fix(core): Make async event processing safe for crash and shutdown paths#5878runningcode wants to merge 4 commits into
runningcode wants to merge 4 commits into
Conversation
Document the proposed opt-in async processing pipeline for EventProcessor callbacks, including queue behavior, flush semantics, and testing coverage. Co-Authored-By: Claude <noreply@anthropic.com>
Add EventProcessor.processAsync callbacks and an opt-in SentryOptions flag to run late event processing off the caller thread. Use a dedicated bounded queue for async callbacks, record queue overflow drops, and make flush/close drain accepted async work before downstream queues. Co-Authored-By: Claude <noreply@anthropic.com>
The async event processing stage moved work to another thread without establishing what state that work may touch. Four issues followed. close() flushed downstream queues before quiescing the async stage, so tasks finishing afterwards handed envelopes to a transport that was already closing, and anything still queued when the timeout expired was discarded with no client report. Quiesce the async stage first and record queue_overflow for work that could not be drained. Captures whose hint coordinates across threads are no longer deferred. DiskFlushNotification captures (uncaught exceptions, ANRv2, tombstones) block the caller until the transport reports the event flushed to disk; queueing put the fatal event behind everything already enqueued, so the caller's flush timeout could expire before it was ever sent. Backfillable and Cached captures carry retry/delete state the caller reads as soon as capture returns, and TransactionEnd captures force-finish the running transaction before the process dies. The scope passed to capture* is a live view over the caller's thread-local scope, so the async stage could read tags, sessions, and transactions belonging to unrelated later work. It is now cloned at submit time. Hint left its attachment fields unsynchronized while both threads could still reach them. The attachment list is now guarded by the existing lock and the single-reference fields are volatile. Also stop holding the executor monitor across awaitTermination, which could stall a capture on an app thread for the whole shutdown timeout.
Contributor
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Fixes
- Make async event processing safe for crash and shutdown paths ([#5878](https://github.com/getsentry/sentry-java/pull/5878))If none of the above apply, you can opt out of this check by adding |
📲 Install BuildsAndroid
|
Contributor
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 6b019b7 | 343.31 ms | 417.23 ms | 73.91 ms |
| d15471f | 342.08 ms | 415.44 ms | 73.35 ms |
| 8687935 | 332.52 ms | 362.23 ms | 29.71 ms |
| 5b1a06b | 352.27 ms | 413.70 ms | 61.43 ms |
| 91bb874 | 314.47 ms | 440.00 ms | 125.53 ms |
| 0ee65e9 | 321.06 ms | 361.24 ms | 40.18 ms |
| e63ad34 | 323.67 ms | 390.33 ms | 66.67 ms |
| 33a08cc | 267.08 ms | 340.45 ms | 73.37 ms |
| 27d7cf8 | 397.90 ms | 498.65 ms | 100.75 ms |
| ee747ae | 405.43 ms | 485.70 ms | 80.28 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 6b019b7 | 0 B | 0 B | 0 B |
| d15471f | 1.58 MiB | 2.13 MiB | 559.54 KiB |
| 8687935 | 1.58 MiB | 2.19 MiB | 619.17 KiB |
| 5b1a06b | 0 B | 0 B | 0 B |
| 91bb874 | 1.58 MiB | 2.13 MiB | 559.07 KiB |
| 0ee65e9 | 0 B | 0 B | 0 B |
| e63ad34 | 0 B | 0 B | 0 B |
| 33a08cc | 1.58 MiB | 2.12 MiB | 555.28 KiB |
| 27d7cf8 | 1.58 MiB | 2.12 MiB | 549.42 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
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.
📜 Description
Supersedes #5558. Same feature, rebased onto current
main, with four concurrency fixes found in review.The original PR moved the late processing stage to another thread without establishing what state that work is allowed to touch. The fixes narrow the async region to work that is actually safe to defer.
1.
close()drained downstream before quiescing the async stage. Tasks finishing after the transport flush handed envelopes to a transport that was already closing, and anything still queued when the timeout expired was dropped byshutdownNow()with no client report. WithisRestarting=truethe timeout is 0, so the whole queue vanished silently. The async stage is now quiesced first, and undrained work is recorded asqueue_overflowfor its data category.2. Crash captures went through the queue.
UncaughtExceptionHandlerIntegrationblocks onwaitFlush()until the transport reports the event flushed to disk before letting the process die. Queueing put the fatal event behind everything already enqueued — with the defaultmaxQueueSizeof 30, the 15s flush timeout could expire before the crash was ever sent. Captures whose hint implementsDiskFlushNotification,Backfillable,Cached, orTransactionEndnow run inline; the latter three also carry retry/delete state or force-finish a transaction that the caller depends on synchronously. This covers ANRv2, tombstones, and outbox replay as well.3. The async task read a live scope. The scope handed to
capture*is aCombinedScopeViewover the caller's thread-local scope. By the time the async stage ran, the caller may have pushed or popped scopes, changed tags, or started a different transaction — sofinalizeTransactioncould force-finish a transaction unrelated to the event. The scope is now cloned at submit time.4.
Hintwas shared mutable state. Its attachment list and five reference fields were unsynchronized while both threads could still reach them. The list is now guarded by the existing lock and the single-reference fields arevolatile.Also stops holding the executor monitor across
awaitTermination, which could stall a capture on an app thread for the entire shutdown timeout — an ANR on Android's main thread.💡 Motivation and Context
Issues 1 and 2 lose events outright; issue 3 can corrupt unrelated transactions. All three are reachable with
enableAsyncProcessing=trueon paths users do not control.The feature remains opt-in and defaults to
false, so behavior is unchanged unless enabled.💚 How did you test it?
Nine tests across
SentryClientTestand a newAsyncEventProcessingExecutorTest. Each was verified to fail against the pre-fix implementation and pass after — the two that initially passed either way were reworked until they discriminated../gradlew :sentry:check :sentry-android-core:testReleaseUnitTestpasses.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Issues raised in review that this PR does not address, since they are design questions rather than correctness bugs:
maxQueueSize(default 30). A metrics-heavy app can fill it and starve errors. Worth a dedicated option or reserved headroom forDataCategory.Error.process*Asyncmethods are near-exact copies of their sync counterparts (~200 lines). Could be parameterized.processAsync(...)where they do not capture stale request, thread-local, or UI state.