Fix room list duplicate-detection telemetry crashing before it can report (#6791)
* Add room dupe regression tests * Fix telemetry path for dedupe discovery
This commit is contained in:
Jenna Vassar
GitHub
parent
432a7712c4
commit
cbc677b80d
4 changed files with 165 additions and 10 deletions
|
|
@ -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,
|
||||
)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -112,6 +112,11 @@ class RoomSummaryListProcessor(
|
|||
private suspend fun updateRoomSummaries(updates: List<RoomListEntriesUpdate>, block: suspend MutableList<RoomSummary>.() -> 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"
|
||||
)
|
||||
)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
)
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue