Fix race condition in MapboxTripSession (#11778) (#11850)

* Fix race condition in MapboxTripSession

* Add test

* Add changelog

* Register NavigationRoutesFinishedObserver as well

* Fix test

* Fix test

* Add java doc

* Fix test

* Fix typos. Add comment.

Co-authored-by: Stanislav <[email protected]>
GitOrigin-RevId: b5de4d13cb1dc493ff8b25d83ea9c1fa0859bdb9

Author: 2 people

changelog/unreleased/bugfixes/11778.md

@@ -0,0 +1 @@
+- Fix the race condition when canceling Active Guidance from a background thread that does not immediately cancel Route Progress updates. 

navigation/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt

@@ -50,6 +50,7 @@ import com.mapbox.navigation.core.arrival.ArrivalObserver
 import com.mapbox.navigation.core.arrival.ArrivalProgressObserver
 import com.mapbox.navigation.core.arrival.AutoArrivalController
 import com.mapbox.navigation.core.directions.session.DirectionsSession
+import com.mapbox.navigation.core.directions.session.DirectionsSessionRoutes
 import com.mapbox.navigation.core.directions.session.RoutesExtra
 import com.mapbox.navigation.core.directions.session.RoutesObserver
 import com.mapbox.navigation.core.directions.session.RoutesSetStartedParams
@@ -596,8 +597,13 @@ class MapboxNavigation @VisibleForTesting internal constructor(
         tripSessionLocationEngine = NavigationComponentProvider.createTripSessionLocationEngine(
             locationOptions = navigationOptions.locationOptions,
         )
+
+        directionsSession = NavigationComponentProvider.createDirectionsSession(routerWrapper)
+        directionsSession.registerSetNavigationRoutesFinishedObserver(navigationSession)
+
         tripSession = NavigationComponentProvider.createTripSession(
             tripService = tripService,
+            directionsSession = directionsSession,
             tripSessionLocationEngine = tripSessionLocationEngine,
             navigator = navigator,
             threadController,
@@ -608,8 +614,6 @@ class MapboxNavigation @VisibleForTesting internal constructor(
         tripSession.registerStateObserver(navigationSession)
         tripSession.registerStateObserver(historyRecordingStateHandler)
 
-        directionsSession = NavigationComponentProvider.createDirectionsSession(routerWrapper)
-        directionsSession.registerSetNavigationRoutesFinishedObserver(navigationSession)
         if (reachabilityObserverId == null) {
             reachabilityObserverId = ReachabilityService.addReachabilityObserver(
                 connectivityHandler,
@@ -1340,6 +1344,16 @@ class MapboxNavigation @VisibleForTesting internal constructor(
                                 "to ${directionsSession.routes.firstOrNull()?.id}",
                         ),
                     )
+
+                    // Even though we are not setting new routes here,
+                    // we need to inform that the operation (setNavigationRoutesStarted) is finished
+                    directionsSession.setNavigationRoutesFinished(
+                        DirectionsSessionRoutes(
+                            acceptedRoutes = directionsSession.routes,
+                            ignoredRoutes = directionsSession.ignoredRoutes,
+                            setRoutesInfo = setRoutesInfo,
+                        ),
+                    )
                 } else {
                     historyRecordingStateHandler.setRoutes(routes)
                     when (val processedRoutes = setRoutesToTripSession(routes, setRoutesInfo)) {

navigation/src/main/java/com/mapbox/navigation/core/NavigationComponentProvider.kt

@@ -107,13 +107,15 @@ internal object NavigationComponentProvider {
 
     fun createTripSession(
         tripService: TripService,
+        directionsSession: DirectionsSession,
         tripSessionLocationEngine: TripSessionLocationEngine,
         navigator: MapboxNativeNavigator,
         threadController: ThreadController,
         repeatRerouteAfterOffRouteDelaySeconds: Int,
     ): TripSession = PerformanceTracker.trackPerformanceSync("createTripSession") {
         MapboxTripSession(
             tripService,
+            directionsSession,
             tripSessionLocationEngine,
             navigator = navigator,
             threadController,

navigation/src/main/java/com/mapbox/navigation/core/directions/session/DirectionsSession.kt

@@ -25,7 +25,19 @@ internal interface DirectionsSession : RouteRefresh {
 
     val initialLegIndex: Int
 
+    /**
+     * Notify that route setting process has started.
+     * During the process of routes update, NavSDK skips all the [NavigationStatus] updates.
+     *
+     * Make sure to call [setNavigationRoutesFinished] when done to release the updates.
+     * */
     fun setNavigationRoutesStarted(params: RoutesSetStartedParams)
+
+    /**
+     * Notify that route setting process has finished.
+     *
+     * @param routes DirectionsSessionRoutes
+     */
     fun setNavigationRoutesFinished(routes: DirectionsSessionRoutes)
 
     /**

navigation/src/main/java/com/mapbox/navigation/core/trip/session/MapboxTripSession.kt

@@ -17,6 +17,7 @@ import com.mapbox.navigation.base.trip.model.RouteLegProgress
 import com.mapbox.navigation.base.trip.model.RouteProgress
 import com.mapbox.navigation.base.trip.model.roadobject.UpcomingRoadObject
 import com.mapbox.navigation.core.SetRoutes
+import com.mapbox.navigation.core.directions.session.DirectionsSession
 import com.mapbox.navigation.core.internal.utils.initialLegIndex
 import com.mapbox.navigation.core.internal.utils.mapToReason
 import com.mapbox.navigation.core.navigator.getCurrentBannerInstructions
@@ -57,6 +58,7 @@ import kotlinx.coroutines.cancelChildren
 import kotlinx.coroutines.launch
 import java.util.concurrent.CopyOnWriteArraySet
 import java.util.concurrent.TimeUnit
+import java.util.concurrent.atomic.AtomicBoolean
 import kotlin.time.Duration.Companion.seconds
 import kotlin.time.ExperimentalTime
 import kotlin.time.TimeSource
@@ -66,6 +68,7 @@ import kotlin.time.minus
  * Default implementation of [TripSession]
  *
  * @param tripService TripService
+ * @param directionsSession Directions session
  * @param tripSessionLocationEngine the location engine
  * @param navigator Native navigator
  * @param threadController controller for main/io jobs
@@ -75,6 +78,7 @@ import kotlin.time.minus
 @MainThread
 internal class MapboxTripSession(
     override val tripService: TripService,
+    private val directionsSession: DirectionsSession,
     private val tripSessionLocationEngine: TripSessionLocationEngine,
     private val navigator: MapboxNativeNavigator,
     private val threadController: ThreadController,
@@ -86,12 +90,18 @@ internal class MapboxTripSession(
         private const val LOG_CATEGORY = "MapboxTripSession"
     }
 
-    private var isUpdatingRoute = false
     private var updateLegIndexJob: Job? = null
 
+    @VisibleForTesting
+    internal val isUpdatingRoute = AtomicBoolean(false)
+
     @VisibleForTesting
     internal var primaryRoute: NavigationRoute? = null
 
+    init {
+        registerSetRoutesObservers()
+    }
+
     override suspend fun setRoutes(
         routes: List<NavigationRoute>,
         setRoutes: SetRoutes,
@@ -466,7 +476,7 @@ internal class MapboxTripSession(
                     "Error processing native status update: origin=$origin, status=$status.\n" +
                         "Error: $error\n" +
                         "MapboxTripSession state: " +
-                        "isUpdatingRoute=$isUpdatingRoute, primaryRoute=${primaryRoute?.id}"
+                        "isUpdatingRoute=${isUpdatingRoute.get()}, primaryRoute=${primaryRoute?.id}"
                 }
                 throw NativeStatusProcessingError(error)
             }
@@ -791,7 +801,7 @@ internal class MapboxTripSession(
 
         // we should skip RouteProgress, BannerInstructions, isOffRoute state updates while
         // setting a new route
-        if (isUpdatingRoute) {
+        if (isUpdatingRoute.get()) {
             logD("route progress update dropped - updating routes", LOG_CATEGORY)
             return
         }
@@ -934,10 +944,33 @@ internal class MapboxTripSession(
         }
     }
 
+    /**
+     * Executes a route update transaction, setting the `isUpdatingRoute` flag to true
+     * Works alongside with the observers registered in [registerSetRoutesObservers].
+     * This ensures that during the execution of the provided function, any incoming
+     * route progress updates are ignored, preventing potential race conditions
+     */
     private inline fun <T> updateRouteTransaction(func: () -> T): T {
-        isUpdatingRoute = true
-        return func().also {
-            isUpdatingRoute = false
+        isUpdatingRoute.set(true)
+        return try {
+            func()
+        } finally {
+            isUpdatingRoute.set(false)
+        }
+    }
+
+    /**
+     * Registers an observer to detect an upcoming routes change even before `setRoutes` is called.
+     * This helps handle race conditions by setting `isUpdatingRoute` early, allowing the session to
+     * ignore route progress updates that may arrive from the navigator during the route update process.
+     */
+    private fun registerSetRoutesObservers() {
+        directionsSession.registerSetNavigationRoutesStartedObserver {
+            isUpdatingRoute.set(true)
+        }
+
+        directionsSession.registerSetNavigationRoutesFinishedObserver {
+            isUpdatingRoute.set(false)
         }
     }
 }

navigation/src/test/java/com/mapbox/navigation/core/MapboxNavigationBaseTest.kt

@@ -384,6 +384,7 @@ internal open class MapboxNavigationBaseTest {
         every {
             NavigationComponentProvider.createTripSession(
                 tripService = tripService,
+                directionsSession = directionsSession,
                 tripSessionLocationEngine = tripSessionLocationEngine,
                 navigator = navigator,
                 threadController,

navigation/src/test/java/com/mapbox/navigation/core/MapboxNavigationTest.kt

@@ -1851,13 +1851,15 @@ internal class MapboxNavigationTest : MapboxNavigationBaseTest() {
                 every { id } returns "id3"
             }
             every { directionsSession.routes } returnsMany listOf(listOf(route1), listOf(route2))
+            every { directionsSession.ignoredRoutes } returns emptyList()
 
             mapboxNavigation.setNavigationRoutes(listOf(route1, route3))
 
             coVerify(exactly = 0) {
                 tripSession.setRoutes(any(), any())
             }
-            verify(exactly = 0) {
+
+            verify(exactly = 1) {
                 directionsSession.setNavigationRoutesFinished(any())
             }
         }
@@ -2742,13 +2744,15 @@ internal class MapboxNavigationTest : MapboxNavigationBaseTest() {
         every {
             NavigationComponentProvider.createTripSession(
                 tripService = any(),
+                directionsSession = any(),
                 tripSessionLocationEngine = any(),
                 navigator = any(),
                 any(),
                 any(),
             )
         } returns MapboxTripSession(
             tripService,
+            directionsSession,
             tripSessionLocationEngine,
             navigator,
             threadController,

navigation/src/test/java/com/mapbox/navigation/core/trip/session/MapboxTripSessionNoSetupTest.kt

@@ -15,6 +15,7 @@ import com.mapbox.navigation.base.internal.factory.RoadObjectFactory.toUpcomingR
 import com.mapbox.navigation.base.options.LocationOptions
 import com.mapbox.navigation.base.trip.model.roadobject.UpcomingRoadObject
 import com.mapbox.navigation.core.SetRoutes
+import com.mapbox.navigation.core.directions.session.DirectionsSession
 import com.mapbox.navigation.core.infra.TestLocationProvider
 import com.mapbox.navigation.core.infra.recorders.BannerInstructionsObserverRecorder
 import com.mapbox.navigation.core.infra.recorders.OffRouteObserverRecorder
@@ -577,6 +578,8 @@ private fun buildTripSession(
         every { hasServiceStarted() } returns false
     }
 
+    val directionsSession: DirectionsSession = mockk(relaxUnitFun = true)
+
     val parentJob = SupervisorJob()
     val testScope = CoroutineScope(parentJob + TestCoroutineDispatcher())
     val threadController = spyk<ThreadController>()
@@ -592,6 +595,7 @@ private fun buildTripSession(
     }
     return MapboxTripSession(
         tripService,
+        directionsSession,
         TripSessionLocationEngine(LocationOptions.Builder().build()),
         nativeNavigator,
         threadController,