RUM-8054: Fix background session precondition for refresh and new session flows#2872
RUM-8054: Fix background session precondition for refresh and new session flows#2872saraSr5 wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes RUM session precondition attribution when a new session is started while the app is in the background, ensuring background sessions are labeled as background_launch / prewarm instead of inheriting end-reason-based preconditions.
Changes:
- Added background-first precondition derivation to
refresh(expiredSession:)andstartNewSession(). - Refactored initial session creation to reuse a shared background-precondition helper.
- Added unit tests covering background session starts across inactivity timeout, max duration expiration, explicit stop, prewarming, and user-launch-in-background scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
DatadogRUM/Sources/RUMMonitor/Scopes/RUMApplicationScope.swift |
Applies background-aware session precondition logic across session creation/renewal paths and centralizes mapping via a helper. |
DatadogRUM/Tests/RUMMonitor/Scopes/RUMApplicationScopeTests.swift |
Adds targeted tests validating correct precondition assignment for background session starts and confirming telemetry expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| startPrecondition = .userAppLaunch // UISceneDelegate-based apps always start in background | ||
| case .backgroundLaunch, .prewarming: | ||
| startPrecondition = preconditionForNewBackgroundSession(context: context) | ||
| default: |
There was a problem hiding this comment.
LaunchReason is a public (non-frozen) enum from another module. To keep compile-time warnings when new cases are added, consider using @unknown default instead of default in this switch (similar to DatadogInternal/Sources/Context/AppState.swift:171).
| default: | |
| @unknown default: |
| var startPrecondition: RUMSessionPrecondition? = nil | ||
|
|
||
| if lastSessionEndReason == .timeOut { | ||
| // If the launch reason is uncertain (e.g. on tvOS), the background check falls through to the end-reason logic. |
There was a problem hiding this comment.
This comment says launch reason uncertainty happens “e.g. on tvOS”, but on tvOS/watchOS process() always defers through LaunchReasonResolver, so _process (and thus refresh) should only see a resolved reason. Consider rewording to reference .uncertain generally (or iOS fallback) to avoid misleading future readers.
| // If the launch reason is uncertain (e.g. on tvOS), the background check falls through to the end-reason logic. | |
| // If no background-session precondition can be derived for the current context, fall through to the end-reason logic. |
| // If the launch reason is uncertain (e.g. on tvOS), the background check falls through to the end-reason logic. | ||
| if context.applicationStateHistory.currentState == .background, | ||
| let backgroundPrecondition = preconditionForNewBackgroundSession(context: context) { |
There was a problem hiding this comment.
This comment says launch reason uncertainty happens “e.g. on tvOS”, but on tvOS/watchOS process() always defers through LaunchReasonResolver, so _process (and thus startNewSession) should only see a resolved reason. Consider rewording to reference .uncertain generally (or iOS fallback) to avoid misleading future readers.
| switch context.launchInfo.launchReason { | ||
| case .backgroundLaunch: | ||
| return .backgroundLaunch | ||
| case .prewarming: | ||
| return .prewarm | ||
| case .userLaunch: | ||
| // Normal: a user-launched process went to background. Caller uses end-reason-based precondition. | ||
| return nil | ||
| default: | ||
| dependencies.telemetry.error( | ||
| "Starting session in background with unexpected launch reason: \(context.launchInfo.launchReason)" | ||
| ) | ||
| return nil |
There was a problem hiding this comment.
Same as above: consider using @unknown default here instead of default so new LaunchReason cases added in the future trigger a compiler warning (see e.g. DatadogInternal/Sources/Context/AppState.swift:171).
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let backgroundContext: DatadogContext = .mockWith( | ||
| launchInfo: .mockWith( | ||
| launchReason: .backgroundLaunch, | ||
| processLaunchDate: .mockDecember15th2019At10AMUTC() | ||
| ), | ||
| applicationStateHistory: .mockAppInBackground(since: currentTime) | ||
| ) |
There was a problem hiding this comment.
backgroundContext is built with DatadogContext.mockWith(...) without passing sdkInitDate, so it falls back to Date() (see TestUtilities/Sources/Mocks/DatadogInternal/DatadogContextMock.swift). Because session end metrics read context.sdkInitDate and context.launchInfo.processLaunchDate, this makes the test setup time-dependent and less deterministic. Consider passing a fixed sdkInitDate (e.g. currentTime) and (optionally) reusing the same launchInfo as the initial sdkContext so the context represents a single process lifecycle consistently.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
saraSr5
deleted the
sara.susano/RUM-8054/background-session-start-reason
branch
saraSr5
restored the
sara.susano/RUM-8054/background-session-start-reason
branch
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
What and why?
When a new RUM session starts while the app is in the background, it should be tagged with background_launch or prewarm as its session_precondition. The two session renewal paths (refresh(expiredSession:) and startNewSession()) were ignoring app state and always deriving the precondition from the previous session's end reason, so background sessions were incorrectly labelled inactivity_timeout, max_duration, or explicit_stop.
How?
Extracted a private helper preconditionForNewBackgroundSession(context:) that maps launchInfo.launchReason to the correct background precondition (.backgroundLaunch → .backgroundLaunch, .prewarming → .prewarm, .userLaunch → nil to fall through to end-reason logic).
Applied a background-first check in both refresh(expiredSession:) and startNewSession(), mirroring the existing behaviour in createInitialSession(). Also refactored createInitialSession() to use the same shared helper.
Review checklist
make api-surfacewhen adding new APIs