From cbc677b80dd11f7d634e86f5463f288fed3ba343 Mon Sep 17 00:00:00 2001 From: Jenna Vassar Date: Fri, 15 May 2026 01:43:21 -0700 Subject: [PATCH] Fix room list duplicate-detection telemetry crashing before it can report (#6791) * Add room dupe regression tests * Fix telemetry path for dedupe discovery --- .../impl/datasource/RoomListDataSourceTest.kt | 100 +++++++++++++++++- .../impl/roomlist/RoomListEntriesUpdateExt.kt | 12 +-- .../impl/roomlist/RoomSummaryListProcessor.kt | 7 +- .../roomlist/RoomSummaryListProcessorTest.kt | 56 +++++++++- 4 files changed, 165 insertions(+), 10 deletions(-) diff --git a/features/home/impl/src/test/kotlin/io/element/android/features/home/impl/datasource/RoomListDataSourceTest.kt b/features/home/impl/src/test/kotlin/io/element/android/features/home/impl/datasource/RoomListDataSourceTest.kt index 1ce5061356..ace1ef2eaa 100644 --- a/features/home/impl/src/test/kotlin/io/element/android/features/home/impl/datasource/RoomListDataSourceTest.kt +++ b/features/home/impl/src/test/kotlin/io/element/android/features/home/impl/datasource/RoomListDataSourceTest.kt @@ -14,6 +14,8 @@ import io.element.android.features.home.impl.FakeDateTimeObserver import io.element.android.libraries.androidutils.system.DateTimeObserver import io.element.android.libraries.dateformatter.test.FakeDateFormatter import io.element.android.libraries.matrix.api.roomlist.RoomListService +import io.element.android.libraries.matrix.test.A_ROOM_ID +import io.element.android.libraries.matrix.test.A_ROOM_ID_2 import io.element.android.libraries.matrix.test.notificationsettings.FakeNotificationSettingsService import io.element.android.libraries.matrix.test.room.aRoomSummary import io.element.android.libraries.matrix.test.roomlist.FakeDynamicRoomList @@ -100,11 +102,107 @@ class RoomListDataSourceTest { } } + /** + * Tracking issue #4182: rooms duplicated in the room list around midnight. + * + * If the SDK ever leaks a list containing the same roomId twice (the suspected cause of #4182), + * the UI mapper's `distinctBy` safety net in [RoomListDataSource.buildAndEmitAllRooms] must + * remove the duplicate AND `analyticsService.trackError` must fire so the team can root-cause + * it via Sentry. + */ + @Test + fun `when SDK summaries source contains duplicate roomIds, UI layer dedupes and reports trackError`() = runTest { + val analyticsService = FakeAnalyticsService() + val duplicatedSummaries = listOf( + aRoomSummary(roomId = A_ROOM_ID), + aRoomSummary(roomId = A_ROOM_ID), + aRoomSummary(roomId = A_ROOM_ID_2), + ) + val roomList = FakeDynamicRoomList(summaries = MutableStateFlow(duplicatedSummaries)) + val roomListService = FakeRoomListService( + createRoomListLambda = { roomList } + ).apply { + postState(RoomListService.State.Running) + } + val roomListDataSource = createRoomListDataSource( + roomListService = roomListService, + analyticsService = analyticsService, + ) + + roomListDataSource.roomSummariesFlow.test { + roomListDataSource.launchIn(backgroundScope) + val list = awaitItem() + assertThat(list.map { it.roomId }).containsExactly(A_ROOM_ID, A_ROOM_ID_2).inOrder() + assertThat(analyticsService.trackedErrors).hasSize(1) + } + } + + /** + * Tracking issue #4182. + * + * Targeted scenario: a `DateChanged` tick fires after an initial SDK emit, then a follow-up + * SDK emit lands (mimicking "midnight, then a new message arrives"). Even though the diffCache + * is bypassed during the rebuild (`useCache = false`), the final state must contain each + * roomId exactly once and trackError must not fire on a happy path. + */ + @Test + fun `interleaved date change and SDK update with overlapping content does not produce duplicates`() = runTest { + val analyticsService = FakeAnalyticsService() + val summariesFlow = MutableStateFlow( + listOf( + aRoomSummary(roomId = A_ROOM_ID), + aRoomSummary(roomId = A_ROOM_ID_2), + ) + ) + val roomList = FakeDynamicRoomList(summaries = summariesFlow) + val roomListService = FakeRoomListService( + createRoomListLambda = { roomList } + ).apply { + postState(RoomListService.State.Running) + } + val dateTimeObserver = FakeDateTimeObserver() + val roomListDataSource = createRoomListDataSource( + roomListService = roomListService, + dateTimeObserver = dateTimeObserver, + analyticsService = analyticsService, + ) + + roomListDataSource.roomSummariesFlow.test { + roomListDataSource.launchIn(backgroundScope) + val initial = awaitItem() + assertThat(initial.map { it.roomId }).containsExactly(A_ROOM_ID, A_ROOM_ID_2).inOrder() + + // Midnight ticks while the cache holds [A_ROOM_ID, A_ROOM_ID_2] + dateTimeObserver.given(DateTimeObserver.Event.DateChanged(Instant.MIN, Instant.now())) + val afterMidnight = awaitItem() + assertThat(afterMidnight.map { it.roomId }).containsExactly(A_ROOM_ID, A_ROOM_ID_2).inOrder() + + // A new message bumps A_ROOM_ID — different unread count makes the StateFlow see this + // as a new value + summariesFlow.value = listOf( + aRoomSummary(roomId = A_ROOM_ID, numUnreadMessages = 1), + aRoomSummary(roomId = A_ROOM_ID_2), + ) + val afterMessage = awaitItem() + assertThat(afterMessage.map { it.roomId }).containsExactly(A_ROOM_ID, A_ROOM_ID_2).inOrder() + assertThat(afterMessage.map { it.roomId }.toSet()).hasSize(afterMessage.size) + + // Second midnight rebuild after the new message + dateTimeObserver.given(DateTimeObserver.Event.DateChanged(Instant.MIN, Instant.now())) + val afterSecondMidnight = awaitItem() + assertThat(afterSecondMidnight.map { it.roomId }).containsExactly(A_ROOM_ID, A_ROOM_ID_2).inOrder() + assertThat(afterSecondMidnight.map { it.roomId }.toSet()).hasSize(afterSecondMidnight.size) + + assertThat(analyticsService.trackedErrors).isEmpty() + } + } + private fun TestScope.createRoomListDataSource( roomListService: FakeRoomListService = FakeRoomListService(), roomListRoomSummaryFactory: RoomListRoomSummaryFactory = aRoomListRoomSummaryFactory(), notificationSettingsService: FakeNotificationSettingsService = FakeNotificationSettingsService(), dateTimeObserver: FakeDateTimeObserver = FakeDateTimeObserver(), + analyticsService: FakeAnalyticsService = FakeAnalyticsService(), ) = RoomListDataSource( roomListService = roomListService, roomListRoomSummaryFactory = roomListRoomSummaryFactory, @@ -112,6 +210,6 @@ class RoomListDataSourceTest { notificationSettingsService = notificationSettingsService, sessionCoroutineScope = backgroundScope, dateTimeObserver = dateTimeObserver, - analyticsService = FakeAnalyticsService(), + analyticsService = analyticsService, ) } diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomListEntriesUpdateExt.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomListEntriesUpdateExt.kt index 27b19ebf19..5d8367bb99 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomListEntriesUpdateExt.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomListEntriesUpdateExt.kt @@ -16,25 +16,25 @@ import org.matrix.rustcomponents.sdk.RoomListEntriesUpdate internal fun RoomListEntriesUpdate.describe(): String { return when (this) { is RoomListEntriesUpdate.Set -> { - "Set #$index to '${value.displayName()}'" + "Set #$index to '${value.id()}'" } is RoomListEntriesUpdate.Append -> { - "Append ${values.map { "'" + it.displayName() + "'" }}" + "Append ${values.map { "'" + it.id() + "'" }}" } is RoomListEntriesUpdate.PushBack -> { - "PushBack '${value.displayName()}'" + "PushBack '${value.id()}'" } is RoomListEntriesUpdate.PushFront -> { - "PushFront '${value.displayName()}'" + "PushFront '${value.id()}'" } is RoomListEntriesUpdate.Insert -> { - "Insert at #$index: '${value.displayName()}'" + "Insert at #$index: '${value.id()}'" } is RoomListEntriesUpdate.Remove -> { "Remove #$index" } is RoomListEntriesUpdate.Reset -> { - "Reset all to ${values.map { "'" + it.displayName() + "'" }}" + "Reset all to ${values.map { "'" + it.id() + "'" }}" } RoomListEntriesUpdate.PopBack -> { "PopBack" diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomSummaryListProcessor.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomSummaryListProcessor.kt index 968a768fa2..afdac3db12 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomSummaryListProcessor.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomSummaryListProcessor.kt @@ -112,6 +112,11 @@ class RoomSummaryListProcessor( private suspend fun updateRoomSummaries(updates: List, block: suspend MutableList.() -> Unit) = withContext( coroutineContext ) { + // Capture the description before applying updates: applyUpdate consumes each Room via + // `entry.use { ... }` which destroys it, and the duplicate-detection branch below reads + // id() through `describe()`. Without this capture the trackError call crashes before it + // can be reported. + val updatesDescription = updates.description() mutex.withLock { val current = roomSummaries.replayCache.lastOrNull() val mutableRoomSummaries = current.orEmpty().toMutableList() @@ -126,7 +131,7 @@ class RoomSummaryListProcessor( analyticsService.trackError( IllegalStateException( "Found duplicates in room summaries after a list update from the SDK: $duplicates. " + - "Updates: ${updates.description()}" + "Updates: $updatesDescription" ) ) } diff --git a/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomSummaryListProcessorTest.kt b/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomSummaryListProcessorTest.kt index 6fac54b904..d951b3d339 100644 --- a/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomSummaryListProcessorTest.kt +++ b/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomSummaryListProcessorTest.kt @@ -173,16 +173,68 @@ class RoomSummaryListProcessorTest { assertThat(summaries.value[index].roomId).isEqualTo(A_ROOM_ID_3) } + /** + * Tracking issue #4182 / #5031: rooms duplicated in the room list. + * + * If duplicates are present in the upstream summaries flow, the dedupe safety net in + * [RoomSummaryListProcessor.updateRoomSummaries] must remove them and report the incident via + * [analyticsService.trackError]. Uses an empty update to drive the dedupe path without + * passing a Rust Room through the destroy-on-use path. + */ + @Test + fun `pre-existing duplicates in summaries are deduped on next update and trackError fires`() = runTest { + summaries.value = listOf( + aRoomSummary(roomId = A_ROOM_ID), + aRoomSummary(roomId = A_ROOM_ID), // simulated SDK-side leak + aRoomSummary(roomId = A_ROOM_ID_2), + ) + val analyticsService = FakeAnalyticsService() + val processor = createProcessor(analyticsService = analyticsService) + + processor.postUpdate(emptyList()) + + assertThat(summaries.value.map { it.roomId }).containsExactly(A_ROOM_ID, A_ROOM_ID_2).inOrder() + assertThat(analyticsService.trackedErrors).hasSize(1) + } + + /** + * Tracking issue #4182 / #5031. + * + * Insert is the most likely Rust-SDK trigger for a duplicate-room report: it blindly inserts + * a new entry at an index without checking whether the roomId already exists. Before the + * describe-capture fix, the dedupe branch in [updateRoomSummaries] would call `Room.id()` + * on an already-destroyed Room (because [applyUpdate] consumes each value via + * `entry.use { ... }`) and crash before [trackError] could be invoked. This test guards the + * fix: the Insert is processed, the list is emitted deduplicated, and the tracked error + * message carries the human-readable description of the offending update. + */ + @Test + fun `Insert that triggers dedupe is reported via trackError without crashing`() = runTest { + summaries.value = listOf(aRoomSummary(roomId = A_ROOM_ID)) + val analyticsService = FakeAnalyticsService() + val processor = createProcessor(analyticsService = analyticsService) + + processor.postUpdate(listOf(RoomListEntriesUpdate.Insert(0u, aRustRoom(A_ROOM_ID)))) + + assertThat(summaries.value.map { it.roomId }).containsExactly(A_ROOM_ID) + assertThat(analyticsService.trackedErrors).hasSize(1) + val message = analyticsService.trackedErrors.single().message.orEmpty() + assertThat(message).contains("Found duplicates") + assertThat(message).contains("Insert at #0") + } + private fun aRustRoom(roomId: RoomId = A_ROOM_ID) = FakeFfiRoom( roomId = roomId, latestEventLambda = { LatestEventValue.None } ) - private fun TestScope.createProcessor() = RoomSummaryListProcessor( + private fun TestScope.createProcessor( + analyticsService: FakeAnalyticsService = FakeAnalyticsService(), + ) = RoomSummaryListProcessor( summaries, FakeFfiRoomListService(), coroutineContext = StandardTestDispatcher(testScheduler), roomSummaryFactory = RoomSummaryFactory(), - analyticsService = FakeAnalyticsService(), + analyticsService = analyticsService, ) }