Skip to content

Commit

Permalink
Do not delete events from the last forward chunk
Browse files Browse the repository at this point in the history
We get end up with missing messages by the combination of
- deleting the last forward chunk when receiving a new one
- not adding events to a chunk that are already found in another chunk

Accordingly, when using chunk tokens to load more messages, those
messages that were not added to a chunk due to a /sync chunk will get
lost. More thorough steps to reproduce:

- Receive e.g. 30 new messages while offline
- Use /sync in the room overview, this will fetch the latest 10 events
- Open a chat in the past before the latest unread messages
- Scroll down a little, in order to fill the message gap and load all
  unread messages
- Close the chat
- Receive another e.g. 60 messages while offline
- Re-open the chat at some time in the past, before the latest 70
  messages
  => messages from the old /sync chunk will be missing

Change-Id: Ia3f2d2715a3edfd0b3fe5c3d48a02ade4ea49c4d
  • Loading branch information
SpiritCroc committed Apr 30, 2022
1 parent cbc29d0 commit ef57e60
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,21 @@ import org.matrix.android.sdk.internal.extensions.assertIsManaged
import org.matrix.android.sdk.internal.session.room.timeline.PaginationDirection
import timber.log.Timber

internal fun ChunkEntity.moveEventsFrom(chunkToMerge: ChunkEntity, direction: PaginationDirection) {
assertIsManaged()
val localRealm = this.realm
val eventsToMerge = if (direction == PaginationDirection.FORWARDS) {
chunkToMerge.timelineEvents.sort(TimelineEventEntityFields.DISPLAY_INDEX, Sort.ASCENDING)
} else {
chunkToMerge.timelineEvents.sort(TimelineEventEntityFields.DISPLAY_INDEX, Sort.DESCENDING)
}
eventsToMerge.forEach {
if (addTimelineEventFromMove(localRealm, it, direction)) {
chunkToMerge.timelineEvents.remove(it)
}
}
}

internal fun ChunkEntity.merge(roomId: String, chunkToMerge: ChunkEntity, direction: PaginationDirection) {
assertIsManaged()
val localRealm = this.realm
Expand Down Expand Up @@ -165,6 +180,17 @@ private fun ChunkEntity.addTimelineEventFromMerge(realm: Realm, timelineEventEnt
timelineEvents.add(copied)
}

private fun ChunkEntity.addTimelineEventFromMove(realm: Realm, event: TimelineEventEntity, direction: PaginationDirection): Boolean {
val eventId = event.eventId
if (timelineEvents.find(eventId) != null) {
return false
}
event.displayIndex = nextDisplayIndex(direction)
handleThreadSummary(realm, eventId, event)
timelineEvents.add(event)
return true
}

/**
* Upon copy of the timeline events we should update the latestMessage TimelineEventEntity with the new one
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import org.matrix.android.sdk.internal.crypto.DefaultCryptoService
import org.matrix.android.sdk.internal.database.helper.addIfNecessary
import org.matrix.android.sdk.internal.database.helper.addTimelineEvent
import org.matrix.android.sdk.internal.database.helper.createOrUpdate
import org.matrix.android.sdk.internal.database.helper.moveEventsFrom
import org.matrix.android.sdk.internal.database.helper.updateThreadSummaryIfNeeded
import org.matrix.android.sdk.internal.database.mapper.asDomain
import org.matrix.android.sdk.internal.database.mapper.toEntity
Expand Down Expand Up @@ -364,6 +365,15 @@ internal class RoomSyncHandler @Inject constructor(private val readReceiptHandle
aggregator: SyncResponsePostTreatmentAggregator): ChunkEntity {
val lastChunk = ChunkEntity.findLastForwardChunkOfRoom(realm, roomEntity.roomId)
if (isLimited && lastChunk != null) {
Timber.i("Deleting last forward chunk (${lastChunk.identifier()})")
// Add events that oldPrev may have dropped since they were already in lastChunk
val oldPrev = lastChunk.prevChunk
if (oldPrev != null && oldPrev.nextToken != lastChunk.prevToken) {
// If the tokens mismatch, this means we have chained them due to duplicated events.
// In this case, we need to make sure to re-add possibly dropped events (which would have
// been duplicates otherwise)
oldPrev.moveEventsFrom(lastChunk, PaginationDirection.FORWARDS)
}
lastChunk.deleteOnCascade(deleteStateEvents = false, canDeleteRoot = true)
}
val chunkEntity = if (!isLimited && lastChunk != null) {
Expand Down

0 comments on commit ef57e60

Please sign in to comment.