RUM-16113: Prevent stale views from overwriting last_view_event

Author: ambushwork

features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/RumDataWriter.kt

@@ -83,7 +83,19 @@ internal class RumDataWriter(
     @WorkerThread
     internal fun onDataWritten(data: Any, rawData: ByteArray) {
         when (data) {
-            is ViewEvent -> sdkCore.writeLastViewEvent(rawData)
+            is ViewEvent -> onViewEventWritten(data, rawData)
+        }
+    }
+
+    @WorkerThread
+    private fun onViewEventWritten(data: ViewEvent, rawData: ByteArray) {
+        val rumContext = sdkCore.getFeatureContext(Feature.RUM_FEATURE_NAME, useContextThread = false)
+        val currentViewId = rumContext[RumContext.VIEW_ID] as? String
+        val currentViewStartedAt = rumContext[RumContext.VIEW_TIMESTAMP] as? Long ?: 0L
+        val isCurrentView = data.view.id == currentViewId
+        val isNewerView = data.date > currentViewStartedAt
+        if (isCurrentView || isNewerView) {
+            sdkCore.writeLastViewEvent(rawData)
         }
     }
 

features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/RumDataWriterTest.kt

@@ -7,6 +7,7 @@
 package com.datadog.android.rum.internal.domain
 
 import com.datadog.android.api.InternalLogger
+import com.datadog.android.api.feature.Feature
 import com.datadog.android.api.storage.EventType
 import com.datadog.android.api.storage.RawBatchEvent
 import com.datadog.android.core.internal.storage.TelemetryAwareEventBatchWriter
@@ -46,6 +47,7 @@ import org.mockito.kotlin.anyOrNull
 import org.mockito.kotlin.argumentCaptor
 import org.mockito.kotlin.doReturn
 import org.mockito.kotlin.doThrow
+import org.mockito.kotlin.never
 import org.mockito.kotlin.times
 import org.mockito.kotlin.verify
 import org.mockito.kotlin.verifyNoInteractions
@@ -336,6 +338,9 @@ internal class RumDataWriterTest {
     fun `M persist the event into the NDK crash folder W onDataWritten(){ViewEvent+dir exists}`(
         @Forgery viewEvent: ViewEvent
     ) {
+        // Given - the event belongs to the current view
+        stubCurrentView(viewEvent.view.id, viewEvent.date)
+
         // When
         testedWriter.onDataWritten(viewEvent, fakeSerializedData)
 
@@ -344,6 +349,133 @@ internal class RumDataWriterTest {
         verifyNoInteractions(mockInternalLogger)
     }
 
+    @Test
+    fun `M call writeLastViewEvent W onDataWritten() { ViewEvent, update of the current view }`(
+        @Forgery viewEvent: ViewEvent
+    ) {
+        // Given - view A is the current view, its update is older than nothing else
+        stubCurrentView(VIEW_A_ID, VIEW_A_TIMESTAMP)
+
+        // When - view A emits an update, complete or not
+        val fakeData = writeViewEvent(viewEvent, VIEW_A_ID, VIEW_A_TIMESTAMP, isActive = false)
+
+        // Then
+        verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeData)
+    }
+
+    @Test
+    fun `M call writeLastViewEvent W onDataWritten() { ViewEvent of a newer view }`(
+        @Forgery viewEvent: ViewEvent
+    ) {
+        // Given - view A is still the current view in context: the context is only updated once
+        // the event has been written, so it is one event behind
+        stubCurrentView(VIEW_A_ID, VIEW_A_TIMESTAMP)
+
+        // When - view B, which started after view A, emits its first event
+        val fakeViewBData = writeViewEvent(viewEvent, VIEW_B_ID, VIEW_B_TIMESTAMP, isActive = true)
+
+        // Then - the newer view takes over the persisted snapshot
+        verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeViewBData)
+    }
+
+    @Test
+    fun `M call writeLastViewEvent W onDataWritten() { ViewEvent, no current view }`(
+        @Forgery viewEvent: ViewEvent
+    ) {
+        // Given - no view is active anymore
+        whenever(
+            rumMonitor.mockSdkCore.getFeatureContext(Feature.RUM_FEATURE_NAME, false)
+        ) doReturn emptyMap()
+
+        // When - the last view completes
+        val fakeData = writeViewEvent(viewEvent, VIEW_A_ID, VIEW_A_TIMESTAMP, isActive = false)
+
+        // Then
+        verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeData)
+    }
+
+    @Test
+    fun `M NOT call writeLastViewEvent W onDataWritten() { ViewEvent, stale view still marked active }`(
+        @Forgery viewEvent: ViewEvent
+    ) {
+        // Given - view A was stopped while resources were still pending, view B is the current view
+        stubCurrentView(VIEW_B_ID, VIEW_B_TIMESTAMP)
+
+        // When - a pending resource of view A completes: view A is not complete yet, so it still
+        // emits an event with isActive = true
+        val fakeStaleViewAData = writeViewEvent(viewEvent, VIEW_A_ID, VIEW_A_TIMESTAMP, isActive = true)
+
+        // Then - the stale view does not overwrite the snapshot of view B
+        verify(rumMonitor.mockSdkCore, never()).writeLastViewEvent(fakeStaleViewAData)
+    }
+
+    @Test
+    fun `M NOT call writeLastViewEvent W onDataWritten() { ViewEvent, stale view completing }`(
+        @Forgery viewEvent: ViewEvent
+    ) {
+        // Given - view B is the current view
+        stubCurrentView(VIEW_B_ID, VIEW_B_TIMESTAMP)
+
+        // When - the last pending event of view A completes it
+        val fakeCompletedViewAData = writeViewEvent(viewEvent, VIEW_A_ID, VIEW_A_TIMESTAMP, isActive = false)
+
+        // Then - the completion of the stale view does not overwrite the snapshot of view B
+        verify(rumMonitor.mockSdkCore, never()).writeLastViewEvent(fakeCompletedViewAData)
+    }
+
+    @Test
+    fun `M NOT read the RUM context on the context thread W onDataWritten() { ViewEvent }`(
+        @Forgery viewEvent: ViewEvent
+    ) {
+        // Given
+        stubCurrentView(viewEvent.view.id, viewEvent.date)
+
+        // When
+        testedWriter.onDataWritten(viewEvent, fakeSerializedData)
+
+        // Then - reading on the context thread would deadlock: the context thread is waiting for
+        // this event to be handled
+        verify(rumMonitor.mockSdkCore).getFeatureContext(Feature.RUM_FEATURE_NAME, false)
+        verify(rumMonitor.mockSdkCore, never()).getFeatureContext(Feature.RUM_FEATURE_NAME, true)
+    }
+
+    // endregion
+
+    // region Internal
+
+    /**
+     * Stubs the RUM feature context to describe the given view as the current one.
+     */
+    private fun stubCurrentView(viewId: String, viewTimestamp: Long) {
+        whenever(
+            rumMonitor.mockSdkCore.getFeatureContext(Feature.RUM_FEATURE_NAME, false)
+        ) doReturn mapOf(
+            RumContext.VIEW_ID to viewId,
+            RumContext.VIEW_TIMESTAMP to viewTimestamp
+        )
+    }
+
+    /**
+     * Notifies the writer that a view event was written for the given view, and returns the
+     * serialized data used for that event.
+     */
+    private fun writeViewEvent(
+        viewEvent: ViewEvent,
+        viewId: String,
+        viewTimestamp: Long,
+        isActive: Boolean
+    ): ByteArray {
+        val serializedData = "$viewId-$viewTimestamp".toByteArray(Charsets.UTF_8)
+        testedWriter.onDataWritten(
+            viewEvent.copy(
+                date = viewTimestamp,
+                view = viewEvent.view.copy(id = viewId, isActive = isActive)
+            ),
+            serializedData
+        )
+        return serializedData
+    }
+
     // endregion
 
     // region accessibility
@@ -401,6 +533,11 @@ internal class RumDataWriterTest {
     // endregion
 
     companion object {
+        private const val VIEW_A_ID = "view-a"
+        private const val VIEW_B_ID = "view-b"
+        private const val VIEW_A_TIMESTAMP = 1_000L
+        private const val VIEW_B_TIMESTAMP = 2_000L
+
         val rumMonitor = GlobalRumMonitorTestConfiguration()
 
         @TestConfigurationsProvider