matrix-org / matrix-rust-sdk

Matrix Client-Server SDK for Rust
Apache License 2.0
1.19k stars 239 forks source link

Room list service: cold cache cleanup #2548

Open stefanceriu opened 1 year ago

stefanceriu commented 1 year ago

Later edit

Room list caches don't get cleaned up properly:

STRs

1) Join a room 2) Let the app fill up it's cold cache 3) Close the app and leave the room from a different client 4) Come back to the app and see the room never go away 5) Extra step: clear the cache and see it behave normal again

Original report

I'm finding myself in a weird situation in which following receiving an invite to the same DM multiple times (bug on web) the invites room list contains an invalidated item that doesn't want to go away.

I checked the proxy responses and the invites are always empty

 "invites": {
      "count": 0
    },

On the app side though, when the app starts, i get a diff back

▿ 1 element
  ▿ 0 : RoomListEntriesUpdate
    ▿ reset : 1 element
      ▿ values : 1 element
        ▿ 0 : RoomListEntry
          ▿ invalidated : 1 element
            - roomId : "!redacted:element.io"

Presumably the cold cache is not being cleaned up properly. This is probably a regression from when we moved the Invites list to the first request.

If you ask me we should just not do

.add_cached_list(
                SlidingSyncList::builder(INVITES_LIST_NAME)
Hywan commented 1 year ago

Let device A get room1 in its cache. Let device B enters room1 and leaves it. User goes back to device A, room1 is still present in the cache, as expected… until a sync happens, which should remove room1.

The cache/state cannot be updated before a sync is done. The sync is per device.

However, your point 4 says: “Come back to the app and see the room never go away”. The annoying part is “never go away”. I understand that the sync never broadcasts the event saying a room has been left.

Please, can you open a bug on https://github.com/matrix-org/sliding-sync/?

stefanceriu commented 1 year ago

I understand that the sync never broadcasts the event saying a room has been left.

As I understand it that extra room comes from the cold cache. Shouldn't the cold cache clear up any extra rooms when all rooms reaches the full size?

Hywan commented 1 year ago

That could be a workaround, but it doesn't fix the underlying bug, which is that the sync misses events.

I am not inclined to implement a workaround because:

  1. It's not so easy to do,
  2. It can have false-positive. Keep in mind that depending of how the sliding sync request is configured (filters and so on), the maximum number of rooms can vary. Thus, even if detecting when all rooms are fully loaded is easy, it's not a commitment that we have all the data.

About having false-positive, one may argue that if a room is missing from the cache, it's likely to be synced again later on though. However, I would really prefer to see the bug fixed on the sliding sync proxy instead.