feat(android): Recover MemoryLimiter app exits on startup (JAVA-687) - #6111
0xadam-brown wants to merge 5 commits into
Conversation
|
📲 Install BuildsAndroid
|
| // While this is specifically an ANR enricher we discriminate enrichment application | ||
| // on the broader AbnormalExit hints for now. | ||
| return hint instanceof AbnormalExit; | ||
| return hint instanceof AnrV2Integration.AnrV2Hint; |
There was a problem hiding this comment.
Note: AbnormalExit has two implementations: AnrV2Hint and AnrHint. We can safely ignore AnrHint here because ApplicationExitInfoEventProcessor only processes hints that are Backfillable, and AnrHint isn't.
That's a clarity win, too, because AnrHint belongs to the older ANR flow that doesn't make use of ApplicationExitInfo.
| public fun buildReport (Landroid/app/ApplicationExitInfo;Z)Lio/sentry/android/core/ApplicationExitInfoHistoryDispatcher$Report; | ||
| public fun getLabel ()Ljava/lang/String; | ||
| public fun getLastReportedTimestamp ()Ljava/lang/Long; | ||
| public fun getTargetReason ()I |
There was a problem hiding this comment.
TombstonePolicy is @ApiStatus.Internal.
| * backfilled with persisted launch state or kept as a lighter historical record. | ||
| */ | ||
| @ApiStatus.Internal | ||
| public static final class MemoryLimiterHint extends BlockingFlushHint implements Backfillable { |
There was a problem hiding this comment.
I've delayed because the change involves updating how we currently handle the AbnormalExit interface (which our *Hint types implement). That involves some design work, which is worth treating separately.
I'll be sure to merge this PR and that^^ one in the same release.
Introduce a new MemoryLimiterIntegration that captures process deaths attributable to Android 17's new [MemoryLimiter](https://source.android.com/docs/core/perf/memory-limiter#process-monitoring) system service (see also [here](https://android-developers.googleblog.com/2026/06/prioritizing-memory-efficiency-steps-for-android-17.html)). Process death info is extracted from ApplicationExitInfo on the next app launch. We then enrich it with persisted SDK state and send it to Relay as a fatal Sentry event. Integration is experimental; is only available for Android API >= 37; and is disabled by default.
Add a data-driven test over every ApplicationExitInfo importance band, including the default fallback, to lock the getProcessVisibility mapping.
… prefix Match the "MemoryLimiter:" prefix instead of the full "MemoryLimiter:AnonSwap" string. Per AOSP, AnonSwap is the only MemoryLimiter kill sub-reason on Android 17, but the memory and swap limits it also tracks may start killing in a future release; matching the namespace prefix keeps capturing those without a code change, while the colon still anchors matching to the MemoryLimiter namespace. The raw description is retained on the event mechanism.
c38bf03 to
cc0773d
Compare
| sentryException.setModule("io.sentry.android.core"); | ||
| sentryException.setMechanism(mechanism); | ||
| return sentryException; | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
it would be interesting to know how grouping will work for empty stacktraces. Will every event be grouped into a single issue? Or do e.g. mechanism values play a role here to and cause a split up into multiple groups.
There was a problem hiding this comment.
Ooo, excellent question, and I hadn't thought this through.
Based on our Monolith code, looks like at present all MemoryLimiter events (whether latest or historical) would be merged into a single issue.
If we want grouping, we could do so via SDK fingerprinting.
Some candidates to group by:
- process_visibility (visible, not_visible, or cached)
- importance (the raw input to our process_visibility classification; IMO better as context than as a grouping mechanism)
- kill reason (see below)
- process name (for multi-process apps, which are admittedly probably quite rare)
- latest vs historical (I'm not in favor, as it's a distinction without a difference w/r/t underlying causes)
- something else? (Nothing obvious here, including nothing obvious among ApplicationExitInfo properties we aren't currently making use of)
Candidate (1) comes from the Android folks (link)
Atm, "kill reason" only has one member (:AnonSwap), but AOSP code has constants for :Swap and :Memory that might be used in the future.
Prior art
Just ANRs. In addition to the normal grouping by stack trace and exception type, we also fingerprint by foreground vs background. (And ANRs with only system frames in their stack trace are all dumped into either a foreground or a background bucket, with stack trace and exception differences ignored.)
My vote
I like fingerprinting by process_visibility (and possibly also process name), as the Google folks think it's a distinction that matters and I suspect users will too. But I'm open to the current single bucket approach as well, etc.
Thoughts?
0xadam-brown
added
the
deep-dive
| * MemoryLimiter integration} hasn't been enabled. | ||
| */ | ||
| @ApiStatus.Experimental | ||
| public void setReportHistoricalMemoryLimiterExits( |
There was a problem hiding this comment.
Note that the two-boolean approach (enabled + report historical) rather than a single enum is taken from our existing SentryAndroidOptions APIs for ANRs and tombstones.
There was a problem hiding this comment.
Ideally we shouldn't even offer historical reporting, as this has been a cause more quite some confusion (e.g. why are my events not enriched). But I'm fine with having it for the sake of consistency 😅
There was a problem hiding this comment.
I'm open to either 😄 (so let me know if you think we should drop the "historical" ones). We've baked the idea into our ApplicationExitInfoPolicy type, but I could have the MemoryLimiterPolicy no-op for it.
Regardless, I've updated the SentryAndroidOptions.setReportHistoricalMemoryLimiterExits() Javadoc to note that historical exits aren't enriched with contextual Sentry data.
runningcode
left a comment
runningcode
left a comment
There was a problem hiding this comment.
The overall pattern looks good to me! just some comments.
| androidOptions, | ||
| dateProvider, | ||
| new MemoryLimiterPolicy(androidOptions))); | ||
| } catch (Throwable e) { |
There was a problem hiding this comment.
Do we expect anything here other than a RejectedExecutionException or whatever that exception is? We should document the other exceptions we expect or otherwise document whey catching throwable is the only option here.
There was a problem hiding this comment.
All great callouts, and this also deserves an across-the-board audit at some point. For now leaving as-is b/c:
- we catch
Throwablefor.submit()throughout our code base (including in the other ApplicationExitInfo integrations), - I poked around a bit and at least one implementation of
.submit()explicitly throws NPEs.
...other exceptions are probably possible.
There was a problem hiding this comment.
I thought that was the pattern we wanted to stop spreading in our codebase otherwise we don't know at all if the code works or is always throwing an NPE or ArrayIndexOutOfBoundsException.
I already approved so not blocking because of this.
Also just curious for my future self, which implementation of our executor throws an NPE?
There was a problem hiding this comment.
I thought that was the pattern we wanted to stop spreading in our codebase otherwise we don't know at all if the code works or is always throwing an NPE or ArrayIndexOutOfBoundsException.
Indeed! but in this case I think it'd be best if we addressed all calls of .submit() at once. (Otherwise we'll do the investigation now, only to have the result implemented in one place, causing future us to wonder about the difference and re-investigate, etc.)
Also just curious for my future self, which implementation of our executor throws an NPE?
Any that call ScheduledThreadPoolExecutor.submit(...) under the hood, as the latter invokes ScheduledThreadPoolExecutor.schedule(...), which throws an NPE if any of its args is null.
That's^^ at least SentryExecutorService, but could be others.
|
|
||
| @Override | ||
| public void close() throws IOException { | ||
| if (androidOptions != null) { |
There was a problem hiding this comment.
nit: do we need to ensure the task isn't on the executorservice anymore here? seems like the submission is a fire and forget so unless we're calling register and close right after then seems safe that this is a no-op but asking anyways.
There was a problem hiding this comment.
yeah it's quite common right now to only do a "fire and forget", simply because .onClose() implies that the whole SDK will be closed, and thus it's executor service will shutdown too
There was a problem hiding this comment.
Good question, and it's safe. Canceling would be a marginal improvement, in that if register --> close happened back-to-back before the task runs, we'd skip needless work. The ANR and tombstone integrations do the same as we do here; keeping as-is for simplicity and conformity.
markushi
left a comment
markushi
left a comment
There was a problem hiding this comment.
Looking good, left a few minor comments. I'm holding off to approving it for now, due to the follow up PR.
|
|
||
| @Override | ||
| public void close() throws IOException { | ||
| if (androidOptions != null) { |
There was a problem hiding this comment.
yeah it's quite common right now to only do a "fire and forget", simply because .onClose() implies that the whole SDK will be closed, and thus it's executor service will shutdown too
| final SentryEvent event = new SentryEvent(); | ||
| event.setMessage(message); | ||
| event.setLevel(SentryLevel.FATAL); | ||
| event.setPlatform(SentryBaseEvent.DEFAULT_PLATFORM); |
There was a problem hiding this comment.
probably worth double checking, if "java" is right here
There was a problem hiding this comment.
Good ask, and I actually triple-checked this b/c it seemed wrong to me too. It's right, though, b/c:
- Throughout sentry-java we only ever use two values: "native" or "java".
- The value corresponds to Relay's protocol enum
- That enum doesn't include "android" or anything else potentially relevant to us besides "java" and "native".
- "java" is the default if "native" doesn't apply (eg, AnrV2Integration uses "java")
- Using "java" displays correctly in the Sentry UI (see the screenshot in the PR description).
| sentryException.setModule("io.sentry.android.core"); | ||
| sentryException.setMechanism(mechanism); | ||
| return sentryException; | ||
| } |
There was a problem hiding this comment.
it would be interesting to know how grouping will work for empty stacktraces. Will every event be grouped into a single issue? Or do e.g. mechanism values play a role here to and cause a split up into multiple groups.
| * MemoryLimiter integration} hasn't been enabled. | ||
| */ | ||
| @ApiStatus.Experimental | ||
| public void setReportHistoricalMemoryLimiterExits( |
There was a problem hiding this comment.
Ideally we shouldn't even offer historical reporting, as this has been a cause more quite some confusion (e.g. why are my events not enriched). But I'm fine with having it for the sake of consistency 😅
3 participants
📜 Description
Introduces a new
MemoryLimiterIntegrationthat captures process deaths attributable to Android 17's new MemoryLimiter system service (see also here).Process death info is extracted from ApplicationExitInfo on the next app launch. We then enrich it with persisted SDK state and send it to Relay as a fatal Sentry event.
Integration is experimental; is disabled by default; and is only available for Android API >= 37.
💡 Motivation and Context
Android 17 (API 37) introduced a new system service called MemoryLimiter that's responsible for killing app processes if they threaten to consume too much system memory.
The tricky part for us is that MemoryLimiter-caused process deaths don't involve an exception or a stack trace, making them invisible to our current instrumentation. This PR fills the gap by using ApplicationExitInfo to extract info about relevant exits on subsequent app launches. It does so by piggybacking on the ApplicationExitInfo processing pipeline used by ANRs and tombstones.
resolves: JAVA-687
Basic flow
Each integration's ApplicationExitInfoHistoryDispatcher asks the ActivityManager for all recorded exits and does the following per policy:
Note: there's definitely room for optimization here, in that each integration (when enabled) creates its own ApplicationExitInfoHistoryDispatcher, each dispatcher asks ActivityManager for the exit list, and each dispatcher scans the same exit list looking for matches against the policy it manages. But that's work for another day. Atm, this PR simply extends the pattern that already existed with ANRs and tombstones.
Screenshot
I highlighted relevant info via the red boxes.
Screenshot URL: link
💚 How did you test it?
REASON_OTHERand a"MemoryLimiter:AnonSwap"description. It'd be nice to see what actual OEM OS's return in the wild, however.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Event path vs session path
The follow-on PR from (1) is needed because the current PR only covers the event path from the diagram below. The follow-on will cover the session path: