fix(android): drop inflated app start for background-spawned processes - #6526
fix(android): drop inflated app start for background-spawned processes#6526alwx wants to merge 3 commits into
Conversation
On API 35+ use the process start reason (ApplicationStartInfo#getReason()) to detect background-spawned processes (e.g. FCM push) and drop the app start, avoiding inflated app_start_cold/warm. Fixes #6382
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| + appStartReason | ||
| + ")."); | ||
| promise.resolve(null); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Bug: The code assumes metrics.getAppStartReason() returns a lowercase string, but the actual format from the external library is unverified and may differ, causing the feature to fail silently.
Severity: MEDIUM
Suggested Fix
Add an integration test to verify the actual return value and format of metrics.getAppStartReason() from the sentry-android dependency. Adjust the isBackgroundAppStartReason() method to handle the actual output format, whether it's an integer, an uppercase string, or something else.
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:
packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java#L414-L418
Potential issue: The code assumes that the `getAppStartReason()` method from the
external `sentry-android` library returns a lowercase string (e.g., `"push"`,
`"alarm"`). However, the actual return format is unverified. The underlying Android API
returns integer constants, and it's unknown how the Sentry library transforms them. If
the return value is an integer, an uppercase string, or another format, the
`isBackgroundAppStartReason()` check will always fail. This would cause the feature to
silently not work, meaning background-triggered app starts would not be filtered out,
leading to inflated app start metrics.
Did we get this right? 👍 / 👎 to inform future reviews.
# Conflicts: # CHANGELOG.md
| if (isBackgroundAppStartReason(appStartReason)) { | ||
| logger.log( | ||
| SentryLevel.WARNING, |
There was a problem hiding this comment.
Bug: The new logic to filter background app starts in fetchNativeAppStart() lacks integration test coverage. A regression could go undetected, reintroducing inflated app start metrics.
Severity: LOW
Suggested Fix
Add an integration test for fetchNativeAppStart(). This test should mock an AppStartMetrics object where getAppStartReason() returns a background reason (e.g., "push") and isAppLaunchedInForeground() is true. Verify that the promise is resolved with null, confirming the app start metric is correctly dropped.
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:
packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java#L410-L412
Potential issue: The new logic in `fetchNativeAppStart` filters out app start metrics
from background processes to prevent metric inflation. However, this critical filtering
path is not covered by any integration tests. While a unit test validates the
`isBackgroundAppStartReason()` helper method, there is no test that simulates a scenario
where `isAppLaunchedInForeground()` is `true` but `getAppStartReason()` returns a
background reason like `"push"`. This means a future regression in the interaction
between these checks within `fetchNativeAppStart()` would go undetected, potentially
reintroducing the metric inflation issue this change aims to solve.
Also affects:
packages/core/android/src/test/java/io/sentry/react/RNSentryAppStartTest.java:1~45
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0925ea0. Configure here.
| + ")."); | ||
| promise.resolve(null); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Warm starts wrongly dropped
Medium Severity
isBackgroundAppStartReason keys off the process start reason, which stays fixed for the process lifetime. After a background spawn (e.g. push), the first inflated open is correctly dropped, but later warm starts in the same process still report that reason and are dropped too, even though those spans are re-anchored at activity creation and are not inflated.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 0925ea0. Configure here.
|
Closing it in favor of getsentry/sentry-java#5841 |
1 participant
📢 Type of change
📜 Description
On API 35+ use the process start reason (
ApplicationStartInfo#getReason()) to detect background-spawned processes (e.g. FCM push) and drop the app start, avoiding inflatedapp_start_cold/app_start_warm.💡 Motivation and Context
When Android spawns the process in the background and the user opens the app later, the native SDK keeps the app start anchored at background process creation, inflating cold/warm start by the whole idle gap. Similar to how the Cocoa SDK drops pre-warmed starts.
Fixes #6382
💚 How did you test it?
Added Android unit tests (
RNSentryAppStartTest);./gradlew :RNSentry:testDebugUnitTestpassing.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
On API < 35
getAppStartReason()returns null; fully fixing those needs an upstream sentry-java change.