From 3473e24355a4ad99e9906060b7efd4173063edf3 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Tue, 20 Oct 2026 16:56:33 +0200 Subject: [PATCH 1/3] ref(android): [Nav3 Lifecycle 1] Add navigation lease coordinator Introduce the lifecycle-aware lease state machine that selects which navigation root may publish shared scope state and bind navigation transactions. Refs JAVA-274 --- gradle/libs.versions.toml | 1 + sentry-android-navigation3/build.gradle.kts | 3 + .../lifecycle/NavLeaseCoordinator.kt | 416 +++++++++ .../lifecycle/NavLeaseCoordinatorTest.kt | 800 ++++++++++++++++++ 4 files changed, 1220 insertions(+) create mode 100644 sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt create mode 100644 sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index e5c2f9e217..2afcc5d48b 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -110,6 +110,7 @@ androidx-core-ktx = { module = "androidx.core:core-ktx", version = "1.7.0" } androidx-fragment-ktx = { module = "androidx.fragment:fragment-ktx", version = "1.3.5" } androidx-lifecycle-common-java8 = { module = "androidx.lifecycle:lifecycle-common-java8", version.ref = "androidxLifecycle" } androidx-lifecycle-process = { module = "androidx.lifecycle:lifecycle-process", version.ref = "androidxLifecycle" } +androidx-lifecycle-runtime-compose = { module = "androidx.lifecycle:lifecycle-runtime-compose", version = "2.8.3" } androidx-navigation-runtime = { module = "androidx.navigation:navigation-runtime", version.ref = "androidxNavigation" } androidx-navigation-compose = { module = "androidx.navigation:navigation-compose", version.ref = "androidxNavigation" } androidx-navigation-fragment = { module = "androidx.navigation:navigation-fragment-ktx", version.ref = "androidxNavigation" } diff --git a/sentry-android-navigation3/build.gradle.kts b/sentry-android-navigation3/build.gradle.kts index b9d02bb49d..2628bddfcc 100644 --- a/sentry-android-navigation3/build.gradle.kts +++ b/sentry-android-navigation3/build.gradle.kts @@ -62,6 +62,9 @@ dependencies { implementation(projects.sentry) compileOnly(libs.androidx.compose.runtime) + // Provides lifecycle types used to coordinate overlapping navigation roots. Hosts get it at + // runtime through Compose UI 1.7+ and navigation3-ui, both of which depend on it. + compileOnly(libs.androidx.lifecycle.runtime.compose) testImplementation(libs.androidx.compose.ui.test.junit4) testImplementation(libs.androidx.test.core) 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 new file mode 100644 index 0000000000..53dcd04bf6 --- /dev/null +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt @@ -0,0 +1,416 @@ +package io.sentry.compose.navigation3.lifecycle + +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.LifecycleOwner +import io.sentry.IScope +import io.sentry.ITransaction +import io.sentry.SpanStatus + +/** + * Manages a lease in order to determine which [NavScopes] instance can bind nav transactions and + * publish nav data to a shared [IScope]. + * + * `NavScopes` instances register as [candidates][Candidate] for lease ownership via + * [registerCandidate]. + * + * Note: "Owner" below refers to the `LifecycleOwner` whose lifecycle events the candidate's + * `NavScopes` relays. + * + * **Thread safety** + * + * This class is ***not*** thread-safe. All `NavScopes` instances using this coordinator must + * confine their access to the same thread. + */ +@Suppress("TooManyFunctions") +internal class NavLeaseCoordinator { + + private val candidates = mutableListOf() + + /** Candidate responsible for the most recently published nav data in the shared scope. */ + private var publishedDataCandidate: Candidate? = null + + private var leaseHolder: Candidate? = null + + /** + * Counter that orders candidates by when this coordinator most recently observed them in resumed + * state. + * + * Each resume, including registration of an already-resumed owner, assigns the next value to that + * candidate's [Candidate.resumeOrder]. This lets fallback selection prefer the most recently + * resumed eligible candidate without relying on clock timestamps. Zero is reserved for candidates + * that have not been observed resumed and therefore cannot be chosen as fallbacks. + */ + private var resumeOrder: Long = 0 + + /** + * Registers [owner] as a candidate for the nav lease. No-ops if `owner` is already registered or + * [lifecycleState] is [destroyed][Lifecycle.State.DESTROYED]. + * + * [onPublishData] publishes `owner`'s staged nav data to the given scope when `owner` acquires + * the lease. [onClearPublishedData] clears `owner`'s published nav data from the given scope when + * its data is the most recently published and either another owner acquires the lease or `owner` + * is unregistered. + */ + fun registerCandidate( + owner: Any, + lifecycleState: Lifecycle.State, + onPublishData: (IScope) -> Unit, + onClearPublishedData: (IScope) -> Unit, + ) { + if (findCandidate(owner) != null) { + return + } + + if (lifecycleState == Lifecycle.State.DESTROYED) { + return + } + + val initialResumeOrder = if (lifecycleState == Lifecycle.State.RESUMED) ++resumeOrder else 0 + candidates += + Candidate(owner, onPublishData, onClearPublishedData, lifecycleState, initialResumeOrder) + } + + /** Records that [owner] started. */ + fun startCandidate(owner: Any) { + findCandidate(owner)?.start() + } + + /** + * Records that [owner] resumed. + * + * Restores the lease to `owner` if it already applied a back stack update and [scope] is + * non-null. + * + * A `null` scope (e.g., the SDK is disabled) still records the lifecycle transition so the + * candidate remains eligible for fallback once a scope becomes available. + */ + fun resumeCandidate(owner: Any, scope: IScope?) { + findCandidate(owner)?.let { + it.resume(++resumeOrder) + + if (scope != null && it.hasAppliedInitialBackStackUpdate) { + transferLeaseTo(it, scope) + } + } + } + + /** + * Records that [owner] paused. + * + * Hands the lease to another owner only if that owner is resumed (e.g., a Fragment whose dialog + * is being dismissed); otherwise keeps it. A paused leaseholder may still be the topmost visible + * owner (e.g., beneath a translucent window or in picture-in-picture mode), and any other paused + * owner is beneath it. A displaced owner gets the lease back when it resumes. + * + * A `null` scope (e.g., the SDK is disabled) still records the lifecycle transition so the + * candidate remains eligible for fallback once a scope becomes available. + */ + fun pauseCandidate(owner: Any, scope: IScope?) { + findCandidate(owner)?.let { + it.pause() + + if (leaseHolder === it) { + mostRecentlyResumedCandidate(excluding = it, resumedOnly = true)?.let { next -> + handOffLease(next, scope) + } + } + } + } + + /** + * Records that [owner] stopped. + * + * As with pausing, hands the lease to another owner only if that owner is resumed; otherwise the + * stopped owner keeps it. Keeping it lets a lone owner go on recording navigation that happens + * while it isn't resumed (e.g., while the app is in the background, or after it restarts but + * before it resumes), including starting nav transactions. + * + * A paused owner doesn't get the lease: when the leaseholder stops before it, the app is + * typically going to the background (e.g., a translucent leaseholder stops before the owner + * beneath it), and that owner isn't what the user last saw. A paused owner that comes back to the + * foreground gets the lease when it resumes. + * + * A `null` scope (e.g., the SDK is disabled) still records the lifecycle transition so the + * candidate remains eligible for fallback once a scope becomes available. + */ + fun stopCandidate(owner: Any, scope: IScope?) { + findCandidate(owner)?.let { + it.stop() + + if (leaseHolder === it) { + mostRecentlyResumedCandidate(resumedOnly = true)?.let { next -> handOffLease(next, scope) } + } + } + } + + /** + * Stops tracking [owner] (e.g., because it was destroyed or its effect left the composition). + * No-ops if `owner` isn't registered. + * + * Finishes and unbinds `owner`'s nav transaction, and clears its published nav data if that data + * is the most recently published. If `owner` holds the lease, hands it to the eligible candidate + * most recently observed resumed, even if that candidate is now only started or paused, or + * releases it if there's none. + * + * A `null` scope (e.g., the SDK is disabled) still removes the candidate, but its transaction is + * left to time out, its published data is left in place, and the lease is released rather than + * handed off. + */ + fun unregisterCandidate(owner: Any, scope: IScope?) { + findCandidate(owner)?.let { + finishTransaction(it, scope) + candidates.remove(it) + + if (publishedDataCandidate === it) { + publishedDataCandidate = null + if (scope != null) { + it.onClearPublishedData(scope) + } + } + + if (leaseHolder === it) { + handOffLease(mostRecentlyResumedCandidate(), scope) + } + } + } + + fun hasLease(owner: Any): Boolean = leaseHolder?.owner === owner + + /** + * Records that the registered [owner] is about to apply a back-stack update and acquires the nav + * lease if the owner's lifecycle permits it. + * + * An owner that isn't resumed doesn't take the lease from a resumed leaseholder, as it isn't the + * one the user is interacting with (e.g., a preloaded ViewPager2 page next to the current one). + * Instead, it acquires the lease when it resumes. + * + * If the lease transfers, ownership and transaction binding are updated before the candidate's + * staged nav data is published. Publication completes before this method returns, so the caller + * can then apply the back-stack update. Later calls for the same [owner] do nothing. + */ + fun onBackStackUpdate(owner: Any, scope: IScope) { + val candidate = findCandidate(owner) ?: return + if (candidate.hasAppliedInitialBackStackUpdate) { + return + } + + candidate.hasAppliedInitialBackStackUpdate = true + + val holder = leaseHolder + val wouldDisplaceResumedHolder = + holder != null && + holder.state == Lifecycle.State.RESUMED && + candidate.state != Lifecycle.State.RESUMED + + if (candidate.canAcquireLeaseOnFirstUpdate && !wouldDisplaceResumedHolder) { + transferLeaseTo(candidate, scope) + } + } + + /** Whether [transaction] is a nav transaction retained for any registered owner. */ + fun isNavTransaction(transaction: ITransaction): Boolean = candidates.any { + it.transaction === transaction + } + + /** + * Records [transaction] as the nav transaction of [owner], which must hold the lease. Ignored + * otherwise, as only the leaseholder starts nav transactions. + * + * If [owner] loses the lease, the transaction is unbound but retained: it's rebound if [owner] + * regains the lease while it's still unfinished and the scope is free, and finished when [owner] + * is [unregistered][unregisterCandidate]. It's forgotten once [owner] retires it; see + * [releaseTransaction]. + */ + fun retainTransaction(owner: Any, transaction: ITransaction) { + val candidate = leaseHolder ?: return + if (candidate.owner !== owner) { + return + } + candidate.transaction = transaction + } + + /** + * Forgets [transaction] as the nav transaction of [owner], if it's the one retained. Called when + * [owner] retires the transaction (e.g., it navigated away), so it's never rebound later. + * + * Retiring is distinct from losing the lease: a transaction that's still waiting for child spans + * isn't [finished][ITransaction.isFinished] yet, so without this, regaining the lease would + * rebind a transaction whose destination [owner] has already left. + */ + fun releaseTransaction(owner: Any, transaction: ITransaction) { + val candidate = findCandidate(owner) ?: return + if (candidate.transaction === transaction) { + candidate.transaction = null + } + } + + private fun findCandidate(owner: Any): Candidate? = candidates.firstOrNull { it.owner === owner } + + /** + * The eligible candidate most recently observed resumed, other than [excluding]. With + * [resumedOnly], only candidates that are resumed right now are considered; otherwise, started + * and paused ones are too. + */ + private fun mostRecentlyResumedCandidate( + excluding: Candidate? = null, + resumedOnly: Boolean = false, + ): Candidate? = + candidates + .filter { + it !== excluding && + it.hasAppliedInitialBackStackUpdate && + it.state.isAtLeast(Lifecycle.State.STARTED) && + it.resumeOrder > 0 && + (!resumedOnly || it.state == Lifecycle.State.RESUMED) + } + .maxByOrNull { it.resumeOrder } + + /** + * Hands the current lease to [next]. Without a [scope], the lease is released instead, as [next] + * could not publish its staged data. + */ + private fun handOffLease(next: Candidate?, scope: IScope?) { + if (scope != null) { + transferLeaseTo(next, scope) + } else { + leaseHolder = null + } + } + + private fun transferLeaseTo(candidate: Candidate?, scope: IScope) { + if (leaseHolder === candidate) { + return + } + + if (candidate != null) { + // Unbind another owner's transaction without finishing it, so it can be restored if that + // owner returns. If this owner's retained transaction has since finished, discard it + // instead of binding a finished transaction to the shared scope. + scope.withTransaction { current -> + if (current != null && candidates.any { it !== candidate && it.transaction === current }) { + scope.clearTransaction() + } + } + + val transaction = candidate.transaction + if (transaction?.isFinished == true) { + clearTransactionFromScope(candidate, scope) + candidate.transaction = null + } else if (transaction != null) { + // The navigation lease doesn't give this owner control over an unrelated transaction + // already bound to the shared scope. Restore the owner's retained nav transaction only if + // the scope is free or already holds that same transaction. + scope.withTransaction { current -> + if (current == null || current === transaction) { + scope.transaction = transaction + } + } + } + } + + leaseHolder = candidate + if (candidate != null) { + // Clear the previous publisher's nav data before publishing the candidate's, so the + // previous owner's values don't linger where the candidate has nothing staged to replace + // them. + val previousPublisher = publishedDataCandidate + if (previousPublisher != null && previousPublisher !== candidate) { + previousPublisher.onClearPublishedData(scope) + } + publishedDataCandidate = candidate + candidate.onPublishData(scope) + } + } + + /** Finishes and unbinds [candidate]'s nav transaction, then forgets it. */ + private fun finishTransaction(candidate: Candidate, scope: IScope?) { + // No need to finish the transaction if we don't have a scope (e.g., the SDK is disabled), as + // the transaction can't be sent and it will time out anyway. Not finishing it has the added + // advantage of keeping SDK work inside `configureScope`'s error handling. + if (scope != null) { + val transaction = candidate.transaction + if (transaction != null && !transaction.isFinished) { + transaction.finish(transaction.status ?: SpanStatus.OK) + } + clearTransactionFromScope(candidate, scope) + } + + candidate.transaction = null + } + + private fun clearTransactionFromScope(candidate: Candidate, scope: IScope) { + val transaction = candidate.transaction ?: return + scope.withTransaction { current -> + if (current === transaction) { + scope.clearTransaction() + } + } + } + + /** A [LifecycleOwner] that has registered itself as a candidate for lease ownership. */ + private class Candidate( + val owner: Any, + val onPublishData: (IScope) -> Unit, + val onClearPublishedData: (IScope) -> Unit, + initialState: Lifecycle.State, + initialResumeOrder: Long, + ) { + + /** The candidate's lifecycle state, as last observed. */ + var state: Lifecycle.State = initialState + private set + + /** + * Whether the candidate has been observed STOPPED since + * [registration][NavLeaseCoordinator.registerCandidate]. + */ + private var wasStopped: Boolean = false + + var transaction: ITransaction? = null + + /** + * See [NavLeaseCoordinator.resumeOrder]. Non-zero once the candidate has been observed RESUMED + * since [registration][NavLeaseCoordinator.registerCandidate]. + */ + var resumeOrder: Long = initialResumeOrder + private set + + var hasAppliedInitialBackStackUpdate: Boolean = false + + /** + * Whether the candidate's lifecycle permits its first back stack update since + * [registration][NavLeaseCoordinator.registerCandidate] to acquire the nav lease, including + * taking it from another owner that isn't resumed: it's resumed, or it has neither resumed nor + * stopped since registration (i.e., it's still coming up for the first time). A paused or + * restarted candidate is returning to the screen rather than arriving on it. + * + * This checks the candidate's own lifecycle only; it does not indicate whether the candidate + * has already applied its first update, nor account for a resumed leaseholder (see + * [NavLeaseCoordinator.onBackStackUpdate]). + */ + val canAcquireLeaseOnFirstUpdate: Boolean + get() = state == Lifecycle.State.RESUMED || (resumeOrder == 0L && !wasStopped) + + fun start() { + // Attaching to an already-resumed lifecycle replays ON_START before ON_RESUME, so only move + // up from CREATED. + if (!state.isAtLeast(Lifecycle.State.STARTED)) { + state = Lifecycle.State.STARTED + } + } + + fun resume(order: Long) { + resumeOrder = order + state = Lifecycle.State.RESUMED + } + + fun pause() { + state = Lifecycle.State.STARTED + } + + fun stop() { + state = Lifecycle.State.CREATED + wasStopped = true + } + } +} 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 new file mode 100644 index 0000000000..6b3af236ac --- /dev/null +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt @@ -0,0 +1,800 @@ +package io.sentry.compose.navigation3.lifecycle + +import androidx.lifecycle.Lifecycle +import com.google.common.truth.Truth.assertThat +import io.sentry.ITransaction +import io.sentry.Scope +import io.sentry.SentryOptions +import io.sentry.SpanStatus +import org.junit.Test +import org.mockito.kotlin.any +import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.verify +import org.mockito.kotlin.verifyNoInteractions +import org.mockito.kotlin.whenever + +class NavLeaseCoordinatorTest { + + private val owner = Any() + private val incoming = Any() + private val incumbent = Any() + private val previous = Any() + + private val transaction = mock() + private val unrelatedTransaction = mock() + + @Test + fun `a registered created owner acquires the nav lease on its first back stack update`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.register(owner, Lifecycle.State.CREATED) + + navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(fixture.published).containsExactly(owner) + } + + @Test + fun `registering the same owner twice preserves its first publication callback`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.register(owner, Lifecycle.State.CREATED) + + navLeaseCoordinator.registerCandidate( + owner, + Lifecycle.State.RESUMED, + { fixture.published += "duplicate" }, + { fixture.cleared += "duplicate" }, + ) + navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + + assertThat(fixture.published).containsExactly(owner) + } + + @Test + fun `registering a destroyed owner does not make it eligible for the nav lease`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + + fixture.register(owner, Lifecycle.State.DESTROYED) + navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() + assertThat(fixture.published).isEmpty() + } + + @Test + fun `equal but distinct owners do not share the nav lease`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + val registeredOwner = String(charArrayOf('a')) + val equalOwner = String(charArrayOf('a')) + fixture.acquireLease(registeredOwner) + + val result = navLeaseCoordinator.hasLease(equalOwner) + + assertThat(result).isFalse() + assertThat(navLeaseCoordinator.hasLease(registeredOwner)).isTrue() + } + + @Test + fun `starting a created owner leaves it eligible to acquire the nav lease on its first back stack update`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.register(owner, Lifecycle.State.CREATED) + + navLeaseCoordinator.startCandidate(owner) + navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + } + + @Test + fun `restarting a stopped owner does not take the nav lease on its first back stack update`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.register(owner, Lifecycle.State.CREATED) + navLeaseCoordinator.stopCandidate(owner, fixture.scope) + fixture.acquireLease(incumbent) + + navLeaseCoordinator.startCandidate(owner) + navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(incumbent)).isTrue() + assertThat(fixture.published).containsExactly(incumbent) + } + + @Test + fun `resuming an owner before its first back stack update leaves the nav lease unassigned`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.register(owner, Lifecycle.State.CREATED) + + navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() + assertThat(fixture.published).isEmpty() + } + + @Test + fun `resuming a stopped owner after its first back stack update transfers the nav lease`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(incumbent) + fixture.register(owner, Lifecycle.State.CREATED) + navLeaseCoordinator.stopCandidate(owner, fixture.scope) + navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + + navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(fixture.published).containsExactly(incumbent, owner).inOrder() + } + + @Test + fun `a paused owner cannot acquire the nav lease on its first back stack update`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.register(owner, Lifecycle.State.RESUMED) + navLeaseCoordinator.pauseCandidate(owner, fixture.scope) + + navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() + assertThat(fixture.published).isEmpty() + } + + @Test + fun `an owner that isn't resumed doesn't take the nav lease from a resumed leaseholder`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(incumbent) + + fixture.acquireLease(owner, state = Lifecycle.State.STARTED) + + assertThat(navLeaseCoordinator.hasLease(incumbent)).isTrue() + assertThat(fixture.published).containsExactly(incumbent) + } + + @Test + fun `an owner kept from the nav lease by a resumed leaseholder acquires it when it resumes`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(incumbent) + fixture.acquireLease(owner, state = Lifecycle.State.STARTED) + + navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(fixture.published).containsExactly(incumbent, owner).inOrder() + } + + @Test + fun `an owner that isn't resumed takes the nav lease from a paused leaseholder`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(incumbent) + navLeaseCoordinator.pauseCandidate(incumbent, fixture.scope) + + fixture.acquireLease(owner, state = Lifecycle.State.CREATED) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + } + + @Test + fun `an owner that isn't resumed takes the nav lease from a stopped leaseholder`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(incumbent) + navLeaseCoordinator.pauseCandidate(incumbent, fixture.scope) + navLeaseCoordinator.stopCandidate(incumbent, fixture.scope) + + fixture.acquireLease(owner, state = Lifecycle.State.CREATED) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(fixture.published).containsExactly(incumbent, owner).inOrder() + } + + @Test + fun `a resumed owner takes the nav lease from a resumed leaseholder`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(incumbent) + + fixture.acquireLease(owner) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + } + + @Test + fun `acquiring the nav lease clears the previous owner's published nav data before publishing`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + val calls = mutableListOf() + navLeaseCoordinator.registerCandidate( + previous, + Lifecycle.State.RESUMED, + onPublishData = { calls += "publish previous" }, + onClearPublishedData = { calls += "clear previous" }, + ) + navLeaseCoordinator.onBackStackUpdate(previous, fixture.scope) + navLeaseCoordinator.registerCandidate( + incoming, + Lifecycle.State.RESUMED, + onPublishData = { calls += "publish incoming" }, + onClearPublishedData = { calls += "clear incoming" }, + ) + + navLeaseCoordinator.onBackStackUpdate(incoming, fixture.scope) + + assertThat(calls) + .containsExactly("publish previous", "clear previous", "publish incoming") + .inOrder() + } + + @Test + fun `regaining the nav lease doesn't clear the owner's own published nav data`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(previous) + fixture.acquireLease(owner) + navLeaseCoordinator.pauseCandidate(owner, null) + fixture.cleared.clear() + + navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(fixture.published).containsExactly(previous, owner, owner).inOrder() + assertThat(fixture.cleared).isEmpty() + } + + @Test + fun `releasing the nav lease leaves the owner's published nav data in place`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(previous) + fixture.acquireLease(owner) + fixture.cleared.clear() + + navLeaseCoordinator.pauseCandidate(owner, null) + + assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() + assertThat(fixture.cleared).isEmpty() + } + + @Test + fun `later back stack updates do not republish nav data or reclaim the nav lease`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + val first = Any() + val second = Any() + fixture.acquireLease(first) + fixture.acquireLease(second, state = Lifecycle.State.RESUMED) + + navLeaseCoordinator.onBackStackUpdate(first, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(second)).isTrue() + assertThat(fixture.published).containsExactly(first, second).inOrder() + } + + @Test + fun `pausing the leaseholder hands the nav lease to the most recently resumed owner that's still resumed`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + val older = Any() + val recent = Any() + fixture.acquireLease(older) + fixture.acquireLease(recent) + fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + + navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(recent)).isTrue() + assertThat(fixture.published.last()).isSameInstanceAs(recent) + } + + @Test + fun `pausing the leaseholder keeps the nav lease when other eligible owners are only paused`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner) + navLeaseCoordinator.pauseCandidate(owner, fixture.scope) + fixture.acquireLease(incoming) + + navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(incoming)).isTrue() + assertThat(fixture.published).containsExactly(owner, incoming).inOrder() + } + + @Test + fun `a displaced paused owner regains the nav lease when it resumes`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner) + navLeaseCoordinator.pauseCandidate(owner, fixture.scope) + fixture.acquireLease(incoming) + navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + + navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(fixture.published).containsExactly(owner, incoming, owner).inOrder() + } + + @Test + fun `pausing the leaseholder skips a resumed owner without a back stack update`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + val eligible = Any() + val uninitialized = Any() + fixture.acquireLease(eligible) + fixture.register(uninitialized, Lifecycle.State.RESUMED) + fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + + navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(eligible)).isTrue() + assertThat(navLeaseCoordinator.hasLease(uninitialized)).isFalse() + } + + @Test + fun `pausing the only eligible owner keeps the nav lease`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner) + + navLeaseCoordinator.pauseCandidate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(fixture.published).containsExactly(owner) + } + + @Test + fun `pausing the only eligible owner without a scope keeps the nav lease`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner) + + navLeaseCoordinator.pauseCandidate(owner, null) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + } + + @Test + fun `resuming a paused leaseholder does not republish its nav data`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner) + navLeaseCoordinator.pauseCandidate(owner, fixture.scope) + + navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(fixture.published).containsExactly(owner) + } + + @Test + fun `stopping the leaseholder restores a resumed owner`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(previous) + fixture.acquireLease(owner, state = Lifecycle.State.RESUMED) + + navLeaseCoordinator.stopCandidate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(previous)).isTrue() + assertThat(fixture.published.last()).isSameInstanceAs(previous) + } + + @Test + fun `stopping the only eligible owner keeps the nav lease`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner) + navLeaseCoordinator.pauseCandidate(owner, fixture.scope) + + navLeaseCoordinator.stopCandidate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(fixture.published).containsExactly(owner) + } + + @Test + fun `stopping the only eligible owner without a scope keeps the nav lease`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner) + navLeaseCoordinator.pauseCandidate(owner, null) + + navLeaseCoordinator.stopCandidate(owner, null) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + } + + @Test + fun `stopping the leaseholder doesn't hand the nav lease to a paused owner`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(previous) + navLeaseCoordinator.pauseCandidate(previous, fixture.scope) + fixture.acquireLease(owner) + navLeaseCoordinator.pauseCandidate(owner, fixture.scope) + + navLeaseCoordinator.stopCandidate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(navLeaseCoordinator.hasLease(previous)).isFalse() + assertThat(fixture.published).containsExactly(previous, owner).inOrder() + assertThat(fixture.cleared).containsExactly(previous) + } + + @Test + fun `a paused owner passed over when the leaseholder stopped gets the nav lease when it resumes`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(previous) + navLeaseCoordinator.pauseCandidate(previous, fixture.scope) + fixture.acquireLease(owner) + navLeaseCoordinator.pauseCandidate(owner, fixture.scope) + navLeaseCoordinator.stopCandidate(owner, fixture.scope) + + navLeaseCoordinator.resumeCandidate(previous, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(previous)).isTrue() + assertThat(fixture.published).containsExactly(previous, owner, previous).inOrder() + } + + @Test + fun `restarting a stopped leaseholder keeps the nav lease`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner) + navLeaseCoordinator.pauseCandidate(owner, fixture.scope) + navLeaseCoordinator.stopCandidate(owner, fixture.scope) + + navLeaseCoordinator.startCandidate(owner) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(fixture.published).containsExactly(owner) + } + + @Test + fun `stopping an owner excludes it from later nav lease fallback`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(previous) + fixture.acquireLease(owner) + navLeaseCoordinator.pauseCandidate(previous, fixture.scope) + navLeaseCoordinator.stopCandidate(previous, fixture.scope) + + navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(previous)).isFalse() + assertThat(fixture.published).containsExactly(previous, owner).inOrder() + } + + @Test + fun `resuming without a scope keeps the owner eligible for nav lease fallback`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.register(owner, Lifecycle.State.CREATED) + navLeaseCoordinator.resumeCandidate(owner, null) + navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + fixture.acquireLease(incoming, state = Lifecycle.State.CREATED) + + navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + } + + @Test + fun `resuming without a scope does not transfer the nav lease`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(incumbent) + fixture.register(owner, Lifecycle.State.CREATED) + navLeaseCoordinator.stopCandidate(owner, fixture.scope) + navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + + navLeaseCoordinator.resumeCandidate(owner, null) + + assertThat(navLeaseCoordinator.hasLease(incumbent)).isTrue() + assertThat(fixture.published).containsExactly(incumbent) + } + + @Test + fun `pausing the leaseholder without a scope releases the nav lease without publishing a fallback`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(previous) + fixture.acquireLease(owner, state = Lifecycle.State.RESUMED) + + navLeaseCoordinator.pauseCandidate(owner, null) + + assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() + assertThat(navLeaseCoordinator.hasLease(previous)).isFalse() + assertThat(fixture.published).containsExactly(previous, owner).inOrder() + } + + @Test + fun `stopping the leaseholder without a scope releases the nav lease without publishing a fallback`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(previous) + fixture.acquireLease(owner, state = Lifecycle.State.RESUMED) + + navLeaseCoordinator.stopCandidate(owner, null) + + assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() + assertThat(navLeaseCoordinator.hasLease(previous)).isFalse() + assertThat(fixture.published).containsExactly(previous, owner).inOrder() + } + + @Test + fun `an unregistered owner's nav transaction is not retained`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + val other = Any() + fixture.acquireLease(owner) + + navLeaseCoordinator.retainTransaction(other, transaction) + navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + + verifyNoInteractions(transaction) + } + + @Test + fun `a nav transaction recorded without the lease is ignored`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner) + fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + + navLeaseCoordinator.retainTransaction(owner, transaction) + navLeaseCoordinator.unregisterCandidate(incoming, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(navLeaseCoordinator.isNavTransaction(transaction)).isFalse() + assertThat(fixture.scope.transaction).isNull() + + navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + + verifyNoInteractions(transaction) + } + + @Test + fun `an unfinished nav transaction is restored when its owner regains the lease`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner, transaction = transaction) + fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + + assertThat(fixture.scope.transaction).isNull() + navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(transaction) + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + verify(transaction, never()).finish(any()) + } + + @Test + fun `restoring a nav transaction preserves an unrelated scope transaction`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner, transaction = transaction) + fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + fixture.scope.transaction = unrelatedTransaction + + navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(unrelatedTransaction) + } + + @Test + fun `a finished nav transaction is discarded when its owner regains the lease`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner, transaction = transaction) + fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + whenever(transaction.isFinished).thenReturn(true) + + navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + + assertThat(fixture.scope.transaction).isNull() + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(navLeaseCoordinator.isNavTransaction(transaction)).isFalse() + } + + @Test + fun `a released unfinished nav transaction is not restored when its owner regains the lease`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner, transaction = transaction) + fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + + navLeaseCoordinator.releaseTransaction(owner, transaction) + navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(fixture.scope.transaction).isNull() + assertThat(navLeaseCoordinator.isNavTransaction(transaction)).isFalse() + } + + @Test + fun `releasing a nav transaction other than the retained one keeps the retained one restorable`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner, transaction = transaction) + fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + + navLeaseCoordinator.releaseTransaction(owner, unrelatedTransaction) + navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(fixture.scope.transaction).isSameInstanceAs(transaction) + } + + @Test + fun `a displaced owner's retained nav transaction remains identifiable until unregistering`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner, transaction = transaction) + fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + + assertThat(navLeaseCoordinator.isNavTransaction(transaction)).isTrue() + assertThat(navLeaseCoordinator.isNavTransaction(unrelatedTransaction)).isFalse() + + navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.isNavTransaction(transaction)).isFalse() + } + + @Test + fun `acquiring the nav lease binds its retained transaction before publishing nav data`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + val transactionAtPublication = mutableListOf() + navLeaseCoordinator.registerCandidate( + owner, + Lifecycle.State.RESUMED, + onPublishData = { transactionAtPublication += it.transaction }, + onClearPublishedData = {}, + ) + navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + navLeaseCoordinator.retainTransaction(owner, transaction) + fixture.scope.transaction = transaction + fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + + navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(transactionAtPublication).containsExactly(null, transaction).inOrder() + } + + @Test + fun `unregistering the leaseholder finishes its transaction and clears its published nav data`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner, transaction = transaction) + + navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + + verify(transaction).finish(SpanStatus.OK) + assertThat(fixture.scope.transaction).isNull() + assertThat(fixture.cleared).containsExactly(owner) + assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() + } + + @Test + fun `unregistering a former leaseholder preserves the current owner's published nav data`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(previous) + fixture.acquireLease(owner, state = Lifecycle.State.RESUMED) + navLeaseCoordinator.pauseCandidate(owner, fixture.scope) + fixture.cleared.clear() + + navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(previous)).isTrue() + assertThat(fixture.cleared).isEmpty() + } + + @Test + fun `unregistering the leaseholder hands the nav lease to the most recently resumed eligible owner`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + val older = Any() + fixture.acquireLease(older) + navLeaseCoordinator.pauseCandidate(older, fixture.scope) + fixture.acquireLease(previous) + navLeaseCoordinator.pauseCandidate(previous, fixture.scope) + fixture.acquireLease(owner) + + navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(previous)).isTrue() + assertThat(fixture.published.last()).isSameInstanceAs(previous) + assertThat(fixture.cleared).containsExactly(older, previous, owner).inOrder() + } + + @Test + fun `unregistering the leaseholder without a scope releases the nav lease`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(previous) + fixture.acquireLease(owner) + fixture.cleared.clear() + + navLeaseCoordinator.unregisterCandidate(owner, null) + + assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() + assertThat(navLeaseCoordinator.hasLease(previous)).isFalse() + assertThat(fixture.published).containsExactly(previous, owner).inOrder() + assertThat(fixture.cleared).isEmpty() + } + + @Test + fun `unregistering the leaseholder without a scope doesn't finish its nav transaction`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + fixture.acquireLease(owner, transaction = transaction) + + navLeaseCoordinator.unregisterCandidate(owner, null) + + verify(transaction, never()).finish(any()) + } + + @Test + fun `unregistering a former leaseholder finishes its retained transaction without clearing an unrelated one`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + whenever(transaction.status).thenReturn(SpanStatus.CANCELLED) + fixture.acquireLease(owner, transaction = transaction) + fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + fixture.scope.transaction = unrelatedTransaction + fixture.cleared.clear() + + navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + + verify(transaction).finish(SpanStatus.CANCELLED) + assertThat(fixture.scope.transaction).isSameInstanceAs(unrelatedTransaction) + assertThat(fixture.cleared).isEmpty() + } + + private class Fixture { + val navLeaseCoordinator = NavLeaseCoordinator() + val scope = Scope(SentryOptions()) + val published = mutableListOf() + val cleared = mutableListOf() + + fun register(owner: Any, state: Lifecycle.State) { + navLeaseCoordinator.registerCandidate( + owner, + state, + onPublishData = { published += owner }, + onClearPublishedData = { cleared += owner }, + ) + } + + fun acquireLease( + owner: Any, + state: Lifecycle.State = Lifecycle.State.RESUMED, + transaction: ITransaction? = null, + ) { + register(owner, state) + navLeaseCoordinator.onBackStackUpdate(owner, scope) + if (transaction != null) { + navLeaseCoordinator.retainTransaction(owner, transaction) + scope.transaction = transaction + } + } + + companion object { + fun create(): Fixture = Fixture() + } + } +} From bc5ca853e3d5d7f16672af93ad16056d8ba88f73 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Wed, 21 Oct 2026 09:20:24 +0200 Subject: [PATCH 2/3] fix(android): Release nav lease references when cleanup throws Always forget departing navigation candidates when transaction cleanup or lease handoff fails. Preserve a successor selected before a callback throws. Add regression coverage for cleanup failures and successor lease ownership. Refs JAVA-274 Co-Authored-By: Codex --- .../lifecycle/NavLeaseCoordinator.kt | 36 +++-- .../lifecycle/NavLeaseCoordinatorTest.kt | 153 +++++++++++++++++- 2 files changed, 176 insertions(+), 13 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 53dcd04bf6..3e3492a1f2 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 @@ -155,21 +155,37 @@ internal class NavLeaseCoordinator { * A `null` scope (e.g., the SDK is disabled) still removes the candidate, but its transaction is * left to time out, its published data is left in place, and the lease is released rather than * handed off. + * + * Coordinator references are released even if scope cleanup or lease handoff throws. The + * exception is left to the caller to handle; remaining scope cleanup may be incomplete. */ fun unregisterCandidate(owner: Any, scope: IScope?) { findCandidate(owner)?.let { - finishTransaction(it, scope) - candidates.remove(it) - - if (publishedDataCandidate === it) { - publishedDataCandidate = null - if (scope != null) { - it.onClearPublishedData(scope) + try { + finishTransaction(it, scope) + candidates.remove(it) + + if (publishedDataCandidate === it) { + publishedDataCandidate = null + if (scope != null) { + it.onClearPublishedData(scope) + } } - } - if (leaseHolder === it) { - handOffLease(mostRecentlyResumedCandidate(), scope) + if (leaseHolder === it) { + handOffLease(mostRecentlyResumedCandidate(), scope) + } + } finally { + // Scope observers and transaction callbacks can throw. Never retain a departing owner + // just because cleanup failed, and preserve any successor selected during handoff. + candidates.remove(it) + it.transaction = null + if (publishedDataCandidate === it) { + publishedDataCandidate = null + } + if (leaseHolder === it) { + leaseHolder = null + } } } } 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 6b3af236ac..a720439906 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 @@ -2,12 +2,15 @@ package io.sentry.compose.navigation3.lifecycle import androidx.lifecycle.Lifecycle import com.google.common.truth.Truth.assertThat +import io.sentry.IScope import io.sentry.ITransaction import io.sentry.Scope import io.sentry.SentryOptions import io.sentry.SpanStatus +import org.junit.Assert.assertThrows import org.junit.Test import org.mockito.kotlin.any +import org.mockito.kotlin.doThrow import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.verify @@ -765,18 +768,161 @@ class NavLeaseCoordinatorTest { assertThat(fixture.cleared).isEmpty() } + @Test + fun `unregistering the leaseholder forgets the owner when finishing its transaction throws`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + val failure = IllegalStateException("finish failed") + fixture.acquireLease(owner, transaction = transaction) + doThrow(failure).whenever(transaction).finish(SpanStatus.OK) + + val thrown = + assertThrows(IllegalStateException::class.java) { + navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + } + + assertThat(thrown).isSameInstanceAs(failure) + assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() + assertThat(navLeaseCoordinator.isNavTransaction(transaction)).isFalse() + + fixture.published.clear() + navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + fixture.acquireLease(incoming) + + assertThat(fixture.published).containsExactly(incoming) + assertThat(fixture.cleared).isEmpty() + } + + @Test + fun `unregistering the leaseholder forgets the owner when unbinding its transaction throws`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + val failure = IllegalStateException("unbind failed") + val failingScope = mock() + fixture.acquireLease(owner, transaction = transaction) + doThrow(failure).whenever(failingScope).withTransaction(any()) + + val thrown = + assertThrows(IllegalStateException::class.java) { + navLeaseCoordinator.unregisterCandidate(owner, failingScope) + } + + assertThat(thrown).isSameInstanceAs(failure) + assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() + assertThat(navLeaseCoordinator.isNavTransaction(transaction)).isFalse() + + fixture.published.clear() + navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + fixture.acquireLease(incoming) + + assertThat(fixture.published).containsExactly(incoming) + assertThat(fixture.cleared).isEmpty() + } + + @Test + fun `unregistering the leaseholder forgets the owner when clearing its published nav data throws`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + val failure = IllegalStateException("clear failed") + fixture.acquireLease(owner, onClearPublishedData = { throw failure }) + + val thrown = + assertThrows(IllegalStateException::class.java) { + navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + } + + assertThat(thrown).isSameInstanceAs(failure) + assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() + + fixture.published.clear() + navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + fixture.acquireLease(incoming) + + assertThat(fixture.published).containsExactly(incoming) + assertThat(navLeaseCoordinator.hasLease(incoming)).isTrue() + } + + @Test + fun `unregistering the leaseholder forgets the owner when restoring a fallback transaction throws`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + val failure = IllegalStateException("fallback restoration failed") + val failingScope = mock() + fixture.acquireLease(previous, transaction = transaction) + fixture.acquireLease(owner) + doThrow(failure).whenever(failingScope).withTransaction(any()) + + val thrown = + assertThrows(IllegalStateException::class.java) { + navLeaseCoordinator.unregisterCandidate(owner, failingScope) + } + + assertThat(thrown).isSameInstanceAs(failure) + assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() + assertThat(navLeaseCoordinator.isNavTransaction(transaction)).isTrue() + + fixture.published.clear() + fixture.cleared.clear() + navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + fixture.acquireLease(incoming) + + assertThat(fixture.published).containsExactly(incoming) + assertThat(fixture.cleared).doesNotContain(owner) + } + + @Test + fun `unregistering the leaseholder preserves the fallback lease when publishing fallback nav data throws`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + val failure = IllegalStateException("fallback publication failed") + var failPublication = false + navLeaseCoordinator.registerCandidate( + previous, + Lifecycle.State.RESUMED, + onPublishData = { + if (failPublication) { + throw failure + } + }, + onClearPublishedData = {}, + ) + navLeaseCoordinator.onBackStackUpdate(previous, fixture.scope) + fixture.acquireLease(owner, transaction = transaction) + failPublication = true + + val thrown = + assertThrows(IllegalStateException::class.java) { + navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + } + + assertThat(thrown).isSameInstanceAs(failure) + assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() + assertThat(navLeaseCoordinator.hasLease(previous)).isTrue() + assertThat(navLeaseCoordinator.isNavTransaction(transaction)).isFalse() + + fixture.published.clear() + navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + + assertThat(fixture.published).isEmpty() + assertThat(navLeaseCoordinator.hasLease(previous)).isTrue() + } + private class Fixture { val navLeaseCoordinator = NavLeaseCoordinator() val scope = Scope(SentryOptions()) val published = mutableListOf() val cleared = mutableListOf() - fun register(owner: Any, state: Lifecycle.State) { + fun register( + owner: Any, + state: Lifecycle.State, + onClearPublishedData: (IScope) -> Unit = { cleared += owner }, + ) { navLeaseCoordinator.registerCandidate( owner, state, onPublishData = { published += owner }, - onClearPublishedData = { cleared += owner }, + onClearPublishedData = onClearPublishedData, ) } @@ -784,8 +930,9 @@ class NavLeaseCoordinatorTest { owner: Any, state: Lifecycle.State = Lifecycle.State.RESUMED, transaction: ITransaction? = null, + onClearPublishedData: (IScope) -> Unit = { cleared += owner }, ) { - register(owner, state) + register(owner, state, onClearPublishedData) navLeaseCoordinator.onBackStackUpdate(owner, scope) if (transaction != null) { navLeaseCoordinator.retainTransaction(owner, transaction) From a010c9e29341760391acbebab208cbfcacd96fa9 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Wed, 21 Oct 2026 09:53:37 +0200 Subject: [PATCH 3/3] fix(android): Keep the nav leaseholder as the published data owner transferLeaseTo assigned the lease before clearing the previous owner's published nav data. When that callback threw, the new leaseholder was recorded against a stale publisher, so a later handoff cleared the wrong owner's data and never cleared the leaseholder's own. Always record the incoming candidate as the publisher once it takes the lease. Add regression coverage for handoff cleanup failures. Refs JAVA-274 --- .../lifecycle/NavLeaseCoordinator.kt | 96 +++++++++----- .../navigation3/SentryNavEffectTest.kt | 8 -- .../lifecycle/NavLeaseCoordinatorTest.kt | 121 ++++++++++++++++++ 3 files changed, 187 insertions(+), 38 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 3e3492a1f2..74773c1ddb 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 @@ -16,6 +16,19 @@ import io.sentry.SpanStatus * Note: "Owner" below refers to the `LifecycleOwner` whose lifecycle events the candidate's * `NavScopes` relays. * + * **Exception semantics** + * + * Every method in this class with an `IScope` parameter can throw arbitrary exceptions due to: + * + * 1. the [onPublishData][registerCandidate] and [onClearPublishedData][registerCandidate] callbacks + * supplied at registration; + * 2. the `IScope` the method is handed, whose mutators notify every observer registered through + * `SentryOptions.addScopeObserver` – including the host app's – in an unguarded loop; and + * 3. `finish()` on a retained nav transaction, which may run the host app's event processors. + * + * This class restores its own bookkeeping before the exception leaves, so the coordinator remains + * usable. + * * **Thread safety** * * This class is ***not*** thread-safe. All `NavScopes` instances using this coordinator must @@ -83,6 +96,8 @@ internal class NavLeaseCoordinator { * * A `null` scope (e.g., the SDK is disabled) still records the lifecycle transition so the * candidate remains eligible for fallback once a scope becomes available. + * + * Can throw arbitrary exceptions. See "Exception semantics" in the class KDoc. */ fun resumeCandidate(owner: Any, scope: IScope?) { findCandidate(owner)?.let { @@ -104,6 +119,8 @@ internal class NavLeaseCoordinator { * * A `null` scope (e.g., the SDK is disabled) still records the lifecycle transition so the * candidate remains eligible for fallback once a scope becomes available. + * + * Can throw arbitrary exceptions. See "Exception semantics" in the class KDoc. */ fun pauseCandidate(owner: Any, scope: IScope?) { findCandidate(owner)?.let { @@ -132,6 +149,8 @@ internal class NavLeaseCoordinator { * * A `null` scope (e.g., the SDK is disabled) still records the lifecycle transition so the * candidate remains eligible for fallback once a scope becomes available. + * + * Can throw arbitrary exceptions. See "Exception semantics" in the class KDoc. */ fun stopCandidate(owner: Any, scope: IScope?) { findCandidate(owner)?.let { @@ -156,36 +175,34 @@ internal class NavLeaseCoordinator { * left to time out, its published data is left in place, and the lease is released rather than * handed off. * - * Coordinator references are released even if scope cleanup or lease handoff throws. The - * exception is left to the caller to handle; remaining scope cleanup may be incomplete. + * Coordinator references are released even if scope cleanup or lease handoff throws. See + * "Exception semantics" in the class KDoc. The remaining scope cleanup is then skipped. */ fun unregisterCandidate(owner: Any, scope: IScope?) { - findCandidate(owner)?.let { - try { - finishTransaction(it, scope) - candidates.remove(it) + val candidate = findCandidate(owner) ?: return - if (publishedDataCandidate === it) { - publishedDataCandidate = null - if (scope != null) { - it.onClearPublishedData(scope) - } - } + try { + finishTransaction(candidate, scope) + candidates.remove(candidate) - if (leaseHolder === it) { - handOffLease(mostRecentlyResumedCandidate(), scope) - } - } finally { - // Scope observers and transaction callbacks can throw. Never retain a departing owner - // just because cleanup failed, and preserve any successor selected during handoff. - candidates.remove(it) - it.transaction = null - if (publishedDataCandidate === it) { - publishedDataCandidate = null - } - if (leaseHolder === it) { - leaseHolder = null - } + if (publishedDataCandidate === candidate) { + publishedDataCandidate = null + scope?.let { candidate.onClearPublishedData(it) } + } + + if (leaseHolder === candidate) { + handOffLease(mostRecentlyResumedCandidate(), scope) + } + } finally { + // Scope observers and transaction callbacks can throw. Never retain a departing owner + // just because cleanup failed, and preserve any successor selected during handoff. + candidates.remove(candidate) + candidate.transaction = null + if (publishedDataCandidate === candidate) { + publishedDataCandidate = null + } + if (leaseHolder === candidate) { + leaseHolder = null } } } @@ -203,6 +220,8 @@ internal class NavLeaseCoordinator { * If the lease transfers, ownership and transaction binding are updated before the candidate's * staged nav data is published. Publication completes before this method returns, so the caller * can then apply the back-stack update. Later calls for the same [owner] do nothing. + * + * Can throw arbitrary exceptions. See "Exception semantics" in the class KDoc. */ fun onBackStackUpdate(owner: Any, scope: IScope) { val candidate = findCandidate(owner) ?: return @@ -293,6 +312,14 @@ internal class NavLeaseCoordinator { } } + /** + * Hands the lease to [candidate], or releases it given `null`. + * + * Maintains the invariant that, while a lease is held, the leaseholder is also the candidate + * recorded as having published the shared scope's nav data. The invariant holds even when a + * publish or clear callback throws part way through the handoff. See "Exception semantics" in the + * class KDoc. + */ private fun transferLeaseTo(candidate: Candidate?, scope: IScope) { if (leaseHolder === candidate) { return @@ -330,11 +357,20 @@ internal class NavLeaseCoordinator { // previous owner's values don't linger where the candidate has nothing staged to replace // them. val previousPublisher = publishedDataCandidate - if (previousPublisher != null && previousPublisher !== candidate) { - previousPublisher.onClearPublishedData(scope) + try { + if (previousPublisher != null && previousPublisher !== candidate) { + previousPublisher.onClearPublishedData(scope) + } + publishedDataCandidate = candidate + candidate.onPublishData(scope) + } finally { + // Scope observers can throw. A candidate owns the scope's nav data from the moment it + // takes the lease, so record it as the publisher even if clearing the previous owner's + // data threw. Leaving the previous owner recorded here would strand both owners' values + // b/c the next handoff would clear data the candidate may have already replaced, and would + // never clear the candidate's own. + publishedDataCandidate = candidate } - publishedDataCandidate = candidate - candidate.onPublishData(scope) } } diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectTest.kt index 7b8a4b9946..acc8445b57 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectTest.kt @@ -15,7 +15,6 @@ import io.sentry.Breadcrumb import io.sentry.Hint import io.sentry.IScope import io.sentry.IScopes -import io.sentry.ITransaction import io.sentry.Scope import io.sentry.ScopeCallback import io.sentry.SentryOptions @@ -550,13 +549,6 @@ class SentryNavEffectTest { return navigationContext[BACKSTACK_KEY] as? List> } - private fun ITransaction.navigationBackStack(): List>? { - val navigationContext = contexts[NAVIGATION_CONTEXT_KEY] as? Map<*, *> ?: return null - - @Suppress("UNCHECKED_CAST") - return navigationContext[BACKSTACK_KEY] as? List> - } - private companion object { const val NAVIGATION_CONTEXT_KEY = "Navigation" const val BACKSTACK_KEY = "Back Stack" 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 a720439906..61e5ef2968 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 @@ -17,6 +17,7 @@ import org.mockito.kotlin.verify import org.mockito.kotlin.verifyNoInteractions import org.mockito.kotlin.whenever +@Suppress("LargeClass") class NavLeaseCoordinatorTest { private val owner = Any() @@ -267,6 +268,126 @@ class NavLeaseCoordinatorTest { assertThat(fixture.cleared).isEmpty() } + /** + * An owner that leaves must not leave its nav data behind on the shared scope. + * + * The coordinator records which owner last published nav data. Each handoff uses that record to + * clear the previous owner's screen and back stack, then publishes the incoming owner's. If the + * clear attempt throws, the coordinator must still record the incoming owner as the one that last + * published. + * + * This test runs two handoffs over three owners. The first owner publishes; the second takes the + * lease; and then the first owner's clear attempt throws. The third then takes the lease. The + * test verifies that third handoff clears the second owner's data, and not the first's. That lets + * us ensure the second problem listed below doesn't happen. + * + * In the setup above, if the coordinator didn't record the second owner's data after the first + * owner's clear attempt threw, two things would happen when handing off to the third owner: + * + * 1. Harmless: The handoff logic would try to clear the first owner's data again. That'd remove + * nothing because the scope no longer holds the values the first owner published. + * 2. Problematic: The handoff would never clear the second owner's data. Instead, the second + * owner's data would stay on the scope, so later events would report stale nav data. + */ + @Test + fun `if a clear attempt throws during a handoff, the next handoff clears the new leaseholder's nav data`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + val failure = IllegalStateException("clear failed") + var failClear = true + val firstOwner = previous + val secondOwner = incoming + val thirdOwner = owner + fixture.acquireLease( + firstOwner, + onClearPublishedData = { + if (failClear) { + failClear = false + throw failure + } + fixture.cleared += firstOwner + }, + ) + fixture.register(secondOwner, Lifecycle.State.RESUMED) + + val thrown = + assertThrows(IllegalStateException::class.java) { + navLeaseCoordinator.onBackStackUpdate(secondOwner, fixture.scope) + } + + assertThat(thrown).isSameInstanceAs(failure) + assertThat(navLeaseCoordinator.hasLease(secondOwner)).isTrue() + + fixture.acquireLease(thirdOwner) + + assertThat(fixture.cleared).containsExactly(secondOwner) + } + + /** + * The same guarantee as + * [if a clear attempt throws during a handoff, the next handoff clears the new leaseholder's nav data], + * but for a failure while publishing instead of while clearing. + * + * This case already worked before the fix, because the coordinator records the incoming owner + * before it publishes. The test exists to catch a future change that moves the record after the + * publish. + */ + @Test + fun `if a publish attempt throws during a handoff, the next handoff clears the new leaseholder's nav data`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + val failure = IllegalStateException("publish failed") + val firstOwner = previous + val secondOwner = incoming + val thirdOwner = owner + fixture.acquireLease(firstOwner) + navLeaseCoordinator.registerCandidate( + secondOwner, + Lifecycle.State.RESUMED, + onPublishData = { throw failure }, + onClearPublishedData = { fixture.cleared += secondOwner }, + ) + + val thrown = + assertThrows(IllegalStateException::class.java) { + navLeaseCoordinator.onBackStackUpdate(secondOwner, fixture.scope) + } + + assertThat(thrown).isSameInstanceAs(failure) + assertThat(navLeaseCoordinator.hasLease(secondOwner)).isTrue() + + fixture.acquireLease(thirdOwner) + + assertThat(fixture.cleared).containsExactly(firstOwner, secondOwner).inOrder() + } + + @Test + fun `unregistering an owner that threw while clearing its nav data preserves the new leaseholder's nav data`() { + val fixture = Fixture.create() + val navLeaseCoordinator = fixture.navLeaseCoordinator + val failure = IllegalStateException("clear failed") + var failClear = true + fixture.acquireLease( + previous, + onClearPublishedData = { + if (failClear) { + failClear = false + throw failure + } + fixture.cleared += previous + }, + ) + fixture.register(incoming, Lifecycle.State.RESUMED) + assertThrows(IllegalStateException::class.java) { + navLeaseCoordinator.onBackStackUpdate(incoming, fixture.scope) + } + + navLeaseCoordinator.unregisterCandidate(previous, fixture.scope) + + assertThat(navLeaseCoordinator.hasLease(incoming)).isTrue() + assertThat(fixture.cleared).isEmpty() + } + @Test fun `later back stack updates do not republish nav data or reclaim the nav lease`() { val fixture = Fixture.create()