From 10ee57cb1c1cc890b5e565ffe766ad838b8d67eb Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Fri, 6 Nov 2026 06:02:41 +0100 Subject: [PATCH 1/4] 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 57a00e38a0..12b600b013 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,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 7a95528c32..f0a0e78386 100644 --- a/sentry-android-navigation3/build.gradle.kts +++ b/sentry-android-navigation3/build.gradle.kts @@ -69,6 +69,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 0810c41f27..3c949384f5 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 e2791e3141..803a4bad66 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. @@ -307,14 +312,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 ( @@ -329,11 +337,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]. * @@ -502,6 +527,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 39568fa2a2..8bce50a60d 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 2b496552c9..e34b746f5e 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 59916420c7..f5460d1a3e 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 6164f479ba..2b8547b408 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 2a9138b93e..c46fe6a99a 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 ae13a38d27..ec24919b23 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 @@ -1433,6 +1549,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>() @@ -1444,14 +1563,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() @@ -1532,11 +1659,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 0488c7d1c1..5072e35ccf 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 5a98b2aae5a1578d0a51fe0f7cc51dfc826d4064 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Fri, 6 Nov 2026 06:03:16 +0100 Subject: [PATCH 2/4] changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 12b600b013..36a50a484a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,7 +52,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 From 7033eade7ba0749499ef29b748f67e2a93b1896d Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Wed, 11 Nov 2026 12:02:06 +0100 Subject: [PATCH 3/4] ref(android): Address Nav3 lifecycle review feedback Hold a candidate's host Activity weakly so a candidate that fails to unregister can't keep a destroyed Activity alive, replace the resumedOnly flag with a minimumState parameter, and tidy the README footnotes. --- .../lifecycle/NavLeaseCoordinator.kt | 31 ++++++++++++------- .../compose/navigation3/lifecycle/README.md | 11 +++---- 2 files changed, 23 insertions(+), 19 deletions(-) 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 803a4bad66..2049b07ecc 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 @@ -12,6 +12,7 @@ import androidx.lifecycle.LifecycleOwner import io.sentry.IScope import io.sentry.IScopes import io.sentry.SentryLevel +import java.lang.ref.WeakReference /** * Manages a lease in order to determine which [NavScopes] instance can bind nav transactions and @@ -88,8 +89,8 @@ internal class NavLeaseCoordinator { val initialResumeOrder = if (lifecycleState == Lifecycle.State.RESUMED) ++resumeOrder else 0 val candidate = Candidate(rootInfo, lifecycleState, initialResumeOrder, publisher, scopes) - // Only list the candidate once addObserver() succeeds, so a failure can't leak the host - // Activity. + // Only list the candidate once addObserver() succeeds, so a failure can't leave behind a + // candidate that never observes its nav root's lifecycle. lifecycle.addObserver(candidate) candidates += candidate return candidate @@ -180,13 +181,12 @@ internal class NavLeaseCoordinator { /** * The eligible candidate most recently observed to be in a resumed state, other than [excluding]. * - * If [resumedOnly] is set to `true`, only candidates that are resumed right now are considered; - * if `false`, started and paused candidates are also included. Candidates hosted by - * [excludingHostedBy] are skipped. + * Only candidates whose current state is at least [minimumState] are considered. Candidates + * hosted by [excludingHostedBy] are skipped. */ private fun mostRecentlyResumedCandidate( excluding: Candidate? = null, - resumedOnly: Boolean = false, + minimumState: Lifecycle.State = Lifecycle.State.STARTED, excludingHostedBy: Activity? = null, ): Candidate? = candidates @@ -194,9 +194,8 @@ internal class NavLeaseCoordinator { it !== excluding && (excludingHostedBy == null || it.hostActivity !== excludingHostedBy) && it.canReceiveLease && - it.state.isAtLeast(Lifecycle.State.STARTED) && - it.resumeOrder > 0 && - (!resumedOnly || it.state == Lifecycle.State.RESUMED) + it.state.isAtLeast(minimumState) && + it.resumeOrder > 0 } .maxByOrNull { it.resumeOrder } @@ -237,7 +236,7 @@ internal class NavLeaseCoordinator { val next = mostRecentlyResumedCandidate( excluding = candidate, - resumedOnly = true, + minimumState = Lifecycle.State.RESUMED, excludingHostedBy = candidate.hostActivity.takeIf { candidate.isHostActivityPaused }, ) when { @@ -265,7 +264,9 @@ internal class NavLeaseCoordinator { // beneath it), and that nav root isn't what the user last saw. A paused nav root that comes // back to the foreground will acquire the lease when it resumes. if (leaseHolder === candidate) { - mostRecentlyResumedCandidate(resumedOnly = true)?.let { next -> handOffLease(next, scope) } + mostRecentlyResumedCandidate(minimumState = Lifecycle.State.RESUMED)?.let { next -> + handOffLease(next, scope) + } } } @@ -372,7 +373,13 @@ internal class NavLeaseCoordinator { private val scopes: IScopes, ) : NavLeaseRegistration, LifecycleEventObserver { - val hostActivity: Activity? = rootInfo.activity + // Held weakly because candidates live in a process-wide coordinator: if disposal fails before + // the candidate is removed, it must not keep its destroyed host Activity alive. + private val hostActivityRef: WeakReference? = rootInfo.activity?.let(::WeakReference) + + /** The Activity hosting this nav root, or `null` if it's unknown or has been collected. */ + val hostActivity: Activity? + get() = hostActivityRef?.get() /** Whether the host Activity is resumed; false when its lifecycle is unavailable. */ val isHostActivityResumed: Boolean 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 e34b746f5e..e296066ee5 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 @@ -39,17 +39,14 @@ While nav roots overlap (e.g., during an Activity transition), the nav transacti 2. the incoming nav root that may crash before it becomes visible, but not both. `NavLeaseCoordinator` opts for (2) so that the incoming destination populates crash context and any work performed is -associated with it† – the idea being that start-up logic is more likely to cause a crash or produce interesting spans than an outgoing -destination's steady state. - -> † Associating work with the incoming transaction means spans produced by the outgoing nav root -may appear as if they were produced by the incoming nav root (or vice versa, were our policy -reversed). +associated with it – the idea being that start-up logic is more likely to cause a crash or produce interesting spans than an outgoing +destination's steady state. As a consequence, spans produced by the outgoing nav root may appear as if they were produced by the incoming +nav root (or vice versa, were our policy reversed). ### Lease handoff policy The lease is acquired preemptively by an incoming nav root's `NavScopes` instance when it applies its first back stack update -(`onBackStackUpdate`).† +(`onBackStackUpdate`)†. A displaced nav root remains eligible for lease restoration until it stops (`ON_STOP`). It gets the lease back when it resumes (`ON_RESUME`), or sooner if the new nav root leaves or pauses while the displaced nav root is still visible. If restored, the displaced nav From 688b4a72aa5707f1875434af2f8a5da81da17f62 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Wed, 11 Nov 2026 13:16:37 +0100 Subject: [PATCH 4/4] ref(android): Measure the Nav3 ui.load retry window with a Deadline The window for retrying a nav transaction blocked by an Activity's ui.load transaction was measured by subtracting SentryDates from options.dateProvider. That clock is replaceable, falls back to wall-clock time unless both dates carry a nanoTime, and doesn't count while an Android device suspends. Record an io.sentry.time.Deadline when the publication is blocked instead, on the ticker from options.getMonotonicTicker(), which StagedNavTransaction now takes as a constructor parameter so tests can drive it with TestMonotonicTicker. A retry exactly at the end of the 10-second window no longer qualifies. --- sentry-android-navigation3/build.gradle.kts | 1 + .../navigation3/lifecycle/NavScopes.kt | 3 +- .../compose/navigation3/lifecycle/README.md | 4 +- .../lifecycle/StagedNavTransaction.kt | 25 +++++---- .../lifecycle/StagedNavTransactionTest.kt | 54 +++++++++++++++---- 5 files changed, 64 insertions(+), 23 deletions(-) diff --git a/sentry-android-navigation3/build.gradle.kts b/sentry-android-navigation3/build.gradle.kts index f0a0e78386..d9d4faa95f 100644 --- a/sentry-android-navigation3/build.gradle.kts +++ b/sentry-android-navigation3/build.gradle.kts @@ -70,6 +70,7 @@ dependencies { compileOnly(libs.androidx.compose.ui) testImplementation(projects.sentryAndroidCore) + testImplementation(projects.sentryTestSupport) 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/NavScopes.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt index 8bce50a60d..b8115cacaf 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 @@ -76,7 +76,8 @@ internal class NavScopes( /** This instance's registration with the [coordinator], while it's attached. */ private var registration: NavLeaseRegistration? = null - private val stagedNavTransaction = StagedNavTransaction(delegate) + private val stagedNavTransaction = + StagedNavTransaction(delegate, delegate.options.monotonicTicker) private val stagedNavData = StagedNavData() private val navStatePublisher = NavStatePublisher() 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 e296066ee5..9670faa20f 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 @@ -193,7 +193,7 @@ acquisition: on its first back stack update, a candidate has no destination to s 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`). +`UI_LOAD_RETRY_WINDOW_SECONDS` 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 @@ -229,7 +229,7 @@ document but AOSP and androidx have historically upheld: 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`). + or when the system's idle timeout fires (see `UI_LOAD_RETRY_WINDOW_SECONDS`). 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 f5460d1a3e..d8ec37f45e 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,9 +3,10 @@ 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 io.sentry.time.Deadline +import io.sentry.time.MonotonicTicker import java.lang.ref.WeakReference import java.util.concurrent.TimeUnit @@ -24,7 +25,7 @@ internal const val ACTIVITY_UI_LOAD_ORIGIN: String = "auto.ui.activity" * `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) +internal const val UI_LOAD_RETRY_WINDOW_SECONDS: Long = 10 /** * Holds a nav transaction staged for publication to a shared [IScope]. @@ -54,7 +55,10 @@ internal val UI_LOAD_RETRY_WINDOW_NANOS: Long = TimeUnit.SECONDS.toNanos(10) * This class is ***not*** thread-safe. Apply the thread safety policy from [NavScopes]. */ @Suppress("TooManyFunctions") -internal class StagedNavTransaction(private val scopes: IScopes) { +internal class StagedNavTransaction( + private val scopes: IScopes, + private val ticker: MonotonicTicker, +) { /** * The request most recently passed to [stage], whatever became of its transaction. Cleared when @@ -134,12 +138,12 @@ internal class StagedNavTransaction(private val scopes: IScopes) { /** * 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. + * only within [UI_LOAD_RETRY_WINDOW_SECONDS] of the blocked attempt, 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) { + if (blocked.retryDeadline.hasPassed()) { blockedPublication = null return } @@ -248,17 +252,20 @@ internal class StagedNavTransaction(private val scopes: IScopes) { if (canHold(current, transaction) || current.spanContext.origin != ACTIVITY_UI_LOAD_ORIGIN) { return null } - return BlockedPublication(WeakReference(current), scopes.options.dateProvider.now()) + return BlockedPublication( + uiLoad = WeakReference(current), + retryDeadline = Deadline.after(ticker, UI_LOAD_RETRY_WINDOW_SECONDS, TimeUnit.SECONDS), + ) } /** - * A nav publication blocked by [uiLoad] at [blockedAt]. + * A nav publication blocked by [uiLoad], which may be retried until [retryDeadline]. * * 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, + val retryDeadline: Deadline, ) } 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 5072e35ccf..82c45a1df9 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,15 +7,14 @@ 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 io.sentry.time.TestMonotonicTicker +import java.util.concurrent.TimeUnit import org.junit.Assert.assertThrows import org.junit.Test import org.mockito.kotlin.any @@ -509,13 +508,48 @@ class StagedNavTransactionTest { fixture.scope.transaction = uiLoadTransaction stagedNavTransaction.publishTo(fixture.scope) fixture.scope.clearTransaction() - fixture.nowNanos = UI_LOAD_RETRY_WINDOW_NANOS + fixture.ticker.advance( + TimeUnit.SECONDS.toNanos(UI_LOAD_RETRY_WINDOW_SECONDS) - 1, + TimeUnit.NANOSECONDS, + ) + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `retryPublishIfBlocked() - measures the retry window from the blocked publication`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.ticker.advance(UI_LOAD_RETRY_WINDOW_SECONDS, TimeUnit.SECONDS) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + fixture.ticker.advance(UI_LOAD_RETRY_WINDOW_SECONDS - 1, TimeUnit.SECONDS) stagedNavTransaction.retryPublishIfBlocked(fixture.scope) assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) } + @Test + fun `retryPublishIfBlocked() - retries nothing exactly at the end of 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.ticker.advance(UI_LOAD_RETRY_WINDOW_SECONDS, TimeUnit.SECONDS) + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + @Test fun `retryPublishIfBlocked() - retries nothing after the retry window`() { val fixture = Fixture.create() @@ -524,7 +558,7 @@ class StagedNavTransactionTest { fixture.scope.transaction = uiLoadTransaction stagedNavTransaction.publishTo(fixture.scope) fixture.scope.clearTransaction() - fixture.nowNanos = UI_LOAD_RETRY_WINDOW_NANOS + 1 + fixture.ticker.advance(UI_LOAD_RETRY_WINDOW_SECONDS + 1, TimeUnit.SECONDS) stagedNavTransaction.retryPublishIfBlocked(fixture.scope) @@ -539,10 +573,9 @@ class StagedNavTransactionTest { stagedNavTransaction.stage(request) fixture.scope.transaction = uiLoadTransaction stagedNavTransaction.publishTo(fixture.scope) - fixture.nowNanos = UI_LOAD_RETRY_WINDOW_NANOS + 1 + fixture.ticker.advance(UI_LOAD_RETRY_WINDOW_SECONDS, TimeUnit.SECONDS) stagedNavTransaction.retryPublishIfBlocked(fixture.scope) fixture.scope.clearTransaction() - fixture.nowNanos = 0 stagedNavTransaction.retryPublishIfBlocked(fixture.scope) @@ -848,8 +881,8 @@ class StagedNavTransactionTest { val scope = Scope(SentryOptions()) val logger = mock() - /** The time [scopes]' date provider reports, in nanoseconds. */ - var nowNanos = 0L + /** Measures [stagedNavTransaction]'s retry window. */ + val ticker = TestMonotonicTicker() /** Whether [scopes] declines to start transactions, returning a no-op transaction instead. */ var declinesTransactions = false @@ -865,7 +898,6 @@ class StagedNavTransactionTest { SentryOptions().apply { setLogger(this@Fixture.logger) isDebug = true - dateProvider = SentryDateProvider { SentryNanotimeDate(Date(0), nowNanos) } } ) whenever(scopes.startTransaction(any(), any())) @@ -881,7 +913,7 @@ class StagedNavTransactionTest { } } - val stagedNavTransaction = StagedNavTransaction(scopes) + val stagedNavTransaction = StagedNavTransaction(scopes, ticker) init { scope.options.addScopeObserver(