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.
| * <p>Only available on Android API ≥ 37. | ||
| */ | ||
| @ApiStatus.Internal | ||
| public final class MemoryLimiterIntegration implements Integration, Closeable { |
There was a problem hiding this comment.
Should we have a @RequiresAPI(37) on this class? We check the API level before initializing the class in AndroidOptionsInitializer but internally we also check buildInfoProvider in the register method and then no-op.
Completely unrelated, I feel like this lint check is probably broken in the codebase but it still makes for good documentation.
There was a problem hiding this comment.
Reasonable ask, but it'd break our callers (both those invoking register() and those invoking the nested MemoryLimiterHint, which doesn't require API 37).
(Note: we also check the API level in register() because it's public, and power users could register the integration manually.)
| this.shouldEnrich = shouldEnrich; | ||
| } | ||
|
|
||
| public long timestamp() { |
There was a problem hiding this comment.
since im deep in the weeds on clocks here 😆 . should we document what this timestamp represents?
is it wall clock, monotonick clock? millis or nanos?
There was a problem hiding this comment.
Nice – added a doc + updated the field name for clarity.
| } | ||
|
|
||
| @ApiStatus.Experimental | ||
| public boolean isMemoryLimiterEnabled() { |
There was a problem hiding this comment.
| public boolean isMemoryLimiterEnabled() { | |
| public boolean getMemoryLimiterEnabled() { |
this creates a nicer kotlin synthetic property of memoryLimiterEnabled instead of isMemoryLimiterEnabled
There was a problem hiding this comment.
Agreed we'd be doing Kotlin callers a favor, but leaving this (and the below) as-is because we use is* throughout SentryAndroidOptions and SentryOptions.
| } | ||
|
|
||
| @ApiStatus.Experimental | ||
| public boolean isReportHistoricalMemoryLimiterExits() { |
There was a problem hiding this comment.
| public boolean isReportHistoricalMemoryLimiterExits() { | |
| public boolean getReportHistoricalMemoryLimiterExits() { |
same suggestion as above. if you use get here then we get a single property for both the getter and setter from kotlin.
| import org.robolectric.shadows.ShadowActivityManager.ApplicationExitInfoBuilder | ||
|
|
||
| @RunWith(AndroidJUnit4::class) | ||
| @Config(sdk = [31]) |
There was a problem hiding this comment.
do we need 37 to test this properly?
There was a problem hiding this comment.
We actually don't need 37 b/c the MemoryLimiter code path doesn't use new API 37 framework methods (just 31). In fact we can't use 37 here because our current Robolectric version (4.15) only lets us go up to API 35. (We follow the module convention of shadowing at the min AEI API and injecting the effective version via a mocked BuildInfoProvider.)
I've added a comment explaining as much, as it's admittedly confusing.
| 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.
| @Override | ||
| public void register(@NotNull IScopes scopes, @NotNull SentryOptions options) { | ||
| androidOptions = | ||
| Objects.requireNonNull( |
There was a problem hiding this comment.
This could be slightly tricky to debug based on the log message since it isn't clear if options was null or if it wasn't an instanceOf SentryAndroidOptions. How do other register methods handle this?
There was a problem hiding this comment.
How do other register methods handle this?
Good question: just like we do here. In fact, all the initial checks in this method are taken from (many instances of) prior art. Again, probably worth changing across-the-board at some point.
|
|
||
| @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.
| public int getTargetReason() { | ||
| return ApplicationExitInfo.REASON_ANR; | ||
| public boolean matches(final @NotNull ApplicationExitInfo exitInfo) { | ||
| return exitInfo.getReason() == ApplicationExitInfo.REASON_ANR; |
There was a problem hiding this comment.
just curious, is this an unrelated bugfix?
There was a problem hiding this comment.
Nw – this is a required (and mechanical) part of the refactor. We can't simply use the target reason int any more b/c MemoryLimiter matches against both the reason and the description.
| return; | ||
| } | ||
|
|
||
| if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.CINNAMON_BUN) { |
There was a problem hiding this comment.
I mentioned this at the top of the class, but is the correct pattern to check this before constructing the class or like this during register?
There was a problem hiding this comment.
...also a great question, and also the same answer: this is me copying a lot of our prior art. I suspect my own instincts line up with yours, but I figured it'd be best to keep things consistent throughout our integrations. We can standardize at some point if we care to 👍
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: