feat(sample): [Nav3 Lifecycle 5] Add Nav3 lifecycle scenarios to sample app

sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/SentryNavOptions.kt

@@ -3,7 +3,7 @@ package io.sentry.compose.navigation3
 import androidx.compose.runtime.Immutable
 import org.jetbrains.annotations.ApiStatus
 
-// Keep the default low: every captured entry may require argument extraction and recursive
+// Keep the default low, as every captured entry may require argument extraction and recursive
 // sanitization when navigation changes are observed.
 private const val DEFAULT_MAX_CAPTURED_BACK_STACK_ENTRIES = 5
 

sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt

@@ -19,8 +19,8 @@ import io.sentry.SpanStatus
  * **Exception semantics**
  *
  * Methods in this class with an `IScope` parameter can throw arbitrary exceptions†, which propagate
- * to the caller. This class restores its own bookkeeping first, so the coordinator remains usable
- * if the exception is caught.
+ * to the caller. Each method updates its own bookkeeping before calling into the scope, so the
+ * coordinator remains usable if the exception is caught.
  *
  * † E.g., `IScope` can throw arbitrary exceptions when scope mutators notify registered observers –
  * including the host app – in an unguarded loop, or when `finish()` is called on a retained nav
@@ -172,35 +172,38 @@ 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. See
-   * "Exception semantics" in the class KDoc. The remaining scope cleanup is then skipped.
+   * Can throw arbitrary exceptions. See "Exception semantics" in the class KDoc. Coordinator
+   * references to `owner` are released before any scope call, so they're released even if scope
+   * cleanup or lease handoff throws. The remaining scope cleanup is then skipped.
    */
   fun unregisterCandidate(owner: Any, scope: IScope?) {
     val candidate = findCandidate(owner) ?: return
+    val transaction = candidate.transaction
+    val wasPublisher = publishedDataCandidate === candidate
+    val wasLeaseHolder = leaseHolder === candidate
 
-    try {
-      finishTransaction(candidate, scope)
-      candidates.remove(candidate)
+    candidates.remove(candidate)
+    candidate.transaction = null
+    if (wasPublisher) {
+      publishedDataCandidate = null
+    }
+    if (wasLeaseHolder) {
+      leaseHolder = null
+    }
 
-      if (publishedDataCandidate === candidate) {
-        publishedDataCandidate = null
-        scope?.let { candidate.onClearPublishedData(it) }
-      }
+    // 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) {
+      return
+    }
 
-      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
-      }
+    finishTransaction(transaction, scope)
+    if (wasPublisher) {
+      candidate.onClearPublishedData(scope)
+    }
+    if (wasLeaseHolder) {
+      transferLeaseTo(mostRecentlyResumedCandidate(), scope)
     }
   }
 
@@ -334,8 +337,8 @@ internal class NavLeaseCoordinator {
 
       val transaction = candidate.transaction
       if (transaction?.isFinished == true) {
-        clearTransactionFromScope(candidate, scope)
         candidate.transaction = null
+        clearTransactionFromScope(transaction, scope)
       } 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
@@ -348,47 +351,40 @@ internal class NavLeaseCoordinator {
       }
     }
 
+    val previousPublisher = publishedDataCandidate
     leaseHolder = candidate
     if (candidate != null) {
+      // Record the candidate as the publisher before calling into the scope: it owns the scope's
+      // nav data from the moment it takes the lease. If clearing the previous owner's data then
+      // throws, the next handoff still clears the candidate's data rather than never clearing it.
+      //
       // 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
-      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
+      if (previousPublisher != null && previousPublisher !== candidate) {
+        previousPublisher.onClearPublishedData(scope)
       }
+      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)
+  /**
+   * Finishes [transaction] if it's unfinished, and unbinds it from [scope].
+   *
+   * Callers must forget the transaction first.
+   */
+  private fun finishTransaction(transaction: ITransaction?, scope: IScope) {
+    if (transaction == null) {
+      return
     }
-
-    candidate.transaction = null
+    if (!transaction.isFinished) {
+      transaction.finish(transaction.status ?: SpanStatus.OK)
+    }
+    clearTransactionFromScope(transaction, scope)
   }
 
-  private fun clearTransactionFromScope(candidate: Candidate, scope: IScope) {
-    val transaction = candidate.transaction ?: return
+  private fun clearTransactionFromScope(transaction: ITransaction, scope: IScope) {
     scope.withTransaction { current ->
       if (current === transaction) {
         scope.clearTransaction()

sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavData.kt

@@ -4,7 +4,7 @@ import io.sentry.IScope
 import io.sentry.PropagationContext
 
 /**
- * Holds navigation state staged for publication: a screen name and back stack nav context.
+ * Holds navigation state staged for publication.
  *
  * Only state that occupies a single slot in the shared [IScope] is staged. Nav events (i.e.,
  * breadcrumbs) only append to the scope, so they can't overwrite another instance's data and are

sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt

@@ -269,25 +269,9 @@ class NavLeaseCoordinatorTest {
   }
 
   /**
-   * 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.
+   * The coordinator records the incoming owner as the publisher before calling into the scope, so a
+   * clear callback that throws mid-handoff doesn't strand the incoming owner's nav data: the next
+   * handoff still clears it.
    */
   @Test
   fun `if a clear attempt throws during a handoff, the next handoff clears the new leaseholder's nav data`() {
@@ -323,71 +307,6 @@ class NavLeaseCoordinatorTest {
     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()
@@ -914,120 +833,6 @@ class NavLeaseCoordinatorTest {
     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<IScope>()
-    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<IScope>()
-    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())