fix(core): Report no-op executor tasks as cancelled - #5874
Merged
Conversation
NoOpSentryExecutorService returned a FutureTask that is never run and never cancelled, so a caller had no way to tell a dropped task from a queued one: isCancelled() and isDone() both stay false forever, and get() blocks until it times out. Return an already-cancelled Future instead, matching what SentryExecutorService already does when its work queue is full, and state the requirement on ISentryExecutorService so future implementations honour it. CancelledFuture moves out of SentryExecutorService so both services share one implementation. An ActivityLifecycleIntegration test asserted a freshly scheduled ttfd timeout was not cancelled while running on the no-op executor, so it was really pinning the old pending-forever behaviour rather than the scheduling it names. It now uses DeferredExecutorService, which queues. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📲 Install BuildsAndroid
|
runningcode
marked this pull request as ready for review
July 30, 2026 11:46
runningcode
requested review from
0xadam-brown,
adinauer,
markushi and
romtsn
as code owners
July 30, 2026 11:46
runningcode
added
the
sanity-check
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
0xadam-brown
approved these changes
Jul 30, 2026
0xadam-brown
left a comment
0xadam-brown
left a comment
Member
There was a problem hiding this comment.
Nice! A few optional nits; otherwise lgtm pending fix of the SentryBot issue.
Co-authored-by: arb <adam.brown@sentry.io>
Co-authored-by: arb <adam.brown@sentry.io>
runningcode
enabled auto-merge (squash)
July 30, 2026 12:46
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
4 participants
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
NoOpSentryExecutorServicereturnednew FutureTask<>(() -> null)fromsubmit/schedule— a task that is never run and never cancelled. Callers had no way to distinguish it from a genuinely queued task:isCancelled()andisDone()both stayfalseforever, andget()blocks until its timeout.It now returns an already-cancelled
Future, matching whatSentryExecutorServicealready does when its work queue is full.CancelledFuturemoves out ofSentryExecutorServiceinto a shared package-private class, andISentryExecutorServicedocuments the requirement so future implementations honour it.No public API change —
CancelledFutureis package-private and no existing signature changed shape, so.apifiles are untouched.💡 Motivation and Context
Found by cursor here. That PR's
PersistingScopeObserverusesfuture.isCancelled()to detect a flush task the executor dropped, and releases itshasPendingFlushlatch when it sees one. Against a no-op executor the latch would be set with no task to ever clear it, silently stopping scope persistence for the rest of the process.That is not a reachable bug today because the observer is only installed after
options.activate()has swapped in a real executor so this more to prevent regressions in the future. ButsetExecutorServiceis public, and the contract the caller relies on was never written down anywhere. Fixing it here, offmain, keeps #5791 focused and makes the guarantee available to any other caller that wants to check whether its task was accepted.💚 How did you test it?
Tests pass
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
#5791 builds on this guarantee for its scope-write batching.