From 8ad3200d74a9ceff381de648983117655f42918e Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Fri, 6 Nov 2026 06:02:41 +0100 Subject: [PATCH 1/2] feat(android): Prefer an Activity's ui.load transaction over Nav3 transactions The Nav3 lease is now also withdrawn when an Activity is created over the leaseholder, not only when it resumes. On API 29+, creation is reported from onActivityPreCreated, before ActivityLifecycleIntegration binds the new Activity's ui.load transaction, so the leaseholder's nav transaction no longer holds the scope's transaction slot and ui.load binds, as with Nav2. Errors and spans from the new Activity's onCreate and onStart are no longer attributed to the leaseholder on any API level. Because ui.load now binds, returning quickly to a nav root while the closed Activity's ui.load is still running would leave the nav root's transaction blocked. The coordinator therefore retries binding the leaseholder's nav transaction once when another Activity is destroyed, within 10s of the blocked attempt. Below API 29, there's no onActivityPreCreated, so the new Activity's ui.load stays unbound as before; this is documented in the package README. --- CHANGELOG.md | 1 + sentry-android-navigation3/build.gradle.kts | 1 + .../lifecycle/NavActivityWatcher.kt | 75 +++- .../lifecycle/NavLeaseCoordinator.kt | 53 ++- .../navigation3/lifecycle/NavScopes.kt | 8 +- .../compose/navigation3/lifecycle/README.md | 45 +- .../lifecycle/StagedNavTransaction.kt | 94 ++++- .../CoordinatedBackStackObserverTest.kt | 2 + ...SentryNavEffectLifecycleIntegrationTest.kt | 388 +++++++++++++++++- .../lifecycle/NavLeaseCoordinatorTest.kt | 134 +++++- .../lifecycle/StagedNavTransactionTest.kt | 239 +++++++++++ 11 files changed, 1006 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d6df0298302..282ddf7e7f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ ### Behavioral Changes - Nav3: Returning to an Activity or Fragment with its own `SentryNavEffect` within the app (e.g., closing the Activity that covered it, swiping back to its ViewPager2 page, or dismissing a dialog over it) now starts a new navigation transaction for the current destination if the previous one finished, so spans produced upon return can now be consistently captured ([#6482](https://github.com/getsentry/sentry-java/pull/6482)) +- Nav3: `ui.load` transactions are bound to `IScope` in preference to Nav3 transactions when transitioning between Activities, matching Nav2 behavior (API 29+) ([#XXXX](https://github.com/getsentry/sentry-java/pull/XXXX)) ### Fixes diff --git a/sentry-android-navigation3/build.gradle.kts b/sentry-android-navigation3/build.gradle.kts index 7ad51b43904..1837bafa672 100644 --- a/sentry-android-navigation3/build.gradle.kts +++ b/sentry-android-navigation3/build.gradle.kts @@ -66,6 +66,7 @@ dependencies { compileOnly(libs.androidx.compose.runtime) compileOnly(libs.androidx.compose.ui) + testImplementation(projects.sentryAndroidCore) testImplementation(libs.androidx.compose.ui.test.junit4) testImplementation(libs.androidx.fragment.ktx) testImplementation(libs.androidx.test.core) diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt index 0810c41f27a..3c949384f57 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt @@ -3,14 +3,39 @@ package io.sentry.compose.navigation3.lifecycle import android.app.Activity import android.app.Application import android.app.Application.ActivityLifecycleCallbacks +import android.os.Build import android.os.Bundle /** * Watches every Activity of the host application and reports the Activity lifecycle events that - * [NavLeaseCoordinator] acts on. + * [NavLeaseCoordinator] acts on, so that the coordinator needn't know how they differ across API + * levels. * - * - [onActivityResumed] is called when any Activity resumes, including Activities without a - * `SentryNavEffect`. + * Each event is reported relative to `ActivityLifecycleIntegration`, which binds an Activity's + * `ui.load` transaction to the scope when the Activity is created and removes it when the Activity + * is destroyed: + * + * - [onActivityCreatedOrResumed] is called when an Activity is created and again when it resumes, + * with [ActivityEvent.CREATED] or [ActivityEvent.RESUMED] as its event. On API 29+, creation is + * reported before `ActivityLifecycleIntegration` tries to bind the Activity's `ui.load` + * transaction; below API 29, after. So below API 29, a withdrawal on creation comes too late for + * that `ui.load` transaction, which stays unbound (see "Non-candidate roots and lease withdrawal" + * in the package README). + * + * - [onActivityDestroyed] is called when an Activity is destroyed, after + * `ActivityLifecycleIntegration` has removed the Activity's `ui.load` transaction from the scope. + * + * **Android ordering assumptions** + * + * The watcher relies on the following callback orders, which Android doesn't document but AOSP has + * historically upheld: + * + * - `Application` calls `ActivityLifecycleCallbacks` in registration order. Below API 29, + * `ActivityLifecycleIntegration`'s callbacks run before this class's, provided the SDK is + * initialized before this class starts [watching][watch]. + * + * - On API 29+, every `onActivityPreCreated` callback runs before any `onActivityCreated` callback, + * and every `onActivityDestroyed` callback before any `onActivityPostDestroyed` callback. * * **Lifetime** * @@ -18,7 +43,19 @@ import android.os.Bundle * `defaultNavLeaseCoordinator`, which owns the watcher used in practice, they're meant to live as * long as the process. */ -internal class NavActivityWatcher(private val onActivityResumed: (activity: Activity) -> Unit) { +internal class NavActivityWatcher( + private val onActivityCreatedOrResumed: (activity: Activity, event: ActivityEvent) -> Unit, + private val onActivityDestroyed: (activity: Activity) -> Unit, +) { + + /** + * The Activity lifecycle event passed to [onActivityCreatedOrResumed], described by [description] + * in past tense (e.g., "created") for log messages. + */ + enum class ActivityEvent(val description: String) { + CREATED("created"), + RESUMED("resumed"), + } private var isWatching = false @@ -33,11 +70,23 @@ internal class NavActivityWatcher(private val onActivityResumed: (activity: Acti private inner class Callbacks : ActivityLifecycleCallbacks { - override fun onActivityResumed(activity: Activity) { - this@NavActivityWatcher.onActivityResumed(activity) + // Only called on API 29+, before any callback's onActivityCreated. + override fun onActivityPreCreated(activity: Activity, savedInstanceState: Bundle?) { + onActivityCreatedOrResumed(activity, ActivityEvent.CREATED) + } + + // Covers API levels below 29, as they don't call onActivityPreCreated. Runs after + // ActivityLifecycleIntegration's onActivityCreated, which finds the slot taken and never binds + // the Activity's ui.load transaction. This is a known limitation (see the package README). + override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) { + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.Q) { + onActivityCreatedOrResumed(activity, ActivityEvent.CREATED) + } } - override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) = Unit + override fun onActivityResumed(activity: Activity) { + onActivityCreatedOrResumed(activity, ActivityEvent.RESUMED) + } override fun onActivityStarted(activity: Activity) = Unit @@ -47,6 +96,16 @@ internal class NavActivityWatcher(private val onActivityResumed: (activity: Acti override fun onActivitySaveInstanceState(activity: Activity, outState: Bundle) = Unit - override fun onActivityDestroyed(activity: Activity) = Unit + // Covers API levels below 29, which don't call onActivityPostDestroyed. + override fun onActivityDestroyed(activity: Activity) { + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.Q) { + this@NavActivityWatcher.onActivityDestroyed(activity) + } + } + + // Only called on API 29+, after every callback's onActivityDestroyed. + override fun onActivityPostDestroyed(activity: Activity) { + this@NavActivityWatcher.onActivityDestroyed(activity) + } } } diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt index 7851d757ffd..f4ffdab1501 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt @@ -57,9 +57,14 @@ internal class NavLeaseCoordinator { /** * Reports the host application's Activities to this coordinator so that it can detect whether a - * leaseholder is covered by a non-candidate root. + * leaseholder is covered by a non-candidate root, and retry a nav transaction blocked by a closed + * Activity's `ui.load` transaction. */ - private val activityWatcher = NavActivityWatcher(onActivityResumed = ::onAnyActivityResumed) + private val activityWatcher = + NavActivityWatcher( + onActivityCreatedOrResumed = ::onActivityCreatedOrResumed, + onActivityDestroyed = ::onAnyActivityDestroyed, + ) /** * Registers a [nav root][rootInfo] as a lease candidate until disposal or lifecycle destruction. @@ -295,14 +300,17 @@ internal class NavLeaseCoordinator { it.canReceiveLease && it.isActivityNavRoot(activity) } - /** Withdraws the lease when the resumed [activity] appears to cover a non-resumed nav root. */ - private fun onAnyActivityResumed(activity: Activity) { - // Assume the resumed `activity` is covering the current leaseholder if i) the leaseholder - // itself isn't resumed, ii) `activity` isn't the leaseholder's host Activity, and iii) - // `activity` isn't itself a nav root (see [isActivityNavRoot]). (i) checks the leaseholder's - // own state rather than its host Activity's, as they differ for Fragment nav roots: a Fragment - // leaseholder that isn't resumed isn't what the user is interacting with, even if its host - // Activity is resumed. + /** Withdraws the lease when [activity] appears to cover a non-resumed nav root. */ + private fun onActivityCreatedOrResumed( + activity: Activity, + event: NavActivityWatcher.ActivityEvent, + ) { + // Assume the created or resumed `activity` is covering the current leaseholder if i) the + // leaseholder itself isn't resumed, ii) `activity` isn't the leaseholder's host Activity, and + // iii) `activity` isn't itself a nav root (see [isActivityNavRoot]). (i) checks the + // leaseholder's own state rather than its host Activity's, as they differ for Fragment nav + // roots: a Fragment leaseholder that isn't resumed isn't what the user is interacting with, + // even if its host Activity is resumed. val holder = leaseHolder ?: return val hostActivity = holder.hostActivity ?: return if ( @@ -317,11 +325,28 @@ internal class NavLeaseCoordinator { withdrawLease( scope, withdrawalReason = - "${activity.javaClass.name} was resumed while the nav root wasn't resumed", + "${activity.javaClass.name} was ${event.description} while the nav root wasn't resumed", ) } } + /** + * After [activity] is destroyed, asks the leaseholder to retry publication if an Activity's + * `ui.load` blocked it. The publisher decides whether the blocker has left and retry is allowed. + */ + private fun onAnyActivityDestroyed(activity: Activity) { + val holder = leaseHolder ?: return + if (holder.hostActivity === activity) { + return + } + + holder.withScopeOrNull { scope -> + if (scope != null) { + holder.publisher.retryTransactionPublication(scope) + } + } + } + /** * A [nav root][rootInfo] eligible to hold the lease managed by [NavLeaseCoordinator]. * @@ -483,6 +508,12 @@ internal interface INavStatePublisher { /** Publishes the candidate's staged nav state to [scope]. */ fun publishTo(scope: IScope) + /** + * Retries a nav transaction blocked by an Activity's `ui.load`, subject to + * [StagedNavTransaction.retryPublishIfBlocked]. Leaves other published nav state alone. + */ + fun retryTransactionPublication(scope: IScope) + /** Removes published nav state from [scope], retaining it for a later lease acquisition. */ fun unpublishFrom(scope: IScope) diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt index 39568fa2a29..8bce50a60d4 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt @@ -154,6 +154,10 @@ internal class NavScopes( stagedNavTransaction.publishTo(scope) } + override fun retryTransactionPublication(scope: IScope) { + stagedNavTransaction.retryPublishIfBlocked(scope) + } + override fun unpublishFrom(scope: IScope) { stagedNavTransaction.unpublishFrom(scope) stagedNavData.clearPublishedStateFrom(scope) @@ -228,7 +232,9 @@ internal class NavScopes( override fun startNavTransaction(request: NavTransactionRequest) { stagedNavTransaction.stage(request) if (hasLease) { - stagedNavTransaction.publishTo(delegate) + // Don't retry b/c ui.load retry is only for returns to a nav root, not for navigation + // within it. + stagedNavTransaction.publishTo(delegate, canRetry = false) } else { stagedNavData.deferPropagationContextRotation() } diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md index 2b496552c93..e34b746f5eb 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md @@ -153,8 +153,8 @@ leaving the transaction unbound but running until the leaseholder resumes. Non-candidate roots never register, so the coordinator infers their presence from either of two signals: -- another Activity resumes while the leaseholder isn't resumed, unless the resumed Activity is - itself a nav root, which takes the lease when it resumes; or +- another Activity is created or resumed while the leaseholder isn't resumed, unless the resumed + Activity is itself a nav root, which takes the lease when it resumes; or - the leaseholder pauses while its host Activity stays resumed (e.g., a ViewPager2 swipe). (Note that an Activity pauses before its Fragments do when the Activity is backgrounded or covered.) @@ -163,10 +163,19 @@ Non-candidate roots never register, so the coordinator infers their presence fro (e.g., dialogs) go undetected. These two signals are deliberately the only ones: other surfaces that cover a leaseholder without pausing it (e.g., bottom sheets or Compose dialogs) aren't detected, and shouldn't be by adding signals here. -**Limitation (before the covering Activity resumes)**: The lease is withdrawn only once the covering Activity resumes. Until then (i.e., -during its `onCreate` and `onStart`), errors and spans the covering Activity produces are still attributed to the leaseholder. For the same -reason, the covering Activity's `ui.load` transaction isn't bound to the scope: `ActivityLifecycleIntegration` tries to bind it when the -Activity is created, while the leaseholder's nav transaction still holds the scope's transaction slot. +**Limitation (below API 29)**: An Activity created over the leaseholder doesn't get its `ui.load` transaction bound to the scope. There's no +`onActivityPreCreated` below API 29, so the lease is withdrawn only after `ActivityLifecycleIntegration` has tried to bind the Activity's +`ui.load` transaction and found the leaseholder's nav transaction in the scope's transaction slot. The integration doesn't try again, so its +`ui.load` transaction stays unbound, as it did before withdrawal existed. Withdrawal still keeps the Activity's errors and spans from being +attributed to the leaseholder: spans the Activity produces attach to no transaction until one is bound (e.g., the nav transaction for the +first destination of the Activity's own `SentryNavEffect`, if it has one). Closing this gap would take a change to +`ActivityLifecycleIntegration` that affects every app, so it's deliberately left open. + +As a result, an Activity with its own `SentryNavEffect` gets a different transaction bound for its spans depending on the API level. On +API 29+, its `ui.load` transaction is bound, and its first destination's nav transaction isn't started because the slot is taken. Below +API 29, when the leaseholder's nav transaction held the slot as the Activity was created, its first destination's nav transaction is bound +instead. Either way, `ActivityLifecycleIntegration` still sends the `ui.load` transaction, including its TTID and TTFD spans. Only the +parent of auto-instrumented spans (e.g., HTTP or database spans) differs. ### Returning to a nav root @@ -185,6 +194,24 @@ Either way, the transaction is bound only if the scope's transaction slot is fre happen when the candidate publishes (`INavStatePublisher.publishTo`) its nav state, so the coordinator needn't tell a return from a first acquisition: on its first back stack update, a candidate has no destination to start a transaction for yet. +When the user returns by closing an Activity, that Activity's `ui.load` transaction may still be running and hold the slot. It normally +leaves the slot when the closed Activity is destroyed, which happens after the nav root has resumed. If the Activity is destroyed within +`UI_LOAD_RETRY_WINDOW_NANOS` of the blocked attempt, binding the nav root's transaction is retried once (see `onAnyActivityDestroyed`). +Otherwise, the nav root goes without a bound nav transaction until its next navigation or return. + +Only the current leaseholder is asked to retry when an Activity is destroyed. Its publisher checks whether the blocked `ui.load` has left +the slot and whether the retry window is still open. A leaseholder that loses or withdraws its lease forgets the blocked publication. + +The retry has three gaps: + +- A `ui.action` transaction started by the gesture that caused the return (e.g., a ViewPager2 swipe or an in-app back button) can block the + nav transaction. Only Activity `ui.load` transactions trigger this retry. +- Spans produced between the nav root's resume and the closed Activity's destruction may attach to the closed Activity's `ui.load`. +- An Activity left without being destroyed (e.g., when another Activity is reordered to the front) triggers no retry. + +Unbinding a finishing Activity's `ui.load` when it pauses would close the last two gaps, but would change `ActivityLifecycleIntegration` +for every app. Giving navigation priority over `ui.action` is an SDK-wide scope-slot policy decision shared with other integrations. + A nav root that's destroyed and recreated after losing the lease (e.g., on a configuration change) isn't returned to: it registers anew and gets a transaction on its first back stack update. See [#6454](https://github.com/getsentry/sentry-java/issues/6454). @@ -194,7 +221,8 @@ existing Activity comes back to the foreground. ### Android ordering assumptions -`NavLeaseCoordinator` relies on the following sequences, which Android doesn't document but AOSP and androidx have historically upheld: +Besides the callback ordering `NavActivityWatcher` relies on, `NavLeaseCoordinator` relies on the following sequences, which Android doesn't +document but AOSP and androidx have historically upheld: - An Activity's `onActivityResumed` callbacks run before its `Lifecycle` dispatches `ON_RESUME` (see `isActivityNavRoot`). @@ -203,5 +231,8 @@ existing Activity comes back to the foreground. lifecycles do. androidx's `ReportFragment` and `FragmentActivity` ensure this, not the platform. +- A visible Activity that finishes is destroyed once the Activity that replaces it reports idle, + or when the system's idle timeout fires (see `UI_LOAD_RETRY_WINDOW_NANOS`). + Going forward, if an order changes, nav data may be attributed to the wrong nav root, or a nav transaction may go unbound, but nothing would throw. diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt index 59916420c76..f5460d1a3e7 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt @@ -3,8 +3,28 @@ package io.sentry.compose.navigation3.lifecycle import io.sentry.IScope import io.sentry.IScopes import io.sentry.ITransaction +import io.sentry.SentryDate import io.sentry.SentryLevel import io.sentry.SpanStatus +import java.lang.ref.WeakReference +import java.util.concurrent.TimeUnit + +/** + * The origin of the `ui.load` transactions `ActivityLifecycleIntegration` starts for Activities. + * + * Mirrors `ActivityLifecycleIntegration.TRACE_ORIGIN`, which isn't public. + */ +internal const val ACTIVITY_UI_LOAD_ORIGIN: String = "auto.ui.activity" + +/** + * Maximum time to retry a nav transaction if an Activity’s `ui.load` transaction blocks its + * publication. + * + * Android usually destroys a closed Activity once the returned-to Activity goes idle, removing its + * `ui.load` transaction from the shared [IScope]. A busy main thread can delay destruction past + * this window; in that case, the return gets no nav transaction until the next navigation. + */ +internal val UI_LOAD_RETRY_WINDOW_NANOS: Long = TimeUnit.SECONDS.toNanos(10) /** * Holds a nav transaction staged for publication to a shared [IScope]. @@ -26,12 +46,14 @@ import io.sentry.SpanStatus * * Navigation doesn't displace an unfinished transaction already bound to the shared `IScope`. This * includes `ui.load`, `ui.action`, and host-app transactions. Changing their relative priority - * requires an SDK-wide policy. + * requires an SDK-wide policy; this class only retries briefly after a closed Activity's `ui.load` + * releases the slot (see [retryPublishIfBlocked]). * * **Thread safety** * * This class is ***not*** thread-safe. Apply the thread safety policy from [NavScopes]. */ +@Suppress("TooManyFunctions") internal class StagedNavTransaction(private val scopes: IScopes) { /** @@ -47,6 +69,15 @@ internal class StagedNavTransaction(private val scopes: IScopes) { /** The transaction most recently started for [request], unless it has been forgotten since. */ private var transaction: ITransaction? = null + /** + * The most recent [publication][publishTo], if an Activity `ui.load` transaction kept it from + * binding a nav transaction. Cleared when the publication is retried, and when the nav + * transaction is next published, unpublished, finished, or forgotten. + * + * See [retryPublishIfBlocked]. + */ + private var blockedPublication: BlockedPublication? = null + /** * Stages [request], so that the next [publishTo] starts a transaction for it. * @@ -73,25 +104,57 @@ internal class StagedNavTransaction(private val scopes: IScopes) { * Publishes the staged transaction by binding it to [scope], or starts a transaction from the * staged request if needed. An occupied scope slot leaves the transaction unbound and the request * available for later publication. + * + * If [canRetry] is true and an Activity's `ui.load` transaction occupies the scope slot, the + * publication can be retried once the `ui.load` transaction leaves the scope (see + * [retryPublishIfBlocked]). */ - fun publishTo(scope: IScope) { + fun publishTo(scope: IScope, canRetry: Boolean = true) { + blockedPublication = null val request = request ?: return val started = transaction if (started != null && !started.isFinished) { if (scope.isFreeFor(request.name, started)) { bind(scope, started) + } else if (canRetry) { + blockedPublication = scope.blockedPublicationFor(started) } return } transaction = null started?.let { unbind(scope, it) } - startAndBind(scope, request) + // A start declined by the scopes (i.e., a no-op transaction) leaves the slot free, so it isn't + // retried. + if (startAndBind(scope, request) == null && canRetry) { + blockedPublication = scope.blockedPublicationFor(transaction = null) + } + } + + /** + * Retries publication to [scope] after the blocking Activity `ui.load` leaves its slot. Retries + * only within [UI_LOAD_RETRY_WINDOW_NANOS], and never arms another retry. Other blockers do not + * qualify. + */ + fun retryPublishIfBlocked(scope: IScope) { + val blocked = blockedPublication ?: return + if (scopes.options.dateProvider.now().diff(blocked.blockedAt) > UI_LOAD_RETRY_WINDOW_NANOS) { + blockedPublication = null + return + } + val uiLoad = blocked.uiLoad.get() + if (uiLoad != null && !uiLoad.isFinished && scope.transaction === uiLoad) { + // The ui.load transaction still holds the slot; wait for it to leave. + return + } + + publishTo(scope, canRetry = false) } /** Unbinds the transaction from [scope] without finishing it, allowing later publication. */ fun unpublishFrom(scope: IScope) { + blockedPublication = null transaction?.let { unbind(scope, it) } } @@ -101,6 +164,7 @@ internal class StagedNavTransaction(private val scopes: IScopes) { */ fun finish(scope: IScope) { request = null + blockedPublication = null val transaction = transaction ?: return // Forget the transaction before unbinding it: unbinding notifies the host app's scope // observers, which can throw, and a transaction left behind would be bound again later. @@ -115,6 +179,7 @@ internal class StagedNavTransaction(private val scopes: IScopes) { fun forget() { request = null transaction = null + blockedPublication = null } /** Whether the slot is empty, holds a finished transaction, or already holds [transaction]. */ @@ -173,4 +238,27 @@ internal class StagedNavTransaction(private val scopes: IScopes) { ) return false } + + /** + * Returns a blocker only when this scope holds an Activity `ui.load` instead of [transaction]. + * Pass `null` to check whether it blocks a new transaction. + */ + private fun IScope.blockedPublicationFor(transaction: ITransaction?): BlockedPublication? { + val current = this.transaction ?: return null + if (canHold(current, transaction) || current.spanContext.origin != ACTIVITY_UI_LOAD_ORIGIN) { + return null + } + return BlockedPublication(WeakReference(current), scopes.options.dateProvider.now()) + } + + /** + * A nav publication blocked by [uiLoad] at [blockedAt]. + * + * The `ui.load` transaction is held weakly, as it's only compared with the scope's transaction. + * (A collected transaction can't be bound to the scope anymore.) + */ + private class BlockedPublication( + val uiLoad: WeakReference, + val blockedAt: SentryDate, + ) } diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt index 6164f479bab..2b8547b4084 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt @@ -522,6 +522,8 @@ class CoordinatedBackStackObserverTest { private object NoOpNavStatePublisher : INavStatePublisher { override fun publishTo(scope: IScope) = Unit + override fun retryTransactionPublication(scope: IScope) = Unit + override fun unpublishFrom(scope: IScope) = Unit override fun onUnregistered(scope: IScope?) = Unit diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt index 2a9138b93e5..c46fe6a99aa 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt @@ -27,11 +27,19 @@ import androidx.viewpager2.adapter.FragmentStateAdapter import androidx.viewpager2.widget.ViewPager2 import com.google.common.truth.Truth.assertThat import io.sentry.IScope +import io.sentry.IScopes import io.sentry.ISentryClient +import io.sentry.ITransaction import io.sentry.Scope import io.sentry.ScopeType import io.sentry.Scopes -import io.sentry.SentryOptions +import io.sentry.TransactionContext +import io.sentry.TransactionOptions +import io.sentry.android.core.ActivityFramesTracker +import io.sentry.android.core.ActivityLifecycleIntegration +import io.sentry.android.core.BuildInfoProvider +import io.sentry.android.core.SentryAndroidOptions +import io.sentry.android.core.performance.AppStartMetrics import io.sentry.compose.navigation3.lifecycle.NAVIGATION_CONTEXT_KEY import io.sentry.compose.navigation3.lifecycle.NavLeaseCoordinator import org.junit.After @@ -41,6 +49,7 @@ import org.mockito.kotlin.mock import org.mockito.kotlin.whenever import org.robolectric.Robolectric import org.robolectric.Shadows.shadowOf +import org.robolectric.android.controller.ActivityController import org.robolectric.annotation.Config /** @@ -62,6 +71,7 @@ class SentryNavEffectLifecycleIntegrationTest { @After fun tearDown() { + Fixture.current?.close() Fixture.current = null } @@ -89,6 +99,63 @@ class SentryNavEffectLifecycleIntegrationTest { .inOrder() } + /** + * On API 29+, core's `onActivityPreCreated` callback can run while the previous Activity's Nav3 + * transaction is still bound to the scope. Our Nav3 implementation releases it in its own + * `onActivityPreCreated` callback. Android runs all of those callbacks before + * `onActivityCreated`, when core binds the incoming Activity's `ui.load` transaction, so it takes + * the freed slot. + */ + @Test + fun `an incoming activity binds its own ui load transaction instead of its first nav transaction`() { + val fixture = Fixture.create() + fixture.installActivityLifecycleIntegration(CheckoutActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + + home.pause() + Robolectric.buildActivity(CheckoutActivity::class.java).setup() + fixture.idle() + + val uiLoadTransaction = fixture.uiLoadTransactions.single() + assertThat(uiLoadTransaction.operation).isEqualTo("ui.load") + assertThat(fixture.scope.transaction).isSameInstanceAs(uiLoadTransaction) + assertThat(homeTransaction.isFinished).isFalse() + assertThat(fixture.scope.screen).isEqualTo("/Checkout") + assertThat(fixture.scope.navigationEntries()).containsExactly("/Checkout") + } + + /** + * Below API 29, there's no `onActivityPreCreated`, so the outgoing Activity's lease is withdrawn + * from `onActivityCreated` if `ActivityLifecycleIntegration` finds that the scope's transaction + * slot is taken. `ActivityLifecycleIntegration` doesn't try again, so its `ui.load` transaction + * remains unbound. The incoming Activity's first nav transaction takes the freed slot instead (a + * known limitation – see `NavLeaseCoordinator`). + */ + @Test + @Config(sdk = [28]) + fun `below API 29, first nav transaction binds when incoming ui load cannot`() { + val fixture = Fixture.create() + fixture.installActivityLifecycleIntegration(CheckoutActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + + home.pause() + Robolectric.buildActivity(CheckoutActivity::class.java).setup() + fixture.idle() + + val uiLoadTransaction = fixture.uiLoadTransactions.single() + assertThat(uiLoadTransaction.isFinished).isFalse() + val checkoutTransaction = requireNotNull(fixture.scope.transaction) + assertThat(checkoutTransaction.name).isEqualTo("/Checkout") + assertThat(checkoutTransaction.operation).isEqualTo("navigation") + assertThat(homeTransaction.isFinished).isFalse() + assertThat(fixture.scope.screen).isEqualTo("/Checkout") + assertThat(fixture.scope.navigationEntries()).containsExactly("/Checkout") + } + @Test fun `an incoming activity's nav data and transaction survive the outgoing activity stopping`() { val fixture = Fixture.create() @@ -215,6 +282,85 @@ class SentryNavEffectLifecycleIntegrationTest { .inOrder() } + /** + * Known limitation: until the closing Activity is destroyed, its unfinished `ui.load` transaction + * keeps the scope's transaction slot, so the Activity returned to has no nav transaction bound + * yet, and spans produced on return attach to the closing Activity's `ui.load` instead. + */ + @Test + fun `going back keeps the covering activity's unfinished ui load bound until it's destroyed`() { + val fixture = Fixture.create() + fixture.returnToHomeWhileCheckoutUiLoadIsUnfinished() + val uiLoadTransaction = fixture.uiLoadTransactions.single() + + assertThat(fixture.scope.transaction).isSameInstanceAs(uiLoadTransaction) + assertThat(fixture.scope.screen).isEqualTo("/Home") + } + + @Test + fun `going back during the covering activity's ui load starts a nav transaction once it's destroyed`() { + val fixture = Fixture.create() + val (_, checkout) = fixture.returnToHomeWhileCheckoutUiLoadIsUnfinished() + + checkout.stop() + checkout.destroy() + fixture.idle() + + val restarted = requireNotNull(fixture.scope.transaction) + assertThat(restarted.name).isEqualTo("/Home") + assertThat(restarted.operation).isEqualTo("navigation") + assertThat(fixture.uiLoadTransactions.single().isFinished).isTrue() + assertThat(fixture.scope.screen).isEqualTo("/Home") + } + + /** + * Below API 29, the covering Activity's `ui.load` transaction only binds if the scope's + * transaction slot is free when the Activity is created (see `NavLeaseCoordinator`), e.g., + * because Home's nav transaction has already finished. + */ + @Test + @Config(sdk = [28]) + fun `below API 29, going back during the covering activity's ui load starts a nav transaction once it's destroyed`() { + val fixture = Fixture.create() + fixture.installActivityLifecycleIntegration(CheckoutActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + requireNotNull(fixture.scope.transaction).finish() + home.pause() + val checkout = Robolectric.buildActivity(CheckoutActivity::class.java).setup() + fixture.idle() + home.stop() + checkout.pause() + home.restart() + home.resume() + fixture.idle() + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.uiLoadTransactions.single()) + + checkout.stop() + checkout.destroy() + fixture.idle() + + val restarted = requireNotNull(fixture.scope.transaction) + assertThat(restarted.name).isEqualTo("/Home") + assertThat(restarted.operation).isEqualTo("navigation") + assertThat(fixture.scope.screen).isEqualTo("/Home") + } + + @Test + fun `going back during the covering activity's ui load rebinds the unfinished nav transaction once it's destroyed`() { + val fixture = Fixture.create() + val (_, checkout) = + fixture.returnToHomeWhileCheckoutUiLoadIsUnfinished(finishHomeTransaction = false) + val homeTransaction = fixture.homeTransaction + + checkout.stop() + checkout.destroy() + fixture.idle() + + assertThat(fixture.scope.transaction).isSameInstanceAs(homeTransaction) + assertThat(homeTransaction.isFinished).isFalse() + } + // Activity recreation @Test fun `a recreated activity publishes its own nav data and transaction over the destroyed instance's`() { @@ -867,6 +1013,59 @@ class SentryNavEffectLifecycleIntegrationTest { assertThat(homeTransaction.isFinished).isFalse() } + /** + * `ActivityLifecycleIntegration` binds an Activity's `ui.load` transaction when the Activity is + * created, and only if the scope's transaction slot is free. + */ + @Test + fun `an activity without an effect binds its own ui load transaction over a covered activity's nav transaction`() { + val fixture = Fixture.create() + fixture.installActivityLifecycleIntegration(PlainActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + + home.pause() + Robolectric.buildActivity(PlainActivity::class.java).setup() + fixture.idle() + + val uiLoadTransaction = fixture.uiLoadTransactions.single() + assertThat(uiLoadTransaction.operation).isEqualTo("ui.load") + assertThat(fixture.scope.screen).isEqualTo(PlainActivity::class.java.canonicalName) + assertThat(fixture.scope.navigationEntries()).isNull() + assertThat(fixture.scope.transaction).isSameInstanceAs(uiLoadTransaction) + assertThat(homeTransaction.isFinished).isFalse() + } + + /** + * Below API 29, there's no `onActivityPreCreated`, so the lease is withdrawn from + * `onActivityCreated`, after `ActivityLifecycleIntegration` has found the scope's transaction + * slot taken. `ActivityLifecycleIntegration` doesn't try again, so its `ui.load` transaction + * stays unbound (a known limitation – see `NavLeaseCoordinator`). The covered Activity's nav data + * and transaction are still removed, so they aren't attributed to the Activity. The screen + * `ActivityLifecycleIntegration` set for the Activity is left in place, as it isn't the covered + * Activity's. + */ + @Test + @Config(sdk = [28]) + fun `below API 29, activity without an effect leaves no transaction bound`() { + val fixture = Fixture.create() + fixture.installActivityLifecycleIntegration(PlainActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + + home.pause() + Robolectric.buildActivity(PlainActivity::class.java).setup() + fixture.idle() + + assertThat(fixture.uiLoadTransactions.single().isFinished).isFalse() + assertThat(fixture.scope.screen).isEqualTo(PlainActivity::class.java.canonicalName) + assertThat(fixture.scope.navigationEntries()).isNull() + assertThat(fixture.scope.transaction).isNull() + assertThat(homeTransaction.isFinished).isFalse() + } + @Test fun `going back to an activity covered by one without an effect restores its nav data and unfinished transaction`() { val fixture = Fixture.create() @@ -1242,7 +1441,7 @@ class SentryNavEffectLifecycleIntegrationTest { private class Fixture private constructor(tracesSampleRate: Double) { val options = - SentryOptions().apply { + SentryAndroidOptions().apply { dsn = "http://key@localhost/1" defaultScopeType = ScopeType.CURRENT setTracesSampleRate(tracesSampleRate) @@ -1283,6 +1482,74 @@ class SentryNavEffectLifecycleIntegrationTest { events += "$name:$event" } + /** `ui.load` transactions started by [installActivityLifecycleIntegration], in start order. */ + val uiLoadTransactions = mutableListOf() + + /** Home's nav transaction, as set up by [returnToHomeWhileCheckoutUiLoadIsUnfinished]. */ + lateinit var homeTransaction: ITransaction + + /** + * Opens Checkout over Home, binding Checkout's `ui.load` transaction, then returns to Home + * while that `ui.load` transaction is unfinished: Checkout is paused and Home resumed, but + * Checkout isn't stopped or destroyed yet. Finishes Home's nav transaction while Checkout + * covers it unless [finishHomeTransaction] is `false`. + * + * API 29+ only: below API 29, Checkout's `ui.load` transaction can't bind while Home's nav + * transaction holds the scope's transaction slot. + */ + fun returnToHomeWhileCheckoutUiLoadIsUnfinished( + finishHomeTransaction: Boolean = true + ): Pair, ActivityController> { + installActivityLifecycleIntegration(CheckoutActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + idle() + homeTransaction = requireNotNull(scope.transaction) + home.pause() + val checkout = Robolectric.buildActivity(CheckoutActivity::class.java).setup() + idle() + home.stop() + idle() + if (finishHomeTransaction) { + homeTransaction.finish() + } + + checkout.pause() + home.restart() + home.resume() + idle() + return home to checkout + } + + private var activityLifecycleIntegration: ActivityLifecycleIntegration? = null + + /** + * Installs a real [ActivityLifecycleIntegration] that traces only instances of + * [tracedActivityClass], so that each such Activity gets a `ui.load` transaction that competes + * with nav transactions for the scope's transaction slot as it does in an app. + * + * Call it before building any Activity: the SDK registers the integration at initialization, so + * its callbacks run before those the coordinator registers when the first effect attaches. + * + * Limiting it to [tracedActivityClass] keeps the other Activities' nav transactions bindable, + * as the tests expect. + */ + fun installActivityLifecycleIntegration(tracedActivityClass: Class) { + val application: Application = ApplicationProvider.getApplicationContext() + val integration = + ActivityLifecycleIntegration( + TracedActivityApplication(application, tracedActivityClass), + BuildInfoProvider(options.logger), + mock(), + ) + integration.register(UiLoadRecordingScopes(scopes, uiLoadTransactions), options) + activityLifecycleIntegration = integration + } + + fun close() { + activityLifecycleIntegration?.close() + AppStartMetrics.getInstance().clear() + } + /** * The host-app setup under test: one `SentryNavEffect` in an Activity's or Fragment's content. */ @@ -1323,6 +1590,123 @@ class SentryNavEffectLifecycleIntegrationTest { } } + /** + * Stands in for [application] when [ActivityLifecycleIntegration] registers its callbacks, and + * relays the application's Activity lifecycle callbacks to it only for instances of + * [tracedActivityClass]. + */ + private class TracedActivityApplication( + private val application: Application, + private val tracedActivityClass: Class, + ) : Application() { + + private val relays = mutableMapOf() + + override fun registerActivityLifecycleCallbacks(callback: ActivityLifecycleCallbacks) { + val relay = TracedActivityCallbacks(callback, tracedActivityClass) + relays[callback] = relay + application.registerActivityLifecycleCallbacks(relay) + } + + override fun unregisterActivityLifecycleCallbacks(callback: ActivityLifecycleCallbacks) { + relays.remove(callback)?.let { application.unregisterActivityLifecycleCallbacks(it) } + } + } + + /** Relays each callback to [delegate] only for instances of [tracedActivityClass]. */ + @Suppress("TooManyFunctions") + private class TracedActivityCallbacks( + private val delegate: Application.ActivityLifecycleCallbacks, + private val tracedActivityClass: Class, + ) : Application.ActivityLifecycleCallbacks { + + private inline fun relay(activity: Activity, block: () -> Unit) { + if (tracedActivityClass.isInstance(activity)) { + block() + } + } + + override fun onActivityPreCreated(activity: Activity, savedInstanceState: Bundle?) = + relay(activity) { delegate.onActivityPreCreated(activity, savedInstanceState) } + + override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) = + relay(activity) { delegate.onActivityCreated(activity, savedInstanceState) } + + override fun onActivityPostCreated(activity: Activity, savedInstanceState: Bundle?) = + relay(activity) { delegate.onActivityPostCreated(activity, savedInstanceState) } + + override fun onActivityPreStarted(activity: Activity) = + relay(activity) { delegate.onActivityPreStarted(activity) } + + override fun onActivityStarted(activity: Activity) = + relay(activity) { delegate.onActivityStarted(activity) } + + override fun onActivityPostStarted(activity: Activity) = + relay(activity) { delegate.onActivityPostStarted(activity) } + + override fun onActivityPreResumed(activity: Activity) = + relay(activity) { delegate.onActivityPreResumed(activity) } + + override fun onActivityResumed(activity: Activity) = + relay(activity) { delegate.onActivityResumed(activity) } + + override fun onActivityPostResumed(activity: Activity) = + relay(activity) { delegate.onActivityPostResumed(activity) } + + override fun onActivityPrePaused(activity: Activity) = + relay(activity) { delegate.onActivityPrePaused(activity) } + + override fun onActivityPaused(activity: Activity) = + relay(activity) { delegate.onActivityPaused(activity) } + + override fun onActivityPostPaused(activity: Activity) = + relay(activity) { delegate.onActivityPostPaused(activity) } + + override fun onActivityPreStopped(activity: Activity) = + relay(activity) { delegate.onActivityPreStopped(activity) } + + override fun onActivityStopped(activity: Activity) = + relay(activity) { delegate.onActivityStopped(activity) } + + override fun onActivityPostStopped(activity: Activity) = + relay(activity) { delegate.onActivityPostStopped(activity) } + + override fun onActivityPreSaveInstanceState(activity: Activity, outState: Bundle) = + relay(activity) { delegate.onActivityPreSaveInstanceState(activity, outState) } + + override fun onActivitySaveInstanceState(activity: Activity, outState: Bundle) = + relay(activity) { delegate.onActivitySaveInstanceState(activity, outState) } + + override fun onActivityPostSaveInstanceState(activity: Activity, outState: Bundle) = + relay(activity) { delegate.onActivityPostSaveInstanceState(activity, outState) } + + override fun onActivityPreDestroyed(activity: Activity) = + relay(activity) { delegate.onActivityPreDestroyed(activity) } + + override fun onActivityDestroyed(activity: Activity) = + relay(activity) { delegate.onActivityDestroyed(activity) } + + override fun onActivityPostDestroyed(activity: Activity) = + relay(activity) { delegate.onActivityPostDestroyed(activity) } + } + + /** Records each `ui.load` transaction started through [delegate] in [uiLoadTransactions]. */ + private class UiLoadRecordingScopes( + private val delegate: IScopes, + private val uiLoadTransactions: MutableList, + ) : IScopes by delegate { + + override fun startTransaction( + transactionContext: TransactionContext, + transactionOptions: TransactionOptions, + ): ITransaction = + delegate.startTransaction(transactionContext, transactionOptions).also { + if (transactionContext.operation == "ui.load") { + uiLoadTransactions += it + } + } + } + private companion object { fun navEffectView(context: Context, route: String): View { diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt index 30a052a2b66..fc2b1fb5ae2 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt @@ -743,6 +743,62 @@ class NavLeaseCoordinatorTest { // region lease withdrawal when another Activity covers the leaseholder + @Test + fun `another activity being pre-created while the leaseholder is paused withdraws the nav lease`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + + fixture.activityCallbacks.onActivityPreCreated(fixture.coveringActivity, null) + + assertThat(registration.hasLease).isFalse() + assertThat(fixture.unpublished).containsExactly("owner") + } + + @Test + fun `another activity being created while the leaseholder is paused withdraws the lease and unpublishes nav state`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + + fixture.activityCallbacks.onActivityCreated(fixture.coveringActivity, null) + + assertThat(registration.hasLease).isFalse() + assertThat(fixture.unpublished).containsExactly("owner") + } + + @Test + fun `another activity being created while the leaseholder is resumed keeps the nav lease`() { + val fixture = Fixture.create() + val registration = + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.activityCallbacks.onActivityCreated(fixture.coveringActivity, null) + + assertThat(registration.hasLease).isTrue() + assertThat(fixture.unpublished).isEmpty() + } + + @Test + fun `another activity being created while the leaseholder is paused logs the withdrawal`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + + fixture.activityCallbacks.onActivityCreated(fixture.coveringActivity, null) + + verify(fixture.logger) + .log( + SentryLevel.DEBUG, + "Withdrew the Nav3 lease from a nav root hosted by %s: %s.", + fixture.hostActivity.javaClass.name, + "${fixture.coveringActivity.javaClass.name} was created while the nav root wasn't resumed", + ) + } + @Test fun `another activity resuming while the leaseholder is paused withdraws the nav lease`() { val fixture = Fixture.create() @@ -1024,6 +1080,66 @@ class NavLeaseCoordinatorTest { // endregion + // region retrying nav transaction publication when an Activity is destroyed + + @Test + fun `another activity being post-destroyed retries the leaseholder's transaction publication`() { + val fixture = Fixture.create() + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.activityCallbacks.onActivityPostDestroyed(fixture.coveringActivity) + + assertThat(fixture.retried).containsExactly("owner") + } + + /** Unit tests run against a stubbed `Build.VERSION.SDK_INT` of 0, i.e., below API 29. */ + @Test + fun `below API 29, another activity being destroyed retries the leaseholder's transaction publication`() { + val fixture = Fixture.create() + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.activityCallbacks.onActivityDestroyed(fixture.coveringActivity) + + assertThat(fixture.retried).containsExactly("owner") + } + + @Test + fun `the leaseholder's host activity being destroyed doesn't retry its transaction publication`() { + val fixture = Fixture.create() + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.activityCallbacks.onActivityPostDestroyed(fixture.hostActivity) + + assertThat(fixture.retried).isEmpty() + } + + @Test + fun `an activity being destroyed without a leaseholder retries no transaction publication`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + fixture.activityCallbacks.onActivityCreated(fixture.coveringActivity, null) + + fixture.activityCallbacks.onActivityPostDestroyed(fixture.coveringActivity) + + assertThat(fixture.retried).isEmpty() + } + + @Test + fun `an activity being destroyed without a scope retries no transaction publication`() { + val fixture = Fixture.create() + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.withoutScope { + fixture.activityCallbacks.onActivityPostDestroyed(fixture.coveringActivity) + } + + assertThat(fixture.retried).isEmpty() + } + + // endregion + // region lease withdrawal when the leaseholder pauses within a resumed Activity @Test @@ -1399,6 +1515,9 @@ class NavLeaseCoordinatorTest { /** Names of the nav states unpublished, in the order they were unpublished. */ val unpublished = mutableListOf() + /** Names of the nav states asked to retry publishing their nav transactions, in order. */ + val retried = mutableListOf() + /** Names of the nav states notified of unregistration, with the scope they were given. */ val unregistered = mutableListOf>() @@ -1410,14 +1529,22 @@ class NavLeaseCoordinatorTest { /** * Publisher named [name] that, by default, records its publication in [published], its - * unpublishing in [unpublished], and its unregistration in [unregistered]. + * transaction publication retries in [retried], its unpublishing in [unpublished], and its + * unregistration in [unregistered]. */ fun publisher( name: String, onPublishTo: (IScope) -> Unit = { published += name }, + onRetryTransactionPublication: (IScope) -> Unit = { retried += name }, onUnpublishFrom: (IScope) -> Unit = { unpublished += name }, onUnregistered: (IScope?) -> Unit = { unregistered += name to it }, - ): INavStatePublisher = FakeNavStatePublisher(onPublishTo, onUnpublishFrom, onUnregistered) + ): INavStatePublisher = + FakeNavStatePublisher( + onPublishTo, + onRetryTransactionPublication, + onUnpublishFrom, + onUnregistered, + ) /** The application of [hostActivity] and [coveringActivity]. */ val application = mock() @@ -1498,11 +1625,14 @@ class NavLeaseCoordinatorTest { private class FakeNavStatePublisher( private val onPublishTo: (IScope) -> Unit, + private val onRetryTransactionPublication: (IScope) -> Unit, private val onUnpublishFrom: (IScope) -> Unit, private val onUnregistered: (IScope?) -> Unit, ) : INavStatePublisher { override fun publishTo(scope: IScope) = onPublishTo(scope) + override fun retryTransactionPublication(scope: IScope) = onRetryTransactionPublication(scope) + override fun unpublishFrom(scope: IScope) = onUnpublishFrom(scope) override fun onUnregistered(scope: IScope?) = onUnregistered.invoke(scope) diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt index 0488c7d1c17..5072e35ccfe 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt @@ -7,12 +7,15 @@ import io.sentry.ITransaction import io.sentry.NoOpTransaction import io.sentry.Scope import io.sentry.ScopeObserverAdapter +import io.sentry.SentryDateProvider import io.sentry.SentryLevel +import io.sentry.SentryNanotimeDate import io.sentry.SentryOptions import io.sentry.SpanContext import io.sentry.SpanStatus import io.sentry.TransactionContext import io.sentry.TransactionOptions +import java.util.Date import org.junit.Assert.assertThrows import org.junit.Test import org.mockito.kotlin.any @@ -31,6 +34,7 @@ class StagedNavTransactionTest { private val finishedTransaction = mock().also { whenever(it.isFinished).thenReturn(true) } private val hostTransaction = transactionWithOrigin("manual") + private val uiLoadTransaction = transactionWithOrigin(ACTIVITY_UI_LOAD_ORIGIN) @Test fun `publishTo() - keeps the started nav transaction when binding it throws`() { @@ -433,6 +437,237 @@ class StagedNavTransactionTest { assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) } + @Test + fun `retryPublishIfBlocked() - starts a nav transaction blocked by a ui load once it leaves the slot`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `retryPublishIfBlocked() - binds an unfinished nav transaction blocked by a ui load once it leaves the slot`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + stagedNavTransaction.unpublishFrom(fixture.scope) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(started) + assertThat(fixture.startedTransactions).containsExactly(started) + } + + @Test + fun `retryPublishIfBlocked() - starts a nav transaction blocked by a ui load once it finishes`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val uiLoad = transactionWithOrigin(ACTIVITY_UI_LOAD_ORIGIN) + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoad + stagedNavTransaction.publishTo(fixture.scope) + whenever(uiLoad.isFinished).thenReturn(true) + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `retryPublishIfBlocked() - waits while the blocking ui load holds the slot`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isSameInstanceAs(uiLoadTransaction) + + fixture.scope.clearTransaction() + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `retryPublishIfBlocked() - retries within the retry window`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + fixture.nowNanos = UI_LOAD_RETRY_WINDOW_NANOS + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `retryPublishIfBlocked() - retries nothing after the retry window`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + fixture.nowNanos = UI_LOAD_RETRY_WINDOW_NANOS + 1 + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `retryPublishIfBlocked() - retries nothing once the retry window passes while the ui load holds the slot`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.nowNanos = UI_LOAD_RETRY_WINDOW_NANOS + 1 + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + fixture.scope.clearTransaction() + fixture.nowNanos = 0 + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + } + + @Test + fun `retryPublishIfBlocked() - retries only once`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.transaction = transactionWithOrigin(ACTIVITY_UI_LOAD_ORIGIN) + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `retryPublishIfBlocked() - retries nothing after a transaction other than a ui load blocked it`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = hostTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `retryPublishIfBlocked() - starts no new nav transaction when the publication wasn't blocked`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + stagedNavTransaction.publishTo(fixture.scope) + whenever(started.isFinished).thenReturn(true) + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).containsExactly(started) + } + + @Test + fun `retryPublishIfBlocked() - retries nothing after a start the scopes declined`() { + val fixture = Fixture.create(declinesTransactions = true) + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + stagedNavTransaction.publishTo(fixture.scope) + fixture.declinesTransactions = false + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + } + + @Test + fun `retryPublishIfBlocked() - retries nothing once the nav transaction is unpublished`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + stagedNavTransaction.unpublishFrom(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `retryPublishIfBlocked() - retries nothing once the nav transaction is finished`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + stagedNavTransaction.finish(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + } + + @Test + fun `retryPublishIfBlocked() - starts nothing after a ui load blocked a publication that can't be retried`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope, canRetry = false) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `retryPublishIfBlocked() - binds nothing after a ui load blocked a rebind that can't be retried`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + fixture.start() + stagedNavTransaction.unpublishFrom(fixture.scope) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope, canRetry = false) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isNull() + } + @Test fun `unpublishFrom() - unbinds the started nav transaction without finishing it`() { val fixture = Fixture.create() @@ -613,6 +848,9 @@ class StagedNavTransactionTest { val scope = Scope(SentryOptions()) val logger = mock() + /** The time [scopes]' date provider reports, in nanoseconds. */ + var nowNanos = 0L + /** Whether [scopes] declines to start transactions, returning a no-op transaction instead. */ var declinesTransactions = false val startedTransactions = mutableListOf() @@ -627,6 +865,7 @@ class StagedNavTransactionTest { SentryOptions().apply { setLogger(this@Fixture.logger) isDebug = true + dateProvider = SentryDateProvider { SentryNanotimeDate(Date(0), nowNanos) } } ) whenever(scopes.startTransaction(any(), any())) From 884e7401e2d5bcfb5dc92ecf63d2506beaa1d87b Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Fri, 6 Nov 2026 06:03:16 +0100 Subject: [PATCH 2/2] changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 282ddf7e7f2..f1064f34d28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ ### Behavioral Changes - Nav3: Returning to an Activity or Fragment with its own `SentryNavEffect` within the app (e.g., closing the Activity that covered it, swiping back to its ViewPager2 page, or dismissing a dialog over it) now starts a new navigation transaction for the current destination if the previous one finished, so spans produced upon return can now be consistently captured ([#6482](https://github.com/getsentry/sentry-java/pull/6482)) -- Nav3: `ui.load` transactions are bound to `IScope` in preference to Nav3 transactions when transitioning between Activities, matching Nav2 behavior (API 29+) ([#XXXX](https://github.com/getsentry/sentry-java/pull/XXXX)) +- Nav3: `ui.load` transactions are bound to `IScope` in preference to Nav3 transactions when transitioning between Activities, matching Nav2 behavior (API 29+) ([#6484](https://github.com/getsentry/sentry-java/pull/6484)) ### Fixes