Skip to content

Commit 24e515e

Browse files
committed
fix(android): Keep Nav3 transactions for navigation without the lease (JAVA-274)
A covered SentryNavEffect now starts its nav transaction when navigation happens and retains it unbound until it acquires the lease, instead of receiving a no-op. Another effect's bound nav transaction no longer stops it from starting one. Propagation context rotations requested without the lease are deferred until the lease is acquired. Fixes navigation in response to an Activity result, which happens before ON_RESUME, losing its transaction.
1 parent a0031dd commit 24e515e

8 files changed

Lines changed: 333 additions & 27 deletions

File tree

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,12 @@ import org.jetbrains.annotations.ApiStatus
7272
*
7373
* Each [lifecycle owner][androidx.lifecycle.LifecycleOwner] may host its own `SentryNavEffect`
7474
* (e.g., one per Activity). During transitions between owners, `SentryNavEffect` coordinates the
75-
* overlapping instances so only one records navigation data at a time. `SentryNavEffect`s
76-
* associated with side-by-side or nested `NavDisplay`s are not supported and can result in
77-
* duplicated or interleaved data and undefined transaction behavior.
75+
* overlapping instances so only one publishes its screen name, back stack, and navigation
76+
* transaction at a time, while breadcrumbs from every instance are recorded as navigation happens.
77+
* Data is attributed to the incoming screen as soon as it's created, before it becomes visible, as
78+
* startup code is the most likely source of crashes and slow spans during a transition.
79+
* `SentryNavEffect`s associated with side-by-side or nested `NavDisplay`s are not supported and can
80+
* result in duplicated or interleaved data and undefined transaction behavior.
7881
*
7982
* @param backStack The navigation backstack to observe.
8083
* @param backStackEntryMapper Maps each entry of the [backStack] to a name and optional arguments

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

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -156,10 +156,24 @@ internal class NavLeaseCoordinator {
156156
}
157157
}
158158

159-
fun bindTransaction(owner: Any, transaction: ITransaction) {
160-
val candidate = leaseHolder ?: return
161-
if (candidate.owner === owner) {
162-
candidate.transaction = transaction
159+
fun isRegistered(owner: Any): Boolean = findCandidate(owner) != null
160+
161+
/** Whether [transaction] is a nav transaction retained for any registered owner. */
162+
fun isNavTransaction(transaction: ITransaction): Boolean = candidates.any {
163+
it.transaction === transaction
164+
}
165+
166+
/**
167+
* Records [transaction] as [owner]'s nav transaction, whether or not [owner] holds the lease.
168+
*
169+
* A transaction retained without the lease is bound to the scope when [owner] acquires the lease,
170+
* if it's still unfinished and the scope is free, and finished when [owner] is
171+
* [unregistered][unregisterCandidate].
172+
*/
173+
fun retainTransaction(owner: Any, transaction: ITransaction) {
174+
val candidate = findCandidate(owner) ?: return
175+
candidate.transaction = transaction
176+
if (leaseHolder === candidate) {
163177
activeTransactionCandidate = candidate
164178
}
165179
}

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

Lines changed: 62 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -83,15 +83,22 @@ import io.sentry.protocol.Contexts
8383
* A possible solution would be to implement a parent span delivery system similar to
8484
* [LocalSentrySpan], but that's a task for another day.
8585
*
86-
* **Only leaseholders generate nav transactions and nav state**
86+
* **Only leaseholders publish nav state and bind nav transactions**
8787
*
8888
* This class works by only allowing `NavScopes` instances holding a lease to publish their
89-
* navigation state and transactions to the underlying `IScope`.
89+
* navigation state and bind their transactions to the underlying `IScope`.
9090
*
9191
* `NavScopes` instances without a lease write to a [stagedNavData] holder, which lets them preserve
9292
* their latest updates without overwriting another instance's published data. When an instance
9393
* acquires the lease, it publishes its staged data to the underlying `IScope`.
9494
*
95+
* Nav transactions are started whether or not an instance holds the lease, so each one begins when
96+
* its destination is reached. Without the lease, a transaction is retained rather than bound, and
97+
* bound once the instance acquires the lease, if it's still unfinished and the scope is free. An
98+
* unbound transaction collects no child spans, so if its instance stays without the lease longer
99+
* than the idle timeout, the transaction finishes empty and is dropped. Propagation context
100+
* rotations requested without the lease are likewise deferred until the lease is acquired.
101+
*
95102
* Breadcrumbs are the exception: they only append to the scope, so every instance records them
96103
* immediately, preserving their real order whether or not it holds the lease.
97104
*
@@ -285,17 +292,31 @@ internal class NavScopes(
285292
}
286293
}
287294

295+
/** Whether this instance currently holds the nav lease. */
296+
fun hasLease(): Boolean = coordinator.hasLease(owner)
297+
298+
/**
299+
* Starts a nav transaction whether or not this instance holds the nav lease, so the transaction
300+
* begins when its destination is actually reached.
301+
*
302+
* Only the leaseholder binds it to the scope (see [NavScope.bindTransaction]). Without the lease,
303+
* the [coordinator] retains it and binds it once this instance acquires the lease, provided it's
304+
* still unfinished and the scope is free.
305+
*
306+
* Returns a no-op transaction while this instance isn't [attached][attach] (in particular, after
307+
* it's [disposed][dispose]), so it can't start transactions that nothing would finish.
308+
*/
288309
override fun startTransaction(
289310
transactionContext: TransactionContext,
290311
transactionOptions: TransactionOptions,
291312
): ITransaction {
292-
if (!coordinator.hasLease(owner)) {
313+
if (!coordinator.isRegistered(owner)) {
293314
return NoOpTransaction.getInstance()
294315
}
295316

296317
return delegate.startTransaction(transactionContext, transactionOptions).also { transaction ->
297318
if (!transaction.isNoOp) {
298-
coordinator.bindTransaction(owner, transaction)
319+
coordinator.retainTransaction(owner, transaction)
299320
}
300321
}
301322
}
@@ -354,8 +375,10 @@ internal class NavScopes(
354375
* breadcrumbs) only append to the scope, so they can't overwrite another instance's data and are
355376
* recorded immediately, irrespective of lease ownership.
356377
*
357-
* Propagation context isn't staged either: like nav transactions, it's only changed by the
358-
* leaseholder, and changes made without the lease are discarded.
378+
* Propagation context isn't staged either, since an earlier propagation context mustn't overwrite
379+
* the scope's current one. Instead, a rotation requested without the lease is
380+
* [deferred][deferPropagationContextRotation] and performed once when the lease is acquired, so the
381+
* destination reached in the meantime still gets a fresh trace.
359382
*/
360383
internal class StagedNavData {
361384

@@ -364,6 +387,9 @@ internal class StagedNavData {
364387
private var hasNavigationContextUpdate = false
365388
private var hasScreenUpdate = false
366389

390+
/** Whether a propagation context rotation was requested while this instance lacked the lease. */
391+
private var hasDeferredPropagationContextRotation = false
392+
367393
// These may differ from the staged values after background updates. We keep the exact published
368394
// references so cleanup preserves host replacements, including structurally equal replacements.
369395
private var publishedNavigationContext: Any? = null
@@ -396,6 +422,11 @@ internal class StagedNavData {
396422
}
397423
}
398424

425+
/** Requests a propagation context rotation for when this instance next [publishes][publishTo]. */
426+
fun deferPropagationContextRotation() {
427+
hasDeferredPropagationContextRotation = true
428+
}
429+
399430
fun publishTo(realScope: IScope) {
400431
// A new leaseholder publishes a complete navigation state. Clearing fields it hasn't staged
401432
// prevents the previous leaseholder's values from lingering after a handoff.
@@ -404,6 +435,10 @@ internal class StagedNavData {
404435
if (hasNavigationContextUpdate) contexts[NAVIGATION_CONTEXT_KEY] else null,
405436
realScope,
406437
)
438+
if (hasDeferredPropagationContextRotation) {
439+
realScope.withPropagationContext { realScope.propagationContext = PropagationContext() }
440+
hasDeferredPropagationContextRotation = false
441+
}
407442
}
408443

409444
/** Called only for the last publishing session; releasing its lease alone does not clear data. */
@@ -437,9 +472,11 @@ internal class StagedNavData {
437472
* An [IScope] that writes to [delegate] only when the current [owner] holds the nav lease.
438473
*
439474
* Without the lease, nav state (screen and nav context) is redirected to [stagedNavData] and
440-
* published if [owner] later acquires the lease, while transaction binding and propagation context
441-
* rotation are discarded. Reads of the scope's app context and transaction, and unbinding [owner]'s
442-
* own transaction, always go to [delegate].
475+
* published if [owner] later acquires the lease. Transaction binding is skipped, as the
476+
* [coordinator] binds [owner]'s retained transaction when it acquires the lease, and propagation
477+
* context rotation is deferred until then. Reading the scope's app context and unbinding [owner]'s
478+
* own transaction always go to [delegate]; reading the scope's transaction does too, except that
479+
* another owner's nav transaction is hidden without the lease (see [transaction]).
443480
*/
444481
internal class NavScope(
445482
private val delegate: IScope,
@@ -462,8 +499,17 @@ internal class NavScope(
462499
override val app: App?
463500
get() = Contexts(delegate.contexts).app
464501

502+
/**
503+
* The transaction bound to the scope. Without the lease, another owner's nav transaction isn't
504+
* reported: it doesn't stop [owner] from starting its own, which the [coordinator] binds in its
505+
* place when [owner] acquires the lease. Non-nav transactions (e.g., the host's) are always
506+
* reported.
507+
*/
465508
override val transaction: ITransaction?
466-
get() = delegate.transaction
509+
get() =
510+
delegate.transaction?.takeUnless { current ->
511+
!hasLease && coordinator.isNavTransaction(current)
512+
}
467513

468514
override fun bindTransaction(transaction: ITransaction) {
469515
if (!hasLease) {
@@ -498,6 +544,8 @@ internal class NavScope(
498544
override fun rotatePropagationContext() {
499545
if (hasLease) {
500546
delegate.withPropagationContext { delegate.propagationContext = PropagationContext() }
547+
} else {
548+
stagedNavData.deferPropagationContextRotation()
501549
}
502550
}
503551
}
@@ -537,7 +585,10 @@ internal interface INavScope {
537585
/** A copy of the scope's app context, if any. */
538586
val app: App?
539587

540-
/** The transaction bound to the scope, if any. */
588+
/**
589+
* The transaction bound to the scope, if any, as far as starting a nav transaction is concerned:
590+
* see [NavScope.transaction].
591+
*/
541592
val transaction: ITransaction?
542593

543594
/** Binds [transaction] to the scope if no transaction is bound to it. */

‎sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectTest.kt‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import io.sentry.Scope
2626
import io.sentry.ScopeCallback
2727
import io.sentry.ScopeType
2828
import io.sentry.Scopes
29+
import io.sentry.SentryDate
2930
import io.sentry.SentryOptions
3031
import io.sentry.SentryTracer
3132
import io.sentry.TransactionContext
@@ -626,6 +627,59 @@ class SentryNavEffectTest {
626627
}
627628
}
628629

630+
@Test
631+
fun `navigating in response to an Activity result gets its transaction once the effect resumes`() {
632+
val fixture = NavTestFixture()
633+
val ownerA = NavTestLifecycle(Lifecycle.State.RESUMED)
634+
val ownerB = NavTestLifecycle()
635+
val backStackA = mutableStateListOf("Home")
636+
val showB = mutableStateOf(false)
637+
val options = SentryNavOptions()
638+
val mapper = BackStackEntryMapper<String> { SentryBackStackEntry(it) }
639+
composeRule.setContent {
640+
CompositionLocalProvider(LocalLifecycleOwner provides ownerA) {
641+
SentryNavEffect(backStackA, mapper, options, fixture.scopes, fixture.coordinator)
642+
}
643+
if (showB.value) {
644+
CompositionLocalProvider(LocalLifecycleOwner provides ownerB) {
645+
SentryNavEffect(listOf("Picker"), mapper, options, fixture.scopes, fixture.coordinator)
646+
}
647+
}
648+
}
649+
650+
// A starts B: A pauses, B is created and takes over, B resumes, then A stops.
651+
composeRule.runOnIdle {
652+
ownerA.pause()
653+
showB.value = true
654+
}
655+
composeRule.runOnIdle {
656+
ownerB.resume()
657+
ownerA.stop()
658+
}
659+
// B finishes with a result: B pauses, then A restarts and navigates before it resumes.
660+
composeRule.runOnIdle {
661+
ownerB.pause()
662+
ownerA.start()
663+
backStackA += "Result"
664+
}
665+
lateinit var beforeResume: SentryDate
666+
composeRule.runOnIdle {
667+
assertThat(fixture.scope.transaction?.name).isEqualTo("/Picker")
668+
assertThat(fixture.scope.breadcrumbs.map { it.data["to"] })
669+
.containsExactly("/Home", "/Picker", "/Result")
670+
.inOrder()
671+
beforeResume = fixture.options.dateProvider.now()
672+
ownerA.resume()
673+
}
674+
675+
composeRule.runOnIdle {
676+
val transaction = requireNotNull(fixture.scope.transaction)
677+
assertThat(transaction.name).isEqualTo("/Result")
678+
assertThat(transaction.startDate.nanoTimestamp()).isAtMost(beforeResume.nanoTimestamp())
679+
assertThat(fixture.scope.screen).isEqualTo("/Result")
680+
}
681+
}
682+
629683
@Test
630684
fun `replacing lifecycle owner disposes the old session and observes the new owner`() {
631685
val fixture = NavTestFixture()
@@ -697,6 +751,14 @@ private class NavTestLifecycle(state: Lifecycle.State = Lifecycle.State.CREATED)
697751
fun pause() {
698752
lifecycle.currentState = Lifecycle.State.STARTED
699753
}
754+
755+
fun start() {
756+
lifecycle.currentState = Lifecycle.State.STARTED
757+
}
758+
759+
fun stop() {
760+
lifecycle.currentState = Lifecycle.State.CREATED
761+
}
700762
}
701763

702764
/** Real SDK state with only the event delivery boundary stubbed. No timers or global SDK state. */

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

Lines changed: 48 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -322,18 +322,63 @@ class NavLeaseCoordinatorTest {
322322
}
323323

324324
@Test
325-
fun `a non-leaseholder cannot bind a nav transaction`() {
325+
fun `an unregistered owner's nav transaction is not retained`() {
326326
val fixture = Fixture.create()
327327
val navLeaseCoordinator = fixture.navLeaseCoordinator
328328
val other = Any()
329329
fixture.acquireLease(owner)
330330

331-
navLeaseCoordinator.bindTransaction(other, transaction)
331+
navLeaseCoordinator.retainTransaction(other, transaction)
332332
navLeaseCoordinator.unregisterCandidate(owner, fixture.scope)
333333

334334
verifyNoInteractions(transaction)
335335
}
336336

337+
@Test
338+
fun `a nav transaction retained without the lease is bound once its owner acquires the lease`() {
339+
val fixture = Fixture.create()
340+
val navLeaseCoordinator = fixture.navLeaseCoordinator
341+
fixture.acquireLease(owner)
342+
fixture.acquireLease(incoming, state = Lifecycle.State.CREATED)
343+
344+
navLeaseCoordinator.retainTransaction(owner, transaction)
345+
assertThat(fixture.scope.transaction).isNull()
346+
347+
navLeaseCoordinator.unregisterCandidate(incoming, fixture.scope)
348+
349+
assertThat(navLeaseCoordinator.hasLease(owner)).isTrue()
350+
assertThat(fixture.scope.transaction).isSameInstanceAs(transaction)
351+
}
352+
353+
@Test
354+
fun `a finished nav transaction retained without the lease is not bound when its owner acquires the lease`() {
355+
val fixture = Fixture.create()
356+
val navLeaseCoordinator = fixture.navLeaseCoordinator
357+
fixture.acquireLease(owner)
358+
fixture.acquireLease(incoming, state = Lifecycle.State.CREATED)
359+
navLeaseCoordinator.retainTransaction(owner, transaction)
360+
whenever(transaction.isFinished).thenReturn(true)
361+
362+
navLeaseCoordinator.unregisterCandidate(incoming, fixture.scope)
363+
364+
assertThat(navLeaseCoordinator.hasLease(owner)).isTrue()
365+
assertThat(fixture.scope.transaction).isNull()
366+
}
367+
368+
@Test
369+
fun `unregistering an owner finishes a nav transaction it retained without the lease`() {
370+
val fixture = Fixture.create()
371+
val navLeaseCoordinator = fixture.navLeaseCoordinator
372+
fixture.acquireLease(owner)
373+
fixture.acquireLease(incoming, state = Lifecycle.State.CREATED)
374+
navLeaseCoordinator.retainTransaction(owner, transaction)
375+
376+
navLeaseCoordinator.unregisterCandidate(owner, fixture.scope)
377+
378+
verify(transaction).finish(SpanStatus.OK)
379+
assertThat(navLeaseCoordinator.hasLease(incoming)).isTrue()
380+
}
381+
337382
@Test
338383
fun `an unfinished nav transaction is restored when its owner regains the lease`() {
339384
val fixture = Fixture.create()
@@ -455,7 +500,7 @@ class NavLeaseCoordinatorTest {
455500
register(owner, state)
456501
navLeaseCoordinator.onBackStackUpdate(owner, scope)
457502
if (transaction != null) {
458-
navLeaseCoordinator.bindTransaction(owner, transaction)
503+
navLeaseCoordinator.retainTransaction(owner, transaction)
459504
scope.transaction = transaction
460505
}
461506
}

0 commit comments

Comments
 (0)